[webkit-reviews] review granted: [Bug 88689] webkit-patch land-safely should set cq? if the patch author is not in committers.py : [Attachment 146670] Adds the feature

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has granted Ryosuke Niwa
<rniwa at webkit.org>'s request for review:
Bug 88689: webkit-patch land-safely should set cq? if the patch author is not
in committers.py
https://bugs.webkit.org/show_bug.cgi?id=88689

Attachment 146670: Adds the feature
https://bugs.webkit.org/attachment.cgi?id=146670&action=review

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=146670&action=review


r+ with some minor comments ...

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:518
>      def _commit_queue_flag(self, mark_for_landing, mark_for_commit_queue):

mark_for_landing and mark_for_commit_queue shouldn't be two different
variables, since True/True doesn't make sense. Can we add a FIXME or something
for this? Also it's not clear to me that 'mark_for_commit_queue' doesn't also
mean "cq+"; a better name would be something like "commit_queue_requested"

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:520
> +	       user_is_committer =
self.committers.contributors_by_email_username(self.username)

can you change 'user_is_committer' to 'user' or 'account', since it's not a
boolean, it's an object, and it can represent non-committers?