Setting selectorText of PageRule accepts invalid selectors

Setting selectorText of PageRule accepts invalid selectors

Allow to set selectorText for PageRule if the selectors are
page rule selectors only.

BUG=580956
Committed: https://crrev.com/d2de318527994d3951ddb09a0bad157c6d85f29d
Cr-Commit-Position: refs/heads/master@{#372675}

[ramya.v, 2016-01-29 09:27:12]

ramya.v@samsung.com changed reviewers:
+ timloh@chromium.org

[ramya.v, 2016-01-29 09:27:13]

PTAL!

Thanks

[rune, 2016-01-29 12:08:35]

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

[rune, 2016-01-29 12:08:36]

This doesn't look right. You check that page pseudos are the only allowed simple
selectors while, but what about page names (supported?) and combinators.
Wouldn't your code accept ":first + :last" as a valid selector.

I don't think it makes sense to involve the CSSSelectorParser for page selectors
as they're much simpler and not really related. You now have two code paths for
parsing them, one when parsing a whole @page rule and one for setting
selectorText. I think the page selector parsing in the page rule code should be
extracted and used instead.

Adding Rob, as he worked on page selectors yesterday.

[rwlbuis, 2016-01-29 15:11:55]

I was about to upload roughly the same patch! :) Apart from the PseudoUnknown
check, I don't see why that is needed.

On 2016/01/29 12:08:36, rune wrote:
> This doesn't look right. You check that page pseudos are the only allowed
simple
> selectors while, but what about page names (supported?) and combinators.
> Wouldn't your code accept ":first + :last" as a valid selector.
> 
> I don't think it makes sense to involve the CSSSelectorParser for page
selectors
> as they're much simpler and not really related. You now have two code paths
for
> parsing them, one when parsing a whole @page rule and one for setting
> selectorText. I think the page selector parsing in the page rule code should
be
> extracted and used instead.

Yep, using the boolean did feel a bit hacky. Above sounds good.

> Adding Rob, as he worked on page selectors yesterday.

Yep, this bug was something I wanted to tackle after:
https://codereview.chromium.org/1637273003/

Of course is it is great if Ramya can do it, same company after all :)

@ramya for adding the test, I think it is best to use CSSPageRuleTest.cpp
instead of
a layout test once 1637273003 lands.

[ramya.v, 2016-02-01 13:09:11]

On 2016/01/29 15:11:55, rwlbuis wrote:
> I was about to upload roughly the same patch! :) Apart from the PseudoUnknown
> check, I don't see why that is needed.
> 
> On 2016/01/29 12:08:36, rune wrote:
> > This doesn't look right. You check that page pseudos are the only allowed
> simple
> > selectors while, but what about page names (supported?) and combinators.
> > Wouldn't your code accept ":first + :last" as a valid selector.
> > 
> > I don't think it makes sense to involve the CSSSelectorParser for page
> selectors
> > as they're much simpler and not really related. You now have two code paths
> for
> > parsing them, one when parsing a whole @page rule and one for setting
> > selectorText. I think the page selector parsing in the page rule code should
> be
> > extracted and used instead.
> 
> Yep, using the boolean did feel a bit hacky. Above sounds good.
> 
> > Adding Rob, as he worked on page selectors yesterday.
> 
> Yep, this bug was something I wanted to tackle after:
> https://codereview.chromium.org/1637273003/
> 
> Of course is it is great if Ramya can do it, same company after all :)
> 
> @ramya for adding the test, I think it is best to use CSSPageRuleTest.cpp
> instead of
> a layout test once 1637273003 lands.



@Rune/Rob Updated as suggested. PTAL!

Thanks

[rune, 2016-02-01 14:09:34]

lgtm

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

https://codereview.chromium.org/1650473002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:197: selector =
CSSParserSelector::create(QualifiedName(nullAtom, typeSelector,
styleSheet->defaultNamespace()));
Namespaces for page names doesn't make sense, and also we never try to match
page names, but that can be handled in a different CL.

[ramya.v, 2016-02-01 15:50:05]

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

https://codereview.chromium.org/1650473002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:197: selector =
CSSParserSelector::create(QualifiedName(nullAtom, typeSelector,
styleSheet->defaultNamespace()));
On 2016/02/01 14:09:34, rune wrote:
> Namespaces for page names doesn't make sense, and also we never try to match
> page names, but that can be handled in a different CL.

Sure will take care in subsequent patch.

[ramya.v, 2016-02-01 15:50:29]

The CQ bit was checked by ramya.v@samsung.com

[commit-bot: I haz the power, 2016-02-01 15:51:04]

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

[commit-bot: I haz the power, 2016-02-01 16:55:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-01 16:56:52]

Description was changed from

==========
Setting selectorText of PageRule accepts invalid selectors

Allow to set selectorText for PageRule if the selectors are
page rule selectors only.

BUG=580956
==========

to

==========
Setting selectorText of PageRule accepts invalid selectors

Allow to set selectorText for PageRule if the selectors are
page rule selectors only.

BUG=580956

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

[commit-bot: I haz the power, 2016-02-01 16:56:53]

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

[Timothy Loh, 2016-02-01 21:55:16]

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

https://codereview.chromium.org/1650473002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParser.cpp:45: while
(!range.atEnd() && range.peek().type() != LeftBraceToken && range.peek().type()
!= SemicolonToken)
Seems wrong, if you specify ":left { bla" as a page selector it should be
invalid (I assume regular selectors don't allow this sort of stuff either).

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

https://codereview.chromium.org/1650473002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:176: CSSSelectorList
CSSParserImpl::parsePageSelector(CSSParserTokenRange prelude,
StyleSheetContents* styleSheet)
prelude -> range? This isn't really a prelude when you set a selector via CSSOM.

https://codereview.chromium.org/1650473002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:193: return
CSSSelectorList(); // Parse error; extra tokens in @page header
@page header -> @page selector (actually I don't think header was ever the
appropriate term)

https://codereview.chromium.org/1650473002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:581: // We only
support a small subset of the css-page spec.
This comment should be moved to parsePageSelector.

https://codereview.chromium.org/1650473002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:584: return nullptr;
FYI I was fairly deliberate with commenting on parse errors so that if we wanted
to add error logging in the future it'd be obvious what all the different parse
errors were. In this case it's pretty obvious from looking at the code, but it
wouldn't hurt to add a comment like "// Parse error, invalid @page selector"

[ramya.v, 2016-02-02 05:43:24]

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

https://codereview.chromium.org/1650473002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParser.cpp:45: while
(!range.atEnd() && range.peek().type() != LeftBraceToken && range.peek().type()
!= SemicolonToken)
On 2016/02/01 21:55:15, Timothy Loh wrote:
> Seems wrong, if you specify ":left { bla" as a page selector it should be
> invalid (I assume regular selectors don't allow this sort of stuff either).

In the existing implementation ":left { bla" is parsed as ":left {}" (empty
declaration block instead of completely invalidating the selector).

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

https://codereview.chromium.org/1650473002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:176: CSSSelectorList
CSSParserImpl::parsePageSelector(CSSParserTokenRange prelude,
StyleSheetContents* styleSheet)
On 2016/02/01 21:55:16, Timothy Loh wrote:
> prelude -> range? This isn't really a prelude when you set a selector via
CSSOM.

Done in https://codereview.chromium.org/1653303002/

https://codereview.chromium.org/1650473002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:193: return
CSSSelectorList(); // Parse error; extra tokens in @page header
On 2016/02/01 21:55:15, Timothy Loh wrote:
> @page header -> @page selector (actually I don't think header was ever the
> appropriate term)

Done in https://codereview.chromium.org/1653303002/

https://codereview.chromium.org/1650473002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:581: // We only
support a small subset of the css-page spec.
On 2016/02/01 21:55:16, Timothy Loh wrote:
> This comment should be moved to parsePageSelector.

Done in https://codereview.chromium.org/1653303002/

https://codereview.chromium.org/1650473002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:584: return nullptr;
On 2016/02/01 21:55:16, Timothy Loh wrote:
> FYI I was fairly deliberate with commenting on parse errors so that if we
wanted
> to add error logging in the future it'd be obvious what all the different
parse
> errors were. In this case it's pretty obvious from looking at the code, but it
> wouldn't hurt to add a comment like "// Parse error, invalid @page selector"

Done in https://codereview.chromium.org/1653303002/

[Timothy Loh, 2016-02-02 05:49:12]

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

https://codereview.chromium.org/1650473002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParser.cpp:45: while
(!range.atEnd() && range.peek().type() != LeftBraceToken && range.peek().type()
!= SemicolonToken)
On 2016/02/02 05:43:24, ramya.v wrote:
> On 2016/02/01 21:55:15, Timothy Loh wrote:
> > Seems wrong, if you specify ":left { bla" as a page selector it should be
> > invalid (I assume regular selectors don't allow this sort of stuff either).
> 
> In the existing implementation ":left { bla" is parsed as ":left {}" (empty
> declaration block instead of completely invalidating the selector). 

Yes, but this function only concerns itself with parsing an @page selector, not
an @page rule.

For a style rule we don't allow this sort of thing:

<style> div { } </style>
<script>
var rule = document.styleSheets[0].cssRules[0];
console.log(rule.selectorText) // "div"
rule.selectorText = "span { bla"
console.log(rule.selectorText) // still "div"
</script>

[ramya.v, 2016-02-02 06:40:17]

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

https://codereview.chromium.org/1650473002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParser.cpp:45: while
(!range.atEnd() && range.peek().type() != LeftBraceToken && range.peek().type()
!= SemicolonToken)
On 2016/02/02 05:49:12, Timothy Loh wrote:
> On 2016/02/02 05:43:24, ramya.v wrote:
> > On 2016/02/01 21:55:15, Timothy Loh wrote:
> > > Seems wrong, if you specify ":left { bla" as a page selector it should be
> > > invalid (I assume regular selectors don't allow this sort of stuff
either).
> > 
> > In the existing implementation ":left { bla" is parsed as ":left {}" (empty
> > declaration block instead of completely invalidating the selector). 
> 
> Yes, but this function only concerns itself with parsing an @page selector,
not
> an @page rule.
> 
> For a style rule we don't allow this sort of thing:
> 
> <style> div { } </style>
> <script>
> var rule = document.styleSheets[0].cssRules[0];
> console.log(rule.selectorText) // "div"
> rule.selectorText = "span { bla"
> console.log(rule.selectorText) // still "div"
> </script>



Got your point. Thanks :) 
Done in https://codereview.chromium.org/1661453002/