[Webkit-unassigned] [Bug 38008] [chromium] Skia needs to fade DragImages

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38008





--- Comment #5 from Evan Stade <estade at chromium.org>  2010-04-27 16:47:51 PST ---
(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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.