New issue
Advanced search Search tips

Issue 794651 link

Starred by 2 users
Status: Started
Owner:
hirosh...@chromium.org
Cc:
japhet@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 787704



Sign in to add a comment

Improve ResourceResponse construction

Project Member Reported by hirosh...@chromium.org, Dec 13 2017 
A tracking issue for misc ResourceResponse improvements.

Currently, ResourceResponse has two major constructor (except for a copy ctor and a ctor for for CrossThreadCopier):
- ResourceResponse() (=> IsNull()==true) and
- ResourceResponse(KURL, mime_type, ...) (=> IsNull()==false).

This issue reduces the use of the default constructor, because in most cases SetURL() is called just after construction, and it is more like the second constructor for non-null ResourceResponse.

If we can enforce the default constructor for intentional null ResourceResponse and the second constructor for non-null ResourceResponse, it would be helpful for removing ResourceResponse::is_null_ and turns ResourseResponse into Optional<ResourseResponse>, unique_ptr<ResourseResponse>, Member<ResourseResponse> or scoped_refptr<ResourseResponse>.
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 13 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5830b993a3529bf00e2149afc0c0b3d268d78d36

commit 5830b993a3529bf00e2149afc0c0b3d268d78d36
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Wed Dec 13 21:21:38 2017

Always initialize ResourceResponse::connection_info_

In a ResourceResponse constructor, connection_info_ is not initialized.
This CL initialize it with the same value as the default constructor.

Bug: 794651,  663175 
Change-Id: I895ed2e7e436ed5a2390da1f54e70ce3b7af26b3
Reviewed-on: https://chromium-review.googlesource.com/825249
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523880}
[modify] https://crrev.com/5830b993a3529bf00e2149afc0c0b3d268d78d36/third_party/WebKit/Source/platform/loader/fetch/ResourceResponse.cpp
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 13 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4ccc82e6053201317cc32e95515e8b8bd7749bf5

commit 4ccc82e6053201317cc32e95515e8b8bd7749bf5
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Wed Dec 13 22:32:45 2017

Make ResourceResponse's non-null ctor arguments optional except for URL

To make ResourceResponse constructible from a KURL only, and
reduce ResourceResponse::Set.*() calls.

This CL merges SetURL() and SetMimeType() calls into the constructor,
and removes redundant |expected_length| and |text_encoding_name|
parameters to the constructor.

Bug: 794651
Change-Id: Id85981184a86c30fe6304453c1158126b09e81c7
Reviewed-on: https://chromium-review.googlesource.com/823262
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523916}
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/core/html/parser/CSSPreloadScannerTest.cpp
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/core/loader/AllowedByNosniffTest.cpp
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/core/loader/MixedContentCheckerTest.cpp
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/core/loader/ProgressTrackerTest.cpp
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/core/loader/resource/CSSStyleSheetResourceTest.cpp
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/core/loader/resource/FontResourceTest.cpp
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/core/loader/resource/MultipartImageResourceParser.cpp
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/core/loader/resource/MultipartImageResourceParserTest.cpp
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/core/timing/PerformanceBaseTest.cpp
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/platform/loader/SubresourceIntegrityTest.cpp
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/platform/loader/fetch/ClientHintsPreferencesTest.cpp
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/platform/loader/fetch/MemoryCacheCorrectnessTest.cpp
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/platform/loader/fetch/RawResourceTest.cpp
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcherTest.cpp
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/platform/loader/fetch/ResourceLoaderTest.cpp
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/platform/loader/fetch/ResourceResponse.h
[modify] https://crrev.com/4ccc82e6053201317cc32e95515e8b8bd7749bf5/third_party/WebKit/Source/platform/loader/fetch/ResourceTest.cpp
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 19 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1b07f31020dd41b811c86f3f18b382f8603b7acd

commit 1b07f31020dd41b811c86f3f18b382f8603b7acd
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Tue Dec 19 00:09:57 2017

Use default member initializers for ResourceResponse

To avoid duplicated intializations among the constructors.
This merges most of the code of two ResourceResponse constructors,
and avoid inconsistent initialization such as that fixed by
https://chromium-review.googlesource.com/825249.

This CL also stop using bit fields in ResourceResponse, because
default member initializer for bit-field requires C++2a.
This doesn't cause much memory regression, because ResourceResponse
is already huge. For example this CL increases
sizeof(ResourceResponse) from 664 to 672 on 64-bit Linux.

Bug: 794651
Change-Id: I398ce2f9f4315f24ec649dfe93c0e2476bdb60e8
Reviewed-on: https://chromium-review.googlesource.com/825523
Reviewed-by: Nate Chapin <japhet@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524869}
[modify] https://crrev.com/1b07f31020dd41b811c86f3f18b382f8603b7acd/third_party/WebKit/Source/platform/loader/fetch/ResourceResponse.cpp
[modify] https://crrev.com/1b07f31020dd41b811c86f3f18b382f8603b7acd/third_party/WebKit/Source/platform/loader/fetch/ResourceResponse.h
►

Sign in to add a comment