[chromium] Accelerated canvas broken in threaded compositing mode

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nat Duca]

CanvasLayerChromium and impl use the accelerated canvas "backbuffer" directly. In threaded compositing mode, this causes partial canvas results to appear onscreen. We need a short term fix for this. Copying?

[Iain Merrick]

Created attachment 118062 [details]
Patch

[Iain Merrick]

Comment on attachment 118062 [details]
Patch

Uploading work-in-progress.

[Iain Merrick]

Hitting a lot of problems with asserts. Probably worth fixing those separately beforehand.

I also get a crash on shader creation in both of the canvas demos on http://annevankesteren.nl/test/html/canvas/demo/. Other canvas pages work fine.

[James Robinson]

Comment on attachment 118062 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118062&action=review

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:108
> +    if (!m_compositorTextureId || m_compositorSize != m_canvasSize) {
> +        TextureAllocator* allocator = textureUpdater.allocator();
> +        ASSERT(allocator);

this jumping back and forth between the canvas context (m_context) and the compositor context is going to be really difficult to get working properly with the thread.

The canvas and compositor contexts are in a shared resource group, so you can bind resource IDs from either context in either context. Could you just do the texture copy from the compositor context?

[Iain Merrick]

Created attachment 118563 [details]
Patch

[Iain Merrick]

Created attachment 118588 [details]
Added a unit test

[Iain Merrick]

Notes:

- Couldn't figure out a great way to unit test this. I looked at ImageLayerChromiumTest and TiledLayerChromium test but neither uses threads. We could maybe use a base class for threaded LayerChromium subclass tests. This at least runs through the code, though.

- Looking ahead a bit, we'll want to swap textures rather than copy if possible. Therefore it makes sense to allocate the texture manually in the canvas context rather than going via TextureManager / TextureAllocator in the compositor context. Memory management will be fiddly, but we can at least blow away the compositor's copy by calling setTextureId(0) at the appropriate time.

- I think we should fold CanvasLayerChromium into WebGLLayerChromium.

[James Robinson]

Comment on attachment 118588 [details]
Added a unit test

View in context: https://bugs.webkit.org/attachment.cgi?id=118588&action=review

I doubt this works when the canvas is resized after an initial composite, have you tested that case?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:77
> +    if (m_rendererTextureId && !m_compositorTextureId) {

what if the compositor texture exists but no longer matches the canvas' size?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:100
> +PassRefPtr<CCLayerImpl> Canvas2DLayerChromium::createCCLayerImpl()
> +{
> +    return CCCanvasLayerImpl::create(m_layerId);
> +}

this is redundant with CanvasLayerChromium's createCCLayerImpl

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:148
> +void Canvas2DLayerChromium::pushPropertiesTo(CCLayerImpl* layer)
>  {
> -    if (layerTreeHost())
> -        layerTreeHost()->startRateLimiter(m_context);
> +    LayerChromium::pushPropertiesTo(layer);
> +
> +    CCCanvasLayerImpl* canvasLayer = static_cast<CCCanvasLayerImpl*>(layer);
> +    canvasLayer->setTextureId(m_compositorTextureId);

this is pretty much the same as CanvasLayerChromium's pushPropertiesTo

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:44
> +class Canvas2DLayerChromium : public LayerChromium {

why are you changing the class hierarchy? it seems like you're just creating extra work for yourself by duplicating pushPropertiesTo()

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:46
> +    static PassRefPtr<Canvas2DLayerChromium> create(GraphicsContext3D*, const IntSize&);

how are you handling canvas resizes?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:49
> +    // Called by ImageBufferSkia.

we typically don't try to document the caller at the callsite. The caller can change at any point and the caller is not part of this interface. if you want to add function-level documentation talk about what the function does, don't talk about what other classes do

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:52
> +    // Called by CanvasRenderingContext2D.

don't say _who_ calls this function, that could change at any point, say _what_ this function does

[Iain Merrick]

(In reply to comment #8)
> (From update of attachment 118588 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118588&action=review
> 
> I doubt this works when the canvas is resized after an initial composite, have you tested that case?

This object is created by ImageBufferSkia, which also takes its size as a constructor parameter and can't be resized afterwards. On a resize it looks like the ImageBuffer will just be blown away and recreated, and that will create a new Canvas2DLayerChromium. I'll check that, though. (And it's not ideal so maybe it should be changed.)

> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:77
> > +    if (m_rendererTextureId && !m_compositorTextureId) {
> 
> what if the compositor texture exists but no longer matches the canvas' size?

See above.

> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:100
> > +PassRefPtr<CCLayerImpl> Canvas2DLayerChromium::createCCLayerImpl()
> > +{
> > +    return CCCanvasLayerImpl::create(m_layerId);
> > +}
> 
> this is redundant with CanvasLayerChromium's createCCLayerImpl

> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:148
> > +void Canvas2DLayerChromium::pushPropertiesTo(CCLayerImpl* layer)
> >  {
> > -    if (layerTreeHost())
> > -        layerTreeHost()->startRateLimiter(m_context);
> > +    LayerChromium::pushPropertiesTo(layer);
> > +
> > +    CCCanvasLayerImpl* canvasLayer = static_cast<CCCanvasLayerImpl*>(layer);
> > +    canvasLayer->setTextureId(m_compositorTextureId);
> 
> this is pretty much the same as CanvasLayerChromium's pushPropertiesTo

Yep, but the texture ID is different.

> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:44
> > +class Canvas2DLayerChromium : public LayerChromium {
> 
> why are you changing the class hierarchy? it seems like you're just creating extra work for yourself by duplicating pushPropertiesTo()

There's more code in the base class that needs to be worked around than there is that can be reused, so it seems like sheer bloat. I'll see if I can rejig it to fit both the canvas and WebGL cases if you prefer, though.

Could possibly hoist the copying/double-buffering into ImageBuffer, but that seems like the wrong place as it's used for other things beside Canvas2D. ImageBuffer does have a copyImage() method, but that looks incompatible with hardware acceleration as it returns an Image.

> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:46
> > +    static PassRefPtr<Canvas2DLayerChromium> create(GraphicsContext3D*, const IntSize&);
> 
> how are you handling canvas resizes?
> 
> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:49
> > +    // Called by ImageBufferSkia.
> 
> we typically don't try to document the caller at the callsite. The caller can change at any point and the caller is not part of this interface. if you want to add function-level documentation talk about what the function does, don't talk about what other classes do

Good catch. I'll just remove these comments. As the methods are overrides they should be documented elsewhere.

> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:52
> > +    // Called by CanvasRenderingContext2D.
> 
> don't say _who_ calls this function, that could change at any point, say _what_ this function does

[Iain Merrick]

Created attachment 118777 [details]
Inheriting from CanvasLayerChromium again

[Iain Merrick]

Restored the class hierarchy to the way it was.

Resizing is actually completely broken in the threaded compositor, but not by this patch. I'll file a separate bug. As far as I can tell, this patch should do the right thing, as ImageBuffer doesn't have a resize method.

[James Robinson]

Comment on attachment 118777 [details]
Inheriting from CanvasLayerChromium again

View in context: https://bugs.webkit.org/attachment.cgi?id=118777&action=review

R=me but add the lost context check back and address other nits before landing.  We don't attempt to handle lost contexts at all, but if we do get one somehow we should just stop displaying the canvas and not attempt to issue any GL commands on the old context.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:107
> +    return m_rendererTextureId && compositedTextureId() && !m_size.isEmpty();

why did you remove the m_context and getGraphicsResetStatusARB() check from here? they seem to still be useful

> Source/WebCore/platform/graphics/chromium/CanvasLayerChromium.h:52
> +    unsigned compositedTextureId() const { return m_compositedTextureId; }

I don't think a protected getter is buying you all that much vs just accessing m_compositedTextureId, since we never override this or do anything else fancy, but that's up to you

> Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:66
> +    const WebGLId rendererTexId = 1;
> +    const WebGLId compositorTexId = 2;

we normally wouldn't abbreviate texture to 'tex' in a variable name

[Iain Merrick]

Comment on attachment 118777 [details]
Inheriting from CanvasLayerChromium again

View in context: https://bugs.webkit.org/attachment.cgi?id=118777&action=review

>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:107
>> +    return m_rendererTextureId && compositedTextureId() && !m_size.isEmpty();
> 
> why did you remove the m_context and getGraphicsResetStatusARB() check from here? they seem to still be useful

Oops, just lost in refactoring. Will fix.

I think I originally removed it because the context was being used on the wrong thread, but that should no longer be the case.

>> Source/WebCore/platform/graphics/chromium/CanvasLayerChromium.h:52
>> +    unsigned compositedTextureId() const { return m_compositedTextureId; }
> 
> I don't think a protected getter is buying you all that much vs just accessing m_compositedTextureId, since we never override this or do anything else fancy, but that's up to you

I mainly wanted to block public access to this field, as the semantics are now different for Canvas2D and WebGL, and this felt like the minimal change.

The getter seems a little neater for use by subclasses, and the generated code should be the same either way, so I'll leave it as is.

>> Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:66
>> +    const WebGLId compositorTexId = 2;
> 
> we normally wouldn't abbreviate texture to 'tex' in a variable name

Done

[Iain Merrick]

Created attachment 118823 [details]
Fixed lost-context check and variable names

[James Robinson]

Comment on attachment 118823 [details]
Fixed lost-context check and variable names

View in context: https://bugs.webkit.org/attachment.cgi?id=118823&action=review

sorry, i missed some threading issues here earlier. see inline comments for some details. this is definitely close but still needs a bit more work

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:65
> +    setTextureId(0);

hmm, this deletes the compositor's texture. unfortunately, the compositor thread may still try to issue an arbitrary number of bind+draw calls for this texture ID after the Canvas2DLayerChromium is destroyed but before the trees are synchronized. i think you need to destroy the compositor's texture on the impl side somehow

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:134
> +    GLC(m_context, m_context->bindTexture(GraphicsContext3D::TEXTURE_2D, compositedTextureId()));
> +    GLC(m_context, m_context->copyTexImage2D(GraphicsContext3D::TEXTURE_2D, 0, GraphicsContext3D::RGBA, 0, 0, m_size.width(), m_size.height(), 0));

i missed this earlier but this is actually still subtly incorrect - the paint pass is done while the compositor thread is still free-running, so it may be issuing draw calls with this texture ID bound while the copy is happening and pick up the wrong frame. you need to do the actual copy itself in some piece of code that is synchronized with the compositor thread

i think one option would be to do this in updateCompositorResources() and issue the copy commands themselves on the compositor's context, not the canvas'

leave the m_context->flush() in here - it's needed for synchronization. you can be assured that no canvas draw calls will be made between paintContentsIfDirty() and updateCompositorResources()

[Iain Merrick]

Created attachment 119028 [details]
Moved copyTexImage2D to updateCompositorResources

[Iain Merrick]

Slight chicken-and-egg problem with texture creation, as we need to push the texture ID in pushPropertiesTo(), but we don't get a context until updateCompositorResources() which is called later.

Resolved by just creating the texture on the main thread. I think this is what we'll want to do in the longer term anyway, so we can just flip the textures rather than copying.

[James Robinson]

(In reply to comment #17)
> Slight chicken-and-egg problem with texture creation, as we need to push the texture ID in pushPropertiesTo(), but we don't get a context until updateCompositorResources() which is called later.

This comment worries me.  pushPropertiesTo() is *after* updateCompositorResources() and has to be.

[Iain Merrick]

I'll check that again. Maybe I was misled by one of the calls being skipped, e.g. because the root layer wasn't created yet.

[Iain Merrick]

Called in that order after all. Will fix patch.

[James Robinson]

Comment on attachment 119028 [details]
Moved copyTexImage2D to updateCompositorResources

View in context: https://bugs.webkit.org/attachment.cgi?id=119028&action=review

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:74
> +    // Compositor resources should have been cleaned up on the compositor thread.
> +    ASSERT(!compositedTextureId());

I'm not sure how you imagine this might work but I don't think it can - the CCCanvasLayerImpl associated with this canvas will be destroyed _after_ the Canvas2DLayerChromium is destroyed.  Thus the composited texture has to outlive the Canvas2DLayerChromium

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:66
> +    unsigned m_backTextureId;
> +    unsigned m_frontTextureId;

why do we have 3 texture IDs for each instance of Canvas2DLayerChromium:

Canvas2DLayerChromium::m_backTextureId
Canvas2DLayerChromium::m_frontTextureId
CanvasLayerChromium::m_compositedTextureId

?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:69
> +    // Owned by the compositor context, which is where the back -> front copy happens.
> +    Platform3DObject m_fbo;

This comment's a bit confusing - the framebuffer object is not the interesting thing to share, it's the texture.  Where the FBO lives or is destroyed is a function of where the copy happens. The FBO could conceivably live on either the Canvas2DLayerChromium or the CCCanvasLayerImpl - it's not a huge deal.

[Iain Merrick]

(In reply to comment #21)
> (From update of attachment 119028 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119028&action=review
> 
> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:74
> > +    // Compositor resources should have been cleaned up on the compositor thread.
> > +    ASSERT(!compositedTextureId());
> 
> I'm not sure how you imagine this might work but I don't think it can - the CCCanvasLayerImpl associated with this canvas will be destroyed _after_ the Canvas2DLayerChromium is destroyed.  Thus the composited texture has to outlive the Canvas2DLayerChromium
[...]
> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:69
> > +    // Owned by the compositor context, which is where the back -> front copy happens.
> > +    Platform3DObject m_fbo;
> 
> This comment's a bit confusing - the framebuffer object is not the interesting thing to share, it's the texture.  Where the FBO lives or is destroyed is a function of where the copy happens. The FBO could conceivably live on either the Canvas2DLayerChromium or the CCCanvasLayerImpl - it's not a huge deal.

In general I was aiming to create and delete stuff in the same object and thread, rather than passing ownership around. Since this class uses two contexts and two threads the comments were meant to indicate what lives where.

> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:66
> > +    unsigned m_backTextureId;
> > +    unsigned m_frontTextureId;
> 
> why do we have 3 texture IDs for each instance of Canvas2DLayerChromium:
> 
> Canvas2DLayerChromium::m_backTextureId
> Canvas2DLayerChromium::m_frontTextureId
> CanvasLayerChromium::m_compositedTextureId
> 
> ?

I was using the last one to indicate that the texture ID has been pushed to the CCCanvasLayerImpl, as it looked like there was some initialization ordering glitch if I created the front buffer lazily in updateCompositorResources. But as discussed offline, the ordering is correct, so the shader compilation failures I'm seeing must be some other weird problem. I'll see if I can get to the root of that.

[Iain Merrick]

Closing in. Very odd bug -- glGetShaderiv isn't returning any results, so Skia assumes shader compilation failed. Possibly something to do with compiling shaders concurrently in two threads, but I really hope not!

[Iain Merrick]

OK, bizarre command buffer errors were due to a lingering usage of the canvas context on the compositor thread. That red herring out of the way, just need to fix the teardown process.

[Iain Merrick]

Created attachment 119261 [details]
Fixed texture allocation

[Iain Merrick]

Reverted a bunch of dumb stuff caused by my own misunderstanding, and now using ManagedTexture for the allocation.

On the upside, the test is a little more involved than before.

[James Robinson]

Comment on attachment 119261 [details]
Fixed texture allocation

View in context: https://bugs.webkit.org/attachment.cgi?id=119261&action=review

We're spiraling in ever and ever closer, like a toilet flushing or some such analogy. I like moving to a managed texture but have some comments inline about the usage. I also think we can simplify the triple of texture IDs down a bit even further. After that, I think we are good to go.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:72
> +    m_rendererTextureId = textureId;

what do you mean by 'renderer' here? that's a novel term for this part of the code

could we call this the back buffer and the other texture the front buffer?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:115
> +    if (host && layerTreeHost() != host)
> +        setTextureManager(host->contentsTextureManager());

i think you want to clear the m_managedTexture when transitioning to a nil CCLayerTreeHost as well

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:132
> +    bool success = m_managedTexture->reserve(m_size, GraphicsContext3D::RGBA);
> +    if (!success)
> +        return;

i like the idea of using managed textures but there is a bit of subtlety. the way we do the lifetime management for content textures that i believe you will want to follow is:

1.) in paintContents(), reserve any textures you may want for this frame. if the reservation fails, then you're over the memory budget and can't draw this layer - mark is as skipped and sadface
2.) in updateCompositorResources(), bind the texture you previous reserved and use it
3.) in unreserveContentsTextures() (which is called at the end of the frame, after uploads and the commit have occured) unreserve the texture

the reason to do the reserve at paint time is to make sure that you get texture memory before all of the other layers that come after you in the iteration, and to make sure that your LRU-ness is properly set. it's fine to reserve without access to the context, no actual GL calls are made until the bindTexture call.

i think that with this code you will frequently not have any memory left by the time you try to reserve the canvas, resulting in sadface

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:135
> +    setCompositedTextureId(m_managedTexture->textureId());

this seems awkward - you still have 3 variables on Canvas2DLayerChromium that are responsible for storing 2 texture IDs:

m_rendererTextureId
m_compositorTextureId
m_managedTexture

it seems to me like what you really want to do is remove m_compositedTextureId from CanvasLayerChromium, make compositedTextureId() virtual and provide different implementations for Canvas2DLayerChromium and WebGLLayerChromium

> Source/WebCore/platform/graphics/chromium/cc/CCCanvasLayerImpl.h:42
> +
> +    // Deletes the attached texture, if any. To prevent this call setTextureId(0) first.

not sure i understand this comment - it doesn't look like there is any code change in this patch and the existing code, as far as i can tell, doesn't delete any textures

> Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:99
> +        InSequence s;

nit we don't typically abbreviate in WebKit and we never use one letter variable names except for i/j loop counters and x/y for geometry code

[Iain Merrick]

Comment on attachment 119261 [details]
Fixed texture allocation

View in context: https://bugs.webkit.org/attachment.cgi?id=119261&action=review

>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:72
>> +    m_rendererTextureId = textureId;
> 
> what do you mean by 'renderer' here? that's a novel term for this part of the code
> 
> could we call this the back buffer and the other texture the front buffer?

Hmm, OK, m_backTextureId and m_frontTexture (as it's a ManagedTexture)

>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:115
>> +        setTextureManager(host->contentsTextureManager());
> 
> i think you want to clear the m_managedTexture when transitioning to a nil CCLayerTreeHost as well

Done

>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:132
>> +        return;
> 
> i like the idea of using managed textures but there is a bit of subtlety. the way we do the lifetime management for content textures that i believe you will want to follow is:
> 
> 1.) in paintContents(), reserve any textures you may want for this frame. if the reservation fails, then you're over the memory budget and can't draw this layer - mark is as skipped and sadface
> 2.) in updateCompositorResources(), bind the texture you previous reserved and use it
> 3.) in unreserveContentsTextures() (which is called at the end of the frame, after uploads and the commit have occured) unreserve the texture
> 
> the reason to do the reserve at paint time is to make sure that you get texture memory before all of the other layers that come after you in the iteration, and to make sure that your LRU-ness is properly set. it's fine to reserve without access to the context, no actual GL calls are made until the bindTexture call.
> 
> i think that with this code you will frequently not have any memory left by the time you try to reserve the canvas, resulting in sadface

Done

>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:135
>> +    setCompositedTextureId(m_managedTexture->textureId());
> 
> this seems awkward - you still have 3 variables on Canvas2DLayerChromium that are responsible for storing 2 texture IDs:
> 
> m_rendererTextureId
> m_compositorTextureId
> m_managedTexture
> 
> it seems to me like what you really want to do is remove m_compositedTextureId from CanvasLayerChromium, make compositedTextureId() virtual and provide different implementations for Canvas2DLayerChromium and WebGLLayerChromium

Hmm, or, again it seems like CanvasLayerChromium isn't providing much bang for the buck. Rather than adding another virtual method, how about we just get rid of CanvasLayerChromium?

I'll try removing its pushPropertiesTo method, as that now behaves differently in the two subclasses. They'll still inherit createCCLayerImpl.

>> Source/WebCore/platform/graphics/chromium/cc/CCCanvasLayerImpl.h:42
>> +    // Deletes the attached texture, if any. To prevent this call setTextureId(0) first.
> 
> not sure i understand this comment - it doesn't look like there is any code change in this patch and the existing code, as far as i can tell, doesn't delete any textures

Leftover from a previous iteration of patch. Will remove.

>> Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:99
>> +        InSequence s;
> 
> nit we don't typically abbreviate in WebKit and we never use one letter variable names except for i/j loop counters and x/y for geometry code

Done

[Iain Merrick]

(In reply to comment #27)
> We're spiraling in ever and ever closer, like a toilet flushing or some such analogy.

Hmm, are you saying I need another couple of flush() calls?

[Iain Merrick]

Created attachment 119423 [details]
Tweaked texture reservation

[Iain Merrick]

James, got time to review this again? Or if not, can you suggest another reviewer?

[Iain Merrick]

Created attachment 119863 [details]
Rebased

[James Robinson]

Comment on attachment 119863 [details]
Rebased

View in context: https://bugs.webkit.org/attachment.cgi?id=119863&action=review

Looks OK. Set cq? when you want this to land.

> Source/WebCore/platform/graphics/chromium/CanvasLayerChromium.cpp:-68
> -    canvasLayer->setTextureId(textureId());
> -    canvasLayer->setHasAlpha(m_hasAlpha);
> -    canvasLayer->setPremultipliedAlpha(m_premultipliedAlpha);
> -}

this would have been much cleaner if you had left this function alone, made the textureId() getter virtual, and provided appropriate overrides in WebGLLayerChromium and Canvas2DLayerChromium

[WebKit Review Bot]

Comment on attachment 119863 [details]
Rebased

Clearing flags on attachment: 119863

Committed r103264: <http://trac.webkit.org/changeset/103264>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.