webkit-patch rebaseline-expectations should share code with rebaseline-all

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

webkit-patch rebaseline-expectations should do the same work as rebaseline-all

[Ojan Vafai]

Created attachment 150499 [details]
Patch

[Dirk Pranke]

Comment on attachment 150499 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:321
> +    name = "rebaseline-all"

I wonder if rebaseline-json or rebaseline-from-stdin would be a better name to indicate this wasn't a user-facing command.

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:350
> +    def _rebaseline_port(self, port_name, test_list):

it wasn't at all obvious what this routine was doing or what the difference was between this and _rebaseline(). Can you change the name or add comments? Something like add_port_to_test_list or something? In particular, this method isn't actually rebaselining anything.

Also, I'm not wild about the fact that you're modifying test_list in place (since I don't think we modify dicts passed as input parameters very often, I wasn't expecting it). Does it maybe make more sense to make test_list be an instance variable or to return a modified version from the method instead?

[Dirk Pranke]

it might also be better to change the subject for this bug; I thought at first you were trying to make both commands do the same thing, not share code.

[Ojan Vafai]

Created attachment 150509 [details]
Patch

[Dirk Pranke]

Comment on attachment 150509 [details]
Patch

All happy. Thanks!

[WebKit Review Bot]

Comment on attachment 150509 [details]
Patch

Clearing flags on attachment: 150509

Committed r121724: <http://trac.webkit.org/changeset/121724>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.