Adds WebElement::imageContents()

Adds WebElement::imageContents()

This function returns the image contents for a given WebElement, if there is appropriate image data.
This can be useful for implementing context-menu based actions on images. Currently it only fetches
bitmap data for <img>s, but we could extend it to other image-like nodes if we wanted to.

R=darin@chromium.org, esprehn@chromium.org

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

[jamesr, 2013-07-25 21:49:58]

Darin - I need to write tests, but what do you think of this API?

[darin (slow to review), 2013-07-25 22:18:44]

LGTM

I'm curious about the cases where the image might be null.  I assume if the
image is rendered on the page, then this call will always be successful, right?

[jamesr, 2013-07-25 22:27:19]

I believe this could return an empty WebImage if:

1.) The WebNode is null
2.) The WebNode isn't an HTMLImageElement
3.) The image hasn't successfully loaded
4.) The image's current frame is bogus

In cases where this is called on the context menu node, the most likely failure
mode is probably something that looks like an image but isn't actually an <img>
- i.e. a <div> overlaying the image or an image served up as a background-image
or in a canvas, etc.  For an actual <img> that the user can see I think this'll
always return a real bitmap.

[darin (slow to review), 2013-07-25 22:38:21]

Makes sense.  The DIV overlay case might be one of those annoying things to
workaround for the intended feature.


On Thu, Jul 25, 2013 at 3:27 PM, <jamesr@chromium.org> wrote:

> I believe this could return an empty WebImage if:
>
> 1.) The WebNode is null
> 2.) The WebNode isn't an HTMLImageElement
> 3.) The image hasn't successfully loaded
> 4.) The image's current frame is bogus
>
> In cases where this is called on the context menu node, the most likely
> failure
> mode is probably something that looks like an image but isn't actually an
> <img>
> - i.e. a <div> overlaying the image or an image served up as a
> background-image
> or in a canvas, etc.  For an actual <img> that the user can see I think
> this'll
> always return a real bitmap.
>
>
https://codereview.chromium.**org/19856004/<https://codereview.chromium.org/1...
>

[esprehn, 2013-07-25 22:52:03]

https://codereview.chromium.org/19856004/diff/2001/Source/web/WebNode.cpp
File Source/web/WebNode.cpp (right):

https://codereview.chromium.org/19856004/diff/2001/Source/web/WebNode.cpp#new...
Source/web/WebNode.cpp:269: WebCore::Image* image =
cachedImage->imageForRenderer(renderer);
This should be a method on HTMLImageElement and this code should check
isHTMLImageElement(node) and then cast to that and then do image->imageData() or
something like that.

We're trying to get people to stop digging down into the render tree like this.

[jamesr, 2013-07-26 17:54:33]

https://codereview.chromium.org/19856004/diff/2001/Source/web/WebNode.cpp
File Source/web/WebNode.cpp (right):

https://codereview.chromium.org/19856004/diff/2001/Source/web/WebNode.cpp#new...
Source/web/WebNode.cpp:269: WebCore::Image* image =
cachedImage->imageForRenderer(renderer);
On 2013/07/25 22:52:03, esprehn wrote:
> This should be a method on HTMLImageElement and this code should check
> isHTMLImageElement(node) and then cast to that and then do image->imageData()
or
> something like that.
> 
> We're trying to get people to stop digging down into the render tree like
this.

I don't follow.  This line isn't reaching into the render tree.  We do access
the renderer on line 265, but that's type checked. 
CachedImage::imageForRenderer() is the same as CachedImage::image() except that
it also checks for the image in the SVG image cache (which is keyed by
RenderObject*, for whatever reason) if the image is an SVG image.

If we instead got to the CachedImage by downcasting the Node to HTMLImageElement
this code wouldn't for for elements that generate RenderImages but aren't
HTMLImageElement.  Today, that means HTMLPlugInImageElement and ImageInputType. 
Maybe that'd be OK but it is less general.

[jamesr, 2013-07-30 00:47:52]

This puts the imageContents() logic down in WebCore::Element, where we can
specialize it as necessary.  For now I've just implemented the HTMLImageElement
variant, but we can expand if necessary.  I've also added a really simple unit
test to exercise this codepath.

[esprehn, 2013-07-30 00:49:23]

On 2013/07/30 00:47:52, jamesr wrote:
> This puts the imageContents() logic down in WebCore::Element, where we can
> specialize it as necessary.  For now I've just implemented the
HTMLImageElement
> variant, but we can expand if necessary.  I've also added a really simple unit
> test to exercise this codepath.

LGTM, looks awesome.

[commit-bot: I haz the power, 2013-07-30 01:14:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/19856004/12001

[commit-bot: I haz the power, 2013-07-30 04:28:35]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_blink_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[darin (slow to review), 2013-07-30 05:02:41]

https://codereview.chromium.org/19856004/diff/12001/Source/web/WebNode.cpp
File Source/web/WebNode.cpp (right):

https://codereview.chromium.org/19856004/diff/12001/Source/web/WebNode.cpp#ne...
Source/web/WebNode.cpp:260: RefPtr<Element> element = unwrap<Element>();
nit: perhaps we should have just put this method on WebElement then?  sorry for
not thinking to suggest this earlier :-/

[jamesr1, 2013-07-30 05:06:31]

That'd just move the type check to the caller, wouldn't it?
On Jul 29, 2013 10:02 PM, <darin@chromium.org> wrote:

>
> https://codereview.chromium.**org/19856004/diff/12001/**
>
Source/web/WebNode.cpp<https://codereview.chromium.org/19856004/diff/12001/Source/web/WebNode.cpp>
> File Source/web/WebNode.cpp (right):
>
> https://codereview.chromium.**org/19856004/diff/12001/**
>
Source/web/WebNode.cpp#**newcode260<https://codereview.chromium.org/19856004/diff/12001/Source/web/WebNode.cpp#newcode260>
> Source/web/WebNode.cpp:260: RefPtr<Element> element = unwrap<Element>();
> nit: perhaps we should have just put this method on WebElement then?
> sorry for not thinking to suggest this earlier :-/
>
>
https://codereview.chromium.**org/19856004/<https://codereview.chromium.org/1...
>

[darin (slow to review), 2013-07-30 05:36:48]

Perhaps.  If you get the WebElement using an API that provides you a
WebElement, then there would be no type check.

-Darin


On Mon, Jul 29, 2013 at 10:06 PM, James Robinson <jamesr@google.com> wrote:

> That'd just move the type check to the caller, wouldn't it?
> On Jul 29, 2013 10:02 PM, <darin@chromium.org> wrote:
>
>>
>> https://codereview.chromium.**org/19856004/diff/12001/**
>>
Source/web/WebNode.cpp<https://codereview.chromium.org/19856004/diff/12001/Source/web/WebNode.cpp>
>> File Source/web/WebNode.cpp (right):
>>
>> https://codereview.chromium.**org/19856004/diff/12001/**
>>
Source/web/WebNode.cpp#**newcode260<https://codereview.chromium.org/19856004/diff/12001/Source/web/WebNode.cpp#newcode260>
>> Source/web/WebNode.cpp:260: RefPtr<Element> element = unwrap<Element>();
>> nit: perhaps we should have just put this method on WebElement then?
>> sorry for not thinking to suggest this earlier :-/
>>
>>
https://codereview.chromium.**org/19856004/<https://codereview.chromium.org/1...
>>
>

[jamesr, 2013-07-30 19:31:26]

On 2013/07/30 05:36:48, darin wrote:
> Perhaps.  If you get the WebElement using an API that provides you a
> WebElement, then there would be no type check.

The immediate user will be one that has a WebNode (it'll use the
content::RenderViewImpl::context_menu_node_), but maybe that's not right.  I
agree having this on WebElement makes more logical sense.  Will move it down.

[darin (slow to review), 2013-07-30 19:33:04]

It's possible that the context node should really be a WebElement as well.
 Although perhaps it makes sense for the context node to be something that
is not an element?  Hmm... text nodes?

-Darin


On Tue, Jul 30, 2013 at 12:31 PM, <jamesr@chromium.org> wrote:

> On 2013/07/30 05:36:48, darin wrote:
>
>> Perhaps.  If you get the WebElement using an API that provides you a
>> WebElement, then there would be no type check.
>>
>
> The immediate user will be one that has a WebNode (it'll use the
> content::RenderViewImpl::**context_menu_node_), but maybe that's not
> right.  I
> agree having this on WebElement makes more logical sense.  Will move it
> down.
>
>
https://codereview.chromium.**org/19856004/<https://codereview.chromium.org/1...
>

[darin (slow to review), 2013-07-30 19:38:47]

LGTM, just fix nits:

https://codereview.chromium.org/19856004/diff/35001/Source/web/WebNode.cpp
File Source/web/WebNode.cpp (right):

https://codereview.chromium.org/19856004/diff/35001/Source/web/WebNode.cpp#ne...
Source/web/WebNode.cpp:52: #include "core/rendering/RenderImage.h"
nit: not needed

https://codereview.chromium.org/19856004/diff/35001/public/web/WebNode.h
File public/web/WebNode.h (right):

https://codereview.chromium.org/19856004/diff/35001/public/web/WebNode.h#newc...
public/web/WebNode.h:35: #include "../platform/WebImage.h"
nit: not needed

[jamesr, 2013-07-30 19:39:50]

On 2013/07/30 19:33:04, darin wrote:
> It's possible that the context node should really be a WebElement as well.
>  Although perhaps it makes sense for the context node to be something that
> is not an element?  Hmm... text nodes?

I think it's reasonable for the context node to be a node, but for this feature
something somewhere is going to have to try to figure out if the context node
itself is the right place to get the image from.  I.e., if there's some text
overlaying an <img>, do I really mean that I want the <img>?  This'll probably
have to be a pile of heuristics, and I'm not sure yet whether this belongs on
the Blink or Chromium side.  I'm leaning towards it belonging on the Chromium
side if it does end up being a feature-specific set of heuristics.

[darin (slow to review), 2013-07-30 19:41:52]

Yeah, makes sense.  I assume the plan is to worry about such heuristics
later.

-Darin


On Tue, Jul 30, 2013 at 12:39 PM, <jamesr@chromium.org> wrote:

> On 2013/07/30 19:33:04, darin wrote:
>
>> It's possible that the context node should really be a WebElement as well.
>>   Although perhaps it makes sense for the context node to be something
>> that
>> is not an element?  Hmm... text nodes?
>>
>
> I think it's reasonable for the context node to be a node, but for this
> feature
> something somewhere is going to have to try to figure out if the context
> node
> itself is the right place to get the image from.  I.e., if there's some
> text
> overlaying an <img>, do I really mean that I want the <img>?  This'll
> probably
> have to be a pile of heuristics, and I'm not sure yet whether this belongs
> on
> the Blink or Chromium side.  I'm leaning towards it belonging on the
> Chromium
> side if it does end up being a feature-specific set of heuristics.
>
>
https://codereview.chromium.**org/19856004/<https://codereview.chromium.org/1...
>

[jamesr, 2013-07-30 21:36:44]

Committed patchset #5 manually as r155179 (presubmit successful).