Bug 60569

Created attachment 92994 [details] Patch

Comment on attachment 92994 [details] Patch Attachment 92994 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8684083

Comment on attachment 92994 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92994&action=review Doesn't build. > Tools/DumpRenderTree/chromium/ImageDiff.cpp:269 > { avoid abbreviations: "cp"

Created attachment 92996 [details] Patch

Comment on attachment 92996 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92996&action=review In general, this looks fine, mostly just style nits. Out of curiosity, what is this for? > Tools/DumpRenderTree/chromium/ImageDiff.cpp:199 > +#define MAX2(a, b) ((a) < (b) ? (b) : (a)) > +#define MIN2(a, b) ((a) < (b) ? (a) : (b)) Do we really need to use macros here? What's wrong with std::min and std::max? > Tools/DumpRenderTree/chromium/ImageDiff.cpp:203 > +#define MAX3(a, b, c) ((a) < (b) ? MAX2((b), (c)) : MAX2((a), (c))) > +#define RED(a) ((a << 24) >> 24) > +#define GREEN(a) ((a << 16) >> 24) > +#define BLUE(a) ((a << 8) >> 24) I would also just make these functions. If you want you can even mark them as inline. > Tools/DumpRenderTree/chromium/ImageDiff.cpp:214 > + for (int y = 0; y < h; y++) { > + for (int x = 0; x < w; x++) { Nit: WebKit style prefers ++y and ++x. > Tools/DumpRenderTree/chromium/ImageDiff.cpp:433 > + fprintf(stderr, "ImageDiff::diffImages\n"); Did you mean to keep this line here? > Tools/DumpRenderTree/chromium/ImageDiff.cpp:442 > + int fileTypeDotOffset = strrchr(outFile, '.') - outFile; > + strncpy(metadataOutFilename, outFile, fileTypeDotOffset); > + strcpy(metadataOutFilename + fileTypeDotOffset, "-meta.json"); Can we just use std::string here? Something like: std::string outFileString(outFile); outFileString = outFileString.substring(0, outFileString.find('.') - 1) + "-meta.json"; > Tools/DumpRenderTree/chromium/ImageDiff.cpp:443 > + fprintf(stderr, "ImageDiff: metadata in %s\n", metadataOutFilename); Do we need this logging statement? > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:156 > - actual_filename, diff_filename] > + actual_filename, diff_filename, > + '--writePercent', '--weighted'] Can we add a new test case rather than replacing an existing test case?

Created attachment 93760 [details] Patch

After discussions with Tony, ImageDiff writes metric to stdout. Output will be processed by chromium.py (see bug 60957).

Comment on attachment 93760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93760&action=review This change LGTM. Did you want this patch reviewed? If so, please set r? and set cq? if you want someone to commit it for you. > Tools/DumpRenderTree/chromium/ImageDiff.cpp:228 > + for (int y = 0; y < h; y++) { > + for (int x = 0; x < w; x++) { Nit: WebKit style prefers ++y and ++x

Tony, Mike had another feature request that was going to impact this, 60957, and 60964; I uploaded these so that Dirk could look at them for context on our discussion of 60957, but intended to do another round of revision today/tomorrow before I was ready for review, which is why I didn't specify r? on this. Ugh, my careless regression on that Nit. Fixed locally.

Comment on attachment 93760 [details] Patch This patch is meant to work together with 60957 and and 60964, but does not depend on them.

Comment on attachment 93760 [details] Patch Rejecting attachment 93760 [details] from review queue. tomhudson@google.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.

Comment on attachment 93760 [details] Patch Rejecting attachment 93760 [details] from commit-queue. tomhudson@google.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.

Comment on attachment 93760 [details] Patch cq- for the x++/++x nit. If you upload a fixed version, I can cq+ it for you to get it landed.

Created attachment 94830 [details] Patch

Comment on attachment 94830 [details] Patch Rejecting attachment 94830 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'land-a..." exit_code: 1 Last 500 characters of output: WebCore.exp.in M Source/WebCore/ChangeLog M Source/WebKit2/ChangeLog M Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp M Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePrivate.h M Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundle.cpp M Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.h r87335 = 3586f9721775466be55e056bd291dec1aea7b1be (refs/remotes/trunk) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/trunk. Full output: http://queues.webkit.org/results/8734587

This failed because the format of the reviewer line is incorrect. I'll upload another copy with a fixed reviewer.

Created attachment 94881 [details] Patch for landing

The commit-queue encountered the following flaky tests while processing attachment 94881 [details]: http/tests/websocket/tests/long-invalid-header.html bug 54131 (author: abarth@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 94881 [details] Patch for landing Clearing flags on attachment: 94881 Committed r87367: <http://trac.webkit.org/changeset/87367>

All reviewed patches have been landed. Closing bug.