[webkit-reviews] review granted: [Bug 186291] EWS for security bugs : [Attachment 342907] [Patch] Part 1 - webkitpy and QueueStatusServer

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jun 18 14:05:11 PDT 2018

Lucas Forschler <lforschler at apple.com> has granted Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 186291: EWS for security bugs

Attachment 342907: [Patch] Part 1 - webkitpy and QueueStatusServer


--- Comment #49 from Lucas Forschler <lforschler at apple.com> ---
Comment on attachment 342907
  --> https://bugs.webkit.org/attachment.cgi?id=342907
[Patch] Part 1 - webkitpy and QueueStatusServer

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

r=me after fixing identified concerns.

> Tools/Scripts/webkitpy/common/net/bugzilla/attachment.py:143
> +	   return Attachment(temp, None)

Can you used a named argument form here for the None value?

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:470
> +	       bug['group'] = self._string_contents(group)

I'm not sure if we will ever get a bug with multiple groups. But, if we did, we
might need to handle this smart. 
What does soup.find('group') return if a bug is in multiple groups?
Let's assert if we find more than one group.

> Tools/Scripts/webkitpy/common/net/statusserver.py:54
> +    def set_host(self, host, use_https=False):

is there any reason not to use_https=True by default?

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:160
> +	       # status server.

Can we add a link to the Bugzilla id for future work?

> Tools/Scripts/webkitpy/tool/main.py:50
> +	   make_option("--status-host-uses-https", action="store_true",
default=False, dest="status_host_uses_https", help="Use HTTPS when querying the
status host."),

let's set default=True for this one.

> Tools/Scripts/webkitpy/tool/main.py:104
> +	       pass

Let's log something here rather than pass.

More information about the webkit-reviews mailing list