Auto generate UseCounter::Feature enum from an .in file

Auto generate UseCounter::Feature enum from an .in file

BUG=433819

[kkristof, 2014-11-17 14:25:46]

kkosztyo.u-szeged@partner.samsung.com changed reviewers:
+ haraken@chromium.org, philipj@opera.com, sigbjornf@opera.com

[haraken, 2014-11-17 14:29:44]

It's great to auto-generate the UseCounter enum, but the current .in file looks
error-prone. For example, if someone removes "UIEventCharCode" from the .in file
by mistake, it will break UseCounters for all the following enums. It would be
great if we could have a syntax more robust to these human errors.

[philipj_slow, 2014-11-17 14:45:23]

I agree, this looks like it will increase the risk of changing the value of a
counter when removing an entry.

I think we should have an explicit value for each entry, but have
make_use_counter.py verify that there are no collisions. It would be nice to
avoid conflicts when two people add new counters at the same time, but I don't
think that's possible while also having a stable value for each entry.

[philipj_slow, 2014-11-17 14:50:17]

https://codereview.chromium.org/729393002/diff/1/Source/core/frame/UseCounter...
File Source/core/frame/UseCounterGenerated.in (right):

https://codereview.chromium.org/729393002/diff/1/Source/core/frame/UseCounter...
Source/core/frame/UseCounterGenerated.in:1: 
There are blank lines sprinkled around this file, is there some grouping I'm not
seeing?

https://codereview.chromium.org/729393002/diff/1/Source/core/frame/UseCounter...
Source/core/frame/UseCounterGenerated.in:75: // Removed from DOM4
I think having the comments at the end of lines was clearer, but I guess one of
the scripts in use don't support that?

[kkristof, 2014-11-17 16:03:45]

On 2014/11/17 14:50:17, philipj wrote:
>
https://codereview.chromium.org/729393002/diff/1/Source/core/frame/UseCounter...
> File Source/core/frame/UseCounterGenerated.in (right):
> 
>
https://codereview.chromium.org/729393002/diff/1/Source/core/frame/UseCounter...
> Source/core/frame/UseCounterGenerated.in:1: 
> There are blank lines sprinkled around this file, is there some grouping I'm
not
> seeing?
> 
>
https://codereview.chromium.org/729393002/diff/1/Source/core/frame/UseCounter...
> Source/core/frame/UseCounterGenerated.in:75: // Removed from DOM4
> I think having the comments at the end of lines was clearer, but I guess one
of
> the scripts in use don't support that?

The in_file.py doesn't support the comments in the end of the line but it
doesn't need too much change.

[kkristof, 2014-11-17 16:09:28]

On 2014/11/17 14:45:23, philipj wrote:
> I agree, this looks like it will increase the risk of changing the value of a
> counter when removing an entry.
> 
> I think we should have an explicit value for each entry, but have
> make_use_counter.py verify that there are no collisions. It would be nice to
> avoid conflicts when two people add new counters at the same time, but I don't
> think that's possible while also having a stable value for each entry.

Okay, I am going to change this.

[kkristof, 2014-11-24 08:59:51]

I have updated the patch now every line has explicit value and the script check
this value.

[haraken, 2014-11-24 09:38:49]

https://codereview.chromium.org/729393002/diff/20001/Source/build/scripts/in_...
File Source/build/scripts/in_file.py (right):

https://codereview.chromium.org/729393002/diff/20001/Source/build/scripts/in_...
Source/build/scripts/in_file.py:135: line = line.split('//')[0]  # remove
comments

I'm not sure but we might want to use '#' instead of '//' for a comment.

https://codereview.chromium.org/729393002/diff/20001/Source/build/scripts/mak...
File Source/build/scripts/make_use_counter.py (right):

https://codereview.chromium.org/729393002/diff/20001/Source/build/scripts/mak...
Source/build/scripts/make_use_counter.py:19: if not value or type(value) is
bool:

Just help me understand: What is the 'type(value) is bool' check for?

https://codereview.chromium.org/729393002/diff/20001/Source/core/frame/UseCou...
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/729393002/diff/20001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.h:58: class UseCounter : public UseCounterGenerated
{

Add final.

[philipj_slow, 2014-11-24 10:44:59]

https://codereview.chromium.org/729393002/diff/20001/Source/build/scripts/in_...
File Source/build/scripts/in_file.py (right):

https://codereview.chromium.org/729393002/diff/20001/Source/build/scripts/in_...
Source/build/scripts/in_file.py:135: line = line.split('//')[0]  # remove
comments
On 2014/11/24 09:38:49, haraken wrote:
> 
> I'm not sure but we might want to use '#' instead of '//' for a comment.

Also, isn't there already some code for handling // at the beginning of a line
that should be removed if this bit is added?

https://codereview.chromium.org/729393002/diff/20001/Source/core/frame/UseCou...
File Source/core/frame/UseCounterGenerated.in (right):

https://codereview.chromium.org/729393002/diff/20001/Source/core/frame/UseCou...
Source/core/frame/UseCounterGenerated.in:5: PageDestruction value=0
Is this format required by the common code, or could it just as well be
PageDestruction=0?

[Timothy Loh, 2014-11-24 11:02:39]

timloh@chromium.org changed reviewers:
+ timloh@chromium.org

[Timothy Loh, 2014-11-24 11:02:41]

I don't understand the point of this patch. So instead of "AttrChildChange =
601,", we now write "AttrChildChange value=601". How is this better / how can we
justify the additional complexity of code generation here?

https://codereview.chromium.org/729393002/diff/20001/Source/build/scripts/mak...
File Source/build/scripts/make_use_counter.py (right):

https://codereview.chromium.org/729393002/diff/20001/Source/build/scripts/mak...
Source/build/scripts/make_use_counter.py:42: 
The whole unique_value thing is a bit awkward, why don't we just write here
something like:

self.features = self.in_file.name_dictionaries

feature_values = set(feature['value'] for feature in self.features)
assert None not in feature_values, "All features should be given values"
assert len(features_values) == len(self.features), "Features should have unique
values"

The error reporting is slightly less useful here, but since you normally only
modify one flag it'll probably be obvious what's wrong.

https://codereview.chromium.org/729393002/diff/20001/Source/build/scripts/mak...
Source/build/scripts/make_use_counter.py:44: ('UseCounterGenerated.h'):
self.generate_header,
Unneeded brackets here

https://codereview.chromium.org/729393002/diff/20001/Source/build/scripts/mak...
Source/build/scripts/make_use_counter.py:46: self._template_context = {
This dict makes more sense to just have inline in generate_header

https://codereview.chromium.org/729393002/diff/20001/Source/core/core_generat...
File Source/core/core_generated.gyp (right):

https://codereview.chromium.org/729393002/diff/20001/Source/core/core_generat...
Source/core/core_generated.gyp:798: {
You almost certainly need to edit a GN file somewhere

[philipj_slow, 2014-11-24 13:14:29]

On 2014/11/24 11:02:41, Timothy Loh wrote:
> I don't understand the point of this patch. So instead of "AttrChildChange =
> 601,", we now write "AttrChildChange value=601". How is this better / how can
we
> justify the additional complexity of code generation here?

The code generator can fail if there are two entries with the same value, which
happened recently:
https://codereview.chromium.org/684783003

That's only happened once AFAIK.

[kkristof, 2014-11-24 15:48:25]

On 2014/11/24 09:38:49, haraken wrote:
>
https://codereview.chromium.org/729393002/diff/20001/Source/build/scripts/in_...
> File Source/build/scripts/in_file.py (right):
> 
>
https://codereview.chromium.org/729393002/diff/20001/Source/build/scripts/in_...
> Source/build/scripts/in_file.py:135: line = line.split('//')[0]  # remove
> comments
> 
> I'm not sure but we might want to use '#' instead of '//' for a comment.
> 
>
https://codereview.chromium.org/729393002/diff/20001/Source/build/scripts/mak...
> File Source/build/scripts/make_use_counter.py (right):
> 
>
https://codereview.chromium.org/729393002/diff/20001/Source/build/scripts/mak...
> Source/build/scripts/make_use_counter.py:19: if not value or type(value) is
> bool:
> 
> Just help me understand: What is the 'type(value) is bool' check for?

When an argument's value doesn't defined the in_file set the value to True which
leads build error later.
eg: someFeature value -> someFeature = True;

> 
>
https://codereview.chromium.org/729393002/diff/20001/Source/core/frame/UseCou...
> File Source/core/frame/UseCounter.h (right):
> 
>
https://codereview.chromium.org/729393002/diff/20001/Source/core/frame/UseCou...
> Source/core/frame/UseCounter.h:58: class UseCounter : public
UseCounterGenerated
> {
> 
> Add final.

[kkristof, 2014-11-24 16:02:07]

On 2014/11/24 10:44:59, philipj wrote:
>
https://codereview.chromium.org/729393002/diff/20001/Source/build/scripts/in_...
> File Source/build/scripts/in_file.py (right):
> 
>
https://codereview.chromium.org/729393002/diff/20001/Source/build/scripts/in_...
> Source/build/scripts/in_file.py:135: line = line.split('//')[0]  # remove
> comments
> On 2014/11/24 09:38:49, haraken wrote:
> > 
> > I'm not sure but we might want to use '#' instead of '//' for a comment.
> 
> Also, isn't there already some code for handling // at the beginning of a line
> that should be removed if this bit is added?
> 
>
https://codereview.chromium.org/729393002/diff/20001/Source/core/frame/UseCou...
> File Source/core/frame/UseCounterGenerated.in (right):
> 
>
https://codereview.chromium.org/729393002/diff/20001/Source/core/frame/UseCou...
> Source/core/frame/UseCounterGenerated.in:5: PageDestruction value=0
> Is this format required by the common code, or could it just as well be
> PageDestruction=0?

Yes we can use this format like this but it doesn't allow spaces around the
operator
eg: PageDestruction=0 will be ok but PageDestruction = 0 will fail because the
line parser will split at the spaces :(

Originally I tried to rewrite the line parser to be smarter but the code became
a little unreadable so at the end I just cut the line at the comment.

[Timothy Loh, 2014-11-24 20:57:53]

On 2014/11/24 13:14:29, philipj wrote:
> On 2014/11/24 11:02:41, Timothy Loh wrote:
> > I don't understand the point of this patch. So instead of "AttrChildChange =
> > 601,", we now write "AttrChildChange value=601". How is this better / how
can
> we
> > justify the additional complexity of code generation here?
> 
> The code generator can fail if there are two entries with the same value,
which
> happened recently:
> https://codereview.chromium.org/684783003
> 
> That's only happened once AFAIK.

This doesn't really solve the problem though. It just makes it so that after the
second patch lands, all the builds will fail. Can we just avoid the problem by
adding a comment that says something like "Don't add blank lines as they may
make conflicting additions merge"? (I assume that without the added blank line
there would've been a merge conflict)

[philipj_slow, 2014-11-24 21:55:25]

On 2014/11/24 20:57:53, Timothy Loh wrote:
> On 2014/11/24 13:14:29, philipj wrote:
> > On 2014/11/24 11:02:41, Timothy Loh wrote:
> > > I don't understand the point of this patch. So instead of "AttrChildChange
=
> > > 601,", we now write "AttrChildChange value=601". How is this better / how
> can
> > we
> > > justify the additional complexity of code generation here?
> > 
> > The code generator can fail if there are two entries with the same value,
> which
> > happened recently:
> > https://codereview.chromium.org/684783003
> > 
> > That's only happened once AFAIK.
> 
> This doesn't really solve the problem though. It just makes it so that after
the
> second patch lands, all the builds will fail. Can we just avoid the problem by
> adding a comment that says something like "Don't add blank lines as they may
> make conflicting additions merge"? (I assume that without the added blank line
> there would've been a merge conflict)

The second patch cannot land without conflict, since its slot was taken by the
first. One would have to resolve the conflict incorrectly and bypass CQ to get a
broken trunk.

That being said, I don't mind the current state of UseCounter.h, and the single
value collision was easily resolved.

[philipj_slow, 2014-11-24 21:56:49]

If this CL goes on, can we call it UseCounterFeatures.in instead? Seems like a
more descriptive name, and more in the style of *.in files, none of which have
"generated" in the file name.

[Timothy Loh, 2014-11-24 22:13:39]

On 2014/11/24 21:55:25, philipj wrote:
> The second patch cannot land without conflict, since its slot was taken by the
> first. One would have to resolve the conflict incorrectly and bypass CQ to get
a
> broken trunk.

Two patches adding an enum could both pass tryjobs if they're triggered before
either patch landing. So if they manage to merge together (which I think
happened last time due to the added blank line) then they would both land, but
every build after would start failing. I'm not sure if this is even an
improvement.

> That being said, I don't mind the current state of UseCounter.h, and the
single
> value collision was easily resolved.

I don't think we should go forwards with this patch.

[philipj_slow, 2014-11-24 22:33:38]

Hmm, I had assumed that someone had manually resolved a conflict and messed it
up, but you're right, it looks like https://codereview.chromium.org/672343002
was able to land without conflict because of the blank line after
SrcsetDroppedCandidate = 573. This could happen in UseCounterGenerated.in as
well and the result would be a broken trunk.