PageRule should not serialize @page in selectorText

PageRule should not serialize @page in selectorText

According to the spec [1]:
The selectorText attribute, on getting, must return the result
of serializing the associated list of CSS page selectors.

Behavior matches Firefox.

[1] https://drafts.csswg.org/cssom/#dom-csspagerule-selectortext

BUG=582098
Committed: https://crrev.com/2947176ac96b969ae7e0f119cc3e082c115d4e4e
Cr-Commit-Position: refs/heads/master@{#372355}

[rwlbuis, 2016-01-28 16:26:39]

Description was changed from

==========
pageRULe WIP

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

to

==========
PageRule should not serialize @page in selectorText

According to the spec [1]:
The selectorText attribute, on getting, must return the result
of serializing the associated list of CSS page selectors.

[1] https://drafts.csswg.org/cssom/#dom-csspagerule-selectortext

BUG=582098
==========

[rwlbuis, 2016-01-28 16:27:07]

Description was changed from

==========
PageRule should not serialize @page in selectorText

According to the spec [1]:
The selectorText attribute, on getting, must return the result
of serializing the associated list of CSS page selectors.

[1] https://drafts.csswg.org/cssom/#dom-csspagerule-selectortext

BUG=582098
==========

to

==========
PageRule should not serialize @page in selectorText

According to the spec [1]:
The selectorText attribute, on getting, must return the result
of serializing the associated list of CSS page selectors.

Behavior matches Firefox.

[1] https://drafts.csswg.org/cssom/#dom-csspagerule-selectortext

BUG=582098
==========

[rwlbuis, 2016-01-28 16:27:35]

rob.buis@samsung.com changed reviewers:
+ rune@opera.com, timloh@chromium.org

[rwlbuis, 2016-01-28 16:27:35]

PTAL.

[rune, 2016-01-28 18:15:47]

How about adding a CSSPageRuleTest.cpp where you test the selector
serialization?

https://codereview.chromium.org/1637273003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPageRule.cpp (left):

https://codereview.chromium.org/1637273003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPageRule.cpp:86: result.appendLiteral(" {
");
If selectorText() is empty, this results in a double space in the serialization.

https://codereview.chromium.org/1637273003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPageRule.cpp (right):

https://codereview.chromium.org/1637273003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPageRule.cpp:61: if
(!pageSpecification.isEmpty() && pageSpecification != starAtom)
starAtom? That shouldn't be allowed in @page selectors?

[rwlbuis, 2016-01-28 22:27:50]

I added the CSSPageRuleTest.cpp as suggested.

https://codereview.chromium.org/1637273003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPageRule.cpp (left):

https://codereview.chromium.org/1637273003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPageRule.cpp:86: result.appendLiteral(" {
");
On 2016/01/28 18:15:47, rune wrote:
> If selectorText() is empty, this results in a double space in the
serialization.

Done.

https://codereview.chromium.org/1637273003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPageRule.cpp (right):

https://codereview.chromium.org/1637273003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPageRule.cpp:61: if
(!pageSpecification.isEmpty() && pageSpecification != starAtom)
On 2016/01/28 18:15:47, rune wrote:
> starAtom? That shouldn't be allowed in @page selectors?

I removed it. Must be ancient code!

[rune, 2016-01-28 23:04:23]

lgtm

[rwlbuis, 2016-01-29 10:47:28]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2016-01-29 10:48:08]

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

[commit-bot: I haz the power, 2016-01-29 11:57:03]

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

[commit-bot: I haz the power, 2016-01-29 11:57:04]

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

[rwlbuis, 2016-01-29 14:56:58]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2016-01-29 14:56:58]

The patchset sent to the CQ was uploaded after l-g-t-m from rune@opera.com
Link to the patchset: https://codereview.chromium.org/1637273003/#ps110001
(title: "Fix win build problem")

[commit-bot: I haz the power, 2016-01-29 14:57:26]

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

[commit-bot: I haz the power, 2016-01-29 16:24:41]

Description was changed from

==========
PageRule should not serialize @page in selectorText

According to the spec [1]:
The selectorText attribute, on getting, must return the result
of serializing the associated list of CSS page selectors.

Behavior matches Firefox.

[1] https://drafts.csswg.org/cssom/#dom-csspagerule-selectortext

BUG=582098
==========

to

==========
PageRule should not serialize @page in selectorText

According to the spec [1]:
The selectorText attribute, on getting, must return the result
of serializing the associated list of CSS page selectors.

Behavior matches Firefox.

[1] https://drafts.csswg.org/cssom/#dom-csspagerule-selectortext

BUG=582098
==========

[commit-bot: I haz the power, 2016-01-29 16:24:41]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2016-01-29 16:26:01]

Description was changed from

==========
PageRule should not serialize @page in selectorText

According to the spec [1]:
The selectorText attribute, on getting, must return the result
of serializing the associated list of CSS page selectors.

Behavior matches Firefox.

[1] https://drafts.csswg.org/cssom/#dom-csspagerule-selectortext

BUG=582098
==========

to

==========
PageRule should not serialize @page in selectorText

According to the spec [1]:
The selectorText attribute, on getting, must return the result
of serializing the associated list of CSS page selectors.

Behavior matches Firefox.

[1] https://drafts.csswg.org/cssom/#dom-csspagerule-selectortext

BUG=582098

Committed: https://crrev.com/2947176ac96b969ae7e0f119cc3e082c115d4e4e
Cr-Commit-Position: refs/heads/master@{#372355}
==========

[commit-bot: I haz the power, 2016-01-29 16:26:02]

Patchset 7 (id:??) landed as
https://crrev.com/2947176ac96b969ae7e0f119cc3e082c115d4e4e
Cr-Commit-Position: refs/heads/master@{#372355}

[Timothy Loh, 2016-01-31 23:54:24]

https://codereview.chromium.org/1637273003/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSPageRuleTest.cpp (right):

https://codereview.chromium.org/1637273003/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSPageRuleTest.cpp:20: if
(helper.cssRules()) {
If this ends up null for some reason, then the test will still pass. You can
ASSERT it isn't null to safely check this (ASSERTs will stop the test if they
fail, while EXPECTs will keep going), although I don't think anyone really pays
much attention to whether people correctly use ASSERT or EXPECT.

I'm not sure the purpose of the if check below either, it seems like you're
already assuming the rule is non-null by dereferencing it before.