Re: Preload scan external CSS for @import (issue 1976463003 by csharrison@chromium.org)

0 views
Skip to first unread message

cshar...@chromium.org

unread,
Jul 25, 2016, 12:30:28 PM7/25/16
to hiro...@chromium.org, jap...@chromium.org, chromium...@chromium.org, tyoshin...@chromium.org, blink-rev...@chromium.org, tfa...@chromium.org, blink-rev...@chromium.org, yo...@yoav.ws, dglazko...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org

hiro...@chromium.org

unread,
Jul 26, 2016, 5:57:10 AM7/26/16
to cshar...@chromium.org, jap...@chromium.org, oilpan-...@chromium.org, chromium...@chromium.org, tyoshin...@chromium.org, blink-rev...@chromium.org, tfa...@chromium.org, blink-rev...@chromium.org, yo...@yoav.ws, dglazko...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org
core/fetch part basically looks good.

+oilpan-reviews for a question in CSSStyleSheetResource.cpp (see the comment).


https://codereview.chromium.org/1976463003/diff/100001/third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp
(right):

https://codereview.chromium.org/1976463003/diff/100001/third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp#newcode85
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:85:
static_cast<StyleSheetResourceClient*>(c)->didAppendFirstData(this);
|*c| must not be destructed during didAppendFirstData() (to prevent
similar issues to crbug.com/612132 (restricted)).

oilpan-reviews, is |*c| kept alive, or should we add
Persistent<StyleSheetResourceClient> protect = c
?
- |c| points to StyleSheetResourceClient, an on-heap subclass of
ResourceClient.
- |c| appears on the C++ stack as an argument of didAddClient() here,
but as ResourceClient*, not as StyleSheetResourceClient*.
- ResourceClient is not on-heap.

https://codereview.chromium.org/1976463003/diff/100001/third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp#newcode87
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:87: if
(!isLoading())
|c| might be removed as client in didAppendFirstData() in general.
So please check that |c| is still an client, i.e.
if (hasClient(c) && !isLoading())

https://codereview.chromium.org/1976463003/diff/100001/third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp
File third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp
(right):

https://codereview.chromium.org/1976463003/diff/100001/third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp#newcode247
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:247:
setResource(static_cast<CSSStyleSheetResource*>(resource),
Resource::DontMarkAsReferenced);
We can use toCSSStyleSheetResource() instead of static_cast<>.

https://codereview.chromium.org/1976463003/diff/100001/third_party/WebKit/Source/core/html/parser/CSSPreloadScannerTest.cpp
File
third_party/WebKit/Source/core/html/parser/CSSPreloadScannerTest.cpp
(right):

https://codereview.chromium.org/1976463003/diff/100001/third_party/WebKit/Source/core/html/parser/CSSPreloadScannerTest.cpp#newcode21
third_party/WebKit/Source/core/html/parser/CSSPreloadScannerTest.cpp:21:
class PreloadSuppressingCSSPreloaderResourceClient : public
CSSPreloaderResourceClient {
Add |final|.

https://codereview.chromium.org/1976463003/

har...@chromium.org

unread,
Jul 26, 2016, 6:01:14 AM7/26/16
to cshar...@chromium.org, hiro...@chromium.org, jap...@chromium.org, oilpan-...@chromium.org, chromium...@chromium.org, tyoshin...@chromium.org, blink-rev...@chromium.org, tfa...@chromium.org, blink-rev...@chromium.org, yo...@yoav.ws, dglazko...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org

https://codereview.chromium.org/1976463003/diff/100001/third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp
(right):

https://codereview.chromium.org/1976463003/diff/100001/third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp#newcode85
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:85:
static_cast<StyleSheetResourceClient*>(c)->didAppendFirstData(this);
On 2016/07/26 09:57:10, hiroshige wrote:
> |*c| must not be destructed during didAppendFirstData() (to prevent
similar
> issues to crbug.com/612132 (restricted)).
>
> oilpan-reviews, is |*c| kept alive, or should we add
> Persistent<StyleSheetResourceClient> protect = c
> ?
> - |c| points to StyleSheetResourceClient, an on-heap subclass of
ResourceClient.
> - |c| appears on the C++ stack as an argument of didAddClient() here,
but as
> ResourceClient*, not as StyleSheetResourceClient*.
> - ResourceClient is not on-heap.

You won't need Persistent<StyleSheetResourceClient>. Oilpan will keep
alive |*c| on the stack since Oilpan keeps alive all on-stack pointers
that exist in the heap region.

https://codereview.chromium.org/1976463003/

cshar...@chromium.org

unread,
Jul 26, 2016, 12:31:53 PM7/26/16
to hiro...@chromium.org, jap...@chromium.org, oilpan-...@chromium.org, har...@chromium.org, chromium...@chromium.org, tyoshin...@chromium.org, blink-rev...@chromium.org, tfa...@chromium.org, blink-rev...@chromium.org, yo...@yoav.ws, dglazko...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org
Thanks!!
On 2016/07/26 09:57:10, hiroshige wrote:
> |c| might be removed as client in didAppendFirstData() in general.
> So please check that |c| is still an client, i.e.
> if (hasClient(c) && !isLoading())

Done.


https://codereview.chromium.org/1976463003/diff/100001/third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp
File third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp
(right):

https://codereview.chromium.org/1976463003/diff/100001/third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp#newcode247
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:247:
setResource(static_cast<CSSStyleSheetResource*>(resource),
Resource::DontMarkAsReferenced);
On 2016/07/26 09:57:10, hiroshige wrote:
> We can use toCSSStyleSheetResource() instead of static_cast<>.

Done.


https://codereview.chromium.org/1976463003/diff/100001/third_party/WebKit/Source/core/html/parser/CSSPreloadScannerTest.cpp
File
third_party/WebKit/Source/core/html/parser/CSSPreloadScannerTest.cpp
(right):

https://codereview.chromium.org/1976463003/diff/100001/third_party/WebKit/Source/core/html/parser/CSSPreloadScannerTest.cpp#newcode21
third_party/WebKit/Source/core/html/parser/CSSPreloadScannerTest.cpp:21:
class PreloadSuppressingCSSPreloaderResourceClient : public
CSSPreloaderResourceClient {
On 2016/07/26 09:57:10, hiroshige wrote:

cshar...@chromium.org

unread,
Aug 1, 2016, 5:59:08 PM8/1/16
to hiro...@chromium.org, jap...@chromium.org, oilpan-...@chromium.org, har...@chromium.org, chromium...@chromium.org, tyoshin...@chromium.org, blink-rev...@chromium.org, tfa...@chromium.org, blink-rev...@chromium.org, yo...@yoav.ws, dglazko...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org
hiroshige, friendly ping :)

https://codereview.chromium.org/1976463003/

hiro...@chromium.org

unread,
Aug 3, 2016, 2:42:54 PM8/3/16
to cshar...@chromium.org, jap...@chromium.org, oilpan-...@chromium.org, har...@chromium.org, chromium...@chromium.org, tyoshin...@chromium.org, blink-rev...@chromium.org, tfa...@chromium.org, blink-rev...@chromium.org, yo...@yoav.ws, dglazko...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org
core/fetch non-OWNER lgtm.
Particularly, I'm not sure about core/html/parser part.

Some bots are red and compile errors looks relevant. Probably we need to add
export-related keywords somewhere.


https://codereview.chromium.org/1976463003/

cshar...@chromium.org

unread,
Aug 3, 2016, 6:55:18 PM8/3/16
to hiro...@chromium.org, jap...@chromium.org, oilpan-...@chromium.org, har...@chromium.org, chromium...@chromium.org, tyoshin...@chromium.org, blink-rev...@chromium.org, tfa...@chromium.org, blink-rev...@chromium.org, yo...@yoav.ws, dglazko...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org
Thanks hiroshige. I added CORE_EXPORT to StyleSheetResourceClient, let's see
what bots think.

japhet@, mind doing a pass?

https://codereview.chromium.org/1976463003/

cshar...@chromium.org

unread,
Aug 10, 2016, 9:24:53 AM8/10/16
to hiro...@chromium.org, jap...@chromium.org, oilpan-...@chromium.org, har...@chromium.org, chromium...@chromium.org, tyoshin...@chromium.org, blink-rev...@chromium.org, tfa...@chromium.org, blink-rev...@chromium.org, yo...@yoav.ws, dglazko...@chromium.org, gavinp...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org
Reply all
Reply to author
Forward
0 new messages