MHTML saving optimization: don't generate CSS code that will be discarded.

MHTML saving optimization: don't generate CSS code that will be discarded.

FrameSerializer::serializeCSSStyleSheet code was generating CSS code for
all style elements in a frame even if they were already included in the
main HTML body generation executed earlier on in
FrameSerializer::serializeFrame. This generated code was simply
discarded later on.

This change detects early on if generating the CSS code will be really needed
and skips it if not. Also, when the CSS can be uniquely identified by its URL
and that URL was already analyzed before the method simply returns.

Tests on a Samsung Galaxy J1 loading a CSS-heavy web page [1] showed roughly
40% less CSS code generated and 20% less accumulative time taken from all
render processes' main threads for the whole save operation.

[1] http://www.creativebloq.com/web-design/examples-css-912710

BUG=651525
Committed: https://crrev.com/f72e2c5e69ca7a44db46622e96f2eaab2849d70a
Cr-Commit-Position: refs/heads/master@{#424032}

[carlosk, 2016-10-06 02:56:41]

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

[commit-bot: I haz the power, 2016-10-06 02:57:04]

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

[carlosk, 2016-10-06 02:59:02]

carlosk@chromium.org changed reviewers:
+ lukasza@chromium.org, tkent@chromium.org

[carlosk, 2016-10-06 02:59:02]

tkent@, lukasza@: PTAL.

[commit-bot: I haz the power, 2016-10-06 03:13:02]

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

[commit-bot: I haz the power, 2016-10-06 03:13:02]

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

[carlosk, 2016-10-06 03:19:50]

Description was changed from

==========
MHTML saving optimization: don't generate CSS code that will be discarded.

FrameSerializer::serializeCSSStyleSheet code was generating CSS code for
all style elements in a frame even if they were already included in the
main HTML body generation executed earlier on in
FrameSerializer::serializeFrame. This generated code was simply
discarded later on.

So this change detects earlier on if the CSS code will be indeed needed
and skips it entirely if not. The only operation still done, and
required, is the parsing of sub items for fetching sub-resources.

Tests on an example CSS-heavy web page [1] showed roughly 40% less CSS code
generated and 20% less accumulative time taken from all render processes'
main threads for the whole save operation.

[1] http://www.creativebloq.com/web-design/examples-css-912710

BUG=651525
==========

to

==========
MHTML saving optimization: don't generate CSS code that will be discarded.

FrameSerializer::serializeCSSStyleSheet code was generating CSS code for
all style elements in a frame even if they were already included in the
main HTML body generation executed earlier on in
FrameSerializer::serializeFrame. This generated code was simply
discarded later on.

So this change detects earlier on if the CSS code will be indeed needed
and skips it entirely if not. The only operation still done, and
required, is the parsing of sub items for fetching sub-resources.

Tests on a Samsung Galaxy J1 loading a CSS-heavy web page [1] showed roughly
40% less CSS code generated and 20% less accumulative time taken from all
render processes' main threads for the whole save operation.

[1] http://www.creativebloq.com/web-design/examples-css-912710

BUG=651525
==========

[carlosk, 2016-10-06 03:22:54]

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

[commit-bot: I haz the power, 2016-10-06 03:23:13]

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

[carlosk, 2016-10-06 03:23:13]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-10-06 05:08:05]

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

[commit-bot: I haz the power, 2016-10-06 05:08:06]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-10-06 17:21:56]

Thanks carlosk@ - this looks like a reasonable optimization (one that we
possible should have made some time ago...).  I just have a question if we can
be more aggressive with the optimization - please see my inline comment below.

https://codereview.chromium.org/2397983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right):

https://codereview.chromium.org/2397983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:371: // Even if no text
is needed, associated resources must be fetched.
Today if 2 frames serialize CSS with the same URIs, then the second one will be
assumed to be identical to the first one (and therefore we won't serialize it
again).  "assumed to be identical" should also cover which resources are
associated with the CSS, right?  In other words - if identical-URI means
identical-CSS (including associated resources), then we can just return early,
right?

Did I get the above right?  Or are there still cases where we might need to
serialize CSS's subresources, despite the fact that a CSS with the same URI was
already serialized in a different frame?

[carlosk, 2016-10-06 18:07:25]

Thanks.

https://codereview.chromium.org/2397983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right):

https://codereview.chromium.org/2397983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:371: // Even if no text
is needed, associated resources must be fetched.
On 2016/10/06 17:21:55, Łukasz Anforowicz wrote:
> Today if 2 frames serialize CSS with the same URIs, then the second one will
be
> assumed to be identical to the first one (and therefore we won't serialize it
> again).  "assumed to be identical" should also cover which resources are
> associated with the CSS, right?  In other words - if identical-URI means
> identical-CSS (including associated resources), then we can just return early,
> right?

I think the process you are describing is the combination of checking with the
frame-local resource URIs stored in |m_resourceURLs| and URIs collected from
previously serialized resources from other frames checked by
|m_delegate.shouldSkipResourceWithURL| (which auto-populates itself at each
check and is shared future renderers). Both of these happen inside
|shouldAddURL| and in a few other places that seem to cover all cases where CSS
resources would be serialized again. 

There tests in FrameSerializerTest that try to identify issues with this logic.

My changes shouldn't affect this mechanism.

> Did I get the above right?  Or are there still cases where we might need to
> serialize CSS's subresources, despite the fact that a CSS with the same URI
was
> already serialized in a different frame?

Now I don't know the answer for this question. I am not an expert in CSS and
even less in all the crazy ways it is used in the Web nowadays. I can only say
that the tests I mentioned above seem to be correct and would catch regressions
in the covered use cases.

[carlosk, 2016-10-07 18:43:52]

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

[commit-bot: I haz the power, 2016-10-07 18:44:24]

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

[carlosk, 2016-10-07 18:47:29]

Thanks.

https://codereview.chromium.org/2397983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right):

https://codereview.chromium.org/2397983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:371: // Even if no text
is needed, associated resources must be fetched.
On 2016/10/06 18:07:24, carlosk wrote:
> On 2016/10/06 17:21:55, Łukasz Anforowicz wrote:
> > Today if 2 frames serialize CSS with the same URIs, then the second one will
> be
> > assumed to be identical to the first one (and therefore we won't serialize
it
> > again).  "assumed to be identical" should also cover which resources are
> > associated with the CSS, right?  In other words - if identical-URI means
> > identical-CSS (including associated resources), then we can just return
early,
> > right?
> 
> I think the process you are describing is the combination of checking with the
> frame-local resource URIs stored in |m_resourceURLs| and URIs collected from
> previously serialized resources from other frames checked by
> |m_delegate.shouldSkipResourceWithURL| (which auto-populates itself at each
> check and is shared future renderers). Both of these happen inside
> |shouldAddURL| and in a few other places that seem to cover all cases where
CSS
> resources would be serialized again. 
> 
> There tests in FrameSerializerTest that try to identify issues with this
logic.
> 
> My changes shouldn't affect this mechanism.
> 
> > Did I get the above right?  Or are there still cases where we might need to
> > serialize CSS's subresources, despite the fact that a CSS with the same URI
> was
> > already serialized in a different frame?
> 
> Now I don't know the answer for this question. I am not an expert in CSS and
> even less in all the crazy ways it is used in the Web nowadays. I can only say
> that the tests I mentioned above seem to be correct and would catch
regressions
> in the covered use cases.

After an offline talk I understood lukasza@'s suggestion of completely avoiding
re-analyzing an already visited CSS. This latest patch tries to do that.

[Łukasz Anforowicz, 2016-10-07 19:07:49]

Thanks - LGTM.

I wonder if the 2nd paragraph of CL description should be reworded after your
recent changes, or if it can stay as-is.

https://codereview.chromium.org/2397983002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right):

https://codereview.chromium.org/2397983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:331: bool acceptableUrl
= url.isValid() && !url.protocolIsData();
nit: Maybe add a comment what is the meaning of |acceptableUrl| OR maybe rename
it to isExternalCssResource or isNotInlineStyle?

https://codereview.chromium.org/2397983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:377: } else {
nit: Maybe code would be cleaner if the loop below would execute unconditionally
(i.e. if the loop would be moved out of "else" body / if the "else" part of "if"
would go away + avoid calling |serializeCSSRule(rule)| in the main "if" body). 
This might also mean no need for a comment explaining why the loop is needed in
the "else" part.  WDYT?

https://codereview.chromium.org/2397983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:378: // Even if no text
is needed, associated resources must be fetched.
nit: I am not sure if this comment will be easy to understand for other
maintainers.  Maybe expand into: "We need to serialize subresources of CSS, even
if the CSS resource itself doesn't need to be serialized (e.g. in cases when
this CSS comes from an inline <style> tag or from a data: URI)".

[commit-bot: I haz the power, 2016-10-07 19:32:28]

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

[commit-bot: I haz the power, 2016-10-07 19:32:29]

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

[carlosk, 2016-10-07 21:02:58]

Patchset #3 (id:60001) has been deleted

[carlosk, 2016-10-07 21:03:30]

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

[commit-bot: I haz the power, 2016-10-07 21:04:10]

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

[carlosk, 2016-10-07 21:42:00]

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

[commit-bot: I haz the power, 2016-10-07 21:42:43]

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

[carlosk, 2016-10-07 21:51:57]

Description was changed from

==========
MHTML saving optimization: don't generate CSS code that will be discarded.

FrameSerializer::serializeCSSStyleSheet code was generating CSS code for
all style elements in a frame even if they were already included in the
main HTML body generation executed earlier on in
FrameSerializer::serializeFrame. This generated code was simply
discarded later on.

So this change detects earlier on if the CSS code will be indeed needed
and skips it entirely if not. The only operation still done, and
required, is the parsing of sub items for fetching sub-resources.

Tests on a Samsung Galaxy J1 loading a CSS-heavy web page [1] showed roughly
40% less CSS code generated and 20% less accumulative time taken from all
render processes' main threads for the whole save operation.

[1] http://www.creativebloq.com/web-design/examples-css-912710

BUG=651525
==========

to

==========
MHTML saving optimization: don't generate CSS code that will be discarded.

FrameSerializer::serializeCSSStyleSheet code was generating CSS code for
all style elements in a frame even if they were already included in the
main HTML body generation executed earlier on in
FrameSerializer::serializeFrame. This generated code was simply
discarded later on.

This change detects early on if generating the CSS code will be really needed
and skips it if not. Also, when the CSS can be uniquely identified by its URL
and that URL was already analyzed before the method simply returns.

Tests on a Samsung Galaxy J1 loading a CSS-heavy web page [1] showed roughly
40% less CSS code generated and 20% less accumulative time taken from all
render processes' main threads for the whole save operation.

[1] http://www.creativebloq.com/web-design/examples-css-912710

BUG=651525
==========

[carlosk, 2016-10-07 21:54:24]

Thanks! I also updated this change description given the latest changes.

https://codereview.chromium.org/2397983002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right):

https://codereview.chromium.org/2397983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:331: bool acceptableUrl
= url.isValid() && !url.protocolIsData();
On 2016/10/07 19:07:48, Łukasz Anforowicz wrote:
> nit: Maybe add a comment what is the meaning of |acceptableUrl| OR maybe
rename
> it to isExternalCssResource or isNotInlineStyle?

Done.

https://codereview.chromium.org/2397983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:377: } else {
On 2016/10/07 19:07:48, Łukasz Anforowicz wrote:
> nit: Maybe code would be cleaner if the loop below would execute
unconditionally
> (i.e. if the loop would be moved out of "else" body / if the "else" part of
"if"
> would go away + avoid calling |serializeCSSRule(rule)| in the main "if" body).

> This might also mean no need for a comment explaining why the loop is needed
in
> the "else" part.  WDYT?

I considered that but thought it would make the method look more confusing.
There is already a good number of if blocks in it and this change would increase
it by two: one inside the for-loop and another following it (we would need 3 in
total for not generating CSS code).

These checks would even make the method longer (actual code lines) when compared
to repeating the loop here.

So I'm leaving this one as is.

https://codereview.chromium.org/2397983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:378: // Even if no text
is needed, associated resources must be fetched.
On 2016/10/07 19:07:48, Łukasz Anforowicz wrote:
> nit: I am not sure if this comment will be easy to understand for other
> maintainers.  Maybe expand into: "We need to serialize subresources of CSS,
even
> if the CSS resource itself doesn't need to be serialized (e.g. in cases when
> this CSS comes from an inline <style> tag or from a data: URI)".

Indeed, this was unclear. I improved the overall comments of the method
including this one. Done.

[carlosk, 2016-10-07 22:00:26]

carlosk@chromium.org changed reviewers:
+ dcheng@chromium.org

[carlosk, 2016-10-07 22:00:27]

dcheng@: PTAL.

Sorry for not waiting for your review tkent@. I'm trying to still get this in
early enough to merge to m55.

[dcheng, 2016-10-07 22:34:28]

LGTM with an optional nit.

Also, I talked with lukasza and it apepars that this is only partially covered
by tests... it would be nice to have more tests. I would be OK with a followup.

https://codereview.chromium.org/2397983002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right):

https://codereview.chromium.org/2397983002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:367: // Some rules have
resources associated with them that we need to
Nit: maybe just do this in one for loop at the end at line 381? We want this for
both branches anyway.

[carlosk, 2016-10-07 23:13:18]

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

[commit-bot: I haz the power, 2016-10-07 23:13:40]

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

[carlosk, 2016-10-07 23:16:07]

Thanks. I'll update tests to cover this case in a follow up CL.

https://codereview.chromium.org/2397983002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right):

https://codereview.chromium.org/2397983002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameSerializer.cpp:367: // Some rules have
resources associated with them that we need to
On 2016/10/07 22:34:28, dcheng wrote:
> Nit: maybe just do this in one for loop at the end at line 381? We want this
for
> both branches anyway.

Yes, done. I talked to lukasza@ and understood his previous suggestion of doing
that.

[commit-bot: I haz the power, 2016-10-08 00:42:17]

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

[commit-bot: I haz the power, 2016-10-08 00:42:18]

Dry run: This issue passed the CQ dry run.

[carlosk, 2016-10-08 00:59:53]

The CQ bit was checked by carlosk@chromium.org

[carlosk, 2016-10-08 00:59:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
lukasza@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2397983002/#ps120001
(title: "Simplified code as lukasza@ suggested.")

[commit-bot: I haz the power, 2016-10-08 01:00:14]

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-10-08 01:05:49]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-10-08 01:07:35]

Description was changed from

==========
MHTML saving optimization: don't generate CSS code that will be discarded.

FrameSerializer::serializeCSSStyleSheet code was generating CSS code for
all style elements in a frame even if they were already included in the
main HTML body generation executed earlier on in
FrameSerializer::serializeFrame. This generated code was simply
discarded later on.

This change detects early on if generating the CSS code will be really needed
and skips it if not. Also, when the CSS can be uniquely identified by its URL
and that URL was already analyzed before the method simply returns.

Tests on a Samsung Galaxy J1 loading a CSS-heavy web page [1] showed roughly
40% less CSS code generated and 20% less accumulative time taken from all
render processes' main threads for the whole save operation.

[1] http://www.creativebloq.com/web-design/examples-css-912710

BUG=651525
==========

to

==========
MHTML saving optimization: don't generate CSS code that will be discarded.

FrameSerializer::serializeCSSStyleSheet code was generating CSS code for
all style elements in a frame even if they were already included in the
main HTML body generation executed earlier on in
FrameSerializer::serializeFrame. This generated code was simply
discarded later on.

This change detects early on if generating the CSS code will be really needed
and skips it if not. Also, when the CSS can be uniquely identified by its URL
and that URL was already analyzed before the method simply returns.

Tests on a Samsung Galaxy J1 loading a CSS-heavy web page [1] showed roughly
40% less CSS code generated and 20% less accumulative time taken from all
render processes' main threads for the whole save operation.

[1] http://www.creativebloq.com/web-design/examples-css-912710

BUG=651525

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

[commit-bot: I haz the power, 2016-10-08 01:07:36]

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