New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 669383 link

Starred by 1 user
Status: Fixed
Owner:
█ tyoshino@chromium.org
Leaves the project on 2018/03/02
Closed: Dec 2016
Cc:
japhet@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Make the role of ResourceLoader, ResourceFetcher and FrameFetchContext clearer

Project Member Reported by tyoshino@chromium.org, Nov 29 2016 
An umbrella bug for efforts to make the role of these classes clearer.

To start with, I'm going to move the logic in ResourceFetcher that is handling calls from WebURLLoaderImpl.
https://codereview.chromium.org/2533683002/

The principle for deciding whether we put a certain logic in ResourceLoader or ResourceFetcher would be that logic specific to a certain loading should live in ResourceLoader.

Comment 1 by tyoshino@chromium.org, Nov 29 2016

Cc: japhet@chromium.org
+japhet fyi
Project Member

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

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

commit eaa629122ad7934f66a6e75b228ce710e71053ff
Author: tyoshino <tyoshino@chromium.org>
Date: Tue Dec 13 07:07:02 2016

Move the code in ResourceFetcher handling calls from WebURLLoaderImpl to ResourceLoader

They're not doing anything specific to the ResourceFetcher but just
consulting the FrameFetchContext via ResourceFetcher. Especially,
didReceiveResponse() contains a lot of back calls to the ResourceLoader.
The logic should just be implemented inside the ResourceLoader.

defersLoading() is also removed since it's used only by the unit tests
and can be replaced by invocation of public method context().

Renamed unary didFail() to handleError() as it's not a WebURLLoaderImpl
implementation and therefore it's confusing that it has the "did"
prefix.

R=yhirano@chromium.org,japhet@chromium.org,tsepez@chromium.org,kinuko@chromium.org
BUG= 669383 

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

[modify] https://crrev.com/eaa629122ad7934f66a6e75b228ce710e71053ff/third_party/WebKit/Source/core/fetch/FetchContext.h
[modify] https://crrev.com/eaa629122ad7934f66a6e75b228ce710e71053ff/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
[modify] https://crrev.com/eaa629122ad7934f66a6e75b228ce710e71053ff/third_party/WebKit/Source/core/fetch/ResourceFetcher.h
[modify] https://crrev.com/eaa629122ad7934f66a6e75b228ce710e71053ff/third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp
[modify] https://crrev.com/eaa629122ad7934f66a6e75b228ce710e71053ff/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp
[modify] https://crrev.com/eaa629122ad7934f66a6e75b228ce710e71053ff/third_party/WebKit/Source/core/fetch/ResourceLoader.h
[modify] https://crrev.com/eaa629122ad7934f66a6e75b228ce710e71053ff/third_party/WebKit/Source/core/loader/resource/FontResourceTest.cpp
[modify] https://crrev.com/eaa629122ad7934f66a6e75b228ce710e71053ff/third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp
[modify] https://crrev.com/eaa629122ad7934f66a6e75b228ce710e71053ff/third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h
[modify] https://crrev.com/eaa629122ad7934f66a6e75b228ce710e71053ff/third_party/WebKit/Source/web/tests/WebFrameTest.cpp

Comment 3 by tyoshino@chromium.org, Dec 13 2016

Status: Fixed (was: Available)
We've decided to
- put the per-load logic in ResourceLoader, and
- put the load start, cache policy and per-context bookkeeping
considering balance.

Comment 4 by tyoshino@chromium.org, Dec 13 2016 

See Nate's analysis in https://codereview.chromium.org/2533683002/#msg27
►

Sign in to add a comment