[webkit-reviews] review denied: [Bug 65171] CSS Regions build bot should build with "--css-regions" and "--css-exclusion" flags : [Attachment 102034] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 65171: CSS Regions build bot should build with "--css-regions" and
"--css-exclusion" flags
https://bugs.webkit.org/show_bug.cgi?id=65171

Attachment 102034: Patch
https://bugs.webkit.org/attachment.cgi?id=102034&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=102034&action=review


> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:728
> +	   if isinstance(features, list) == False:

Better WebKit style would be:

if not isinstance(features, list):

We don't have this kind of checking for other parts of config.json. Why is this
attribute special?

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:729
> +	       raise Exception, "Builder %r: 'features' parameter needs to be a
list of flags that will be passed to build-webkit script. eg. features:
['css-regions']" % builder['name']

I think the more modern way to do this is using constructor syntax: raise
Exception("Builder %r:...")