Bug 49763 - Rebaseline server: add rebaseline queue
Summary: Rebaseline server: add rebaseline queue
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mihai Parparita
URL:
Keywords:
Depends on:
Blocks: 47761
  Show dependency treegraph
 
Reported: 2010-11-18 15:15 PST by Mihai Parparita
Modified: 2010-11-18 20:51 PST (History)
1 user (show)

See Also:


Attachments
Patch (15.96 KB, patch)
2010-11-18 15:20 PST, Mihai Parparita
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Parparita 2010-11-18 15:15:16 PST
Rebaseline server: add rebaseline queue
Comment 1 Mihai Parparita 2010-11-18 15:20:33 PST
Created attachment 74301 [details]
Patch
Comment 2 Tony Chang 2010-11-18 15:42:52 PST
Comment on attachment 74301 [details]
Patch

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

Just some style nits.

> WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/main.js:58
> +    $('toggle-log').addEventListener('click', function() {toggle('log')});

Nit: Spaces in the function body?  You seem to do this in other places.  E.g.,
{ toggle('log'); }

> WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/queue.js:48
> +    this._toggleNode.addEventListener('click', function() {toggle('queue')});

Ditto.

> WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/queue.js:104
> +    if (this._selectNode.selectedIndex == -1) return;

Nit: Move the return to a new line?

> WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/queue.js:118
> +    if (!queueOption) return;

Ditto.

> WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/queue.js:144
> +    // FIXME: actually rebaseline
> +    log('Rebaselining ' + testName + '...');

I see, this is just the UI.  OK.

> WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/util.js:59
> +function log(text, opt_type)

Nit: optType seems a bit more consistent, although I realize optional arguments are a bit different.
Comment 3 Mihai Parparita 2010-11-18 20:49:56 PST
(In reply to comment #2)
> > WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/main.js:58
> > +    $('toggle-log').addEventListener('click', function() {toggle('log')});
> 
> Nit: Spaces in the function body?  You seem to do this in other places.  E.g.,
> { toggle('log'); }

Done.
 
> > WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/queue.js:48
> > +    this._toggleNode.addEventListener('click', function() {toggle('queue')});
> 
> Ditto.

Done.

> > WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/queue.js:104
> > +    if (this._selectNode.selectedIndex == -1) return;
> 
> Nit: Move the return to a new line?

Done.

> > WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/queue.js:118
> > +    if (!queueOption) return;
> 
> Ditto.

Done

> > WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/util.js:59
> > +function log(text, opt_type)
> 
> Nit: optType seems a bit more consistent, although I realize optional arguments are a bit different.

I just dropped the opt_ altogether, in the absence of the JS compiler there isn't really a need to signal that an argument is optional.
Comment 4 Mihai Parparita 2010-11-18 20:51:34 PST
Committed r72364: <http://trac.webkit.org/changeset/72364>