garden-o-matic should have a "rebaseline" button

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

garden-o-matic should have a "rebaseline" button

[Adam Barth]

Created attachment 100651 [details]
work-in-progress

[Adam Barth]

Needs tests.  :)

[Adam Barth]

Created attachment 100774 [details]
Patch

[Adam Barth]

Created attachment 100775 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 100775 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:106
> +        fs = self._tool.filesystem
> +        fs.maybe_make_directory(fs.dirname(target_baseline))
> +        fs.write_binary_file(target_baseline, data)

Eh.  I think filesystem is strictly superior to "fs"

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:134
> +        case IMAGE:
> +            return ['png'];
> +        case TEXT:
> +            return ['txt'];
> +        case IMAGE_TEXT:
> +            return ['txt', 'png'];
> +        default:
> +            // FIXME: Add support for the rest of the result types.
> +            // '-expected.html',
> +            // '-expected-mismatch.html',
> +            // '-expected.wav',
> +            // '-actual.wav',
> +            // ... and possibly more.
> +            return [];

WebKit c++ does not indent case statements.  Seems we should match in JS, no?

[Adam Barth]

Created attachment 100813 [details]
Patch

[Ojan Vafai]

Comment on attachment 100813 [details]
Patch

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

> Tools/ChangeLog:17
> +        2) The baselines are not optimized for redundancy (meaning you can have
> +           identical baselines in both chromium-mac and chromium-win).

Koz is a checkin or two away from having a tool that does this. Then this could use that? We should see if he'd be willing to prioritize getting this done.

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/index.html:23
> +  <div class="toolbar">
> +    <div class="actions">
> +      <a class="rebaseline" href="#">Rebaseline</a>
> +      <a class="dismiss" href="#">Close</a>
> +    </div>
> +    <div class="status"></div>
> +  </div>
> +  <div class="content"></div>

indentation is off here. also, shouldn't these be 4 space indents? i realize we don't have a firm style here, but may as well be consistent on indentation size.

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:126
> +        return ['png'];
> +    case TEXT:
> +        return ['txt'];
> +    case IMAGE_TEXT:
> +        return ['txt', 'png'];

These strings should be constants IMO.

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:144
> +    for (var i= 0; i < failureTypeList.length; ++i) {
> +        if (results.failureTypeToExtensionList(failureTypeList[i]).length > 0)
> +            return true;
> +    }
> +    return false;

https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Array/some

return failureTypeList.some(function(element) {
    return results.failureTypeToExtensionList(failureTypeList[i]).length > 0;
});

[Adam Barth]

(In reply to comment #7)
> (From update of attachment 100813 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100813&action=review
> 
> > Tools/ChangeLog:17
> > +        2) The baselines are not optimized for redundancy (meaning you can have
> > +           identical baselines in both chromium-mac and chromium-win).
> 
> Koz is a checkin or two away from having a tool that does this. Then this could use that? We should see if he'd be willing to prioritize getting this done.

That would be great.  I haven't seen any action on that front in a while.  Is he still working on this?

> > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/index.html:23
> > +  <div class="toolbar">
> > +    <div class="actions">
> > +      <a class="rebaseline" href="#">Rebaseline</a>
> > +      <a class="dismiss" href="#">Close</a>
> > +    </div>
> > +    <div class="status"></div>
> > +  </div>
> > +  <div class="content"></div>
> 
> indentation is off here. also, shouldn't these be 4 space indents? i realize we don't have a firm style here, but may as well be consistent on indentation size.

Sure.

> > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:126
> > +        return ['png'];
> > +    case TEXT:
> > +        return ['txt'];
> > +    case IMAGE_TEXT:
> > +        return ['txt', 'png'];
> 
> These strings should be constants IMO.

kk.

> > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:144
> > +    for (var i= 0; i < failureTypeList.length; ++i) {
> > +        if (results.failureTypeToExtensionList(failureTypeList[i]).length > 0)
> > +            return true;
> > +    }
> > +    return false;
> 
> https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Array/some
> 
> return failureTypeList.some(function(element) {
>     return results.failureTypeToExtensionList(failureTypeList[i]).length > 0;
> });

Yay!  That's way more awesome.

[Adam Barth]

Created attachment 100912 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 100912 [details]
Patch for landing

Clearing flags on attachment: 100912

Committed r91042: <http://trac.webkit.org/changeset/91042>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.