Reland of Use an explicit list of webkit-patch commands instead of using auto-discovery.

Reland of Use an explicit list of webkit-patch commands instead of using auto-discovery. (patchset #1 id:1 of https://codereview.chromium.org/2075073002/ )

Reason for revert:
I believe the problem was that the commands weren't passed to the MultiCommandTool constructor, so command.bind_to_tool(multi_command_tool) wasn't called for the commands.

Original issue's description:
> Revert of Use an explicit list of webkit-patch commands instead of using auto-discovery. (patchset #2 id:20001 of https://codereview.chromium.org/2070333003/ )
>
> Reason for revert:
> Broke rebaseline-o-matic because commands are not bound to tool.
>
> Original issue's description:
> > Use an explicit list of webkit-patch commands instead of using auto-discovery.
> >
> > This is a followup to http://crrev.com/1950773002 and related CLs,
> > which removed remove imports from other __init__.py files; after this,
> > all of the __init__.py files in webkitpy are empty.
> >
> > Before this CL, webkit-patch relies on all commands being imported (by
> > importing the package webkitpy.tool.commands) and then discovering all
> > subclasses of command using the magic method __subclasses__.
> >
> > This CL makes it so that commands that are part of webkit-patch are
> > explicitly listed, and there is no class discovery.
> >
> > BUG=598897
> >
> > Committed: https://crrev.com/b5277f6feda049f0941928ae198ed8fed41e15a2
> > Cr-Commit-Position: refs/heads/master@{#400369}
>
> TBR=dpranke@chromium.org,qyearsley@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=598897
>
> Committed: https://crrev.com/e0afd0716fe7c94d890cf94b8771692bd827884e
> Cr-Commit-Position: refs/heads/master@{#400376}

TBR=dpranke@chromium.org,wangxianzhu@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=598897
Committed: https://crrev.com/1b8faf8a0ea9a61359b5866cacec5d6da573c82a
Cr-Commit-Position: refs/heads/master@{#400487}

[qyearsley, 2016-06-17 16:19:23]

Created Reland of Use an explicit list of webkit-patch commands instead of using
auto-discovery.

[Dirk Pranke, 2016-06-17 16:47:48]

I can't really tell what the fix is here?

[qyearsley, 2016-06-17 16:58:36]

https://codereview.chromium.org/2073893004/diff/130001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/tool/webkit_patch.py (right):

https://codereview.chromium.org/2073893004/diff/130001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/tool/webkit_patch.py:80: ])
Fix is here -- previously, self.commands was set after the MultiCommandTool 
constructor was called, so inside of that constructor, when it hits the lines:

        for command in self.commands:
            command.bind_to_tool(self)

self.commands is empty, and none of the commands got self._tool bound to them.

https://codereview.chromium.org/2073893004/diff/130001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/tool/webkit_patch.py:84: 
Looks like I added an extra newline here -- that should be removed

[Dirk Pranke, 2016-06-17 17:10:31]

okay. why did the unit tests not catch that?

[qyearsley, 2016-06-17 17:41:28]

On 2016/06/17 at 17:10:31, dpranke wrote:
> okay. why did the unit tests not catch that?

Two reasons:
 - Main reason: In the unit test, the commands are passed directly to the
constructor of the example TrivialTool, whereas in the real subclass of
MultiCommandTool, something different was done.
 - Also, the unit test wouldn't have caught it anyway, since of the commands in
the unit test, none of them reference self._tool, except for HelpCommand, which
is set in MultiCommandTool and bound to the tool even if not passed in.

[Dirk Pranke, 2016-06-17 19:09:25]

lgtm w/ potential improvements or follow-up changes.

https://codereview.chromium.org/2073893004/diff/190001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/tool/multi_command_tool.py
(right):

https://codereview.chromium.org/2073893004/diff/190001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/tool/multi_command_tool.py:50:
self.commands = commands or []
can we make name and commands mandatory parameters? Or remove name, since it
seems no one is using it?

Also, weren't we just going to merge this class into webkit_patch.py ?

[qyearsley, 2016-06-17 20:10:06]

https://codereview.chromium.org/2073893004/diff/190001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/tool/multi_command_tool.py
(right):

https://codereview.chromium.org/2073893004/diff/190001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/tool/multi_command_tool.py:50:
self.commands = commands or []
On 2016/06/17 at 19:09:25, Dirk Pranke wrote:
> can we make name and commands mandatory parameters? Or remove name, since it
seems no one is using it?

That makes sense; now done.

> Also, weren't we just going to merge this class into webkit_patch.py ?

Before we merge webkit_patch and multi_command_tool, I want to decide that we're
probably *not* going to have any more "multi-command tools". At this point, it
would be relatively easy to move commands over to separate MultiCommandTool
subclasses, so that we could have multiple tools, e.g.:

  $ Tools/Scripts/rebaseline help
  Usage: rebaseline [options] COMMAND [ARGS]

  Common rebaseline commands:
    from-expectations
    from-json
    from-try-bots
    auto-rebaseline
    rebaseline-server

  $ Tools/Scripts/query help
  Usage: query [options] COMMAND [ARGS]

  Common query commands:
    crash-log
    print-baselines
    print-expectations
    print-flaky-tests

After merging, then it's clearer that there's only one "multi-tool". The
benefits of splitting up the commands that I can think of are:
  (1) making some commands shorter and more memorable
  (2) separating the user interface of commands that serve separate purposes

The disadvantages are:
  (1) this may be a troublesome renaming change in multiple places with no real
practical benefit
  (2) there are a few "miscellaneous commands" with no obvious place to go

Why is the command called "webkit-patch" anyway? I assume this name was made up
a long time ago, before rebaselining became one of the main purposes of the
tool? Even if we keep all these commands under one multi-tool, maybe it would be
nice to rename it (blink-tool? multi-tool?).

[Dirk Pranke, 2016-06-17 20:12:49]

It's called webkit-patch because the original purpose of this tool was to upload
patches to
bugs.webkit.org for review, and then apply and land them.

I expect that almost nobody runs any of these commands any more, and I doubt
more than 
a couple people have *ever* run the query commands. So, I don't think it's worth
spending
any time splitting them into mulltiple tools, we'd just have to retrain people.

[qyearsley, 2016-06-17 20:22:10]

On 2016/06/17 at 20:12:49, dpranke wrote:
> It's called webkit-patch because the original purpose of this tool was to
upload patches to
> bugs.webkit.org for review, and then apply and land them.
> 
> I expect that almost nobody runs any of these commands any more, and I doubt
more than 
> a couple people have *ever* run the query commands. So, I don't think it's
worth spending
> any time splitting them into mulltiple tools, we'd just have to retrain
people.

OK, good to know!

[qyearsley, 2016-06-17 20:22:13]

The CQ bit was checked by qyearsley@chromium.org

[qyearsley, 2016-06-17 20:22:14]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/2073893004/#ps210001
(title: "Remove parameter name, make parameter commands mandatory.")

[commit-bot: I haz the power, 2016-06-17 20:22:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2073893004/210001

[commit-bot: I haz the power, 2016-06-17 20:23:41]

Committed patchset #6 (id:210001)

[commit-bot: I haz the power, 2016-06-17 20:26:21]

Description was changed from

==========
Reland of Use an explicit list of webkit-patch commands instead of using
auto-discovery. (patchset #1 id:1 of https://codereview.chromium.org/2075073002/
)

Reason for revert:
I believe the problem was that the commands weren't passed to the
MultiCommandTool constructor, so command.bind_to_tool(multi_command_tool) wasn't
called for the commands.

Original issue's description:
> Revert of Use an explicit list of webkit-patch commands instead of using
auto-discovery. (patchset #2 id:20001 of
https://codereview.chromium.org/2070333003/ )
> 
> Reason for revert:
> Broke rebaseline-o-matic because commands are not bound to tool.
> 
> Original issue's description:
> > Use an explicit list of webkit-patch commands instead of using
auto-discovery.
> >
> > This is a followup to http://crrev.com/1950773002 and related CLs,
> > which removed remove imports from other __init__.py files; after this,
> > all of the __init__.py files in webkitpy are empty.
> >
> > Before this CL, webkit-patch relies on all commands being imported (by
> > importing the package webkitpy.tool.commands) and then discovering all
> > subclasses of command using the magic method __subclasses__.
> >
> > This CL makes it so that commands that are part of webkit-patch are
> > explicitly listed, and there is no class discovery.
> >
> > BUG=598897
> >
> > Committed: https://crrev.com/b5277f6feda049f0941928ae198ed8fed41e15a2
> > Cr-Commit-Position: refs/heads/master@{#400369}
> 
> TBR=dpranke@chromium.org,qyearsley@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=598897
> 
> Committed: https://crrev.com/e0afd0716fe7c94d890cf94b8771692bd827884e
> Cr-Commit-Position: refs/heads/master@{#400376}

TBR=dpranke@chromium.org,wangxianzhu@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=598897
==========

to

==========
Reland of Use an explicit list of webkit-patch commands instead of using
auto-discovery. (patchset #1 id:1 of https://codereview.chromium.org/2075073002/
)

Reason for revert:
I believe the problem was that the commands weren't passed to the
MultiCommandTool constructor, so command.bind_to_tool(multi_command_tool) wasn't
called for the commands.

Original issue's description:
> Revert of Use an explicit list of webkit-patch commands instead of using
auto-discovery. (patchset #2 id:20001 of
https://codereview.chromium.org/2070333003/ )
>
> Reason for revert:
> Broke rebaseline-o-matic because commands are not bound to tool.
>
> Original issue's description:
> > Use an explicit list of webkit-patch commands instead of using
auto-discovery.
> >
> > This is a followup to http://crrev.com/1950773002 and related CLs,
> > which removed remove imports from other __init__.py files; after this,
> > all of the __init__.py files in webkitpy are empty.
> >
> > Before this CL, webkit-patch relies on all commands being imported (by
> > importing the package webkitpy.tool.commands) and then discovering all
> > subclasses of command using the magic method __subclasses__.
> >
> > This CL makes it so that commands that are part of webkit-patch are
> > explicitly listed, and there is no class discovery.
> >
> > BUG=598897
> >
> > Committed: https://crrev.com/b5277f6feda049f0941928ae198ed8fed41e15a2
> > Cr-Commit-Position: refs/heads/master@{#400369}
>
> TBR=dpranke@chromium.org,qyearsley@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=598897
>
> Committed: https://crrev.com/e0afd0716fe7c94d890cf94b8771692bd827884e
> Cr-Commit-Position: refs/heads/master@{#400376}

TBR=dpranke@chromium.org,wangxianzhu@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=598897

Committed: https://crrev.com/1b8faf8a0ea9a61359b5866cacec5d6da573c82a
Cr-Commit-Position: refs/heads/master@{#400487}
==========

[commit-bot: I haz the power, 2016-06-17 20:26:22]

Patchset 6 (id:??) landed as
https://crrev.com/1b8faf8a0ea9a61359b5866cacec5d6da573c82a
Cr-Commit-Position: refs/heads/master@{#400487}