Refactor WebCore::DragImage.

Refactor WebCore::DragImage.

This CL combines DragImage and DragImageChromiumSkia and changes style
to be more object-oriented and use smart pointers (to clarify ownership
semantics). This makes the resulting code smaller and more consistent
with the rest of Blink.

BUG=

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

[jbroman, 2013-06-11 12:59:36]

This should be functionally identical, but I think it's a little cleaner and
more consistent. What do you think?

R=varunjain since you've looked at DragImageRef recently.
R=others because varunjain isn't an OWNER.

[varunjain, 2013-06-13 20:52:47]

On 2013/06/11 12:59:36, jbroman wrote:
> This should be functionally identical, but I think it's a little cleaner and
> more consistent. What do you think?
> 
> R=varunjain since you've looked at DragImageRef recently.
> R=others because varunjain isn't an OWNER.

Awesome! I have been meaning to do exactly this for a while.. Thanks! :)
LGTM

[jbroman, 2013-06-17 16:45:49]

ping for OWNER review

[jamesr, 2013-06-17 17:51:00]

https://codereview.chromium.org/16715002/diff/23001/Source/WebKit/DEPS
File Source/WebKit/DEPS (right):

https://codereview.chromium.org/16715002/diff/23001/Source/WebKit/DEPS#newcode10
Source/WebKit/DEPS:10: "+third_party/skia",
we've been including skia headers with just #include "SkFooBar.h", since
depending on the skia target pulls third_party/skia/*/include/ onto the include
path. I'd rather keep it that way to keep things consistent

https://codereview.chromium.org/16715002/diff/23001/Source/core/page/Frame.cpp
File Source/core/page/Frame.cpp (right):

https://codereview.chromium.org/16715002/diff/23001/Source/core/page/Frame.cp...
Source/core/page/Frame.cpp:664: return PassOwnPtr<DragImage>();
return nullptr;

https://codereview.chromium.org/16715002/diff/23001/Source/core/platform/Drag...
File Source/core/platform/DragImage.cpp (right):

https://codereview.chromium.org/16715002/diff/23001/Source/core/platform/Drag...
Source/core/platform/DragImage.cpp:74: return PassOwnPtr<DragImage>();
nullptr

https://codereview.chromium.org/16715002/diff/23001/Source/core/platform/Drag...
Source/core/platform/DragImage.cpp:218: widthResizeRatio = maxSize.width() /
(float)srcSize.width();
we pretty much never use c-style cases in Chromium or Blink.  make this a
static_cast<float>() or rejigger the types to avoid it. you might be able to use
some of the (Int|Float)Size helpers to cut down on this per-component math a bit

https://codereview.chromium.org/16715002/diff/23001/Source/core/platform/chro...
File Source/core/platform/chromium/ClipboardChromium.cpp (right):

https://codereview.chromium.org/16715002/diff/23001/Source/core/platform/chro...
Source/core/platform/chromium/ClipboardChromium.cpp:337: return
PassOwnPtr<DragImage>();
nullptr

[jbroman, 2013-06-17 18:52:10]

https://codereview.chromium.org/16715002/diff/23001/Source/WebKit/DEPS
File Source/WebKit/DEPS (right):

https://codereview.chromium.org/16715002/diff/23001/Source/WebKit/DEPS#newcode10
Source/WebKit/DEPS:10: "+third_party/skia",
On 2013/06/17 17:51:00, jamesr wrote:
> we've been including skia headers with just #include "SkFooBar.h", since
> depending on the skia target pulls third_party/skia/*/include/ onto the
include
> path. I'd rather keep it that way to keep things consistent

It's not consistent nowadays (if you do a Code Search, it shows we use relative
paths about 2/3 of the time and absolute paths about 1/3 of the time when we
refer to Skia headers, both inside and outside Blink).

My understanding was that we preferred absolute paths and were moving in that
direction as we went along. http://crrev.com/14450004 is one example of a CL
(that predates me) that removed relative paths to Skia headers in one place and
introduced absolute paths instead.

If that's not preferred, we should probably figure out what the rules *are*, so
that it doesn't end up varying simply by reviewer. Thoughts?

https://codereview.chromium.org/16715002/diff/23001/Source/core/page/Frame.cpp
File Source/core/page/Frame.cpp (right):

https://codereview.chromium.org/16715002/diff/23001/Source/core/page/Frame.cp...
Source/core/page/Frame.cpp:664: return PassOwnPtr<DragImage>();
On 2013/06/17 17:51:00, jamesr wrote:
> return nullptr;

Done.

https://codereview.chromium.org/16715002/diff/23001/Source/core/platform/Drag...
File Source/core/platform/DragImage.cpp (right):

https://codereview.chromium.org/16715002/diff/23001/Source/core/platform/Drag...
Source/core/platform/DragImage.cpp:74: return PassOwnPtr<DragImage>();
On 2013/06/17 17:51:00, jamesr wrote:
> nullptr

Done.

https://codereview.chromium.org/16715002/diff/23001/Source/core/platform/Drag...
Source/core/platform/DragImage.cpp:218: widthResizeRatio = maxSize.width() /
(float)srcSize.width();
On 2013/06/17 17:51:00, jamesr wrote:
> we pretty much never use c-style cases in Chromium or Blink.  make this a
> static_cast<float>() or rejigger the types to avoid it. you might be able to
use
> some of the (Int|Float)Size helpers to cut down on this per-component math a
bit

Agreed. This code is only this way because I modified the code of
fitDragImageToMaxSize as little as possible. But if you want the C-style casts
gone in this CL, then gone they shall be.

I don't see a trivial way to use the {Float,Int}Size methods to clean this up,
though.

https://codereview.chromium.org/16715002/diff/23001/Source/core/platform/chro...
File Source/core/platform/chromium/ClipboardChromium.cpp (right):

https://codereview.chromium.org/16715002/diff/23001/Source/core/platform/chro...
Source/core/platform/chromium/ClipboardChromium.cpp:337: return
PassOwnPtr<DragImage>();
On 2013/06/17 17:51:00, jamesr wrote:
> nullptr

Done.

[jbroman, 2013-06-18 23:25:37]

ping jamesr (PTAL)

[jamesr, 2013-06-18 23:31:32]

lgtm

[commit-bot: I haz the power, 2013-06-18 23:33:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbroman@chromium.org/16715002/33001

[commit-bot: I haz the power, 2013-06-19 02:57:22]

Change committed as 152692