Retrieve resources from media and supports query rules while serializing

Retrieve resources from media and supports query rules while serializing

Traverse rules behind media and supports queries while serializing (and gathering resources
referenced from) stylesheets. Resources from non-matching queries will be
skipped as they haven't been fetched.

BUG=503919
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197813

[ingemara, 2015-06-24 11:41:32]

ingemara@opera.com changed reviewers:
+ hajimehoshi@chromium.org, philipj@opera.com

[ingemara, 2015-06-24 11:55:09]

PTAL. I based the reviewers on the last changes made to PageSerializer by tiger.
If you feel that someone else is more suitable, please suggest other reviewers.

[philipj_slow, 2015-06-24 15:30:40]

The CQ bit was checked by philipj@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-24 15:31:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1203873004/20001

[philipj_slow, 2015-06-24 15:32:23]

This looks good, but perhaps also test that "Resources from non-matching queries
will be skipped as they haven't been fetched." since that doesn't seem to be
explicit in the code. Perhaps test with some always unmatchable media query for
good measure.

[commit-bot: I haz the power, 2015-06-24 18:03:54]

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

[commit-bot: I haz the power, 2015-06-24 18:03:54]

Dry run: Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/68141)

[philipj_slow, 2015-06-24 22:18:58]

On 2015/06/24 18:03:54, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   win_blink_rel on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/68141)

You can ignore this, just another day in the life of win_blink_rel.

[hajimehoshi, 2015-06-25 02:27:05]

lgtm

https://codereview.chromium.org/1203873004/diff/20001/Source/core/page/PageSe...
File Source/core/page/PageSerializer.cpp (right):

https://codereview.chromium.org/1203873004/diff/20001/Source/core/page/PageSe...
Source/core/page/PageSerializer.cpp:328: if (rule->type() ==
CSSRule::IMPORT_RULE) {
switch-case seems better

https://codereview.chromium.org/1203873004/diff/20001/Source/core/page/PageSe...
Source/core/page/PageSerializer.cpp:343: for (unsigned i = 0; i <
ruleList->length(); ++i) {
nit: no {}

[ingemara, 2015-06-25 08:06:52]

On 2015/06/25 02:27:05, hajimehoshi wrote:
> lgtm
> 
>
https://codereview.chromium.org/1203873004/diff/20001/Source/core/page/PageSe...
> File Source/core/page/PageSerializer.cpp (right):
> 
>
https://codereview.chromium.org/1203873004/diff/20001/Source/core/page/PageSe...
> Source/core/page/PageSerializer.cpp:328: if (rule->type() ==
> CSSRule::IMPORT_RULE) {
> switch-case seems better
> 
>
https://codereview.chromium.org/1203873004/diff/20001/Source/core/page/PageSe...
> Source/core/page/PageSerializer.cpp:343: for (unsigned i = 0; i <
> ruleList->length(); ++i) {
> nit: no {}

Thanks, using a switch statement forced me to look into the other CSSRules as
well, where CSSSupportsRule sub-rules weren't picked up either.

[philipj_slow, 2015-06-25 09:07:41]

LGTM, just some nitpickety stuff

https://codereview.chromium.org/1203873004/diff/80001/Source/core/page/PageSe...
File Source/core/page/PageSerializer.cpp (right):

https://codereview.chromium.org/1203873004/diff/80001/Source/core/page/PageSe...
Source/core/page/PageSerializer.cpp:346: case CSSRule::SUPPORTS_RULE: {
Good find with @supports

https://codereview.chromium.org/1203873004/diff/80001/Source/core/page/PageSe...
Source/core/page/PageSerializer.cpp:363: break;
I guess also an ASSERT_NOT_REACH in a default branch so that we're not only
relying on the compiler warning about missing cases.

https://codereview.chromium.org/1203873004/diff/80001/Source/core/page/PageSe...
File Source/core/page/PageSerializer.h (right):

https://codereview.chromium.org/1203873004/diff/80001/Source/core/page/PageSe...
Source/core/page/PageSerializer.h:90: // Serializes the css rule (including any
imported stylesheets), adding referenced resources
https://www.chromium.org/blink/coding-style#TOC-Comments wants a period at the
end, or at least I've been told so in other reviews.

https://codereview.chromium.org/1203873004/diff/80001/Source/web/tests/data/p...
File Source/web/tests/data/pageserializer/css/css_test_page.html (right):

https://codereview.chromium.org/1203873004/diff/80001/Source/web/tests/data/p...
Source/web/tests/data/pageserializer/css/css_test_page.html:26: } /* media not
all */
Don't need these /* end comments */ I think.

https://codereview.chromium.org/1203873004/diff/80001/Source/web/tests/data/p...
Source/web/tests/data/pageserializer/css/css_test_page.html:56: @supports
(background-image: url('brown_background.png')) {
Maybe something trivial like display:block so that it's very clear that the URL
used inside the @supports bit isn't the reason it's serialized.

[ingemara, 2015-06-25 09:30:09]

On 2015/06/25 09:07:41, philipj wrote:
>
https://codereview.chromium.org/1203873004/diff/80001/Source/web/tests/data/p...
> Source/web/tests/data/pageserializer/css/css_test_page.html:26: } /* media not
> all */
> Don't need these /* end comments */ I think.

I guess they made sense when I didn't indent the rule body C++ namespace style,
which I later did.


@hajimehoshi, does the switch statement stuff look OK?

[hajimehoshi, 2015-06-25 09:37:25]

On 2015/06/25 09:30:09, ingemara wrote:
> On 2015/06/25 09:07:41, philipj wrote:
> >
>
https://codereview.chromium.org/1203873004/diff/80001/Source/web/tests/data/p...
> > Source/web/tests/data/pageserializer/css/css_test_page.html:26: } /* media
not
> > all */
> > Don't need these /* end comments */ I think.
> 
> I guess they made sense when I didn't indent the rule body C++ namespace
style,
> which I later did.
> 
> 
> @hajimehoshi, does the switch statement stuff look OK?

still lgtm

[philipj_slow, 2015-06-25 09:38:39]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2015-06-25 09:38:39]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1203873004/#ps100001
(title: "Address nits")

[commit-bot: I haz the power, 2015-06-25 09:39:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1203873004/100001

[commit-bot: I haz the power, 2015-06-25 11:33:28]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197813