Bug 38008 – [chromium] Skia needs to fade DragImages

• Component: Platform
• Status: RESOLVED FIXED

[Evan Stade]

linux chrome side: http://codereview.chromium.org/1756008/show

[Evan Stade]

Created attachment 54100 [details]
try1

[Evan Martin]

changelog has your lowercase tag name stuff in it

[Evan Stade]

Created attachment 54105 [details]
clean up changelog

[David Levin]

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.

[Evan Stade]

(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

[Evan Stade]

Created attachment 54635 [details]
review feedback

[David Levin]

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).

[Evan Stade]

Created attachment 54653 [details]
r->row, c->column

[WebKit Commit Bot]

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 [e-mail]...
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).

[Evan Stade]

Created attachment 54722 [details]
so I managed to bungle the changelog

[David Levin]

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).

[Eric Seidel (no email)]

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. :)

[Evan Stade]

Created attachment 54761 [details]
doing as I'm told

[WebKit Commit Bot]

Comment on attachment 54761 [details]
doing as I'm told

Clearing flags on attachment: 54761

Committed r58641: <http://trac.webkit.org/changeset/58641>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.