New issue
Advanced search Search tips

Issue 632580 link

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

Blocking:
issue 570205
issue 667641



Sign in to add a comment

Make Resource::resourceRequest() and the ResourceRequest given to WebURLLoader exactly the same

Project Member Reported by hirosh...@chromium.org, Jul 29 2016 
[1] made them the same.
[2] made them different again, to fix a regression introduced by [1] ( Issue 623616 ).

It is preferred to make them the same again.
It seems like that would require splitting willSendRequest up into
a prepareRequest step and a sendingTheRequestForRealNow step. (a comment by japhet@ from [2])

[1] https://codereview.chromium.org/1889973002/
[2] https://codereview.chromium.org/2184823006/
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 29 2016 

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

commit 1b4209891f51609fe5aa84e13be6dd482ca734c7
Author: hiroshige <hiroshige@chromium.org>
Date: Fri Jul 29 06:03:10 2016

Call FetchContext::dispatchWillSendRequest() only if an actual load is started

Fixing a regression since [1].
[1] https://codereview.chromium.org/1889973002/

Since [1], FetchContext::dispatchWillSendRequest() was called from
ResourceFetcher::willSendRequest() in the middle of
ResourceFetcher::requestResource(), and might have modified
Resource::m_resourceRequest.
However, if the actual loading is deferred, e.g. when FontResource is not
used, FetchContext::dispatchWillSendRequest() was called without actual
loading, causing stale pending load reports in DevTools.

This CL
- Moves the ResourceFetcher::willSendRequest() call in requestResource() to
  ResourceFetcher::startLoad().
  Now FetchContext::dispatchWillSendRequest() (called from startLoad())
  modifies the local ResourceRequest in startLoad(), not modifying
  ResourceRequest::m_resourceRequest (which is the behavior before [1]).
- Moves setAllowStoredCredentials() call in ResourceFetcher::willSendRequest()
  to its callsites, because this should be done even when the actual loading
  is deferred and should modify Resource::m_resourceRequest.
- Adds a layout test.

BUG= 623616 , 632580

Review-Url: https://codereview.chromium.org/2184823006
Cr-Commit-Position: refs/heads/master@{#408588}

[add] https://crrev.com/1b4209891f51609fe5aa84e13be6dd482ca734c7/third_party/WebKit/LayoutTests/http/tests/inspector/network/font-face-expected.txt
[add] https://crrev.com/1b4209891f51609fe5aa84e13be6dd482ca734c7/third_party/WebKit/LayoutTests/http/tests/inspector/network/font-face.html
[add] https://crrev.com/1b4209891f51609fe5aa84e13be6dd482ca734c7/third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/font-face.html
[modify] https://crrev.com/1b4209891f51609fe5aa84e13be6dd482ca734c7/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 3 2016

Labels: merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e8573e904dd5ed11adb5eac18ac601963e90cc14

commit e8573e904dd5ed11adb5eac18ac601963e90cc14
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Wed Aug 03 07:29:32 2016

Call FetchContext::dispatchWillSendRequest() only if an actual load is started

Fixing a regression since [1].
[1] https://codereview.chromium.org/1889973002/

Since [1], FetchContext::dispatchWillSendRequest() was called from
ResourceFetcher::willSendRequest() in the middle of
ResourceFetcher::requestResource(), and might have modified
Resource::m_resourceRequest.
However, if the actual loading is deferred, e.g. when FontResource is not
used, FetchContext::dispatchWillSendRequest() was called without actual
loading, causing stale pending load reports in DevTools.

This CL
- Moves the ResourceFetcher::willSendRequest() call in requestResource() to
  ResourceFetcher::startLoad().
  Now FetchContext::dispatchWillSendRequest() (called from startLoad())
  modifies the local ResourceRequest in startLoad(), not modifying
  ResourceRequest::m_resourceRequest (which is the behavior before [1]).
- Moves setAllowStoredCredentials() call in ResourceFetcher::willSendRequest()
  to its callsites, because this should be done even when the actual loading
  is deferred and should modify Resource::m_resourceRequest.
- Adds a layout test.

BUG= 623616 , 632580

Review-Url: https://codereview.chromium.org/2184823006
Cr-Commit-Position: refs/heads/master@{#408588}
(cherry picked from commit 1b4209891f51609fe5aa84e13be6dd482ca734c7)

Review URL: https://codereview.chromium.org/2207833002 .

Cr-Commit-Position: refs/branch-heads/2785@{#480}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[add] https://crrev.com/e8573e904dd5ed11adb5eac18ac601963e90cc14/third_party/WebKit/LayoutTests/http/tests/inspector/network/font-face-expected.txt
[add] https://crrev.com/e8573e904dd5ed11adb5eac18ac601963e90cc14/third_party/WebKit/LayoutTests/http/tests/inspector/network/font-face.html
[add] https://crrev.com/e8573e904dd5ed11adb5eac18ac601963e90cc14/third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/font-face.html
[modify] https://crrev.com/e8573e904dd5ed11adb5eac18ac601963e90cc14/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp

Comment 3 by hirosh...@chromium.org, Oct 3 2016

Cc: -japhet@chromium.org
Owner: japhet@chromium.org
Status: Assigned (was: Available)
japhet@ is working on this. CL: https://codereview.chromium.org/2203613003/

Comment 4 by shaochuan@chromium.org, Oct 6 2016

Cc: shaochuan@chromium.org

Comment 5 by shaochuan@chromium.org, Oct 20 2016

Blocking: 570205

Comment 6 by hirosh...@chromium.org, Mar 10 2017

Cc: -hirosh...@chromium.org japhet@chromium.org
Owner: hirosh...@chromium.org
Status: Started (was: Assigned)
Restarted.

I'll also try to clarify what kind of modifications are made to ResourceRequest, after FetchRequest is created, by e.g. removing FetchRequest::mutableResourceRequest() callers.

Comment 7 by hirosh...@chromium.org, Mar 10 2017

Blocking: 667641

Comment 8 by hirosh...@chromium.org, Mar 10 2017 

To clarify what kind of modifications are made to ResourceRequest after FetchRequest is created, I'll reduce non-const usage of FetchRequest::m_resourceRequest, including mutableResourceRequest().
I expect most of them can be avoided.
After I remove unnecessary modifications via FetchRequest::m_resourceRequest or mutableResourceRequest(), all mutableResourceRequest() will represent necessary modifications to ResourceRequest after FetchRequest is created.
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 10 2017 

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

commit a51669425d5a771fb9f80663779135c1deffdb00
Author: hiroshige <hiroshige@chromium.org>
Date: Fri Mar 10 21:35:17 2017

Remove FetchRequest::mutableResourceRequest() where removing is trivial

This CL removes FetchRequest::mutableResourceRequest() calls that are
used to modify ResourceRequest created just before, i.e. replaces:
  FetchRequest request(ResourceRequest(foo), bar);
  request.mutableResourceRequest().baz();
with:
  ResourceRequest resourceRequest(foo);
  resourceRequest.baz();
  FetchRequest request(resourceRequest, bar);
To clarify what kind of modifications to ResourceRequest via
mutableResourceRequest() is needed (https://crbug.com/632580#c8).

BUG=632580

Review-Url: https://codereview.chromium.org/2740253002
Cr-Commit-Position: refs/heads/master@{#456169}

[modify] https://crrev.com/a51669425d5a771fb9f80663779135c1deffdb00/third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp
[modify] https://crrev.com/a51669425d5a771fb9f80663779135c1deffdb00/third_party/WebKit/Source/core/css/CSSImageSetValue.cpp
[modify] https://crrev.com/a51669425d5a771fb9f80663779135c1deffdb00/third_party/WebKit/Source/core/css/CSSImageValue.cpp
[modify] https://crrev.com/a51669425d5a771fb9f80663779135c1deffdb00/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
[modify] https://crrev.com/a51669425d5a771fb9f80663779135c1deffdb00/third_party/WebKit/Source/core/inspector/InspectorResourceContentLoader.cpp
[modify] https://crrev.com/a51669425d5a771fb9f80663779135c1deffdb00/third_party/WebKit/Source/core/loader/LinkLoader.cpp
[modify] https://crrev.com/a51669425d5a771fb9f80663779135c1deffdb00/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcherTest.cpp
Project Member

Comment 10 by bugdroid1@chromium.org, Mar 10 2017 

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

commit b3adecaccaaa4c19f3a94be273e813d81b357a99
Author: hiroshige <hiroshige@chromium.org>
Date: Fri Mar 10 23:09:51 2017

Remove mutableResourceRequest() by FetchRequest::setRequestContext()

It is common that FooResource::fetch()-like functions sets
the request context via mutableResourceRequest().
This CL centralizes such mutableResourceRequest() calls into newly
introduced FetchRequest::setRequestContext(), to limit the ways to
modify a ResourceRequest after it is put in a FetchRequest.

BUG=632580

Review-Url: https://codereview.chromium.org/2741753003
Cr-Commit-Position: refs/heads/master@{#456208}

[modify] https://crrev.com/b3adecaccaaa4c19f3a94be273e813d81b357a99/third_party/WebKit/Source/core/loader/resource/CSSStyleSheetResource.cpp
[modify] https://crrev.com/b3adecaccaaa4c19f3a94be273e813d81b357a99/third_party/WebKit/Source/core/loader/resource/DocumentResource.cpp
[modify] https://crrev.com/b3adecaccaaa4c19f3a94be273e813d81b357a99/third_party/WebKit/Source/core/loader/resource/FontResource.cpp
[modify] https://crrev.com/b3adecaccaaa4c19f3a94be273e813d81b357a99/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
[modify] https://crrev.com/b3adecaccaaa4c19f3a94be273e813d81b357a99/third_party/WebKit/Source/core/loader/resource/ScriptResource.cpp
[modify] https://crrev.com/b3adecaccaaa4c19f3a94be273e813d81b357a99/third_party/WebKit/Source/platform/loader/fetch/FetchRequest.h
[modify] https://crrev.com/b3adecaccaaa4c19f3a94be273e813d81b357a99/third_party/WebKit/Source/platform/loader/fetch/RawResource.cpp
[modify] https://crrev.com/b3adecaccaaa4c19f3a94be273e813d81b357a99/third_party/WebKit/Source/platform/loader/testing/MockResource.cpp
[modify] https://crrev.com/b3adecaccaaa4c19f3a94be273e813d81b357a99/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
Project Member

Comment 12 by bugdroid1@chromium.org, Mar 10 2017 

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

commit 2779971c35e04a0a5b5a4c5d977cacb6090b12b7
Author: hiroshige <hiroshige@chromium.org>
Date: Fri Mar 10 23:24:11 2017

Merge mutableResourceRequest() calls in XSLStyleSheetResource

BUG=632580

Review-Url: https://codereview.chromium.org/2744593005
Cr-Commit-Position: refs/heads/master@{#456214}

[modify] https://crrev.com/2779971c35e04a0a5b5a4c5d977cacb6090b12b7/third_party/WebKit/Source/core/loader/resource/XSLStyleSheetResource.cpp
Project Member

Comment 13 by bugdroid1@chromium.org, Mar 23 2017 

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

commit 059d59531760005f8bfa5a03eab31fb9e3203ff5
Author: kinuko <kinuko@chromium.org>
Date: Thu Mar 23 07:42:31 2017

Make prepareRequest() a separate callback of FetchContext

- Split prepareRequest() from dispatchWillSendRequest() and willStartLoading()
- Move AppCache hook code into prepareRequest(), which looks more relevant
- Rename willStartLoading() to recordLoadingActivity() and makes it take const request

This still doesn't make dispatchWillSendRequest() take
const ResourceRequest& (see crbug.com/632580), but (I think) makes things
slightly more consistent & moves fetching logic out of context into fetcher
abstraction implementors like ResourceFetcher.

BUG= 695279 , 632580

Review-Url: https://codereview.chromium.org/2747203002
Cr-Commit-Position: refs/heads/master@{#459016}

[modify] https://crrev.com/059d59531760005f8bfa5a03eab31fb9e3203ff5/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
[modify] https://crrev.com/059d59531760005f8bfa5a03eab31fb9e3203ff5/third_party/WebKit/Source/core/loader/FrameFetchContext.h
[modify] https://crrev.com/059d59531760005f8bfa5a03eab31fb9e3203ff5/third_party/WebKit/Source/core/loader/PingLoader.cpp
[modify] https://crrev.com/059d59531760005f8bfa5a03eab31fb9e3203ff5/third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp
[modify] https://crrev.com/059d59531760005f8bfa5a03eab31fb9e3203ff5/third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.h
[modify] https://crrev.com/059d59531760005f8bfa5a03eab31fb9e3203ff5/third_party/WebKit/Source/platform/loader/fetch/FetchContext.cpp
[modify] https://crrev.com/059d59531760005f8bfa5a03eab31fb9e3203ff5/third_party/WebKit/Source/platform/loader/fetch/FetchContext.h
[modify] https://crrev.com/059d59531760005f8bfa5a03eab31fb9e3203ff5/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
[modify] https://crrev.com/059d59531760005f8bfa5a03eab31fb9e3203ff5/third_party/WebKit/Source/platform/loader/fetch/ResourceLoader.cpp
►

Sign in to add a comment