Preload scan external CSS for @import

Preload scan external CSS for @import

This change adds a StyleSheetResourceClient on all CSS
preloads. The client holds a CSSPreloadScanner which receives
the stream of CSS as it comes in.

This is guarded by two Blink settings,
cssExternalScannerPreload and cssExternalScannerNoPreload.
The former allows for scanning and preloading and the latter just
allows for scanning, as a baseline for the overhead of the feature.

This is partially a reland of
https://codereview.chromium.org/1819593002, which was
reverted manually here:
https://codereview.chromium.org/1947053002/

The revert was due to a crash and some faulty logic with
regard to all css preload requests being considered
"referenced". This CL attempts to fix that by adding an enum
to mark a ResourceClient as "passive".

BUG=610437, 596676
Committed: https://crrev.com/c016ef022ac17d029d61bc79d8a8996c053556f3
Cr-Commit-Position: refs/heads/master@{#414756}

[Charlie Harrison, 2016-05-12 22:28:03]

Description was changed from

==========
Preload scan external CSS for @import

This change adds a StyleSheetResourceClient on all CSS
preloads. The client holds a CSSPreloadScanner which receives
the stream of CSS as it comes in.
==========

to

==========
Preload scan external CSS for @import

This change adds a StyleSheetResourceClient on all CSS
preloads. The client holds a CSSPreloadScanner which receives
the stream of CSS as it comes in.

BUG=610437,596676
==========

[Charlie Harrison, 2016-05-12 22:28:03]

csharrison@chromium.org changed reviewers:
+ hiroshige@chromium.org

[Charlie Harrison, 2016-05-12 22:31:01]

hiroshige@, can you take a look please?

I did the easiest thing possible to add a "passive" resource client that don't
cause preloads to be referenced.

Other than that, my main worry is holding a strong reference from the client to
the preload. Maybe we just want weak references?

I'm not sure when preloads will die in the new WeakMemoryCache design.

[hiroshige, 2016-05-13 17:35:34]

On 2016/05/12 22:31:01, csharrison wrote:
> hiroshige@, can you take a look please?
> 
> I did the easiest thing possible to add a "passive" resource client that don't
> cause preloads to be referenced.
> 
> Other than that, my main worry is holding a strong reference from the client
to
> the preload. Maybe we just want weak references?
Is this about the reference from CSSPreloaderResourceClient to the preloaded
Resource?
(|CSSPreloaderResourceClient::m_resource| is a strong reference, though)

Do you have specific cases/tests where Resource should be destructed while the
preloader is attached?

I feel currently we don't have a clear way to make a "weak" ResourceClient,
because having a client itself (even the client doesn't have a strong reference
to the Resource) and MemoryCache affects the lifetime of Resource.
If we make MemoryCache Weak and ResourceClient on-heap (Issue 587663), I expect
the situation will be better.

[Charlie Harrison, 2016-05-13 18:03:04]

On 2016/05/13 17:35:34, hiroshige wrote:
> On 2016/05/12 22:31:01, csharrison wrote:
> > hiroshige@, can you take a look please?
> > 
> > I did the easiest thing possible to add a "passive" resource client that
don't
> > cause preloads to be referenced.
> > 
> > Other than that, my main worry is holding a strong reference from the client
> to
> > the preload. Maybe we just want weak references?
> Is this about the reference from CSSPreloaderResourceClient to the preloaded
> Resource?
> (|CSSPreloaderResourceClient::m_resource| is a strong reference, though)
> 
> Do you have specific cases/tests where Resource should be destructed while the
> preloader is attached?
> 
> I feel currently we don't have a clear way to make a "weak" ResourceClient,
> because having a client itself (even the client doesn't have a strong
reference
> to the Resource) and MemoryCache affects the lifetime of Resource.
> If we make MemoryCache Weak and ResourceClient on-heap (Issue 587663), I
expect
> the situation will be better.

Yeah, the current patch uses strong references. The only problem would be
keeping preloads around longer than they would normally.

AFAICT, preloads are kept alive by ResourceFetcher, which is cleared when
parsing is finished, or when the ResourceFetcher is destructed.

The resource clients in this patch are scoped to HTMLResourcePreloader, which is
also scoped to the parser. So in this case things work out okay. In fact, most
clients looking to passively observe preloads will probably be scoped in this
way too, so maybe a general weak resource client is not really needed.

[hiroshige, 2016-05-17 15:26:23]

Let me clarify the context for this CL:
The original CL [1] was landed but caused Issue 608310.
Fix for Issue 608310 was attempted in [2] but finally [1] was reverted.
This CL is the reland of [1], plus "passive" ResourceClient support.

[1] https://codereview.chromium.org/1819593002
[2] https://codereview.chromium.org/1937033002

"passive" ResourceClient support looks good with some comments.

How this CL fix Issue 608310?
I'll also look into the issue more in detail.

[Charlie Harrison, 2016-05-17 15:29:12]

On 2016/05/17 15:26:23, hiroshige wrote:
> Let me clarify the context for this CL:
> The original CL [1] was landed but caused Issue 608310.
> Fix for Issue 608310 was attempted in [2] but finally [1] was reverted.
> This CL is the reland of [1], plus "passive" ResourceClient support.
> 
> [1] https://codereview.chromium.org/1819593002
> [2] https://codereview.chromium.org/1937033002
> 
> "passive" ResourceClient support looks good with some comments.
> 
> How this CL fix Issue 608310?
> I'll also look into the issue more in detail.

Yeah the context is correct. I also switched around from using
RuntimeEnabledFeatures to Settings, because the experiment will have multiple
groups. I'll update the description with some context.

[Charlie Harrison, 2016-05-17 16:05:22]

Description was changed from

==========
Preload scan external CSS for @import

This change adds a StyleSheetResourceClient on all CSS
preloads. The client holds a CSSPreloadScanner which receives
the stream of CSS as it comes in.

BUG=610437,596676
==========

to

==========
Preload scan external CSS for @import

This change adds a StyleSheetResourceClient on all CSS
preloads. The client holds a CSSPreloadScanner which receives
the stream of CSS as it comes in.

This is a reland of 
https://codereview.chromium.org/1819593002, which was
reverted manually here:
https://codereview.chromium.org/1947053002/

The revert was due to a crash and some faulty logic with
regard to all css preload requests being considered
"referenced". This CL attempts to fix that by adding a bool
to mark a ResourceClient as "passive".

BUG=610437,596676
==========

[Charlie Harrison, 2016-05-23 18:09:38]

csharrison@chromium.org changed reviewers:
+ japhet@chromium.org

[Charlie Harrison, 2016-05-23 18:09:39]

+japhet
I had some trouble writing up a test that kills the resource client before it
gets notified for some data. Are there existing tests you can point me to as
reference?

[Nate Chapin, 2016-06-10 19:00:10]

On 2016/05/23 18:09:39, csharrison wrote:
> +japhet
> I had some trouble writing up a test that kills the resource client before it
> gets notified for some data. Are there existing tests you can point me to as
> reference?

Sorry, I missed this issue. Feel free nag me via IM or IRC if/when this happens
in the future :)

This should be pretty straightforward with a unit test.
ResourceFetcherTest.ResponseOnCancel is a decent example for doing wild things
with ResourceClient callbacks. You can probably reverse it and cancel inside of
responseReceived(), instead of trying to responseReceived() inside of
cancellation?

[hiroshige, 2016-06-17 12:15:17]

Sorry for my slow response.

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.h (right):

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.h:53: void
didAddClient(ResourceClient*) override;
We also have to modify didAddClient() if we want to call didFirstAppendData()
when a request hits cached Resource in MemoryCache.
See RawResource::didAddClient() for example.

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.h:131: void
addClient(ResourceClient*, bool passive = false);
Defining and using enum instead of |bool| will be clearer.

Also, the name |passive| looks like more general than this flag actually does --
controlling whether to update PreloadReferenced status.
More specific enum value names like |MarkAsReferenced| / |DontMarkAsReferenced|
is better.

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.h:259: void
willAddClientOrObserver(bool passive = false);
Please do not set default value for |passive| here, because
willAddClientOrObserver() is an internal method of Resource and making the
|passive| value always explicit will result in better code and easier bug
detection in the future.

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/StyleSheetResourceClient.h (right):

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/StyleSheetResourceClient.h:45: //
CSSStyleSheetResource.
Please mention that didAppendFirstData() is not called for StyleSheetResource
other than CSSStyleSheetResource, i.e. XSLStyleSheetResource.

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

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:248:
CSSPreloaderResourceClient::~CSSPreloaderResourceClient()
We don't need this destructor if we make CSSPreloaderResourceClient to be a
ResourceOwner. (clearResource() is called by ResourceOwner's prefinalizer)

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:267: void
CSSPreloaderResourceClient::clearResource()
We can remove this if we make CSSPreloaderResourceClient to be a ResourceOwner.

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

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.h:88: class
CSSPreloaderResourceClient : public StyleSheetResourceClient {
It's better to make CSSPreloaderResourceClient to inherit GarbageCollected and
ResourceOwner<StyleSheetResource>.
StyleSheetResourceClient is now GarbageCollectedMixin.

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.h:92: void
notifyFinished(Resource*) override;
notifyFinished() is not called for StyleSheetResourceClient.
setCSSStyleSheet() or setXSLStyleSheet() is called instead when finished.

[Charlie Harrison, 2016-07-13 18:18:14]

And sorry for my even slower response :)

In order to make the resource client be a ResourceOwner, I needed to thread the
PreloadReferencePolicy through the ResourceOwner code. I think this is okay, but
if you don't like it I can revert back to the client being a faux-owner.

japhet@ thanks for the pointer about the unit tests. I was originally trying to
unit test the HTMLResourcePreloader which looks like it needs a full fledged
DocumentLoader, etc. Instead I modified the code to mock out the preloading
functionality and wrote a smaller test as you hinted at.

Thanks for everyone's patience!

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.h (right):

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.h:53: void
didAddClient(ResourceClient*) override;
On 2016/06/17 at 12:15:17, hiroshige wrote:
> We also have to modify didAddClient() if we want to call didFirstAppendData()
when a request hits cached Resource in MemoryCache.
> See RawResource::didAddClient() for example.

Done.

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.h:131: void
addClient(ResourceClient*, bool passive = false);
On 2016/06/17 12:15:17, hiroshige wrote:
> Defining and using enum instead of |bool| will be clearer.
> 
> Also, the name |passive| looks like more general than this flag actually does
--
> controlling whether to update PreloadReferenced status.
> More specific enum value names like |MarkAsReferenced| /
|DontMarkAsReferenced|
> is better.

Done.

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.h:259: void
willAddClientOrObserver(bool passive = false);
On 2016/06/17 12:15:17, hiroshige wrote:
> Please do not set default value for |passive| here, because
> willAddClientOrObserver() is an internal method of Resource and making the
> |passive| value always explicit will result in better code and easier bug
> detection in the future.

Done.

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/StyleSheetResourceClient.h (right):

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/StyleSheetResourceClient.h:45: //
CSSStyleSheetResource.
On 2016/06/17 12:15:17, hiroshige wrote:
> Please mention that didAppendFirstData() is not called for StyleSheetResource
> other than CSSStyleSheetResource, i.e. XSLStyleSheetResource.

Done.

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

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:248:
CSSPreloaderResourceClient::~CSSPreloaderResourceClient()
On 2016/06/17 12:15:17, hiroshige wrote:
> We don't need this destructor if we make CSSPreloaderResourceClient to be a
> ResourceOwner. (clearResource() is called by ResourceOwner's prefinalizer)

Done.

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:267: void
CSSPreloaderResourceClient::clearResource()
On 2016/06/17 12:15:17, hiroshige wrote:
> We can remove this if we make CSSPreloaderResourceClient to be a
ResourceOwner.

Done.

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

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.h:88: class
CSSPreloaderResourceClient : public StyleSheetResourceClient {
On 2016/06/17 12:15:17, hiroshige wrote:
> It's better to make CSSPreloaderResourceClient to inherit GarbageCollected and
> ResourceOwner<StyleSheetResource>.
> StyleSheetResourceClient is now GarbageCollectedMixin.

Done.

https://codereview.chromium.org/1976463003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.h:92: void
notifyFinished(Resource*) override;
On 2016/06/17 12:15:17, hiroshige wrote:
> notifyFinished() is not called for StyleSheetResourceClient.
> setCSSStyleSheet() or setXSLStyleSheet() is called instead when finished.

Replaced it with setCSSStyleSheet().

[Charlie Harrison, 2016-07-19 23:15:07]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 23:16:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 23:19:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 23:19:39]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Harrison, 2016-07-19 23:41:59]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 23:42:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 00:35:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 00:35:15]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Charlie Harrison, 2016-07-25 16:21:49]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 16:22:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-07-25 16:30:28]

friendly ping

[commit-bot: I haz the power, 2016-07-25 16:34:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-25 16:34:44]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Harrison, 2016-07-25 16:42:33]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 16:42:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 17:51:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-25 17:51:43]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[hiroshige, 2016-07-26 09:57:09]

hiroshige@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[hiroshige, 2016-07-26 09:57:10]

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/Sou...
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp (right):

https://codereview.chromium.org/1976463003/diff/100001/third_party/WebKit/Sou...
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/Sou...
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/Sou...
File third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp (right):

https://codereview.chromium.org/1976463003/diff/100001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/core/html/parser/CSSPreloadScannerTest.cpp
(right):

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

[haraken, 2016-07-26 10:01:05]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-07-26 10:01:06]

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

https://codereview.chromium.org/1976463003/diff/100001/third_party/WebKit/Sou...
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.

[Charlie Harrison, 2016-07-26 15:59:37]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 15:59:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-07-26 16:31:52]

Thanks!!

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

https://codereview.chromium.org/1976463003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:87: if
(!isLoading())
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/Sou...
File third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp (right):

https://codereview.chromium.org/1976463003/diff/100001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/core/html/parser/CSSPreloadScannerTest.cpp
(right):

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

Done.

[commit-bot: I haz the power, 2016-07-26 16:51:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-26 16:51:54]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Charlie Harrison, 2016-08-01 21:59:08]

hiroshige, friendly ping :)

[hiroshige, 2016-08-03 18:42:53]

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.

[Charlie Harrison, 2016-08-03 22:54:44]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 22:55:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-08-03 22:55:17]

Thanks hiroshige. I added CORE_EXPORT to StyleSheetResourceClient, let's see
what bots think.

japhet@, mind doing a pass?

[commit-bot: I haz the power, 2016-08-04 00:47:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-04 00:47:56]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-08-10 13:24:53]

japhet@, friendly ping

[Nate Chapin, 2016-08-25 18:41:50]

core/fetch/ LGTM, I'll defer to others on the parser bits.

[Charlie Harrison, 2016-08-25 18:43:59]

csharrison@chromium.org changed reviewers:
+ kouhei@chromium.org

[Charlie Harrison, 2016-08-25 18:44:00]

Thanks! Note there are some fetchy things in the parser code (like
ResourceClients), so you may want to take a look at that if you're so inclined.

+kouhei@ for html/parser review.

[Charlie Harrison, 2016-08-26 04:16:37]

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

https://codereview.chromium.org/1976463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:278: const
String& chunk = resource->decodedText();
Let's add UMA here in a followup for the time it takes to perform this copy and
scan, so we know how much overhead this feature is adding.

[haraken, 2016-08-26 04:36:13]

Oilpan part LGTM

[kouhei (in TOK), 2016-08-26 06:11:09]

core lgtm

[Charlie Harrison, 2016-08-26 12:08:41]

Description was changed from

==========
Preload scan external CSS for @import

This change adds a StyleSheetResourceClient on all CSS
preloads. The client holds a CSSPreloadScanner which receives
the stream of CSS as it comes in.

This is a reland of 
https://codereview.chromium.org/1819593002, which was
reverted manually here:
https://codereview.chromium.org/1947053002/

The revert was due to a crash and some faulty logic with
regard to all css preload requests being considered
"referenced". This CL attempts to fix that by adding a bool
to mark a ResourceClient as "passive".

BUG=610437,596676
==========

to

==========
Preload scan external CSS for @import

This change adds a StyleSheetResourceClient on all CSS
preloads. The client holds a CSSPreloadScanner which receives
the stream of CSS as it comes in.

This is guarded by two Blink settings,
cssExternalScannerPreload and cssExternalScannerNoPreload.
The former allows for scanning and preloading and the latter just
allows for scanning, as a baseline for the overhead of the feature.

This is partially a reland of 
https://codereview.chromium.org/1819593002, which was
reverted manually here:
https://codereview.chromium.org/1947053002/

The revert was due to a crash and some faulty logic with
regard to all css preload requests being considered
"referenced". This CL attempts to fix that by adding an enum
to mark a ResourceClient as "passive".

BUG=610437,596676
==========

[Charlie Harrison, 2016-08-26 12:15:55]

Thanks everyone. I think I also need sign off for the changes to
public/platform.
+japhet for that, for a simple flag addition. Could you PTAL?

[Charlie Harrison, 2016-08-26 13:33:32]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 13:33:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 15:30:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 15:30:48]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2016-08-26 18:07:27]

public/platform LGTM

[Charlie Harrison, 2016-08-26 18:08:28]

Thanks, everyone! Sorry for all the delays in landing this :/

[Charlie Harrison, 2016-08-26 18:08:37]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-08-26 18:08:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, haraken@chromium.org, kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/1976463003/#ps160001
(title: "rebase + minor style tweaks")

[commit-bot: I haz the power, 2016-08-26 18:09:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 18:30:25]

Description was changed from

==========
Preload scan external CSS for @import

This change adds a StyleSheetResourceClient on all CSS
preloads. The client holds a CSSPreloadScanner which receives
the stream of CSS as it comes in.

This is guarded by two Blink settings,
cssExternalScannerPreload and cssExternalScannerNoPreload.
The former allows for scanning and preloading and the latter just
allows for scanning, as a baseline for the overhead of the feature.

This is partially a reland of 
https://codereview.chromium.org/1819593002, which was
reverted manually here:
https://codereview.chromium.org/1947053002/

The revert was due to a crash and some faulty logic with
regard to all css preload requests being considered
"referenced". This CL attempts to fix that by adding an enum
to mark a ResourceClient as "passive".

BUG=610437,596676
==========

to

==========
Preload scan external CSS for @import

This change adds a StyleSheetResourceClient on all CSS
preloads. The client holds a CSSPreloadScanner which receives
the stream of CSS as it comes in.

This is guarded by two Blink settings,
cssExternalScannerPreload and cssExternalScannerNoPreload.
The former allows for scanning and preloading and the latter just
allows for scanning, as a baseline for the overhead of the feature.

This is partially a reland of 
https://codereview.chromium.org/1819593002, which was
reverted manually here:
https://codereview.chromium.org/1947053002/

The revert was due to a crash and some faulty logic with
regard to all css preload requests being considered
"referenced". This CL attempts to fix that by adding an enum
to mark a ResourceClient as "passive".

BUG=610437,596676
==========

[commit-bot: I haz the power, 2016-08-26 18:30:27]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-08-26 18:33:04]

Description was changed from

==========
Preload scan external CSS for @import

This change adds a StyleSheetResourceClient on all CSS
preloads. The client holds a CSSPreloadScanner which receives
the stream of CSS as it comes in.

This is guarded by two Blink settings,
cssExternalScannerPreload and cssExternalScannerNoPreload.
The former allows for scanning and preloading and the latter just
allows for scanning, as a baseline for the overhead of the feature.

This is partially a reland of 
https://codereview.chromium.org/1819593002, which was
reverted manually here:
https://codereview.chromium.org/1947053002/

The revert was due to a crash and some faulty logic with
regard to all css preload requests being considered
"referenced". This CL attempts to fix that by adding an enum
to mark a ResourceClient as "passive".

BUG=610437,596676
==========

to

==========
Preload scan external CSS for @import

This change adds a StyleSheetResourceClient on all CSS
preloads. The client holds a CSSPreloadScanner which receives
the stream of CSS as it comes in.

This is guarded by two Blink settings,
cssExternalScannerPreload and cssExternalScannerNoPreload.
The former allows for scanning and preloading and the latter just
allows for scanning, as a baseline for the overhead of the feature.

This is partially a reland of
https://codereview.chromium.org/1819593002, which was
reverted manually here:
https://codereview.chromium.org/1947053002/

The revert was due to a crash and some faulty logic with
regard to all css preload requests being considered
"referenced". This CL attempts to fix that by adding an enum
to mark a ResourceClient as "passive".

BUG=610437,596676

Committed: https://crrev.com/c016ef022ac17d029d61bc79d8a8996c053556f3
Cr-Commit-Position: refs/heads/master@{#414756}
==========

[commit-bot: I haz the power, 2016-08-26 18:33:05]

Patchset 9 (id:??) landed as
https://crrev.com/c016ef022ac17d029d61bc79d8a8996c053556f3
Cr-Commit-Position: refs/heads/master@{#414756}