linux chrome side: http://codereview.chromium.org/1756008/show
Created attachment 54100 [details] try1
changelog has your lowercase tag name stuff in it
Created attachment 54105 [details] clean up changelog
Comment on attachment 54105 [details] clean up changelog r- for swapping the x and y variable names. WebCore/platform/chromium/DragImageChromiumSkia.cpp:79 + for (int x = 0; x < image->height(); ++x) { y for height and x for width would be more typical. I thought the params passed to getaddr32 were reversed and then realized it was the naming. WebKit/chromium/tests/DragImageTest.cpp: + // This is not implemented, so we don't do any output validation. And why don't we do any validation now? WebKit/chromium/ChangeLog:9 + (WebCore::TEST): This isn't really correct. It is typically the function name. WebKit/chromium/ChangeLog:6 + additional tests This feels lacking. Perhaps it would be just as well to omit this text.
(In reply to comment #4) > (From update of attachment 54105 [details]) > r- for swapping the x and y variable names. > > WebCore/platform/chromium/DragImageChromiumSkia.cpp:79 > + for (int x = 0; x < image->height(); ++x) { > y for height and x for width would be more typical. I thought the params passed > to getaddr32 were reversed and then realized it was the naming. oops. will fix. > > WebKit/chromium/tests/DragImageTest.cpp: > + // This is not implemented, so we don't do any output validation. > And why don't we do any validation now? well, a couple reasons. First, there is no DragImage function for checking the value of an individual pixel (so checking pixel values wouldn't be portable across platforms) Second, different platforms may want to handle this function differently, since it is purely aesthetic. I implemented it literally according to the function name, but I think other platforms (particularly mac) might want to do something cooler (like <http://chromium.googlecode.com/issues/attachment?aid=6416915653984701464&name=expected_result.png&inline=1>). Third, pixel tests seem to be a path to pain in general (as you can see with pixel layout tests: trivially correct changes require rebaselining). > > WebKit/chromium/ChangeLog:9 > + (WebCore::TEST): > This isn't really correct. It is typically the function name. I guess the prepare-changelog script doesn't deal well with macros. Should I replace it with a TestGroup.TestCase style name? > > WebKit/chromium/ChangeLog:6 > + additional tests > This feels lacking. Perhaps it would be just as well to omit this text. ok
Created attachment 54635 [details] review feedback
Comment on attachment 54635 [details] review feedback Loved x and y just had a problem with them being reversed. Sorry, but I couldn't parse r and c until you told me it stood for row and column (so use either x and y or row and column).
Created attachment 54653 [details] r->row, c->column
Comment on attachment 54653 [details] r->row, c->column Rejecting patch 54653 from commit-queue. Unexpected failure when landing patch! Please file a bug against webkit-patch. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--test', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 54653, '--parent-command=commit-queue', '--no-update']" exit_code: 1 Logging in as eseidel@chromium.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=54653&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=38008&ctype=xml Processing 1 patch from 1 bug. Cleaning working directory Processing patch 54653 from bug 38008. ERROR: /Users/eseidel/Projects/CommitQueue/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Created attachment 54722 [details] so I managed to bungle the changelog
Comment on attachment 54722 [details] so I managed to bungle the changelog If this doesn't require a re-review, i.e. it is just some ChangeLog fix that you'd do by hand and then submit if you were doing it on your local machine, then don't put it up to re-review and use the time of reviewers that would be spent reviewing other patches :( I suspect you are doing this to use this commit queue. Here's how you can do this: Adjust the patch as if you were going to commit it (Change the "Reviewed by NOBODY (OOPS!)." to whoever reviewed it) and then put the patch in the bug and only mark it as cq+ (but leave the review field blank).
Webkit-patch land-safely will set the reviewer in the changelog for you and the post your partch with cq+ Basically it does exactly what dave said. Except automatically. :)
Created attachment 54761 [details] doing as I'm told
Comment on attachment 54761 [details] doing as I'm told Clearing flags on attachment: 54761 Committed r58641: <http://trac.webkit.org/changeset/58641>
All reviewed patches have been landed. Closing bug.