Use newImageSnapshot() to get an image from a Canvas

Use SkImage::newImageSnapshot() in CanvasIMageSource::getImageSourceForCanvas().

When repeatedly creating a pattern from a canvas or when doing a canvas-to-canvas copy,  additional memory is allocated even if the canvas' content don't change between the calls to createPattern() (cf benchmark in the bug 344804, message #7).

The idea is to use a SkImage obtained by calling newImageSnapshot() on the SkSurface encapsulated by the HTMLCanvasElement. This way a new bitmap is created only if the canvas is changed.

This CL depends on the CLs:
- https://codereview.chromium.org/416543002/ (merged)
- https://codereview.chromium.org/406673003/ for SkImage::isOpaque() (merged)


BUG=344804, 170021, 257428
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182967

[Justin Novosad, 2014-07-02 19:04:18]

https://codereview.chromium.org/358893002/diff/30001/Source/platform/blink_pl...
File Source/platform/blink_platform.gypi (right):

https://codereview.chromium.org/358893002/diff/30001/Source/platform/blink_pl...
Source/platform/blink_platform.gypi:613: 'graphics/ImagePattern.cpp',
Would be clearer to call this SkiaImagePattern

https://codereview.chromium.org/358893002/diff/30001/Source/platform/graphics...
File Source/platform/graphics/ImagePattern.cpp (right):

https://codereview.chromium.org/358893002/diff/30001/Source/platform/graphics...
Source/platform/graphics/ImagePattern.cpp:35: #include <v8.h>
Why is this needed?

https://codereview.chromium.org/358893002/diff/30001/Source/platform/graphics...
File Source/platform/graphics/ImagePattern.h (right):

https://codereview.chromium.org/358893002/diff/30001/Source/platform/graphics...
Source/platform/graphics/ImagePattern.h:26: * OF THIS SOFTWARE, EVEN IF ADVISED
OF THE POSSIBILITY OF SUCH DAMAGE.
For new files use the license header in src/third_party/WebKit/LICENSE

[Rémi Piotaix, 2014-07-02 20:21:33]

https://codereview.chromium.org/358893002/diff/30001/Source/platform/blink_pl...
File Source/platform/blink_platform.gypi (right):

https://codereview.chromium.org/358893002/diff/30001/Source/platform/blink_pl...
Source/platform/blink_platform.gypi:613: 'graphics/ImagePattern.cpp',
On 2014/07/02 19:04:18, junov wrote:
> Would be clearer to call this SkiaImagePattern

Done.

https://codereview.chromium.org/358893002/diff/30001/Source/platform/graphics...
File Source/platform/graphics/ImagePattern.cpp (right):

https://codereview.chromium.org/358893002/diff/30001/Source/platform/graphics...
Source/platform/graphics/ImagePattern.cpp:35: #include <v8.h>
On 2014/07/02 19:04:18, junov wrote:
> Why is this needed?
For: 
L89: v8::Isolate::GetCurrent()..... ?

https://codereview.chromium.org/358893002/diff/30001/Source/platform/graphics...
File Source/platform/graphics/ImagePattern.h (right):

https://codereview.chromium.org/358893002/diff/30001/Source/platform/graphics...
Source/platform/graphics/ImagePattern.h:26: * OF THIS SOFTWARE, EVEN IF ADVISED
OF THE POSSIBILITY OF SUCH DAMAGE.
On 2014/07/02 19:04:18, junov wrote:
> For new files use the license header in src/third_party/WebKit/LICENSE

Done.

[Stephen Chennney, 2014-07-02 20:57:33]

I bit drive-by, sorry, but a couple of things caught my eye.

A meta question: What's the context of this change? Could you expand the CL
description?

On 2014/07/02 20:21:33, Rémi Piotaix wrote:
>
https://codereview.chromium.org/358893002/diff/30001/Source/platform/blink_pl...
> File Source/platform/blink_platform.gypi (right):
> 
>
https://codereview.chromium.org/358893002/diff/30001/Source/platform/blink_pl...
> Source/platform/blink_platform.gypi:613: 'graphics/ImagePattern.cpp',
> On 2014/07/02 19:04:18, junov wrote:
> > Would be clearer to call this SkiaImagePattern
> 
> Done.

We're trying to avoid creating names like SkiaImagePattern, particularly when
there is no ImagePattern and no other implementation of ImagePattern. I suggest
StaticBitmapPattern or StaticPattern, as it uses StaticBitmapImage.

> 
>
https://codereview.chromium.org/358893002/diff/30001/Source/platform/graphics...
> File Source/platform/graphics/ImagePattern.cpp (right):
> 
>
https://codereview.chromium.org/358893002/diff/30001/Source/platform/graphics...
> Source/platform/graphics/ImagePattern.cpp:35: #include <v8.h>
> On 2014/07/02 19:04:18, junov wrote:
> > Why is this needed?
> For: 
> L89: v8::Isolate::GetCurrent()..... ?
> 
>
https://codereview.chromium.org/358893002/diff/30001/Source/platform/graphics...
> File Source/platform/graphics/ImagePattern.h (right):
> 
>
https://codereview.chromium.org/358893002/diff/30001/Source/platform/graphics...
> Source/platform/graphics/ImagePattern.h:26: * OF THIS SOFTWARE, EVEN IF
ADVISED
> OF THE POSSIBILITY OF SUCH DAMAGE.
> On 2014/07/02 19:04:18, junov wrote:
> > For new files use the license header in src/third_party/WebKit/LICENSE
> 
> Done.

[Rémi Piotaix, 2014-07-02 21:40:48]

On 2014/07/02 20:57:33, Stephen Chenney wrote:
> I bit drive-by, sorry, but a couple of things caught my eye.
> 
> A meta question: What's the context of this change? Could you expand the CL
> description?

Sure, no problem. I edited the description and added a link to the related issue
on crbug.
I'm new to blink/skia. Feel free to ask if my description is not good enough or
if i'm mistaken somewhere.

> 
> On 2014/07/02 20:21:33, Rémi Piotaix wrote:
> >
>
https://codereview.chromium.org/358893002/diff/30001/Source/platform/blink_pl...
> > File Source/platform/blink_platform.gypi (right):
> > 
> >
>
https://codereview.chromium.org/358893002/diff/30001/Source/platform/blink_pl...
> > Source/platform/blink_platform.gypi:613: 'graphics/ImagePattern.cpp',
> > On 2014/07/02 19:04:18, junov wrote:
> > > Would be clearer to call this SkiaImagePattern
> > 
> > Done.
> 
> We're trying to avoid creating names like SkiaImagePattern, particularly when
> there is no ImagePattern and no other implementation of ImagePattern. I
suggest
> StaticBitmapPattern or StaticPattern, as it uses StaticBitmapImage.

Ok, I'll rename the class to StaticBitmapPattern in the next patch set.

[Rémi Piotaix, 2014-07-04 18:56:28]

I tried to implement StaticBitmapImage::draw() but I am probably wrong. This is
NOT the final implementation.

[Rémi Piotaix, 2014-07-10 14:16:09]

On 2014/07/04 18:56:28, Rémi Piotaix wrote:
> I tried to implement StaticBitmapImage::draw() but I am probably wrong. This
is
> NOT the final implementation.

New implementation of StaticBitmapImage::draw().

Still waiting the code in https://codereview.chromium.org/345463009 to be merged
in order to be able to link the chrome executable.

[Justin Novosad, 2014-07-29 14:56:54]

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/BitmapPattern.cpp (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/BitmapPattern.cpp:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
Use the new abbreviated header (see
http://www.chromium.org/blink/coding-style#TOC-License)

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/BitmapPattern.cpp:41: namespace WebCore {
WebCore -> blink

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/BitmapPattern.cpp:59: shader = adoptRef(new
SkColorShader(SK_ColorTRANSPARENT));
Why not just return nullptr to make calling code abort the draw?

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/BitmapPattern.cpp:84: bm2.eraseARGB(0x00, 0x00, 0x00,
0x00);
Would be more efficient to clear just the border areas

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/BitmapPattern.cpp:86:
canvas.drawBitmap(m_tileImage->bitmap(), 0, 0);
You should set-up an SkPaint that uses the Src xfermode to do a direct copy,
without alpha blending. That is necessary if you remove the eraseARGB. Since
m_externalMemoryAllocated is a member of the base class, you should managed in a
centralized way in Pattern. Start by making it private and define a
setExternalMemoryAllocated method that takes car of computing the delta and
updating v8

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/BitmapPattern.cpp:92:
v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(m_externalMemoryAllocated);
The adjustment needs to be a delta between the old and new value of
m_externalMemoryAllocated

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/BitmapPattern.cpp:94: return shader;
return shader.release();

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/BitmapPattern.h (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/BitmapPattern.h:39: class PLATFORM_EXPORT BitmapPattern
: public Pattern {
Why is this exported?

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/BitmapPattern.h:41: static PassRefPtr<Pattern>
create(PassRefPtr<Image> tileImage, bool repeatX, bool repeatY)
Should return PassRefPtr<BitmapPattern>. Implicit cast to a base class ptr works
fine

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DImageBufferSurface.h (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/Canvas2DImageBufferSurface.h:34: #include "Image.h"
Avoid unnecessary dependency in header -> use forward declaration.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/Canvas2DImageBufferSurface.h:68: virtual
PassRefPtr<SkImage> newImageSnapshot() const OVERRIDE { return
m_layerBridge->newImageSnapshot(); }
You will have to move implementation to .cpp if Image is forward declared.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/Image.h (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/Image.h:36: #include
"third_party/skia/include/core/SkImage.h"
forward declare instead.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/Image.h:73: virtual PassRefPtr<SkImage> image() {
return nullptr; }
move to .cpp if SkImage is forward declared.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/ImageBufferSurface.h (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/ImageBufferSurface.h:34: #include "Image.h"
No need for this header.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/Pattern.h (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/Pattern.h:35: #include
"platform/graphics/StaticBitmapImage.h"
No need for include.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/Pattern.h:48: virtual ~Pattern();
The pattern destructor should be responsible for for notifying v8 that
externalMemoryAllocated is going to 0.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/Pattern.h:68: int m_externalMemoryAllocated;
should probably be intptr_t.  What type does v8 expect?  Make it private.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/StaticBitmapImage.cpp (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapImage.cpp:50:
m_image->setGpuToCpuCachingMechanism(SkImage::kWholeBitmap_CachingMechanism);
Mechanism -> Mode or Strategy?

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/StaticBitmapImage.h (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapImage.h:35: #include "SkImage.h"
This can be forward declared.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapImage.h:39: namespace WebCore {
WebCore -> blink

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapImage.h:41: class PLATFORM_EXPORT
StaticBitmapImage : public Image {
Does this really need to be exported?

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapImage.h:51: PassRefPtr<SkImage> image()
OVERRIDE { return m_image; }
move to cpp

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapImage.h:54:
StaticBitmapImage(PassRefPtr<SkImage>);
This class should have a virtual destructor (even if empty) to ensure that
m_image is not leaked.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/StaticBitmapPattern.cpp (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.cpp:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
Short header

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.cpp:34: 
No need for 2 blank lines.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.cpp:36: #include "SkCanvas.h"
we now prefer third_party/skia/include/core/...

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.cpp:46: if (image) {
Is this conditional necessary?

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.cpp:59: shader = adoptRef(new
SkColorShader(SK_ColorTRANSPARENT));
why not return null?

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.cpp:84: bm2.eraseARGB(0x00, 0x00,
0x00, 0x00);
could clear just borders

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.cpp:86: m_tileImage->draw(&canvas,
SkIntToScalar(0), SkIntToScalar(0), 0);
use Src xfermode

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.cpp:93:
v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(m_externalMemoryAllocated);
delta?

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/StaticBitmapPattern.h (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
Short header please.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.h:39: class PLATFORM_EXPORT
StaticBitmapPattern : public Pattern {
Why export?

[Rémi Piotaix, 2014-07-29 17:18:24]

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/BitmapPattern.cpp (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/BitmapPattern.cpp:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2014/07/29 14:56:51, junov wrote:
> Use the new abbreviated header (see
> http://www.chromium.org/blink/coding-style#TOC-License)

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/BitmapPattern.cpp:41: namespace WebCore {
On 2014/07/29 14:56:51, junov wrote:
> WebCore -> blink

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/BitmapPattern.cpp:59: shader = adoptRef(new
SkColorShader(SK_ColorTRANSPARENT));
On 2014/07/29 14:56:51, junov wrote:
> Why not just return nullptr to make calling code abort the draw?

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/BitmapPattern.cpp:94: return shader;
On 2014/07/29 14:56:51, junov wrote:
> return shader.release();

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/BitmapPattern.h (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/BitmapPattern.h:39: class PLATFORM_EXPORT BitmapPattern
: public Pattern {
On 2014/07/29 14:56:51, junov wrote:
> Why is this exported?

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/BitmapPattern.h:41: static PassRefPtr<Pattern>
create(PassRefPtr<Image> tileImage, bool repeatX, bool repeatY)
On 2014/07/29 14:56:52, junov wrote:
> Should return PassRefPtr<BitmapPattern>. Implicit cast to a base class ptr
works
> fine

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DImageBufferSurface.h (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/Canvas2DImageBufferSurface.h:34: #include "Image.h"
On 2014/07/29 14:56:52, junov wrote:
> Avoid unnecessary dependency in header -> use forward declaration.

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/Canvas2DImageBufferSurface.h:68: virtual
PassRefPtr<SkImage> newImageSnapshot() const OVERRIDE { return
m_layerBridge->newImageSnapshot(); }
On 2014/07/29 14:56:52, junov wrote:
> You will have to move implementation to .cpp if Image is forward declared.

Canvas2DLayerBridge already include SkImage

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/Image.h (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/Image.h:36: #include
"third_party/skia/include/core/SkImage.h"
On 2014/07/29 14:56:52, junov wrote:
> forward declare instead.

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/Image.h:73: virtual PassRefPtr<SkImage> image() {
return nullptr; }
On 2014/07/29 14:56:52, junov wrote:
> move to .cpp if SkImage is forward declared.

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/ImageBufferSurface.h (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/ImageBufferSurface.h:34: #include "Image.h"
On 2014/07/29 14:56:52, junov wrote:
> No need for this header.

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/Pattern.h (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/Pattern.h:35: #include
"platform/graphics/StaticBitmapImage.h"
On 2014/07/29 14:56:52, junov wrote:
> No need for include.

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/Pattern.h:48: virtual ~Pattern();
On 2014/07/29 14:56:52, junov wrote:
> The pattern destructor should be responsible for for notifying v8 that
> externalMemoryAllocated is going to 0.

Already in Pattern.cpp ?

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/StaticBitmapImage.cpp (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapImage.cpp:50:
m_image->setGpuToCpuCachingMechanism(SkImage::kWholeBitmap_CachingMechanism);
On 2014/07/29 14:56:52, junov wrote:
> Mechanism -> Mode or Strategy?

Strategy :D
Done

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/StaticBitmapImage.h (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapImage.h:35: #include "SkImage.h"
On 2014/07/29 14:56:52, junov wrote:
> This can be forward declared.

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapImage.h:39: namespace WebCore {
On 2014/07/29 14:56:52, junov wrote:
> WebCore -> blink

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapImage.h:41: class PLATFORM_EXPORT
StaticBitmapImage : public Image {
On 2014/07/29 14:56:53, junov wrote:
> Does this really need to be exported?

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapImage.h:51: PassRefPtr<SkImage> image()
OVERRIDE { return m_image; }
On 2014/07/29 14:56:53, junov wrote:
> move to cpp

Seems to work with forward-declaration, no need to move.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapImage.h:54:
StaticBitmapImage(PassRefPtr<SkImage>);
On 2014/07/29 14:56:53, junov wrote:
> This class should have a virtual destructor (even if empty) to ensure that
> m_image is not leaked.

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/StaticBitmapPattern.cpp (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.cpp:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2014/07/29 14:56:53, junov wrote:
> Short header

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.cpp:34: 
On 2014/07/29 14:56:53, junov wrote:
> No need for 2 blank lines.

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.cpp:36: #include "SkCanvas.h"
On 2014/07/29 14:56:53, junov wrote:
> we now prefer third_party/skia/include/core/...

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.cpp:46: if (image) {
On 2014/07/29 14:56:53, junov wrote:
> Is this conditional necessary?

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.cpp:59: shader = adoptRef(new
SkColorShader(SK_ColorTRANSPARENT));
On 2014/07/29 14:56:53, junov wrote:
> why not return null?

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/StaticBitmapPattern.h (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2014/07/29 14:56:53, junov wrote:
> Short header please.

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.h:39: class PLATFORM_EXPORT
StaticBitmapPattern : public Pattern {
On 2014/07/29 14:56:54, junov wrote:
> Why export?

Done.

[Rémi Piotaix, 2014-07-30 18:59:13]

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/BitmapPattern.cpp (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/BitmapPattern.cpp:84: bm2.eraseARGB(0x00, 0x00, 0x00,
0x00);
On 2014/07/29 14:56:51, junov wrote:
> Would be more efficient to clear just the border areas

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/BitmapPattern.cpp:86:
canvas.drawBitmap(m_tileImage->bitmap(), 0, 0);
On 2014/07/29 14:56:51, junov wrote:
> You should set-up an SkPaint that uses the Src xfermode to do a direct copy,
> without alpha blending. That is necessary if you remove the eraseARGB. Since
> m_externalMemoryAllocated is a member of the base class, you should managed in
a
> centralized way in Pattern. Start by making it private and define a
> setExternalMemoryAllocated method that takes car of computing the delta and
> updating v8

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/BitmapPattern.cpp:92:
v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(m_externalMemoryAllocated);
On 2014/07/29 14:56:51, junov wrote:
> The adjustment needs to be a delta between the old and new value of
> m_externalMemoryAllocated

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/Pattern.h (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/Pattern.h:68: int m_externalMemoryAllocated;
On 2014/07/29 14:56:52, junov wrote:
> should probably be intptr_t.  What type does v8 expect?  Make it private.

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
File Source/platform/graphics/StaticBitmapPattern.cpp (right):

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.cpp:59: shader = adoptRef(new
SkColorShader(SK_ColorTRANSPARENT));
On 2014/07/29 17:18:23, Rémi Piotaix wrote:
> On 2014/07/29 14:56:53, junov wrote:
> > why not return null?
> 
> Done.

Reverted, was breaking some tests (and also breaks the 'shader cache' in
Pattern::shader()  )

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.cpp:84: bm2.eraseARGB(0x00, 0x00,
0x00, 0x00);
On 2014/07/29 14:56:53, junov wrote:
> could clear just borders

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.cpp:86: m_tileImage->draw(&canvas,
SkIntToScalar(0), SkIntToScalar(0), 0);
On 2014/07/29 14:56:53, junov wrote:
> use Src xfermode

Done.

https://codereview.chromium.org/358893002/diff/210001/Source/platform/graphic...
Source/platform/graphics/StaticBitmapPattern.cpp:93:
v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(m_externalMemoryAllocated);
On 2014/07/29 14:56:53, junov wrote:
> delta?

Done.

[Rémi Piotaix, 2014-08-04 15:37:29]

New patch: Cache system in SkImage[_Gpu] is now used for shaders created from
SkImage (when needed).

[Justin Novosad, 2014-08-05 17:31:26]

https://codereview.chromium.org/358893002/diff/290001/Source/platform/graphic...
File Source/platform/graphics/Pattern.cpp (right):

https://codereview.chromium.org/358893002/diff/290001/Source/platform/graphic...
Source/platform/graphics/Pattern.cpp:81: PassRefPtr<SkShader>
BitmapBackedPattern::createShader(SkShader::ShaderLocation preferredLocation)
unused argument should be commented-out

https://codereview.chromium.org/358893002/diff/290001/Source/platform/graphic...
Source/platform/graphics/Pattern.cpp:107:
paint.setXfermode(SkXfermode::Create(SkXfermode::kSrc_Mode))->unref();
You should use the version of setXfermode that takes a SkXfermode::Mode instead
of creating an SkXfermode

https://codereview.chromium.org/358893002/diff/290001/Source/platform/graphic...
File Source/platform/graphics/Pattern.h (right):

https://codereview.chromium.org/358893002/diff/290001/Source/platform/graphic...
Source/platform/graphics/Pattern.h:78: class BitmapBackedPattern : public
Pattern {
This should be in its own header and I think it should be called
BitmapPatternBase

[Rémi Piotaix, 2014-08-06 18:09:14]

https://codereview.chromium.org/358893002/diff/290001/Source/platform/graphic...
File Source/platform/graphics/Pattern.cpp (right):

https://codereview.chromium.org/358893002/diff/290001/Source/platform/graphic...
Source/platform/graphics/Pattern.cpp:81: PassRefPtr<SkShader>
BitmapBackedPattern::createShader(SkShader::ShaderLocation preferredLocation)
On 2014/08/05 17:31:26, junov wrote:
> unused argument should be commented-out

Done.

https://codereview.chromium.org/358893002/diff/290001/Source/platform/graphic...
Source/platform/graphics/Pattern.cpp:107:
paint.setXfermode(SkXfermode::Create(SkXfermode::kSrc_Mode))->unref();
On 2014/08/05 17:31:26, junov wrote:
> You should use the version of setXfermode that takes a SkXfermode::Mode
instead
> of creating an SkXfermode

Done.

https://codereview.chromium.org/358893002/diff/290001/Source/platform/graphic...
File Source/platform/graphics/Pattern.h (right):

https://codereview.chromium.org/358893002/diff/290001/Source/platform/graphic...
Source/platform/graphics/Pattern.h:78: class BitmapBackedPattern : public
Pattern {
On 2014/08/05 17:31:26, junov wrote:
> This should be in its own header and I think it should be called
> BitmapPatternBase

Done.

[Justin Novosad, 2014-09-30 17:37:56]

lgtm with minor comments.

We just branched. The time to throw this in is now.

Fire in the hole!

https://codereview.chromium.org/358893002/diff/470001/Source/platform/graphic...
File Source/platform/graphics/Image.cpp (right):

https://codereview.chromium.org/358893002/diff/470001/Source/platform/graphic...
Source/platform/graphics/Image.cpp:248: PassRefPtr<SkImage> Image::image()
Confusing because blink has an Image type.  should call this skImage() IMHO

https://codereview.chromium.org/358893002/diff/470001/Source/platform/graphic...
File Source/platform/graphics/Pattern.h (right):

https://codereview.chromium.org/358893002/diff/470001/Source/platform/graphic...
Source/platform/graphics/Pattern.h:64: bool isRepeatXY() { return (m_repeatMode
& RepeatModeXY) == RepeatModeXY; }
This could just be "return m_repeatMode == RepeatModeXY"

[Rémi Piotaix, 2014-09-30 17:48:12]

https://codereview.chromium.org/358893002/diff/470001/Source/platform/graphic...
File Source/platform/graphics/Image.cpp (right):

https://codereview.chromium.org/358893002/diff/470001/Source/platform/graphic...
Source/platform/graphics/Image.cpp:248: PassRefPtr<SkImage> Image::image()
On 2014/09/30 17:37:56, junov wrote:
> Confusing because blink has an Image type.  should call this skImage() IMHO

Done.

https://codereview.chromium.org/358893002/diff/470001/Source/platform/graphic...
File Source/platform/graphics/Pattern.h (right):

https://codereview.chromium.org/358893002/diff/470001/Source/platform/graphic...
Source/platform/graphics/Pattern.h:64: bool isRepeatXY() { return (m_repeatMode
& RepeatModeXY) == RepeatModeXY; }
On 2014/09/30 17:37:56, junov wrote:
> This could just be "return m_repeatMode == RepeatModeXY"

Done.

[Rémi Piotaix, 2014-09-30 17:48:37]

The CQ bit was checked by piotaixr@chromium.org

[commit-bot: I haz the power, 2014-09-30 17:49:05]

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

[commit-bot: I haz the power, 2014-09-30 19:40:32]

Committed patchset #26 (id:490001) as 182967