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.

BUG=596676
Committed: https://crrev.com/c3698d693def17f7f4c2d17f462de63ee06fe10d
Cr-Commit-Position: refs/heads/master@{#390678}

[Charlie Harrison, 2016-03-21 21:50:53]

Description was changed from

==========
[TEST] preload scan css for @import

BUG=
==========

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=596676
==========

[Charlie Harrison, 2016-03-22 03:51:37]

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

[commit-bot: I haz the power, 2016-03-22 03:51:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819593002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819593002/80001

[commit-bot: I haz the power, 2016-03-22 04:15:45]

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

[commit-bot: I haz the power, 2016-03-22 04:15:46]

Dry run: Try jobs failed on following builders:
  linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)

[Charlie Harrison, 2016-03-29 14:43:19]

csharrison@chromium.org changed reviewers:
+ japhet@chromium.org, yoav@yoav.ws

[Charlie Harrison, 2016-03-29 14:43:20]

Does the architecture of this look sound? I'm worried that adding a
ResourceClient to all CSS preloads will violate some aspect of "was this preload
used?" logic.

Initial lab testing shows good results, though they are noisy. The biggest wins
will be for large CSS files under heavy network load.

https://codereview.chromium.org/1819593002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1819593002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:53:
CSSPreloadImport status=experimental
This is just for A/B testing using WebPageTest. I imagine we wouldn't have to
launch this under a feature flag.

[Yoav Weiss, 2016-03-30 21:38:31]

All in all, this looks fine, but I'm not sure about the "was preload used?"
question. I'll dig further tomorrow.

https://codereview.chromium.org/1819593002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/preload/external_css_import_preload.html
(right):

https://codereview.chromium.org/1819593002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/preload/external_css_import_preload.html:12:
var entry = performance.getEntriesByType("resource")[3];
You need to make sure that the entry you have here is the right resource,
otherwise, the test runs a risk of being fragile.

https://codereview.chromium.org/1819593002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/TextResource.cpp (left):

https://codereview.chromium.org/1819593002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/TextResource.cpp:35: ASSERT(m_data);
ASSERT that position is not null

https://codereview.chromium.org/1819593002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.h (right):

https://codereview.chromium.org/1819593002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.h:42:
DISALLOW_NEW();
spurious change or is the order here meaningful?

[Charlie Harrison, 2016-03-31 03:59:58]

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

[Charlie Harrison, 2016-03-31 03:59:59]

+kouhei@ I think this solution may replace some of the the need for a streaming
CSS parser?

https://codereview.chromium.org/1819593002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/TextResource.cpp (left):

https://codereview.chromium.org/1819593002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/TextResource.cpp:35: ASSERT(m_data);
On 2016/03/30 at 21:38:31, Yoav Weiss wrote:
> ASSERT that position is not null

Done.

https://codereview.chromium.org/1819593002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.h (right):

https://codereview.chromium.org/1819593002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.h:42:
DISALLOW_NEW();
On 2016/03/30 at 21:38:31, Yoav Weiss wrote:
> spurious change or is the order here meaningful?

Oops, spurious. Sorry.

[kouhei (in TOK), 2016-04-01 05:15:37]

If not in a hurry, can we discuss this design at the CAM meet up?

On 2016/03/31 03:59:59, csharrison wrote:
> +kouhei@ I think this solution may replace some of the the need for a
streaming
> CSS parser?
I'm not sure how this relates to the streaming CSS parser. The streaming CSS
parser is to avoid blocking main thread for a long time.

[kinuko, 2016-04-01 05:19:18]

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

[kinuko, 2016-04-01 05:19:19]

If kouhei@ will be discussing this when he's in town that'd be great.  I have
some general drive-by comment:

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

https://codereview.chromium.org/1819593002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:111:
c->didAppendData(this);
So we call streamDecodedText in each appendData?  The code looks simple enough
but are we sure that this wouldn't add non-trivial computational/memory overhead
in worst cases?

(As has been commented in each experiment) in general it'd be good to make it
clearer what metrics we'll be watching to make sure this doesn't add regression.

[Charlie Harrison, 2016-04-01 13:07:18]

kouhei@, yeah happy to chat about this in CAM!

A full streaming css parser is good, but my impression was that from 558105 and
512705 a big issue is delayed fetch of resources (using import and font-face). A
streaming scanner is probably a lower hurdle to solve this than a streaming
parser.

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

https://codereview.chromium.org/1819593002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:111:
c->didAppendData(this);
On 2016/04/01 05:19:19, kinuko wrote:
> So we call streamDecodedText in each appendData?  The code looks simple enough
> but are we sure that this wouldn't add non-trivial computational/memory
overhead
> in worst cases?
> 
> (As has been commented in each experiment) in general it'd be good to make it
> clearer what metrics we'll be watching to make sure this doesn't add
regression.

Yeah it's non ideal, but I couldn't see another way to avoid the copy without
just blindly trusting that the css is ascii. For metrics, Bryan is developing
parse time metrics which we'd look for for a regression.

[Charlie Harrison, 2016-04-01 16:57:39]

bmcquade@ and I discussed this design especially wrt cpu/memory cost and we
decided it would probably be okay to stop streaming data to the scanner after
import rules. This would probably mean only scanning one appendData call.

If we decide to add more functionality to the scanner, we can revisit if
streaming more of the file is necessary.

[Charlie Harrison, 2016-04-11 13:58:07]

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

[Charlie Harrison, 2016-04-11 13:58:08]

FYI I just met with jkarlin@ and kouhei@. The consensus was to move forward with
this patch, while only scanning the first chunk of data after the first
appendData call. I will also explore sending the undecoded char* to the scanner
to avoid a copy.

We'll then hook this up into finch.

+hiroshige@, this patch is a bit stale wrt your changes to ResourceClient, but I
was wondering if you could look over the general approach of adding a
ResourceClient to all CSS preloads. Will that mess up any stats gathering that
we're doing (i.e. was the preload a "hit" or "miss"). Thanks!

[hiroshige, 2016-04-14 08:18:22]

On 2016/04/11 13:58:08, csharrison wrote:
> +hiroshige@, this patch is a bit stale wrt your changes to ResourceClient, but
I
> was wondering if you could look over the general approach of adding a
> ResourceClient to all CSS preloads. Will that mess up any stats gathering that
> we're doing (i.e. was the preload a "hit" or "miss"). Thanks!

Could you
1. Add |void CSSPreloaderResourceClient::notifyFinished(Resource*) override| and
call ResourceClient::removeClient() there
   (to ensure CSSPreloaderResourceClient is not registered as a client when
CSSPreloaderResourceClient is destructed to avoid use-after-free), and
2. Add a reference from CSSPreloaderResourceClient to Resource (e.g. |Resource*
CSSPreloaderResourceClient::m_resource|)
   (We loosely assume ResourceClient has a reference to Resource while
registered as a client)?
These two will make CSSPreloaderResourceClient to do what ordinary
ResourceClient does.

[Charlie Harrison, 2016-04-14 13:55:06]

On 2016/04/14 at 08:18:22, hiroshige wrote:
> On 2016/04/11 13:58:08, csharrison wrote:
> > +hiroshige@, this patch is a bit stale wrt your changes to ResourceClient,
but I
> > was wondering if you could look over the general approach of adding a
> > ResourceClient to all CSS preloads. Will that mess up any stats gathering
that
> > we're doing (i.e. was the preload a "hit" or "miss"). Thanks!
> 
> Could you
> 1. Add |void CSSPreloaderResourceClient::notifyFinished(Resource*) override|
and call ResourceClient::removeClient() there
>    (to ensure CSSPreloaderResourceClient is not registered as a client when
CSSPreloaderResourceClient is destructed to avoid use-after-free), and
Sounds good.
> 2. Add a reference from CSSPreloaderResourceClient to Resource (e.g.
|Resource* CSSPreloaderResourceClient::m_resource|)
>    (We loosely assume ResourceClient has a reference to Resource while
registered as a client)?
> These two will make CSSPreloaderResourceClient to do what ordinary
ResourceClient does.
Why Resource*? If the ResourceClient and the Resource are both on the oilpan
heap, shouldn't we use Member?

[Charlie Harrison, 2016-04-14 14:16:02]

On 2016/04/14 at 13:55:06, csharrison wrote:
> On 2016/04/14 at 08:18:22, hiroshige wrote:
> > On 2016/04/11 13:58:08, csharrison wrote:
> > > +hiroshige@, this patch is a bit stale wrt your changes to ResourceClient,
but I
> > > was wondering if you could look over the general approach of adding a
> > > ResourceClient to all CSS preloads. Will that mess up any stats gathering
that
> > > we're doing (i.e. was the preload a "hit" or "miss"). Thanks!
> > 
> > Could you
> > 1. Add |void CSSPreloaderResourceClient::notifyFinished(Resource*) override|
and call ResourceClient::removeClient() there
> >    (to ensure CSSPreloaderResourceClient is not registered as a client when
CSSPreloaderResourceClient is destructed to avoid use-after-free), and
> Sounds good.
> > 2. Add a reference from CSSPreloaderResourceClient to Resource (e.g.
|Resource* CSSPreloaderResourceClient::m_resource|)
> >    (We loosely assume ResourceClient has a reference to Resource while
registered as a client)?
> > These two will make CSSPreloaderResourceClient to do what ordinary
ResourceClient does.
> Why Resource*? If the ResourceClient and the Resource are both on the oilpan
heap, shouldn't we use Member?

I just realized ResourceClient is not on oilpan yet. For some reason I thought
that work was finished.

[hiroshige, 2016-04-14 14:56:13]

On 2016/04/14 14:16:02, csharrison wrote:
> On 2016/04/14 at 13:55:06, csharrison wrote:
> > On 2016/04/14 at 08:18:22, hiroshige wrote:
> > > On 2016/04/11 13:58:08, csharrison wrote:
> > > > +hiroshige@, this patch is a bit stale wrt your changes to
ResourceClient,
> but I
> > > > was wondering if you could look over the general approach of adding a
> > > > ResourceClient to all CSS preloads. Will that mess up any stats
gathering
> that
> > > > we're doing (i.e. was the preload a "hit" or "miss"). Thanks!
> > > 
> > > Could you
> > > 1. Add |void CSSPreloaderResourceClient::notifyFinished(Resource*)
override|
> and call ResourceClient::removeClient() there
> > >    (to ensure CSSPreloaderResourceClient is not registered as a client
when
> CSSPreloaderResourceClient is destructed to avoid use-after-free), and
> > Sounds good.
> > > 2. Add a reference from CSSPreloaderResourceClient to Resource (e.g.
> |Resource* CSSPreloaderResourceClient::m_resource|)
> > >    (We loosely assume ResourceClient has a reference to Resource while
> registered as a client)?
> > > These two will make CSSPreloaderResourceClient to do what ordinary
> ResourceClient does.
> > Why Resource*? If the ResourceClient and the Resource are both on the oilpan
> heap, shouldn't we use Member?
> 
> I just realized ResourceClient is not on oilpan yet. For some reason I thought
> that work was finished.
Oh, sorry, Persistent<Resource>.

(In general, removeClient() should be called before CSSPreloaderResourceClient
is destructed.)

[Charlie Harrison, 2016-04-14 16:04:04]

Thanks for the review. I've updated the patch set to include a histogram
tracking the number of preloads. The update also only appends data once, then
removes the resource client.

[Charlie Harrison, 2016-04-19 13:21:06]

ping kinuko@ and kouhei@ now that the implementation for this involves only one
read per resource.

My plan for experimentation is to have 3 groups:
1. Control
2. Scanning but not preloading
3. Scanning and preloading

Comparing 1 and 2 will give us the overhead we're incurring. Comparing 2 and 3
(when a @import is scanned), will give us benefits of using the scanner and
relative frequency.

[kouhei (in TOK), 2016-04-21 06:35:03]

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

https://codereview.chromium.org/1819593002/diff/200002/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:106:
c->didAppendData(this);
I'd prefer if the hook is only called on the first appendData. Something like
didFirstAppendData(this)?

https://codereview.chromium.org/1819593002/diff/200002/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/StyleSheetResourceClient.h (right):

https://codereview.chromium.org/1819593002/diff/200002/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/StyleSheetResourceClient.h:43: virtual void
didAppendData(const CSSStyleSheetResource*) {}
const CSSStyleSheetResouce* -> CSSStyleSheetResource&

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

https://codereview.chromium.org/1819593002/diff/200002/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:254: const
String& chunk = resource->decodedText();
Sorry, I was surprised to know that decodedText() is const member func.
decodedText() shouldn't be const, as it actually mutates TextResource internal
state. Let me fix this in separate CL.

[kouhei (in TOK), 2016-04-24 22:26:22]

https://codereview.chromium.org/1819593002/diff/200002/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/StyleSheetResourceClient.h (right):

https://codereview.chromium.org/1819593002/diff/200002/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/StyleSheetResourceClient.h:43: virtual void
didAppendData(const CSSStyleSheetResource*) {}
On 2016/04/21 06:35:03, kouhei wrote:
> const CSSStyleSheetResouce* -> CSSStyleSheetResource&
Retracted per next comment.

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

https://codereview.chromium.org/1819593002/diff/200002/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:254: const
String& chunk = resource->decodedText();
On 2016/04/21 06:35:03, kouhei wrote:
> Sorry, I was surprised to know that decodedText() is const member func.
> decodedText() shouldn't be const, as it actually mutates TextResource internal
> state. Let me fix this in separate CL.
Hmm, thinking this over again, TextResource::decodedText modifies its
TextResourceDecoder state, but it always flush()s it in the end to get it back
to its initial state.

Let me retract the previous comment.

[Charlie Harrison, 2016-04-25 15:14:33]

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

[Charlie Harrison, 2016-04-25 15:14:35]

+isherman for histograms.

I updated the CL to use didAppendFirstData.

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

https://codereview.chromium.org/1819593002/diff/200002/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:106:
c->didAppendData(this);
On 2016/04/21 06:35:03, kouhei wrote:
> I'd prefer if the hook is only called on the first appendData. Something like
> didFirstAppendData(this)?

Done.

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

https://codereview.chromium.org/1819593002/diff/200002/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:254: const
String& chunk = resource->decodedText();
On 2016/04/24 at 22:26:22, kouhei wrote:
> On 2016/04/21 06:35:03, kouhei wrote:
> > Sorry, I was surprised to know that decodedText() is const member func.
> > decodedText() shouldn't be const, as it actually mutates TextResource
internal
> > state. Let me fix this in separate CL.
> Hmm, thinking this over again, TextResource::decodedText modifies its
TextResourceDecoder state, but it always flush()s it in the end to get it back
to its initial state.
> 
> Let me retract the previous comment.

Ack. Thanks for looking into this.

[Ilya Sherman, 2016-04-25 15:48:35]

https://codereview.chromium.org/1819593002/diff/230001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1819593002/diff/230001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:6747: +<histogram
name="CSSPreloaderResourceClient.PreloadCount" units="preloads">
It looks like you're adding a new top-level group name,
"CSSPreloaderResourceClient".  Would it make sense to group this histogram under
one of the existing top-level groups, such as "PreloadScanner" or
"Webcore.Preload"?  If not, could you please think about whether there's a way
to generalize this top-level name somewhat, so that it better corresponds to a
top-level Chrome/Blink concept?  For example, "Preload.CSS" or "CSSPreloader"
might be better as top-level group names.

[kouhei (in TOK), 2016-04-25 22:10:05]

lgtm from my side

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

https://codereview.chromium.org/1819593002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:258: //
TODO(csharrison): Should SegmentedString be populated?
The only caveat of using empty SegmentedString here is that we will get
PreloadRequests without source file/line info. I'm not sure how important that
is.

[Charlie Harrison, 2016-04-25 22:29:12]

Thanks!

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

https://codereview.chromium.org/1819593002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:258: //
TODO(csharrison): Should SegmentedString be populated?
On 2016/04/25 at 22:10:05, kouhei wrote:
> The only caveat of using empty SegmentedString here is that we will get
PreloadRequests without source file/line info. I'm not sure how important that
is.

Yeah I know :/ I think leaving it out is okay, because @import should be the
first thing in the file besides charset. We could include it, but it would
entail a copy.

I'll leave it out for now and we can revisit if we decide to land this
experiment.

https://codereview.chromium.org/1819593002/diff/230001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1819593002/diff/230001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:6747: +<histogram
name="CSSPreloaderResourceClient.PreloadCount" units="preloads">
On 2016/04/25 at 15:48:35, Ilya Sherman wrote:
> It looks like you're adding a new top-level group name,
"CSSPreloaderResourceClient".  Would it make sense to group this histogram under
one of the existing top-level groups, such as "PreloadScanner" or
"Webcore.Preload"?  If not, could you please think about whether there's a way
to generalize this top-level name somewhat, so that it better corresponds to a
top-level Chrome/Blink concept?  For example, "Preload.CSS" or "CSSPreloader"
might be better as top-level group names.

You're completely right. I think was in the TRACE_EVENT style naming mode. I've
renamed to "PreloadScanner.ExternalCSS.PreloadCount".

[Charlie Harrison, 2016-04-28 20:41:57]

kinuko@ do you support moving forward with the plan to experiment in #23?

[kinuko, 2016-04-29 03:30:09]

lgtm

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

https://codereview.chromium.org/1819593002/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:258: //
TODO(csharrison): Should SegmentedString be populated?
nit: make this a TODO-like comment with a bit more context, e.g. "we pass empty
SegmentedString here, which will make preload requests without file/line info. 
TODO(csharrison): consider populating this if it becomes a problem." or
something like that

https://codereview.chromium.org/1819593002/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:265:
m_resource->removeClient(this);
nit: should we also clear m_resource here (and in notifyFinished)?

[Charlie Harrison, 2016-04-29 13:21:39]

Thanks!

isherman@ can you take a look at the update histogram name?

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

https://codereview.chromium.org/1819593002/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:258: //
TODO(csharrison): Should SegmentedString be populated?
On 2016/04/29 03:30:08, kinuko wrote:
> nit: make this a TODO-like comment with a bit more context, e.g. "we pass
empty
> SegmentedString here, which will make preload requests without file/line info.

> TODO(csharrison): consider populating this if it becomes a problem." or
> something like that

Done.

https://codereview.chromium.org/1819593002/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:265:
m_resource->removeClient(this);
On 2016/04/29 03:30:08, kinuko wrote:
> nit: should we also clear m_resource here (and in notifyFinished)?

Done.

[Ilya Sherman, 2016-04-29 15:46:49]

Histogram LGTM, thanks.

[Charlie Harrison, 2016-04-29 17:01:03]

Thanks!

[Charlie Harrison, 2016-04-29 17:01:10]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-04-29 17:01:11]

The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org,
kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1819593002/#ps270001
(title: "kinuko@ review: updated comment and clear m_resource when finished")

[commit-bot: I haz the power, 2016-04-29 17:01:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819593002/270001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819593002/270001

[commit-bot: I haz the power, 2016-04-29 17:07:30]

Committed patchset #15 (id:270001)

[commit-bot: I haz the power, 2016-04-30 17:26:38]

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

[commit-bot: I haz the power, 2016-04-30 18:01:09]

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=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.

BUG=596676

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

[Łukasz Anforowicz, 2016-05-03 20:35:56]

On 2016/04/30 17:26:38, commit-bot: I haz the power wrote:
> Patchset 15 (id:??) landed as
> https://crrev.com/c3698d693def17f7f4c2d17f462de63ee06fe10d
> Cr-Commit-Position: refs/heads/master@{#390678}

This CL made http/tests/security/link-crossorigin-stylesheet-no-cors.html layout
test flaky - see https://crbug.com/608577