[blink] Pass SkBitmap by reference in WebImageLayer::setBitmap

[blink] Pass SkBitmap by reference in WebImageLayer::setBitmap

Although passing SkBitmap by value doesn't copy the underlying pixels, it indeed involves
memcpy (~64 bytes to be precise) to replicate the settings from the other bitmap in the copy constructor.
Along with this, it has to do some book keeping with respect to reference counts, updating pixel lock count
etc.

This overhead, albeit minor, can be avoided by passing SkBitmap as reference.

R=abarth@chromium.org, eseidel@chromium.org

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

[vivekg, 2014-09-18 18:58:19]

Patchset #1 (id:1) has been deleted

[vivekg, 2014-09-18 19:09:46]

PTAL, thanks!

[vivekg, 2014-09-19 16:40:52]

On 2014/09/18 19:09:46, vivekg_ wrote:
> PTAL, thanks!

@abarth any comments?

[jamesr, 2014-09-22 14:25:43]

jamesr@chromium.org changed reviewers:
+ jamesr@chromium.org

[jamesr, 2014-09-22 14:25:44]

Concept fine, execution needs a bit of tweaking.  Have a non-pure-virtual
function here that does nothing is a dangerous intermediate step.

https://codereview.chromium.org/583733002/diff/20001/public/platform/WebImage...
File public/platform/WebImageLayer.h (right):

https://codereview.chromium.org/583733002/diff/20001/public/platform/WebImage...
public/platform/WebImageLayer.h:40: virtual void setBitmap(SkBitmap) { }
leave a FIXME here indicating what the status of this is supposed to be

https://codereview.chromium.org/583733002/diff/20001/public/platform/WebImage...
public/platform/WebImageLayer.h:41: virtual void setImageBitmap(const SkBitmap&
bitmap) { setBitmap(bitmap); }
if this is the new function, shouldn't this be the other way around? i.e.
setBitmap() calls this function and this function is marked pure virtual (since
you're going to land the chromium side first, right?)

[vivekg, 2014-09-22 14:29:27]

vivekg@chromium.org changed reviewers:
+ vivekg@chromium.org

[vivekg, 2014-09-22 14:29:28]

https://codereview.chromium.org/583733002/diff/20001/public/platform/WebImage...
File public/platform/WebImageLayer.h (right):

https://codereview.chromium.org/583733002/diff/20001/public/platform/WebImage...
public/platform/WebImageLayer.h:40: virtual void setBitmap(SkBitmap) { }
On 2014/09/22 at 14:25:44, jamesr wrote:
> leave a FIXME here indicating what the status of this is supposed to be

Sure, will do! Thank you.

https://codereview.chromium.org/583733002/diff/20001/public/platform/WebImage...
public/platform/WebImageLayer.h:41: virtual void setImageBitmap(const SkBitmap&
bitmap) { setBitmap(bitmap); }
On 2014/09/22 at 14:25:44, jamesr wrote:
> if this is the new function, shouldn't this be the other way around? i.e.
setBitmap() calls this function and this function is marked pure virtual (since
you're going to land the chromium side first, right?)

We have to land this first in blink in any case. As without being virtual,
chromium side changes would be no-op and would either break the build or break
the layout tests.

[jamesr, 2014-09-22 14:45:00]

On 2014/09/22 14:29:28, vivekg_ wrote:
> We have to land this first in blink in any case. As without being virtual,
> chromium side changes would be no-op and would either break the build or break
> the layout tests.

I don't think you should, I think you should land the chromium side first then
land this as a pure virtual.  Why will that not work?

[vivekg, 2014-09-22 14:52:50]

On 2014/09/22 at 14:45:00, jamesr wrote:
> On 2014/09/22 14:29:28, vivekg_ wrote:
> > We have to land this first in blink in any case. As without being virtual,
> > chromium side changes would be no-op and would either break the build or
break
> > the layout tests.
> 
> I don't think you should, I think you should land the chromium side first then
land this as a pure virtual.  Why will that not work?

Ah I got what you are saying. Till now, I was thinking to make this changes like
this in blink. Hence I thought it would break. 
Sorry about the confusion. Here it is:

1. Chromium:
class WebImageLayerImpl : public WebImageLayer {
public:
   virtual void setBitmap(SkBitmap bitmap) { setImageBitmap(bitmap); }
   virtual void setImageBitmap(const SkBitmap& bitmap) { ... }
};

2. Blink:
class WebImageLayer {
public:
    virtual void setImageBitmap(const SkBitmap& bitmap) = 0;
};

and modify the call site in GraphicsLayer.cpp

3. Cleanup chromium.

[jamesr, 2014-09-22 14:55:40]

That seems good.

[vivekg, 2014-09-22 15:11:57]

On 2014/09/22 at 14:55:40, jamesr wrote:
> That seems good.

Thank you. Done the changes, PTAL.

[jamesr, 2014-09-22 17:17:39]

lgtm, but I'm pretty sure you will need to leave the old function around as a
forwarding call in order to land this unless there are no chromium-side callers
of setBitmap()

[vivekg, 2014-09-23 02:26:36]

On 2014/09/22 at 17:17:39, jamesr wrote:
> lgtm, but I'm pretty sure you will need to leave the old function around as a
forwarding call in order to land this unless there are no chromium-side callers
of setBitmap()

Fortunately there are no chromium side call sites using setBitmap [1]. 
Once the chromium side change lands, I will throw this patch for the bots to go
green and then will land it if no failures reported.

[1]
https://code.google.com/p/chromium/codesearch#search/&q=WebImageLayer.h&sq=pa...

[vivekg, 2014-09-23 10:43:24]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2014-09-23 10:44:07]

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

[commit-bot: I haz the power, 2014-09-23 11:02:47]

Committed patchset #3 (id:60001) as 182486