Pass InterpolationQuality into DragImage to respect image-rendering:pixelated.

Pass InterpolationQuality into DragImage to respect image-rendering:pixelated.

Previously DragImage would always use RESIZE_LANCZOS3.
It now uses RESIZE_BOX if the interpolation quality is
InterpolationNone.

Layout test depends on this content side CL:
https://codereview.chromium.org/904833004/

BUG=451759
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190023

[jackhou1, 2015-02-05 00:30:59]

jackhou@chromium.org changed reviewers:
+ junov@chromium.org

[jackhou1, 2015-02-05 00:31:00]

junov, could you take a look?

What's the best way to test this?

[dcheng, 2015-02-05 00:34:06]

On 2015/02/05 at 00:31:00, jackhou wrote:
> junov, could you take a look?
> 
> What's the best way to test this?

We can expose a testRunner hook to dump the drag image or something. That's
probably the best approach.

[jackhou1, 2015-02-05 00:42:02]

On 2015/02/05 00:34:06, dcheng wrote:
> On 2015/02/05 at 00:31:00, jackhou wrote:
> > junov, could you take a look?
> > 
> > What's the best way to test this?
> 
> We can expose a testRunner hook to dump the drag image or something. That's
> probably the best approach.

Do you mean something like "testRunner.dumpAsText()" but instead
"testRunner.dumpDragImage(element)"?

Are there any existing tests that do something similar?

[Justin Novosad, 2015-02-05 05:06:24]

https://codereview.chromium.org/886323005/diff/1/Source/core/page/DragControl...
File Source/core/page/DragController.cpp (right):

https://codereview.chromium.org/886323005/diff/1/Source/core/page/DragControl...
Source/core/page/DragController.cpp:829: DragImage::ResizeMethod resizeMethod =
element->computedStyle()->imageRendering() == ImageRenderingPixelated ?
DragImage::ResizeMethod::RESIZE_BOX : DragImage::ResizeMethod::RESIZE_LANCZOS3;

RESIZE_BOX does not exactly match how "image-rendering:pixelated" is handled
elsewhere in the code where InterpolationNone is used. When down-resing,
RESIZE_BOX takes a weighted average rather than nearest neighbor. As far as the
spec is concerned, this is probably okay. But consistency with the rest of blink
is an issue.

https://codereview.chromium.org/886323005/diff/1/Source/platform/DragImage.h
File Source/platform/DragImage.h (right):

https://codereview.chromium.org/886323005/diff/1/Source/platform/DragImage.h#...
Source/platform/DragImage.h:32: #include "skia/ext/image_operations.h"

I realize this dependency is not new to this patch (the cpp file already has
it), but this is an obvious layering violation. You must not use include files
from chromium in blink.  There are two ways around this:
a) use skia directly instead of using chromium's skia wrappers. Blink has it's
own graphics helpers and utilities in Source/platform/graphics.
b) use a dependency inversion pattern to call into chromium the legal way
(example: blink::Platform vs BlinkPlatformImpl)
If we start being cavalier about circular dependencies between blink and
chromium, someday we are going to have a bad time.

I don't want to block this CL on resolving the bad dependency, but I would like
you to avoid spreading it any further. For now, could you just make this
interface use blink's InterpolationQuality enum or skia's SkPaint::FilterLevel?

[Justin Novosad, 2015-02-05 05:17:59]

On 2015/02/05 00:42:02, jackhou1 wrote:
> On 2015/02/05 00:34:06, dcheng wrote:
> > On 2015/02/05 at 00:31:00, jackhou wrote:
> > > junov, could you take a look?
> > > 
> > > What's the best way to test this?
> > 
> > We can expose a testRunner hook to dump the drag image or something. That's
> > probably the best approach.
> 
> Do you mean something like "testRunner.dumpAsText()" but instead
> "testRunner.dumpDragImage(element)"?
> 
> Are there any existing tests that do something similar?

To test DragController, you probably would want Some new testRunner hook, But
for testing DragImage, you could just use a unit test.
The unit test would create a BitmapImage procedurally, and the create a
DragImage from it. Then you can inspect the pixels in the DragImage to verify
that it was scaled and filtered appropriately

[jackhou1, 2015-02-10 01:53:57]

https://codereview.chromium.org/886323005/diff/1/Source/platform/DragImage.h
File Source/platform/DragImage.h (right):

https://codereview.chromium.org/886323005/diff/1/Source/platform/DragImage.h#...
Source/platform/DragImage.h:32: #include "skia/ext/image_operations.h"
On 2015/02/05 05:06:24, junov wrote:
> 
> I realize this dependency is not new to this patch (the cpp file already has
> it), but this is an obvious layering violation. You must not use include files
> from chromium in blink.  There are two ways around this:
> a) use skia directly instead of using chromium's skia wrappers. Blink has it's
> own graphics helpers and utilities in Source/platform/graphics.
> b) use a dependency inversion pattern to call into chromium the legal way
> (example: blink::Platform vs BlinkPlatformImpl)
> If we start being cavalier about circular dependencies between blink and
> chromium, someday we are going to have a bad time.
> 
> I don't want to block this CL on resolving the bad dependency, but I would
like
> you to avoid spreading it any further. For now, could you just make this
> interface use blink's InterpolationQuality enum or skia's
SkPaint::FilterLevel?

Done.

https://codereview.chromium.org/886323005/diff/20001/LayoutTests/fast/images/...
File LayoutTests/fast/images/drag-image.html (right):

https://codereview.chromium.org/886323005/diff/20001/LayoutTests/fast/images/...
LayoutTests/fast/images/drag-image.html:9: testRunner.dumpDragImage();
Added this hook here:
https://codereview.chromium.org/904833004/

[Justin Novosad, 2015-02-10 18:29:18]

lgtm

[jackhou1, 2015-02-11 00:29:11]

New patchsets have been uploaded after l-g-t-m from junov@chromium.org

[Justin Novosad, 2015-02-11 04:30:30]

Patch set 3 lgtm, but rember to wait for the testrunner patch to land first

[Justin Novosad, 2015-02-11 04:30:48]

https://codereview.chromium.org/886323005/diff/40001/LayoutTests/fast/images/...
File LayoutTests/fast/images/drag-image.html (right):

https://codereview.chromium.org/886323005/diff/40001/LayoutTests/fast/images/...
LayoutTests/fast/images/drag-image.html:15: testRunner.dumpDragImage();
Awesome!

[jackhou1, 2015-02-12 03:43:16]

New patchsets have been uploaded after l-g-t-m from junov@chromium.org

[jackhou1, 2015-02-12 03:43:44]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2015-02-12 03:43:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/886323005/60001

[commit-bot: I haz the power, 2015-02-12 05:02:07]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190023