Move cursor property into CSSPropertyParser

Move cursor property into CSSPropertyParser

Move cursor property handling from LegacyCSSPropertyParser
into CSSPropertyParser.

BUG=499780
Committed: https://crrev.com/2b5dabfe0f52227c1e664f2f582aaf47e15c0b8f
Cr-Commit-Position: refs/heads/master@{#362974}

[rwlbuis, 2015-11-20 03:33:36]

Description was changed from

==========
cursor WIP

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

to

==========
Move cursor property into CSSPropertyParser

Move cursor property handling from LegacyCSSPropertyParser
into CSSPropertyParser.

BUG=499780
==========

[rwlbuis, 2015-11-20 03:35:53]

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

[rwlbuis, 2015-11-20 03:35:53]

PTAL. This also starts with the image stuff (image set in this case).

[Timothy Loh, 2015-11-20 04:09:07]

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1868:
ASSERT(token.unitType() == CSSPrimitiveValue::UnitType::Unknown);
This assertion doesn't look right, since we haven't actually checked the unit is
'x' until the line below.

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1894: image =
consumeImageSet(consumeFunction(range));
Doesn't this mean that any invalid -webkit-image-set(..) is just dropped instead
of making us fail parsing? Maybe we should move the consumeFunction call to
consumeImageSet.

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1915: if (image)
looks like this is always true

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1920: }
blank line after here might be nice.

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1928: if (list)
{
Can we remove the copy-paste and fix the allowing random junk here? e.g.

RefPtrWillBeRawPtr<CSSValue> cursorType;
if (inQuirksMode() && id == CSSValueHand) {
    // Non-standard behavior
    cursorType = cssValuePool().createIdentifierValue(CSSValuePointer);
    range.consumeIncludingWhitespace();
} else if ((id >= CSSValueAuto && id <= CSSValueWebkitZoomOut) || id ==
CSSValueCopy || id == CSSValueNone) {
    cursorType = consumeIdent(range);
} else {
    return nullptr;
}

if (!list)
    return cursorType.release();
list->append(cursorType.release());
return list.release();

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1936: return
list.release();
If you didn't notice, we accept "cursor: url(a), bla" with this (and the
current) code, this is the "random junk" I mentioned above.

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:262:
/*inline*/ PassRefPtrWillBeRawPtr<CSSValue>
CSSPropertyParser::createCSSImageValueWithReferrer(const AtomicString& rawValue,
const KURL& url)
?

[Timothy Loh, 2015-11-20 06:56:25]

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1936: return
list.release();
On 2015/11/20 04:09:06, Timothy Loh wrote:
> If you didn't notice, we accept "cursor: url(a), bla" with this (and the
> current) code, this is the "random junk" I mentioned above.

On the other hand, maybe it's better to keep behaviour changes like this
separate? In any case, I think we should remove the copy-paste.

[rwlbuis, 2015-11-20 20:00:21]

PTAL.

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1868:
ASSERT(token.unitType() == CSSPrimitiveValue::UnitType::Unknown);
On 2015/11/20 04:09:07, Timothy Loh wrote:
> This assertion doesn't look right, since we haven't actually checked the unit
is
> 'x' until the line below.

Done.

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1894: image =
consumeImageSet(consumeFunction(range));
On 2015/11/20 04:09:07, Timothy Loh wrote:
> Doesn't this mean that any invalid -webkit-image-set(..) is just dropped
instead
> of making us fail parsing? Maybe we should move the consumeFunction call to
> consumeImageSet.

I think I see what you mean. I guess we really need to return nullptr if
consumeImageSet fails. But as you said that would be a behavior change.

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1915: if (image)
On 2015/11/20 04:09:07, Timothy Loh wrote:
> looks like this is always true

Done.

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1920: }
On 2015/11/20 04:09:06, Timothy Loh wrote:
> blank line after here might be nice.

Done.

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1928: if (list)
{
On 2015/11/20 04:09:07, Timothy Loh wrote:
> Can we remove the copy-paste and fix the allowing random junk here? e.g.
> 
> RefPtrWillBeRawPtr<CSSValue> cursorType;
> if (inQuirksMode() && id == CSSValueHand) {
>     // Non-standard behavior
>     cursorType = cssValuePool().createIdentifierValue(CSSValuePointer);
>     range.consumeIncludingWhitespace();
> } else if ((id >= CSSValueAuto && id <= CSSValueWebkitZoomOut) || id ==
> CSSValueCopy || id == CSSValueNone) {
>     cursorType = consumeIdent(range);
> } else {
>     return nullptr;
> }
> 
> if (!list)
>     return cursorType.release();
> list->append(cursorType.release());
> return list.release();

Done.

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1936: return
list.release();
On 2015/11/20 06:56:25, Timothy Loh wrote:
> On 2015/11/20 04:09:06, Timothy Loh wrote:
> > If you didn't notice, we accept "cursor: url(a), bla" with this (and the
> > current) code, this is the "random junk" I mentioned above.
> 
> On the other hand, maybe it's better to keep behaviour changes like this
> separate? In any case, I think we should remove the copy-paste.

I kept the behavior as-is but removed the copy-paste.

[rwlbuis, 2015-11-28 21:22:18]

Patchset #6 (id:100001) has been deleted

[rwlbuis, 2015-12-02 22:22:09]

Don't forget this one :) It provides a good start for the <image> type handling
which is in progress.

[Timothy Loh, 2015-12-02 23:59:10]

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1457873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1894: image =
consumeImageSet(consumeFunction(range));
On 2015/11/20 20:00:21, rwlbuis wrote:
> On 2015/11/20 04:09:07, Timothy Loh wrote:
> > Doesn't this mean that any invalid -webkit-image-set(..) is just dropped
> instead
> > of making us fail parsing? Maybe we should move the consumeFunction call to
> > consumeImageSet.
> 
> I think I see what you mean. I guess we really need to return nullptr if
> consumeImageSet fails. But as you said that would be a behavior change.

Err this wouldn't be a behavior change, currently we don't consume anything for
an invalid -webkit-image-set. We could make it just return nullptr if
consumeImageSet fails, since at that point we'll never succeed anyway, but I'm
not sure if that'll be good for composition in consumeImage.

https://codereview.chromium.org/1457873002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1457873002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2151: while
(!functionArgs.atEnd()) {
Can't we use the same loop as everywhere else for comma-separated lists?

do {
    ..
} while (consumeComma..);

if (!atEnd())
    return nullptr;

[rwlbuis, 2015-12-03 04:03:22]

> > > Doesn't this mean that any invalid -webkit-image-set(..) is just dropped
> > > instead
> > > of making us fail parsing? Maybe we should move the consumeFunction call
to
> > > consumeImageSet.
> >
> > I think I see what you mean. I guess we really need to return nullptr if
> >consumeImageSet fails. But as you said that would be a behavior change.

> Err this wouldn't be a behavior change, currently we don't consume anything
for
> an invalid -webkit-image-set. We could make it just return nullptr if
> consumeImageSet fails, since at that point we'll never succeed anyway, but I'm
> not sure if that'll be good for composition in consumeImage.

Fair enough, I now made it so it does not consume anything for an invalid
-webkit-image-set.

I also changed it to return nullptr if consumeImageSet fails, I think we can
re-evaluate that choice once we use consumeImageSet in more places.

PTAL.

https://codereview.chromium.org/1457873002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1457873002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2151: while
(!functionArgs.atEnd()) {
On 2015/12/02 23:59:10, Timothy Loh wrote:
> Can't we use the same loop as everywhere else for comma-separated lists?
> 
> do {
>     ..
> } while (consumeComma..);
> 
> if (!atEnd())
>     return nullptr;

Done.

[Timothy Loh, 2015-12-03 04:10:25]

lgtm

https://codereview.chromium.org/1457873002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1457873002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2167: } while
(consumeCommaIncludingWhitespace(args));
This needs to check args.atEnd() right after the loop

[rwlbuis, 2015-12-03 12:32:32]

Patchset #9 (id:180001) has been deleted

[rwlbuis, 2015-12-03 12:34:29]

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

[rwlbuis, 2015-12-03 12:34:29]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1457873002/#ps200001
(title: "Patch for landing")

[commit-bot: I haz the power, 2015-12-03 12:36:21]

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

[commit-bot: I haz the power, 2015-12-03 13:43:17]

Description was changed from

==========
Move cursor property into CSSPropertyParser

Move cursor property handling from LegacyCSSPropertyParser
into CSSPropertyParser.

BUG=499780
==========

to

==========
Move cursor property into CSSPropertyParser

Move cursor property handling from LegacyCSSPropertyParser
into CSSPropertyParser.

BUG=499780
==========

[commit-bot: I haz the power, 2015-12-03 13:43:18]

Committed patchset #9 (id:200001)

[commit-bot: I haz the power, 2015-12-03 13:44:21]

Description was changed from

==========
Move cursor property into CSSPropertyParser

Move cursor property handling from LegacyCSSPropertyParser
into CSSPropertyParser.

BUG=499780
==========

to

==========
Move cursor property into CSSPropertyParser

Move cursor property handling from LegacyCSSPropertyParser
into CSSPropertyParser.

BUG=499780

Committed: https://crrev.com/2b5dabfe0f52227c1e664f2f582aaf47e15c0b8f
Cr-Commit-Position: refs/heads/master@{#362974}
==========

[commit-bot: I haz the power, 2015-12-03 13:44:22]

Patchset 9 (id:??) landed as
https://crrev.com/2b5dabfe0f52227c1e664f2f582aaf47e15c0b8f
Cr-Commit-Position: refs/heads/master@{#362974}