Bug 90413 - webkit-patch rebaseline-expectations should share code with rebaseline-all
Summary: webkit-patch rebaseline-expectations should share code with rebaseline-all
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
Depends on: 90444
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-02 16:51 PDT by Ojan Vafai
Modified: 2012-07-03 04:09 PDT (History)
5 users (show)

See Also:


Attachments
Patch (22.33 KB, patch)
2012-07-02 16:52 PDT, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch (24.42 KB, patch)
2012-07-02 18:01 PDT, Ojan Vafai
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2012-07-02 16:51:18 PDT
webkit-patch rebaseline-expectations should do the same work as rebaseline-all
Comment 1 Ojan Vafai 2012-07-02 16:52:04 PDT
Created attachment 150499 [details]
Patch
Comment 2 Dirk Pranke 2012-07-02 17:04:55 PDT
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?
Comment 3 Dirk Pranke 2012-07-02 17:05:34 PDT
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.
Comment 4 Ojan Vafai 2012-07-02 18:01:02 PDT
Created attachment 150509 [details]
Patch
Comment 5 Dirk Pranke 2012-07-02 18:29:25 PDT
Comment on attachment 150509 [details]
Patch

All happy. Thanks!
Comment 6 WebKit Review Bot 2012-07-02 20:36:24 PDT
Comment on attachment 150509 [details]
Patch

Clearing flags on attachment: 150509

Committed r121724: <http://trac.webkit.org/changeset/121724>
Comment 7 WebKit Review Bot 2012-07-02 20:36:29 PDT
All reviewed patches have been landed.  Closing bug.