Add usage counter for each value of webkit-user-modify

Add usage counter for each value of webkit-user-modify

This patch adds usage counter for each value of webkit-user-modify so
that we can have a better understanding of its current usage, and the
functionality loss about 'read-write-plaintext-only' if the property
is removed.

BUG=687843
Review-Url: https://codereview.chromium.org/2668423004
Cr-Commit-Position: refs/heads/master@{#447741}
Committed: https://chromium.googlesource.com/chromium/src/+/d71997cab809617592987ba993d0261cdde19a9a

[Xiaocheng, 2017-02-02 06:57:06]

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

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

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2017-02-02 07:56:44]

xiaochengh@chromium.org changed reviewers:
+ tkent@chromium.org

[Xiaocheng, 2017-02-02 07:56:45]

PTAL.

[tkent, 2017-02-02 08:01:42]

lgtm

https://codereview.chromium.org/2668423004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2668423004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2035:
NOTREACHED();
Remove NOTREACHED().  There are other valid values such as 'inherit' 'initial'.

[Xiaocheng, 2017-02-02 08:10:15]

Thanks for the review!

https://codereview.chromium.org/2668423004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2668423004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2035:
NOTREACHED();
On 2017/02/02 at 08:01:42, tkent wrote:
> Remove NOTREACHED().  There are other valid values such as 'inherit'
'initial'.

The current implementation only accepts 'read-write', 'read-only' and
'read-write-plaintext-only' as valid values. Other values will be rejected by
|CSSParserFastPaths::isValidKeywordPropertyAndValue|.

So I think it's correct to put NOTREACHED() here.

[tkent, 2017-02-02 08:21:01]

https://codereview.chromium.org/2668423004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2668423004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2035:
NOTREACHED();
On 2017/02/02 at 08:10:15, Xiaocheng wrote:
> On 2017/02/02 at 08:01:42, tkent wrote:
> > Remove NOTREACHED().  There are other valid values such as 'inherit'
'initial'.
> 
> The current implementation only accepts 'read-write', 'read-only' and
'read-write-plaintext-only' as valid values. Other values will be rejected by
|CSSParserFastPaths::isValidKeywordPropertyAndValue|.
> 
> So I think it's correct to put NOTREACHED() here.

Ack.  Please go ahead.

[Xiaocheng, 2017-02-02 08:31:02]

The CQ bit was unchecked by xiaochengh@chromium.org

[Xiaocheng, 2017-02-02 08:31:09]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-02-02 08:31:10]

Thanks.

[commit-bot: I haz the power, 2017-02-02 08:31:43]

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-02-02 08:40:11]

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

[commit-bot: I haz the power, 2017-02-02 08:40:12]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Xiaocheng, 2017-02-02 08:48:08]

xiaochengh@chromium.org changed reviewers:
+ haraken@chromium.org

[Xiaocheng, 2017-02-02 08:48:09]

+haraken

PTAL.

[haraken, 2017-02-02 08:53:49]

LGTM

[Xiaocheng, 2017-02-02 08:55:02]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-02-02 08:55:03]

Thanks!

[commit-bot: I haz the power, 2017-02-02 08:55:13]

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-02-02 10:12:52]

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

[commit-bot: I haz the power, 2017-02-02 10:12:54]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Xiaocheng, 2017-02-02 10:32:00]

The CQ bit was checked by xiaochengh@chromium.org

[commit-bot: I haz the power, 2017-02-02 10:32:09]

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-02-02 11:17:22]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1486031520892220, "parent_rev":
"41cbaaac04a5e4a31285b8794fb02d80bb777f0f", "commit_rev":
"d71997cab809617592987ba993d0261cdde19a9a"}

[commit-bot: I haz the power, 2017-02-02 11:17:52]

Description was changed from

==========
Add usage counter for each value of webkit-user-modify

This patch adds usage counter for each value of webkit-user-modify so
that we can have a better understanding of its current usage, and the
functionality loss about 'read-write-plaintext-only' if the property
is removed.

BUG=687843
==========

to

==========
Add usage counter for each value of webkit-user-modify

This patch adds usage counter for each value of webkit-user-modify so
that we can have a better understanding of its current usage, and the
functionality loss about 'read-write-plaintext-only' if the property
is removed.

BUG=687843

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

[commit-bot: I haz the power, 2017-02-02 11:17:53]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/d71997cab809617592987ba993d0...

[Manuel Rego, 2017-02-03 14:12:06]

rego@igalia.com changed reviewers:
+ rego@igalia.com

[Manuel Rego, 2017-02-03 14:12:07]

https://codereview.chromium.org/2668423004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/2668423004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/UseCounter.h:1451:
CSSValueUserModifyReadOnly = 1798,
Not really important but we usually have comments like:
  // The above items are available in M56 branch.

As M57 branch has just happened, maybe it'd be nice to add this.

This makes easier to follow in which releases the use counters
have been introduced.

[Xiaocheng, 2017-02-03 14:35:13]

https://codereview.chromium.org/2668423004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/2668423004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/UseCounter.h:1451:
CSSValueUserModifyReadOnly = 1798,
On 2017/02/03 at 14:12:07, Manuel Rego wrote:
> Not really important but we usually have comments like:
>   // The above items are available in M56 branch.
> 
> As M57 branch has just happened, maybe it'd be nice to add this.
> 
> This makes easier to follow in which releases the use counters
> have been introduced.

Thanks, with a question.

Counters starting from 1788 are added in M58, and some of them are merged to M57
(1788-1792 are merged, 1797 is merge-approved).

In this case, where should the comment be put at?

[Manuel Rego, 2017-02-03 14:46:22]

https://codereview.chromium.org/2668423004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/2668423004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/UseCounter.h:1451:
CSSValueUserModifyReadOnly = 1798,
On 2017/02/03 14:35:13, Xiaocheng wrote:
> Counters starting from 1788 are added in M58, and some of them are merged to
M57
> (1788-1792 are merged, 1797 is merge-approved).
> 
> In this case, where should the comment be put at?

That's a good question and I don't have a good answer for it.

What I think is that it's valuable to have those comments,
maybe we should put it before 1793; and move 1797 just after 1792.
But it can be confusing too.

Dunno if @haraken has the right answer.

[haraken, 2017-02-03 16:39:02]

On 2017/02/03 14:46:22, Manuel Rego wrote:
>
https://codereview.chromium.org/2668423004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/frame/UseCounter.h (right):
> 
>
https://codereview.chromium.org/2668423004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/UseCounter.h:1451:
> CSSValueUserModifyReadOnly = 1798,
> On 2017/02/03 14:35:13, Xiaocheng wrote:
> > Counters starting from 1788 are added in M58, and some of them are merged to
> M57
> > (1788-1792 are merged, 1797 is merge-approved).
> > 
> > In this case, where should the comment be put at?
> 
> That's a good question and I don't have a good answer for it.
> 
> What I think is that it's valuable to have those comments,
> maybe we should put it before 1793; and move 1797 just after 1792.
> But it can be confusing too.
> 
> Dunno if @haraken has the right answer.

IMO we should avoid moving the counter. I'd add the comment just before 1788
(i.e., ignore what was merged to the previous branch). If we need to worry about
merging, it will anyway lead to a mess.