CSS: Use count unitless 0 supplied as <angle>

CSS: Use count unitless 0 supplied as <angle>

A recent spec change means that 0 is no longer supported as a valid
value for angles.
https://github.com/w3c/csswg-drafts/issues/1162#issuecomment-293637767

We add use counters to detect how often 0 is currently being used as
an angle.

image-orientation and rotate and offset-path's ray() have not yet
shipped, so we don't need use counters for these, we simply reject 0
as an angle.

ConsumeGradientAngleOrPercent now accepts a CSSParserContext by
const reference.

ConsumeGradientLengthOrPercent has been implemented with the same
function signature as ConsumeGradientAngleOrPercent so they can
be called via ConsumeGradientColorStops using the same function pointer type.

BUG=725382
Review-Url: https://codereview.chromium.org/2898133002
Cr-Commit-Position: refs/heads/master@{#474578}
Committed: https://chromium.googlesource.com/chromium/src/+/e24202c3b5752cb040117fe5ae0247c27089766c

[Eric Willigers, 2017-05-23 08:29:11]

ericwilligers@chromium.org changed reviewers:
+ bugsnash@chromium.org

[Eric Willigers, 2017-05-23 08:29:17]

The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 08:29:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 08:32:10]

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

[commit-bot: I haz the power, 2017-05-23 08:32:10]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Eric Willigers, 2017-05-24 03:54:58]

ericwilligers@chromium.org changed reviewers:
+ suzyh@chromium.org

[suzyh_UTC10 (ex-contributor), 2017-05-24 04:47:48]

core/css OWNERS lgtm with similar caveats as on
https://codereview.chromium.org/2888283006. I have not done a full detailed
review of the code change and tests - leaving that in bugsnash's capable hands -
and have essentially just reviewed for style and general strategy.

[Bugs Nash, 2017-05-24 06:24:20]

lgtm with nits

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

https://codereview.chromium.org/2898133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:326:
CSSPrimitiveValue* ConsumeGradientLengthOrPercent(
This seems like an unnecessary wrap, why not call ConsumeLengthOrPercent
directly instead?

https://codereview.chromium.org/2898133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:358:
context.Count(literalZero);
literalZeroFeature is a bit clearer, this kinda reads like passing a const

https://codereview.chromium.org/2898133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:956:
static CSSPrimitiveValue* ConsumeGradientAngleOrPercent(
please add this rename to cl description

https://codereview.chromium.org/2898133002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/css/properties/CSSPropertyAPIImageOrientation.cpp
(right):

https://codereview.chromium.org/2898133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSPropertyAPIImageOrientation.cpp:14:
using namespace CSSPropertyParserHelpers;
please add this namespace change in the apis to the cl description as it seems
unrelated to the rest of the patch

[Eric Willigers, 2017-05-24 06:37:31]

ericwilligers@chromium.org changed reviewers:
+ alancutter@chromium.org

[Eric Willigers, 2017-05-24 06:46:02]

Description was changed from

==========
CSS: Use count literal 0 supplied as <angle>

A recent spec change means that 0 is no longer supported as a valid value
for angles.
https://github.com/w3c/csswg-drafts/issues/1162#issuecomment-293637767

We add use counters to detect how often 0 is currently being used as an
angle.

image-orientation and rotate and offset-path's ray() have not yet shipped,
so we don't need use counters for these, we simply reject 0 as an angle.

BUG=725382
==========

to

==========
CSS: Use count literal 0 supplied as <angle>

A recent spec change means that 0 is no longer supported as a valid value
for angles.
https://github.com/w3c/csswg-drafts/issues/1162#issuecomment-293637767

We add use counters to detect how often 0 is currently being used as an
angle.

image-orientation and rotate and offset-path's ray() have not yet shipped,
so we don't need use counters for these, we simply reject 0 as an angle.

ConsumeGradientAngleOrPercent now accepts a CSSParserContext by
const reference.

We add 'using namespace CSSPropertyParserHelpers' to clients
that use several of the namespace members.

BUG=725382
==========

[Bugs Nash, 2017-05-24 07:01:36]

On 2017/05/24 at 06:37:31, ericwilligers wrote:
> 

As per offline discussion, UnitlessQuirk isn't intended for use in this way, so
suggest new enum or making the argument optional.

[Eric Willigers, 2017-05-24 07:13:18]

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

https://codereview.chromium.org/2898133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:326:
CSSPrimitiveValue* ConsumeGradientLengthOrPercent(
On 2017/05/24 06:24:20, Bugs Nash wrote:
> This seems like an unnecessary wrap, why not call ConsumeLengthOrPercent
> directly instead?

ConsumeGradientLengthOrPercent is passed by function pointer, as the fourth
argument to ConsumeGradientColorStops. 

We can't currrently pass ConsumeLengthOrPercent as it now has different argument
types to ConsumeAngleOrPercent.

We could change ConsumeLengthOrPercent but note it has ~50 callers.

[Eric Willigers, 2017-05-24 17:07:26]

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

https://codereview.chromium.org/2898133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:358:
context.Count(literalZero);
On 2017/05/24 06:24:20, Bugs Nash wrote:
> literalZeroFeature is a bit clearer, this kinda reads like passing a const

Done. Now passing Optional literalZeroFeature.

[Bugs Nash, 2017-05-24 21:46:25]

On 2017/05/24 at 07:13:18, ericwilligers wrote:
>
https://codereview.chromium.org/2898133002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp
(right):
> 
>
https://codereview.chromium.org/2898133002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:326:
CSSPrimitiveValue* ConsumeGradientLengthOrPercent(
> On 2017/05/24 06:24:20, Bugs Nash wrote:
> > This seems like an unnecessary wrap, why not call ConsumeLengthOrPercent
> > directly instead?
> 
> ConsumeGradientLengthOrPercent is passed by function pointer, as the fourth
argument to ConsumeGradientColorStops. 
> 
> We can't currrently pass ConsumeLengthOrPercent as it now has different
argument types to ConsumeAngleOrPercent.
> 
> We could change ConsumeLengthOrPercent but note it has ~50 callers.

I see, yeah this is fine

[Eric Willigers, 2017-05-25 00:22:50]

The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 00:23:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alancutter (OOO until 2018), 2017-05-25 01:53:17]

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

https://codereview.chromium.org/2898133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:326:
CSSPrimitiveValue* ConsumeGradientLengthOrPercent(
I don't understand the purpose of this function, can we avoid adding it?

https://codereview.chromium.org/2898133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:338:
WTF::Optional<UseCounter::Feature> literalZeroFeature) {
DCHECK that the quirk is disallowed iff the feature is null.

https://codereview.chromium.org/2898133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:354:
literalZeroFeature) {
Use the quirk parameter instead. It's completely unused otherwise.

https://codereview.chromium.org/2898133002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h
(right):

https://codereview.chromium.org/2898133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:62:
UnitlessQuirk,
UnitlessQuirk refers to numbers being interpreted as pixels. It should probably
be renamed more explicitly in another patch.

We shouldn't make its meaning more ambiguous by using it for unitless zero
values. Create a new UnitlessZeroQuirk enum to use here.

https://codereview.chromium.org/2898133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:63:
WTF::Optional<UseCounter::Feature> literalZeroFeature);
s/literal/unitless/ here and in the description.

https://codereview.chromium.org/2898133002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/2898133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.h:1620: kZeroAngleTransform =
2010,
Prepend these with "Unitless".

[commit-bot: I haz the power, 2017-05-25 02:37:56]

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

[commit-bot: I haz the power, 2017-05-25 02:37:57]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Eric Willigers, 2017-05-25 02:45:41]

Description was changed from

==========
CSS: Use count literal 0 supplied as <angle>

A recent spec change means that 0 is no longer supported as a valid value
for angles.
https://github.com/w3c/csswg-drafts/issues/1162#issuecomment-293637767

We add use counters to detect how often 0 is currently being used as an
angle.

image-orientation and rotate and offset-path's ray() have not yet shipped,
so we don't need use counters for these, we simply reject 0 as an angle.

ConsumeGradientAngleOrPercent now accepts a CSSParserContext by
const reference.

We add 'using namespace CSSPropertyParserHelpers' to clients
that use several of the namespace members.

BUG=725382
==========

to

==========
CSS: Use count unitless 0 supplied as <angle>

A recent spec change means that 0 is no longer supported as a valid value
for angles.
https://github.com/w3c/csswg-drafts/issues/1162#issuecomment-293637767

We add use counters to detect how often 0 is currently being used as an
angle.

image-orientation and rotate and offset-path's ray() have not yet shipped,
so we don't need use counters for these, we simply reject 0 as an angle.

ConsumeGradientAngleOrPercent now accepts a CSSParserContext by
const reference.

We add 'using namespace CSSPropertyParserHelpers' to clients
that use several of the namespace members.

BUG=725382
==========

[Eric Willigers, 2017-05-25 02:56:41]

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

https://codereview.chromium.org/2898133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:326:
CSSPrimitiveValue* ConsumeGradientLengthOrPercent(
On 2017/05/25 01:53:16, alancutter wrote:
> I don't understand the purpose of this function, can we avoid adding it?

ConsumeLengthOrPercent accepts a CSSParserMode.
For example, in CSSSyntaxDescriptor.cpp we explicitly pass kHTMLStandardMode
regardless of the context mode.

However, ConsumeGradientLengthOrPercent is called by function pointer and now
needs to accept a CSSParserContext const ref, as ConsumeGradientColorStops can
be called instead, and the arguments need to be consistent.

https://codereview.chromium.org/2898133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:338:
WTF::Optional<UseCounter::Feature> literalZeroFeature) {
On 2017/05/25 01:53:16, alancutter wrote:
> DCHECK that the quirk is disallowed iff the feature is null.

Retiring the quirk argument, it was added in an earlier version of this CL
before we used Optional.

https://codereview.chromium.org/2898133002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h
(right):

https://codereview.chromium.org/2898133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:62:
UnitlessQuirk,
On 2017/05/25 01:53:16, alancutter wrote:
> UnitlessQuirk refers to numbers being interpreted as pixels. It should
probably
> be renamed more explicitly in another patch.
> 
> We shouldn't make its meaning more ambiguous by using it for unitless zero
> values. Create a new UnitlessZeroQuirk enum to use here.

Removed.

https://codereview.chromium.org/2898133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:63:
WTF::Optional<UseCounter::Feature> literalZeroFeature);
On 2017/05/25 01:53:16, alancutter wrote:
> s/literal/unitless/ here and in the description.

Done.

https://codereview.chromium.org/2898133002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/2898133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.h:1620: kZeroAngleTransform =
2010,
On 2017/05/25 01:53:16, alancutter wrote:
> Prepend these with "Unitless".

Done.

[alancutter (OOO until 2018), 2017-05-25 03:11:52]

lgtm

Please add tests for the change in behaviour for image-orientation and rotate
and offset-path's ray().

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

https://codereview.chromium.org/2898133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:326:
CSSPrimitiveValue* ConsumeGradientLengthOrPercent(
On 2017/05/25 at 02:56:41, Eric Willigers wrote:
> On 2017/05/25 01:53:16, alancutter wrote:
> > I don't understand the purpose of this function, can we avoid adding it?
> 
> ConsumeLengthOrPercent accepts a CSSParserMode.
> For example, in CSSSyntaxDescriptor.cpp we explicitly pass kHTMLStandardMode
regardless of the context mode.
> 
> However, ConsumeGradientLengthOrPercent is called by function pointer and now
needs to accept a CSSParserContext const ref, as ConsumeGradientColorStops can
be called instead, and the arguments need to be consistent.

Ack. You should make this reason clear in the description.

https://codereview.chromium.org/2898133002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/css/properties/CSSPropertyAPIImageOrientation.cpp
(right):

https://codereview.chromium.org/2898133002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSPropertyAPIImageOrientation.cpp:14:
using namespace CSSPropertyParserHelpers;
I'm not in favour of this use of using namespace, can we split this off to deal
with in a different patch?

[Eric Willigers, 2017-05-25 03:49:44]

Description was changed from

==========
CSS: Use count unitless 0 supplied as <angle>

A recent spec change means that 0 is no longer supported as a valid value
for angles.
https://github.com/w3c/csswg-drafts/issues/1162#issuecomment-293637767

We add use counters to detect how often 0 is currently being used as an
angle.

image-orientation and rotate and offset-path's ray() have not yet shipped,
so we don't need use counters for these, we simply reject 0 as an angle.

ConsumeGradientAngleOrPercent now accepts a CSSParserContext by
const reference.

We add 'using namespace CSSPropertyParserHelpers' to clients
that use several of the namespace members.

BUG=725382
==========

to

==========
CSS: Use count unitless 0 supplied as <angle>

A recent spec change means that 0 is no longer supported as a valid
value for angles.
https://github.com/w3c/csswg-drafts/issues/1162#issuecomment-293637767

We add use counters to detect how often 0 is currently being used as
an angle.

image-orientation and rotate and offset-path's ray() have not yet
shipped, so we don't need use counters for these, we simply reject 0
as an angle.

ConsumeGradientAngleOrPercent now accepts a CSSParserContext by
const reference.

ConsumeGradientLengthOrPercent has been implemented with the same
function signature as ConsumeGradientAngleOrPercent so they can
be called via ConsumeGradientColorStops using the same function pointer type.

BUG=725382
==========

[Eric Willigers, 2017-05-25 03:58:14]

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

https://codereview.chromium.org/2898133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:326:
CSSPrimitiveValue* ConsumeGradientLengthOrPercent(
On 2017/05/25 03:11:52, alancutter wrote:
> On 2017/05/25 at 02:56:41, Eric Willigers wrote:
> > On 2017/05/25 01:53:16, alancutter wrote:
> > > I don't understand the purpose of this function, can we avoid adding it?
> > 
> > ConsumeLengthOrPercent accepts a CSSParserMode.
> > For example, in CSSSyntaxDescriptor.cpp we explicitly pass kHTMLStandardMode
> regardless of the context mode.
> > 
> > However, ConsumeGradientLengthOrPercent is called by function pointer and
now
> needs to accept a CSSParserContext const ref, as ConsumeGradientColorStops can
> be called instead, and the arguments need to be consistent.
> 
> Ack. You should make this reason clear in the description.

Done.

https://codereview.chromium.org/2898133002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/css/properties/CSSPropertyAPIImageOrientation.cpp
(right):

https://codereview.chromium.org/2898133002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/properties/CSSPropertyAPIImageOrientation.cpp:14:
using namespace CSSPropertyParserHelpers;
On 2017/05/25 03:11:52, alancutter wrote:
> I'm not in favour of this use of using namespace, can we split this off to
deal
> with in a different patch?

Done.

[Eric Willigers, 2017-05-25 03:58:34]

The CQ bit was checked by ericwilligers@chromium.org

[Eric Willigers, 2017-05-25 03:58:35]

The patchset sent to the CQ was uploaded after l-g-t-m from suzyh@chromium.org,
bugsnash@chromium.org, alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2898133002/#ps100001
(title: "test")

[commit-bot: I haz the power, 2017-05-25 03:59:30]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 06:04:32]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495684714700780,
"parent_rev": "0a90acb1bae34599f887b69cad29daa243b44b21", "commit_rev":
"e24202c3b5752cb040117fe5ae0247c27089766c"}

[commit-bot: I haz the power, 2017-05-25 06:04:45]

Description was changed from

==========
CSS: Use count unitless 0 supplied as <angle>

A recent spec change means that 0 is no longer supported as a valid
value for angles.
https://github.com/w3c/csswg-drafts/issues/1162#issuecomment-293637767

We add use counters to detect how often 0 is currently being used as
an angle.

image-orientation and rotate and offset-path's ray() have not yet
shipped, so we don't need use counters for these, we simply reject 0
as an angle.

ConsumeGradientAngleOrPercent now accepts a CSSParserContext by
const reference.

ConsumeGradientLengthOrPercent has been implemented with the same
function signature as ConsumeGradientAngleOrPercent so they can
be called via ConsumeGradientColorStops using the same function pointer type.

BUG=725382
==========

to

==========
CSS: Use count unitless 0 supplied as <angle>

A recent spec change means that 0 is no longer supported as a valid
value for angles.
https://github.com/w3c/csswg-drafts/issues/1162#issuecomment-293637767

We add use counters to detect how often 0 is currently being used as
an angle.

image-orientation and rotate and offset-path's ray() have not yet
shipped, so we don't need use counters for these, we simply reject 0
as an angle.

ConsumeGradientAngleOrPercent now accepts a CSSParserContext by
const reference.

ConsumeGradientLengthOrPercent has been implemented with the same
function signature as ConsumeGradientAngleOrPercent so they can
be called via ConsumeGradientColorStops using the same function pointer type.

BUG=725382

Review-Url: https://codereview.chromium.org/2898133002
Cr-Commit-Position: refs/heads/master@{#474578}
Committed:
https://chromium.googlesource.com/chromium/src/+/e24202c3b5752cb040117fe5ae02...
==========

[commit-bot: I haz the power, 2017-05-25 06:04:47]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/e24202c3b5752cb040117fe5ae02...