Initial implementation of display list backed 2D canvases

Initial implementation of display list backed 2D canvases

This change adds a new type of ImageBufferSurface that is backed by
an SkPicture. Instead of rasterizing the canvas on the main thread,
the contents of the SkPicture are fed into the SkPicture of the
containing layer, thus allowing the content to be rasterized by the
compositor.

There are cases that break this rendering model, for example when
a readback is performed on the canvas (e.g. call to getImageData)
When that happens, the canvas backing falls back to being rasterized
on the main thread.

This feature is currently disabled by default and controlled through
web settings. A follow-up chromium patch will expose the feature as an
experiment that can be enable in chrome://flags or at the command line.

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

[Justin Novosad, 2014-07-09 18:14:36]

Please take a look

[Stephen White, 2014-07-09 20:04:08]

This looks really great. Clean and comprehensible.

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.h:99: void resetCanvas(SkCanvas*
canvas)
Nit: probably no reason for this to be inlined. I'd make it out-of-line.

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/ImageBufferSurface.h (right):

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/ImageBufferSurface.h:76: virtual PassRefPtr<SkPicture>
getPicture();
Nit: all the other "{ return 0; }" virtuals above are inline; I'd just make this
one inline too (even though inline virtual is often useless from a perf
standpoint, might as well be consistent.)

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/RecordingImageBufferSurface.cpp (right):

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:46: , m_imageBuffer(0)
You only ever seem to use m_imageBuffer->context() in this class. Could
RecordingImageBufferSurface hold a pointer to the GraphicsContext instead? Would
be less circular.

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:116: return
m_previousFrame;
As discussed, it would be nice to handle the case where the canvas hasn't
changed, but is being repainted for other reasons (scrolling, new dirty rect,
etc). We should be able to keep around the SkPicture and repaint it again in
that case.

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:138: // TODO: handle
transferring complex state from the current
Pls add a bug for this.

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:142: if
(!oldCanvas->isClipRect())
You should probably also check that the clip op is a non-inverted one here.

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/RecordingImageBufferSurface.h (right):

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.h:57: void
fallbackToRasterCanvas();
Bikeshed: "fallback" is a noun; I'd use "fallBackToRasterCanvas" here.

[dshwang, 2014-07-10 13:01:52]

wow, you initiated threaded canvas 2d. I will ask some questions in the tracking
issue.

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/RecordingImageBufferSurface.cpp (right):

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:62:
m_imageBuffer->context()->setTrackOpaqueRegion(true);
how much overhead when we turn on TrackOpaqueRegion? Can we ignore them?

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:114: {
Following code might be needed, because we cannot merge opaque frame with alpha
frame.
if (m_rasterCanvas)
    return nullptr;

[Justin Novosad, 2014-07-11 21:16:08]

New Patch

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.h:99: void resetCanvas(SkCanvas*
canvas)
On 2014/07/09 20:04:07, Stephen White wrote:
> Nit: probably no reason for this to be inlined. I'd make it out-of-line.

Done.

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/ImageBufferSurface.h (right):

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/ImageBufferSurface.h:76: virtual PassRefPtr<SkPicture>
getPicture();
On 2014/07/09 20:04:07, Stephen White wrote:
> Nit: all the other "{ return 0; }" virtuals above are inline; I'd just make
this
> one inline too (even though inline virtual is often useless from a perf
> standpoint, might as well be consistent.)

The reason it is not inline is that returning a PassRefPtr<SkPicture> cannot be
done when SkPicture is only forward declared (destructor of PassRefPtr needs
access to deref())

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/RecordingImageBufferSurface.cpp (right):

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:46: , m_imageBuffer(0)
On 2014/07/09 20:04:08, Stephen White wrote:
> You only ever seem to use m_imageBuffer->context() in this class. Could
> RecordingImageBufferSurface hold a pointer to the GraphicsContext instead?
Would
> be less circular.

Technically that would be just as circular IMHO (although by proxy). Also we
already have a setImageBuffer virtual method on ImageBufferSurface, because
Canvas2DImageBufferSurface also needs access to the ImageBuffer.

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:62:
m_imageBuffer->context()->setTrackOpaqueRegion(true);
On 2014/07/10 13:01:52, dshwang wrote:
> how much overhead when we turn on TrackOpaqueRegion? Can we ignore them?

It is low overhead. This method only tracks simple geometry (rectangles) and it
conservatively does not mark regions as opaque when encountering text, complex
paths, etc.  That is good enough for our purpose.

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:114: {
On 2014/07/10 13:01:52, dshwang wrote:
> Following code might be needed, because we cannot merge opaque frame with
alpha
> frame.
> if (m_rasterCanvas)
>     return nullptr;

Your logic is correct, but the check is not needed because it is redundant with
the if (!m_currentFrame) check in handleOpaqueFrame.

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:116: return
m_previousFrame;
On 2014/07/09 20:04:08, Stephen White wrote:
> As discussed, it would be nice to handle the case where the canvas hasn't
> changed, but is being repainted for other reasons (scrolling, new dirty rect,
> etc). We should be able to keep around the SkPicture and repaint it again in
> that case.
Agreed. I filed a bug about this so we can deal with it in a follow-up patch:
crbug.com/392612

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:138: // TODO: handle
transferring complex state from the current
On 2014/07/09 20:04:08, Stephen White wrote:
> Pls add a bug for this.

Done.

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:142: if
(!oldCanvas->isClipRect())
On 2014/07/09 20:04:08, Stephen White wrote:
> You should probably also check that the clip op is a non-inverted one here.

Already covered by isClipRect. IsClipRect checks whether the clip region
(resulting from the fill rule) is a rectangle.

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/RecordingImageBufferSurface.h (right):

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.h:57: void
fallbackToRasterCanvas();
On 2014/07/09 20:04:08, Stephen White wrote:
> Bikeshed: "fallback" is a noun; I'd use "fallBackToRasterCanvas" here.

Done.

[dshwang, 2014-07-14 11:45:40]

This looks really great. Few nits.

https://codereview.chromium.org/379253002/diff/40001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/379253002/diff/40001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.cpp:162: m_opaqueRegion.reset();
Don't we need to reset m_paintState, m_paintStateIndex, m_pendingCanvasSave and
etc? If not, imho this api is difficult to use.
How about creating new GraphicsContext when creating new m_currentContext?

https://codereview.chromium.org/379253002/diff/40001/Source/platform/graphics...
File Source/platform/graphics/RecordingImageBufferSurface.cpp (right):

https://codereview.chromium.org/379253002/diff/40001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:29: */
nits: Use brand-new license.

https://codereview.chromium.org/379253002/diff/40001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:83: if (m_rasterCanvas)
How about adding ASSERT(!m_currentFrame);?

https://codereview.chromium.org/379253002/diff/40001/Source/platform/graphics...
File Source/platform/graphics/RecordingImageBufferSurface.h (right):

https://codereview.chromium.org/379253002/diff/40001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.h:51: virtual bool
isValid() const OVERRIDE { return true; }
AFAIK, chromium coding style prohibits virtual inline implementation.

[Stephen White, 2014-07-14 15:19:16]

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/ImageBufferSurface.h (right):

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/ImageBufferSurface.h:76: virtual PassRefPtr<SkPicture>
getPicture();
On 2014/07/11 21:16:07, junov wrote:
> On 2014/07/09 20:04:07, Stephen White wrote:
> > Nit: all the other "{ return 0; }" virtuals above are inline; I'd just make
> this
> > one inline too (even though inline virtual is often useless from a perf
> > standpoint, might as well be consistent.)
> 
> The reason it is not inline is that returning a PassRefPtr<SkPicture> cannot
be
> done when SkPicture is only forward declared (destructor of PassRefPtr needs
> access to deref())

Acknowledged.

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/RecordingImageBufferSurface.cpp (right):

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:46: , m_imageBuffer(0)
On 2014/07/11 21:16:08, junov wrote:
> On 2014/07/09 20:04:08, Stephen White wrote:
> > You only ever seem to use m_imageBuffer->context() in this class. Could
> > RecordingImageBufferSurface hold a pointer to the GraphicsContext instead?
> Would
> > be less circular.
> 
> Technically that would be just as circular IMHO (although by proxy). Also we
> already have a setImageBuffer virtual method on ImageBufferSurface, because
> Canvas2DImageBufferSurface also needs access to the ImageBuffer.

I realize that, but this class only needs the GraphicsContext. It would save you
repeated indirections below. Not a big deal, though.

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:142: if
(!oldCanvas->isClipRect())
On 2014/07/11 21:16:08, junov wrote:
> On 2014/07/09 20:04:08, Stephen White wrote:
> > You should probably also check that the clip op is a non-inverted one here.
> 
> Already covered by isClipRect. IsClipRect checks whether the clip region
> (resulting from the fill rule) is a rectangle.

Acknowledged.

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/RecordingImageBufferSurface.h (right):

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.h:57: void
fallbackToRasterCanvas();
On 2014/07/11 21:16:08, junov wrote:
> On 2014/07/09 20:04:08, Stephen White wrote:
> > Bikeshed: "fallback" is a noun; I'd use "fallBackToRasterCanvas" here.
> 
> Done.

Still seems to be "fallback" in the lastest patch?

https://codereview.chromium.org/379253002/diff/40001/Source/platform/graphics...
File Source/platform/graphics/RecordingImageBufferSurface.cpp (right):

https://codereview.chromium.org/379253002/diff/40001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:29: */
On 2014/07/14 11:45:40, dshwang wrote:
> nits: Use brand-new license.

Good point; see http://www.chromium.org/blink/coding-style#TOC-License.

https://codereview.chromium.org/379253002/diff/40001/Source/platform/graphics...
File Source/platform/graphics/RecordingImageBufferSurface.h (right):

https://codereview.chromium.org/379253002/diff/40001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.h:51: virtual bool
isValid() const OVERRIDE { return true; }
On 2014/07/14 11:45:40, dshwang wrote:
> AFAIK, chromium coding style prohibits virtual inline implementation.

For now, we still use the Blink coding style guidelines in Blink.

[Justin Novosad, 2014-07-17 18:34:14]

New Patch.

+jamesr for Source/web and public/web

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/RecordingImageBufferSurface.h (right):

https://codereview.chromium.org/379253002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.h:57: void
fallbackToRasterCanvas();
On 2014/07/14 15:19:15, Stephen White wrote:
> On 2014/07/11 21:16:08, junov wrote:
> > On 2014/07/09 20:04:08, Stephen White wrote:
> > > Bikeshed: "fallback" is a noun; I'd use "fallBackToRasterCanvas" here.
> > 
> > Done.
> 
> Still seems to be "fallback" in the lastest patch?

I changed it everywhere else LOL. That's why all the try jobs failed.

https://codereview.chromium.org/379253002/diff/40001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/379253002/diff/40001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.cpp:162: m_opaqueRegion.reset();
On 2014/07/14 11:45:40, dshwang wrote:
> Don't we need to reset m_paintState, m_paintStateIndex, m_pendingCanvasSave
and
> etc? If not, imho this api is difficult to use.
> How about creating new GraphicsContext when creating new m_currentContext?

To be spec compliant, we have to retain the state. The
recordingImageBufferSurface is responsible for transferring the state from the
old SkCanvas to the new one so that it remains in sync with the GraphicsContext.

https://codereview.chromium.org/379253002/diff/40001/Source/platform/graphics...
File Source/platform/graphics/RecordingImageBufferSurface.cpp (right):

https://codereview.chromium.org/379253002/diff/40001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:29: */
On 2014/07/14 15:19:15, Stephen White wrote:
> On 2014/07/14 11:45:40, dshwang wrote:
> > nits: Use brand-new license.
> 
> Good point; see http://www.chromium.org/blink/coding-style#TOC-License.

Done.

https://codereview.chromium.org/379253002/diff/40001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:83: if (m_rasterCanvas)
On 2014/07/14 11:45:40, dshwang wrote:
> How about adding ASSERT(!m_currentFrame);?

Done.

https://codereview.chromium.org/379253002/diff/40001/Source/platform/graphics...
File Source/platform/graphics/RecordingImageBufferSurface.h (right):

https://codereview.chromium.org/379253002/diff/40001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.h:51: virtual bool
isValid() const OVERRIDE { return true; }
On 2014/07/14 15:19:15, Stephen White wrote:
> On 2014/07/14 11:45:40, dshwang wrote:
> > AFAIK, chromium coding style prohibits virtual inline implementation.
> 
> For now, we still use the Blink coding style guidelines in Blink.
There was a thread on blink-dev (or was it chromium-dev?) last year on this
where the conclusion was that this style rule is obsolete.

[Stephen White, 2014-07-17 18:55:48]

LGTM with nit

https://codereview.chromium.org/379253002/diff/80001/Source/platform/graphics...
File Source/platform/graphics/RecordingImageBufferSurface.cpp (right):

https://codereview.chromium.org/379253002/diff/80001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:44:
m_graphicsContext->setTrackOpaqueRegion(true);
Either the ": 0" clause above can never happen (and should assert instead), or
this indirection needs a NULL ptr check as well. Pick one. :)

[Justin Novosad, 2014-07-17 19:19:10]

Thanks. Still need approval for Source/web and public/web

https://codereview.chromium.org/379253002/diff/80001/Source/platform/graphics...
File Source/platform/graphics/RecordingImageBufferSurface.cpp (right):

https://codereview.chromium.org/379253002/diff/80001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:44:
m_graphicsContext->setTrackOpaqueRegion(true);
On 2014/07/17 18:55:48, Stephen White wrote:
> Either the ": 0" clause above can never happen (and should assert instead), or
> this indirection needs a NULL ptr check as well. Pick one. :)

Done.

[jamesr, 2014-07-17 20:04:39]

WebSettings are per-WebView.  Can this value really be different for different
WebViews in the same process?  What would happen if you migrated a <canvas> from
a document where this setting is true to one where it is false or did a
drawImage() between display list and non display list canvases?

If you just want to flip this globally (which I suspect you do) then this should
be on a global thing, not WebSettings.

[Justin Novosad, 2014-07-17 20:32:07]

On 2014/07/17 20:04:39, jamesr wrote:
> WebSettings are per-WebView.  Can this value really be different for different
> WebViews in the same process?  What would happen if you migrated a <canvas>
from
> a document where this setting is true to one where it is false or did a
> drawImage() between display list and non display list canvases?
> 
> If you just want to flip this globally (which I suspect you do) then this
should
> be on a global thing, not WebSettings.

Conceptually, this is indeed a global option.  But so are many other things I
see in this file.  Do you feel that WebRuntimeFeatures would be a more
appropriate location?
The logic for enabling this option (on the chromium side) is going to depend on
the state of accelerated 2D canvas and gpu rasterization. Those options live
here in WebSettings.

[jamesr, 2014-07-17 20:37:39]

On 2014/07/17 20:32:07, junov wrote:
> On 2014/07/17 20:04:39, jamesr wrote:
> > WebSettings are per-WebView.  Can this value really be different for
different
> > WebViews in the same process?  What would happen if you migrated a <canvas>
> from
> > a document where this setting is true to one where it is false or did a
> > drawImage() between display list and non display list canvases?
> > 
> > If you just want to flip this globally (which I suspect you do) then this
> should
> > be on a global thing, not WebSettings.
> 
> Conceptually, this is indeed a global option.  But so are many other things I
> see in this file.  Do you feel that WebRuntimeFeatures would be a more
> appropriate location?

If it's truly global, yes.

> The logic for enabling this option (on the chromium side) is going to depend
on
> the state of accelerated 2D canvas and gpu rasterization. Those options live
> here in WebSettings.

Should they?

[Justin Novosad, 2014-07-17 20:41:12]

On 2014/07/17 20:37:39, jamesr wrote:
> Should they?
Great question.
Announcing project GTFOOWS :-P

[Justin Novosad, 2014-07-18 18:29:10]

It's done. It is now a RuntimeEnabledFeature.

[Sergey, 2014-07-19 10:52:21]

Hello.

I really admire the overall idea, but it seems that this patch as is degrades
Canvas 2D performance on mobile devices :(

Browsermark 2.1, canvas test: 710 -> 440.
http://ie.microsoft.com/testdrive/mobile/Performance/FishIETank/Default.html: 59
fps -> 54-55 fps and jagged motions

What I did is:
1. Took relatively fresh chromium shell
(https://chromium.googlesource.com/chromium/src.git, SHA
e23ee89aff056caf911a0853267cc978eb67e5fc).
2. Build chrome shell.
3. Apply this patch and change 'if
(RuntimeEnabledFeatures::displayList2dCanvasEnabled()) {' to 'if (true) {'.
4. Build patched chrome shell.
5. Run tests using old binary and new binary on Note 3.

I think we should make a list of the performance/crash problems for this and I
wonder where should be an appropriate place for it?
https://code.google.com/p/chromium/issues/detail?id=392617 ? Actually, it is not
just a thought, but I would like to have a list like that to ensure that the
final patch won't give any stability issues and/or performance degradations.

And one more note: c5bench.com does not pass to the end of the test for both
binaries. It is probably that issue which I already started:
https://code.google.com/p/chromium/issues/detail?id=393916, just letting you
know about it.

[Sergey, 2014-07-21 04:57:11]

A couple of questions to improve my understanding of blink source code common
practices and a suggestion. Please, follow up :)

https://codereview.chromium.org/379253002/diff/100001/Source/platform/graphic...
File Source/platform/graphics/ImageBufferSurface.cpp (left):

https://codereview.chromium.org/379253002/diff/100001/Source/platform/graphic...
Source/platform/graphics/ImageBufferSurface.cpp:63: ASSERT(canvas());
This is probably a newbie comment, but still, why would you delete this ASSERT?

https://codereview.chromium.org/379253002/diff/120001/Source/platform/graphic...
File Source/platform/graphics/ImageBufferSurface.h (left):

https://codereview.chromium.org/379253002/diff/120001/Source/platform/graphic...
Source/platform/graphics/ImageBufferSurface.h:57: virtual ~ImageBufferSurface()
{ }
Is this just for the refactoring? The destructor is still empty, so I wonder, if
this change is actually necessary.

https://codereview.chromium.org/379253002/diff/120001/Source/platform/graphic...
File Source/platform/graphics/RecordingImageBufferSurface.h (right):

https://codereview.chromium.org/379253002/diff/120001/Source/platform/graphic...
Source/platform/graphics/RecordingImageBufferSurface.h:24: 
Could you please group and add a comment which of these methods are derived from
ImageBufferSurface, like it is done in
https://chromium.googlesource.com/chromium/blink/+/master/Source/platform/gra...
?

[Justin Novosad, 2014-07-21 16:58:30]

On 2014/07/19 10:52:21, Sergey wrote:
> Hello.
> 
> I really admire the overall idea, but it seems that this patch as is degrades
> Canvas 2D performance on mobile devices :(

That is because in the initial implementation, the playback always uses software
rasterization when the fallback is triggered. There is a follow-up issue to fix
that.  Right now most canvas demos trigger the fallback path, which also
something that will be fixed. A demo that does not trigger the fallback is
http://ie.microsoft.com/testdrive/performance/penguinmark/

Also, for canvases to be GPU-accelerated with display list canvases enbaled, you
need to make sure the you are running with impl-side painting and GPU
rasterization enabled.  IF that is not the case, you can expect things to be
slower with this patch applied.

> 
> I think we should make a list of the performance/crash problems for this and I
> wonder where should be an appropriate place for it?

Right now, if you look at issue 386601, you will see bunch of issue in the
"blocked on" section. These are all the related issues that must be fixed before
shipping this feature.  So any related problems should be entered as separate
issues and marked as blocking 386601.

> 
> And one more note: http://c5bench.com does not pass to the end of the test for
both
> binaries. It is probably that issue which I already started:
> https://code.google.com/p/chromium/issues/detail?id=393916, just letting you
> know about it.

[Justin Novosad, 2014-07-21 17:26:46]

https://codereview.chromium.org/379253002/diff/100001/Source/platform/graphic...
File Source/platform/graphics/ImageBufferSurface.cpp (left):

https://codereview.chromium.org/379253002/diff/100001/Source/platform/graphic...
Source/platform/graphics/ImageBufferSurface.cpp:63: ASSERT(canvas());
On 2014/07/21 04:57:10, Sergey wrote:
> This is probably a newbie comment, but still, why would you delete this
ASSERT?

That is a good question while writing this patch, at some point, the code was in
a state where canvas() could return a null pointer.  That is no longer the case
with the current implementation, so I should put it back.  Thanks for noticing.

https://codereview.chromium.org/379253002/diff/120001/Source/platform/graphic...
File Source/platform/graphics/ImageBufferSurface.h (left):

https://codereview.chromium.org/379253002/diff/120001/Source/platform/graphic...
Source/platform/graphics/ImageBufferSurface.h:57: virtual ~ImageBufferSurface()
{ }
On 2014/07/21 04:57:10, Sergey wrote:
> Is this just for the refactoring? The destructor is still empty, so I wonder,
if
> this change is actually necessary.

This happened for a similar reason. In an earlier version of this patch, the
destructor was de-referencing ref counted pointers, which forced me to move it
to the .cpp.  This is no longer the case, so I can put it back how it was. 
Thanks.

https://codereview.chromium.org/379253002/diff/120001/Source/platform/graphic...
File Source/platform/graphics/RecordingImageBufferSurface.h (right):

https://codereview.chromium.org/379253002/diff/120001/Source/platform/graphic...
Source/platform/graphics/RecordingImageBufferSurface.h:24: 
On 2014/07/21 04:57:10, Sergey wrote:
> Could you please group and add a comment which of these methods are derived
from
> ImageBufferSurface, like it is done in
>
https://chromium.googlesource.com/chromium/blink/+/master/Source/platform/gra...
> ?
They all are, but yes good idea.

[Justin Novosad, 2014-07-21 18:26:08]

jamesr: Ping.  Code was changed to use WebRuntimeFeatures.

[jamesr, 2014-07-21 19:26:22]

/web/ lgtm

[Justin Novosad, 2014-07-21 19:28:49]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2014-07-21 19:29:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/junov@chromium.org/379253002/160001

[commit-bot: I haz the power, 2014-07-21 20:17:35]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/18049)

[commit-bot: I haz the power, 2014-07-21 20:52:51]

Change committed as 178585