Bug 72566 – Add a list of contribution areas

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ryosuke Niwa]

In order to implement the first iteration of change log analyzer, we need a list of contribution areas.

[Ryosuke Niwa]

Created attachment 115489 [details]
Adds contributionareas.py

[Adam Barth]

Comment on attachment 115489 [details]
Adds contributionareas.py

View in context: https://bugs.webkit.org/attachment.cgi?id=115489&action=review

> Tools/Scripts/webkitpy/common/config/contributionareas.py:73
> +USE_NAME = None

USE_NAME ?

[Eric Seidel (no email)]

Comment on attachment 115489 [details]
Adds contributionareas.py

View in context: https://bugs.webkit.org/attachment.cgi?id=115489&action=review

> Tools/Scripts/webkitpy/common/config/contributionareas.py:51
> +        self._tokens = []
> +        for token in tokens:
> +            if token == USE_NAME:
> +                self._tokens.append(self._name_to_token(name))
> +            else:
> +                self._tokens.append(token)

You want to use map() for this I think.

> Tools/Scripts/webkitpy/common/config/contributionareas.py:82
> +    ContributionArea('ARM JIT', ['arm']),
> +# FIXME: 'Accelerated compositing / GPU acceleration'
> +    ContributionArea('Accessibility', [USE_NAME]),
> +    ContributionArea('Android port', ['android']),
> +    ContributionArea('Animation', [USE_NAME, 'animator']),
> +    ContributionArea('Apple\'s Windows port', ['win', 'windows']),  # FIXME: need to exclude chromium...
> +    ContributionArea('Autotools Build', ['autotools']),
> +    ContributionArea('Basic types and data structures (WTF)', ['wtf']),

I might have used _Area instead.  1. to indicate that it's private to this module 2. to make it shorter to read?

> Tools/Scripts/webkitpy/common/config/contributionareas.py:92
> +    ContributionArea('Cairo', [USE_NAME]),

Instead of of having this special "USE_NAME" value, I would have just had it automatically use the name when tokens is None.

> Tools/Scripts/webkitpy/common/config/contributionareas_unittest.py:54
> +        self._assert_areas_for_touched_files(areas, ['WebCore/css/'], ['CSS'])

Why the trailing / ?

[Eric Seidel (no email)]

Comment on attachment 115489 [details]
Adds contributionareas.py

View in context: https://bugs.webkit.org/attachment.cgi?id=115489&action=review

> Tools/Scripts/webkitpy/common/config/contributionareas.py:44
> +    def __init__(self, name, tokens):

Yeah, just give 'tokens' a default value of = None and use the name when that's the case.  If folks need to specify the name + otehr directories, they should have to explicitly list the name as well.

Also, why is this "tokens" instead of "directories"?  Are the tokens used for some sort of regexp to possibly match many files/directories?

[Ryosuke Niwa]

Comment on attachment 115489 [details]
Adds contributionareas.py

View in context: https://bugs.webkit.org/attachment.cgi?id=115489&action=review

>> Tools/Scripts/webkitpy/common/config/contributionareas.py:73
>> +USE_NAME = None
> 
> USE_NAME ?

Means that the token is the contribution area name itself.

>> Tools/Scripts/webkitpy/common/config/contributionareas.py:92

> 
> Instead of of having this special "USE_NAME" value, I would have just had it automatically use the name when tokens is None.

I had to use both USE_NAME and 'animator' for 'animation' but I guess that's rare enough.

>> Tools/Scripts/webkitpy/common/config/contributionareas_unittest.py:54
>> +        self._assert_areas_for_touched_files(areas, ['WebCore/css/'], ['CSS'])
> 
> Why the trailing / ?

It shouldn't matter but I changed one of them to use / and omitted / from the rest.

[Ryosuke Niwa]

(In reply to comment #4)
> Also, why is this "tokens" instead of "directories"?  Are the tokens used for some sort of regexp to possibly match many files/directories?

Yes. This class is in its infancy :)

[Ryosuke Niwa]

Created attachment 115492 [details]
Fixed per comments

[Eric Seidel (no email)]

Comment on attachment 115492 [details]
Fixed per comments

I may have lead you astray encouraging you to _ prefix your classes.  But this seems fine.  I'm not sure I fully understand the Intersection class, but I'm looking forward to see where this goes.

[Ryosuke Niwa]

(In reply to comment #8)
> (From update of attachment 115492 [details])
> I may have lead you astray encouraging you to _ prefix your classes.  But this seems fine.  I'm not sure I fully understand the Intersection class, but I'm looking forward to see where this goes.

_Intersection class is and's whereas _Area is or's. So if you have _Area('blah', ['foo', 'bar']) then it'll match file paths that contain foo or bar whereas _Area('blah', [Intersection('foo', 'bar')]) only matches file paths that contain both foo and bar.

[WebKit Review Bot]

Comment on attachment 115492 [details]
Fixed per comments

Clearing flags on attachment: 115492

Committed r100562: <http://trac.webkit.org/changeset/100562>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.