[webkit-reviews] review granted: [Bug 201561] results.webkit.org: Configurations should be branch specific : [Attachment 378414] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Sep 16 09:18:33 PDT 2019


dewei_zhu at apple.com has granted Jonathan Bedard <jbedard at apple.com>'s request
for review:
Bug 201561: results.webkit.org: Configurations should be branch specific
https://bugs.webkit.org/show_bug.cgi?id=201561

Attachment 378414: Patch

https://bugs.webkit.org/attachment.cgi?id=378414&action=review




--- Comment #6 from dewei_zhu at apple.com ---
Comment on attachment 378414
  --> https://bugs.webkit.org/attachment.cgi?id=378414
Patch

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

> Tools/resultsdbpy/resultsdbpy/controller/upload_controller.py:170
> +		   branch=branch[0] if branch else None,

Is there some mechanism to ensure branch is aways a list of string rather than
just a string?

> Tools/resultsdbpy/resultsdbpy/model/configuration_context.py:215
> +    def search_for_recent_configuration(self, configuration=Configuration(),
branch=None):

Just want to point out that the default argument `configuration` will only be
initialized once at the function def time. All invocations to this function
will ref to a single configuration object if default value is used.

> Tools/resultsdbpy/resultsdbpy/model/configuration_context_unittest.py:79
> +		   self.database.register_configuration(configuration, None,
old)

Personally, I would prefer to use `timestamp=old` over passing a None argument.

> Tools/resultsdbpy/resultsdbpy/view/static/js/drawer.js:120
> +let configurationsDefinedCallbacks = [];

Where is this variable updated?

> Tools/resultsdbpy/resultsdbpy/view/static/js/drawer.js:134
> +	       configurations = [];
> +	       json.forEach(pair => {
> +		   const config = new Configuration(pair[0]);
> +		   configurations.push(config);
> +	       });

How about `configurations = json.map(pair => new Configuration(pair[0])`?

> Tools/resultsdbpy/resultsdbpy/view/static/js/drawer.js:135
> +	       configurationsDefinedCallbacks.forEach(callback =>
{callback();});

I think `configurationsDefinedCallbacks.forEach(callback => callback());` would
work.


More information about the webkit-reviews mailing list