New issue
Advanced search Search tips

Issue 790778 link

Starred by 1 user
Status: Fixed
Owner:
japhet@chromium.org
Closed: Dec 2017
Cc:
hirosh...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Merge ResourceOwner into ResourceClient

Project Member Reported by japhet@chromium.org, Nov 30 2017 
Currently, there is the ResourceClient interface, which can be used to resouce load callbacks, and ResourceOwner, which adds Resource pointer management to ResourceClient. Some callers use ResourceClient directly, while others use ResourceOwner. There's no reason to have two different ways to interact with a Resource, so merge them.

Once they're merged, we can look at ways to improve the interaction around start-of-request. We currently start a resource load, then in a separate call add the ResourceClient to listen to any callbacks. Ideally that would be a single step.
Project Member

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

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

commit ae15c13530084329ac2509ccded45428d7d7d2c8
Author: Nate Chapin <japhet@chromium.org>
Date: Sat Dec 02 02:03:07 2017

Make DocumentThreadableLoader a ResourceOwner

It had a custom implementation to support use of RawResourceClientStateChecker.
Separate these.

Bug:  790778 , 640291
Change-Id: Id8a29a5ad916429969b47a0258675437de284b55
Reviewed-on: https://chromium-review.googlesource.com/802094
Commit-Queue: Nate Chapin <japhet@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521177}
[modify] https://crrev.com/ae15c13530084329ac2509ccded45428d7d7d2c8/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
[modify] https://crrev.com/ae15c13530084329ac2509ccded45428d7d7d2c8/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h
[modify] https://crrev.com/ae15c13530084329ac2509ccded45428d7d7d2c8/third_party/WebKit/Source/platform/loader/fetch/RawResource.cpp
[modify] https://crrev.com/ae15c13530084329ac2509ccded45428d7d7d2c8/third_party/WebKit/Source/platform/loader/fetch/RawResource.h
Project Member

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

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

commit 6e8fd3eca1608700199771ecc24edd8f4255911a
Author: Nate Chapin <japhet@chromium.org>
Date: Mon Dec 04 21:02:02 2017

Revert "Make DocumentThreadableLoader a ResourceOwner"

This reverts commit ae15c13530084329ac2509ccded45428d7d7d2c8.

Reason for revert:
https://bugs.chromium.org/p/chromium/issues/detail?id=791291
https://bugs.chromium.org/p/chromium/issues/detail?id=791316
https://bugs.chromium.org/p/chromium/issues/detail?id=791288
https://bugs.chromium.org/p/chromium/issues/detail?id=791348
https://bugs.chromium.org/p/chromium/issues/detail?id=791347
https://bugs.chromium.org/p/chromium/issues/detail?id=791450
https://bugs.chromium.org/p/chromium/issues/detail?id=791300
https://bugs.chromium.org/p/chromium/issues/detail?id=791522

Original change's description:
> Make DocumentThreadableLoader a ResourceOwner
> 
> It had a custom implementation to support use of RawResourceClientStateChecker.
> Separate these.
> 
> Bug:  790778 , 640291
> Change-Id: Id8a29a5ad916429969b47a0258675437de284b55
> Reviewed-on: https://chromium-review.googlesource.com/802094
> Commit-Queue: Nate Chapin <japhet@chromium.org>
> Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#521177}

TBR=hiroshige@chromium.org,japhet@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  790778 , 640291
Change-Id: I15938754b57b8142f8d7e9c94f8aff9bdd8329ed
Reviewed-on: https://chromium-review.googlesource.com/806794
Reviewed-by: Nate Chapin <japhet@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521448}
[modify] https://crrev.com/6e8fd3eca1608700199771ecc24edd8f4255911a/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
[modify] https://crrev.com/6e8fd3eca1608700199771ecc24edd8f4255911a/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h
[modify] https://crrev.com/6e8fd3eca1608700199771ecc24edd8f4255911a/third_party/WebKit/Source/platform/loader/fetch/RawResource.cpp
[modify] https://crrev.com/6e8fd3eca1608700199771ecc24edd8f4255911a/third_party/WebKit/Source/platform/loader/fetch/RawResource.h
Project Member

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

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

commit f3b283cf948f10ad8c919c4c8ebec5219f058d28
Author: Nate Chapin <japhet@chromium.org>
Date: Mon Dec 04 22:30:39 2017

Reland "Make DocumentThreadableLoader a ResourceOwner"

It had a custom implementation to support use of RawResourceClientStateChecker.
Separate these.

The original version (https://chromium.googlesource.com/chromium/src/+/ae15c13530084329ac2509ccded45428d7d7d2c8)
failed to update DocumentThreadableLoader::Trace(). It introduced an intermediate
subclass, ResourceOwner<RawResource>, between DocumentThreadableLoader and RawResourceClient, but
because Trace() wasn't updated, ResourceOwner::resource_ was left untraced. The GC clang plugin
would normally catch a mistake like this, but the plugin ignores ResourceOwner due to
https://crbug.com/652966, so it wasn't detected at compile time.

Bug:  790778 , 640291
Change-Id: I114df7eaa97139fbb6aa70b22b2f41019988631f
Reviewed-on: https://chromium-review.googlesource.com/806753
Commit-Queue: Nate Chapin <japhet@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521490}
[modify] https://crrev.com/f3b283cf948f10ad8c919c4c8ebec5219f058d28/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
[modify] https://crrev.com/f3b283cf948f10ad8c919c4c8ebec5219f058d28/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h
[modify] https://crrev.com/f3b283cf948f10ad8c919c4c8ebec5219f058d28/third_party/WebKit/Source/platform/loader/fetch/RawResource.cpp
[modify] https://crrev.com/f3b283cf948f10ad8c919c4c8ebec5219f058d28/third_party/WebKit/Source/platform/loader/fetch/RawResource.h
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 7 2017 

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

commit 9ecc892ac9840a973d568a36afd446a38c3a3b15
Author: Nate Chapin <japhet@chromium.org>
Date: Thu Dec 07 00:37:27 2017

Merge ResourceOwner into ResourceClient

This changes ResourceClient to hold a Member<Resource> as ResourceOwner did. Existing
ResourceOwner subclasses can just switch to the appropriate ResourceClient subclass.
Classes that were using a ResourceClient directly no longer need to hold their own
Member<Resource> and can instead use SetResource/ClearResource/GetResource,
implemented in ResourceClient, to manage the resource.

Bug:  790778 , 652966
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Id21231355a12a27e1241c87dbb153c2a8629de8e
Reviewed-on: https://chromium-review.googlesource.com/794370
Commit-Queue: Nate Chapin <japhet@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522275}
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/css/RemoteFontFaceSource.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/css/StyleRuleImport.cpp
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/css/StyleRuleImport.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/dom/ClassicPendingScript.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/dom/ProcessingInstruction.cpp
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/dom/ProcessingInstruction.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/dom/ScriptLoader.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/html/LinkStyle.cpp
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/html/LinkStyle.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/html/imports/HTMLImportLoader.cpp
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/html/imports/HTMLImportLoader.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/inspector/InspectorResourceContentLoader.cpp
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/loader/DocumentLoader.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/loader/LinkLoader.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/loader/TextTrackLoader.cpp
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/loader/TextTrackLoader.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/loader/modulescript/DocumentModuleScriptFetcher.cpp
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/loader/modulescript/DocumentModuleScriptFetcher.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/loader/resource/DocumentResource.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/loader/resource/FontResource.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/loader/resource/ImageResource.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/loader/resource/LinkFetchResource.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/loader/resource/MockFontResourceClient.cpp
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/loader/resource/MockFontResourceClient.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/loader/resource/ScriptResource.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/loader/resource/TextResource.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/svg/SVGElementProxy.cpp
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/svg/SVGUseElement.cpp
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/core/svg/SVGUseElement.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/platform/loader/BUILD.gn
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/platform/loader/fetch/RawResource.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/platform/loader/fetch/ResourceClient.h
[modify] https://crrev.com/9ecc892ac9840a973d568a36afd446a38c3a3b15/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcherTest.cpp
[delete] https://crrev.com/6ed26f014f76f10e76e80636027a2db9dcbe1664/third_party/WebKit/Source/platform/loader/fetch/ResourceOwner.h

Comment 5 by japhet@chromium.org, Dec 7 2017

Status: Fixed (was: Started)
►

Sign in to add a comment