Unprefix CSS cursor values zoom-in and zoom-out

Unprefix CSS cursor values zoom-in and zoom-out

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/-GuOTe8c32Q/Gd9PuHYD0-EJ

-webkit-zoom-in and -webkit-zoom-out are kept as aliases for now.

BUG=246998
TEST=LayoutTests/fast/css/cursor-parsing.html

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

[philipj_slow, 2014-05-16 14:32:13]

Julien, Ojan, could you take a quick look at the basic approach here to see if
it's sound? I want to make -webkit-zoom-in a simple alias of zoom-in.

Feedback on how to actually test this would also be nice, other than checking
that the CSSOM bits looks OK I don't know how to automate the test for the
cursor rendering itself.

If it looks mostly OK I'll sent out an Intent to Implement and Ship.

[ojan, 2014-05-16 17:23:48]

This LGTM once the intent thread has been lgtm'd and you add a test. Testing
just the CSSOM bits is fine for this. Ideally you'd change all the tests that
use the webkit prefixed one over to use the unprefixed one and just have a
single test for the webkit prefixed values, but that's totally optional.

https://codereview.chromium.org/288303007/diff/1/Source/core/css/CSSPrimitive...
File Source/core/css/CSSPrimitiveValueMappings.h (right):

https://codereview.chromium.org/288303007/diff/1/Source/core/css/CSSPrimitive...
Source/core/css/CSSPrimitiveValueMappings.h:1191: if (m_value.valueID ==
CSSValueCopy)
Nit: Should this be a switch now that it has so many clauses?

[Julien - ping for review, 2014-05-19 09:17:55]

LGTM2, the approach sounds sane. I don't think we have good coverage for cursor
rendering though :(

If you wanted to extend our visual coverage infrastructure, you would get a lot
of extra points though. Unfortunately I don't know this area well enough to give
you some direction there (but I am happy to help if you decide to do it).

https://codereview.chromium.org/288303007/diff/40001/Source/core/css/CSSPrimi...
File Source/core/css/CSSPrimitiveValueMappings.h (right):

https://codereview.chromium.org/288303007/diff/40001/Source/core/css/CSSPrimi...
Source/core/css/CSSPrimitiveValueMappings.h:1196: case CSSValueWebkitZoomOut:
Wouldn't you also get the unprefixed keywords here too?

https://codereview.chromium.org/288303007/diff/40001/Source/core/css/CSSValue...
File Source/core/css/CSSValueKeywords.in (right):

https://codereview.chromium.org/288303007/diff/40001/Source/core/css/CSSValue...
Source/core/css/CSSValueKeywords.in:398: -webkit-zoom-out
Would it be worth it to have the concept of alias_for= for keywords too? (we
have it for properties and it automates the conversion)

[philipj_slow, 2014-05-19 09:52:51]

For testing the visual effect of CSS cursor, is there any infrastructure at all
that can capture the pixels of anything but the window itself, maybe for testing
window decorations or something?

A somewhat less ambitious approach would be to test this using the content API.
With a sufficiently elaborate test it should be possible to load a page, move
the mouse and see that Blink says that the cursor should change. Are there any
existing tests similar to that?

https://codereview.chromium.org/288303007/diff/40001/Source/core/css/CSSPrimi...
File Source/core/css/CSSPrimitiveValueMappings.h (right):

https://codereview.chromium.org/288303007/diff/40001/Source/core/css/CSSPrimi...
Source/core/css/CSSPrimitiveValueMappings.h:1196: case CSSValueWebkitZoomOut:
On 2014/05/19 09:17:55, Julien Chaffraix - CET wrote:
> Wouldn't you also get the unprefixed keywords here too?

I could add them here, but they will be handled by the default case. Only the
keywords which aren't in the same order in RenderStyleConstants.h and
CSSValueKeywords.in need manual handling.

https://codereview.chromium.org/288303007/diff/40001/Source/core/css/CSSValue...
File Source/core/css/CSSValueKeywords.in (right):

https://codereview.chromium.org/288303007/diff/40001/Source/core/css/CSSValue...
Source/core/css/CSSValueKeywords.in:398: -webkit-zoom-out
On 2014/05/19 09:17:55, Julien Chaffraix - CET wrote:
> Would it be worth it to have the concept of alias_for= for keywords too? (we
> have it for properties and it automates the conversion)

AFAICT from the generated CSSPropertyNames.{h,cpp}, an aliased properties get no
CSSPropertyID of its own. Since UseCounter uses CSSPropertyID I don't know how
to add UseCounters for aliased properties, and assume the same problem would
happen for aliased value keywords?

I'm guessing an alias would also make both keywords both stringify to the
unprefixed keyword? Is that a change that can be made without any UseCounter
data?

[philipj_slow, 2014-05-20 07:26:52]

This has its LGTMs on blink-dev now, so a final review of this would be nice. On
the testing front I don't really expect that there's anything that would be
worth the effort, but I am of course open to suggestions.

[Julien - ping for review, 2014-05-21 11:38:10]

LGTM2bis, I don't know much about the content API but I am pretty sure Blink
currently only has access to the content's pixels, not the cursor's or the
window decorations'.

https://codereview.chromium.org/288303007/diff/40001/Source/core/css/CSSValue...
File Source/core/css/CSSValueKeywords.in (right):

https://codereview.chromium.org/288303007/diff/40001/Source/core/css/CSSValue...
Source/core/css/CSSValueKeywords.in:398: -webkit-zoom-out
On 2014/05/19 09:52:51, philipj wrote:
> On 2014/05/19 09:17:55, Julien Chaffraix - CET wrote:
> > Would it be worth it to have the concept of alias_for= for keywords too? (we
> > have it for properties and it automates the conversion)
> 
> AFAICT from the generated CSSPropertyNames.{h,cpp}, an aliased properties get
no
> CSSPropertyID of its own. Since UseCounter uses CSSPropertyID I don't know how
> to add UseCounters for aliased properties, and assume the same problem would
> happen for aliased value keywords?

Yes but we could prevent this problem by having some translation code instead of
aliasing. This is not an issue for now as we don't UseCount CSSValueID.

> I'm guessing an alias would also make both keywords both stringify to the
> unprefixed keyword? Is that a change that can be made without any UseCounter
> data?

Good point. Now that you mention it, CSS transition (that we are in the process
of unprefixing) is not using aliases for this reason. I can't recall what
motivated the decision but it's probably to be on the safe side.

[philipj_slow, 2014-05-21 11:57:58]

https://codereview.chromium.org/288303007/diff/40001/Source/core/css/CSSValue...
File Source/core/css/CSSValueKeywords.in (right):

https://codereview.chromium.org/288303007/diff/40001/Source/core/css/CSSValue...
Source/core/css/CSSValueKeywords.in:398: -webkit-zoom-out
On 2014/05/21 11:38:10, Julien Chaffraix - CET wrote:
> On 2014/05/19 09:52:51, philipj wrote:
> > On 2014/05/19 09:17:55, Julien Chaffraix - CET wrote:
> > > Would it be worth it to have the concept of alias_for= for keywords too?
(we
> > > have it for properties and it automates the conversion)
> > 
> > AFAICT from the generated CSSPropertyNames.{h,cpp}, an aliased properties
get
> no
> > CSSPropertyID of its own. Since UseCounter uses CSSPropertyID I don't know
how
> > to add UseCounters for aliased properties, and assume the same problem would
> > happen for aliased value keywords?
> 
> Yes but we could prevent this problem by having some translation code instead
of
> aliasing. This is not an issue for now as we don't UseCount CSSValueID.

Do you mean a function like mapKeywordToAlias() that would be applied after
we've had a chance to notify UseCounter about the interesting bits? After this
has landed I'll submit a review with UseCounters for -webkit-zoom-in/out. I'll
add you to that review to see if you think there's a reasonable way to add
aliasing to the mix.

[philipj_slow, 2014-05-21 11:58:09]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-05-21 17:53:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/philipj@opera.com/288303007/40001

[philipj_slow, 2014-05-22 08:02:05]

The CQ bit was unchecked by philipj@opera.com

[philipj_slow, 2014-05-22 08:02:17]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-05-22 08:02:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/philipj@opera.com/288303007/40001

[commit-bot: I haz the power, 2014-05-22 17:27:08]

Change committed as 174575