Bug 49629 – [chromium] Compositor needs to manage its VRAM use

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] Compositor needs to manage its VRAM use

[James Robinson]

Created attachment 74050 [details]
Patch

[James Robinson]

Here's a patch for initial feedback.  I believe it is functionally complete but I haven't constructed tests for it yet and there are some debugging printf()s left in.  I can zoom in/out on maps.google.com quite a bit with a low memory limit and observe that it is evicting textures without crashing, so that's a good sign.

I think the biggest design question is whether to use opaque tokens and raw texture ids or to provide a wrapped texture object to callers.  This way seemed a little simpler to implement at first, but see what you think.

[Vangelis Kokkevis]

Comment on attachment 74050 [details]
Patch

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

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:174
> +    return m_contentsDirty || !m_contentsTextureToken || !layerRenderer()->textureManager()->hasTexture(m_contentsTextureToken);

I think we should fold the contentsDirty() call inside updateContents() to make sure that they are called in succession.  Right now there are no explicit guarantees that the texture won't be reclaimed in between.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:316
>      GraphicsContext3D* context = layerRendererContext();

You could, in theory, protect the texture here and unprotect it after the draw to make explicit the fact that the texture should not be reclaimed in between these two steps.

> WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:64
> +        unsigned textureLayerShaderProgram() const { return m_textureLayerShaderProgram; }

Since these are now defined inside RenderSurfaceChromium::SharedValues, you can remove the textureLayerShader* prefix (which is also no longer accurate)

> WebCore/platform/graphics/chromium/TextureManager.cpp:69
> +    ASSERT(!m_textures.get(token).isProtected);

Is there any harm in calling protectTexture twice in a row?

[Vangelis Kokkevis]

I meant to add a couple more comments here:

I like the patch and thanks for the RenderSurface cleanup as well. 

I would prefer if possible to wrap the texture token, id and size in a single object so that there aren't that many implicit assumptions.  How about we do something like the following:

class LayerTexture {
public:
  bool isValid() { /* Texture manager checks if the token is still valid */ }

  bool reserve(IntSize) { /* call the texture manager to create a gl texture and allocate it if needed, or use the existing one if it's still valid and matches the size.  Also protect the texture against deletions. */ }

  bool unreserve() { /* call the texture manager to mark the texture as unprotected. */ } ;

private:
  LayerTexture(TextureManager* m) : m_token(0), m_glId(0), m_textureManager(m) {};

  IntSize m_allocatedSize;
  unsigned m_token;
  unsigned m_glId;
}

Now the various layers hold a pointer to a LayerTexture rather that the gl Id and the allocated size, etc.

Now we could create another method on the LayerChromium that combines the current contentsDirty, updateContents, and draw methods and is what the compositor uses.  For example:

ContentLayerChromium::updateAndDraw() {
  if (!m_layerTexture)
     m_layerTexture = layerManager->createLayerTexture();

  if (!m_layerTexture->isValid() || !m_layerTexture->size() != requiredSize) {
     m_contentsDirty = true;
     m_dirtyRect = m_bounds;
  }

  // After calling reserve we're guaranteed that:
  // a. A gl texture of the appropriate size has been allocated
  // b. The texture has been marked as protected.
  m_layerTexture.reserve(requiredSize, GC3D::RGBA);

  // Guard against out of memory conditions.
  if (!m_layerTexture->isValid())
     return;

  if (m_contentsDirty) {
    // update the texture contents
  }

  draw();

  // Mark texture as unprotected
  m_layerTexture.unreserve();
}


I think most of the logic you currently have in the texture manager will remain intact but be wrapped in different APIs.


(In reply to comment #3)
> (From update of attachment 74050 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74050&action=review
> 
> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:174
> > +    return m_contentsDirty || !m_contentsTextureToken || !layerRenderer()->textureManager()->hasTexture(m_contentsTextureToken);
> 
> I think we should fold the contentsDirty() call inside updateContents() to make sure that they are called in succession.  Right now there are no explicit guarantees that the texture won't be reclaimed in between.
> 
> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:316
> >      GraphicsContext3D* context = layerRendererContext();
> 
> You could, in theory, protect the texture here and unprotect it after the draw to make explicit the fact that the texture should not be reclaimed in between these two steps.
> 
> > WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:64
> > +        unsigned textureLayerShaderProgram() const { return m_textureLayerShaderProgram; }
> 
> Since these are now defined inside RenderSurfaceChromium::SharedValues, you can remove the textureLayerShader* prefix (which is also no longer accurate)
> 
> > WebCore/platform/graphics/chromium/TextureManager.cpp:69
> > +    ASSERT(!m_textures.get(token).isProtected);
> 
> Is there any harm in calling protectTexture twice in a row?

[Vincent Scheib]

Like it + Vangelis's suggestions in context of compositor. 

I wonder about managing all texture use for a renderer / browser. Canvas will be creating textures as well, it would be nice if a single texture memory tracker could be used to free textures from any source. Perhaps we can iterate and refactor that later, but perhaps holding off doing anything we'll just need to undo then. e.g. naming "LayerTexture" vs something like "MemManagedGPUTexture".

[Nat Duca]

(In reply to comment #5)
> I wonder about managing all texture use for a renderer / browser. Canvas will be creating textures as well, ...

The compositor will likely be moving to a secondary thread whereas [I assume] Canvas will live on the main thread. Fusion between the two may not be feasible in such a world.

[James Robinson]

Created attachment 74194 [details]
Patch

[James Robinson]

Here it is with LayerTexture and the other fixes suggested.  I didn't combine update with draw as it sounds like for masks we need to keep them separate.  I'm also not handling allocation failures properly in RenderSurface - I think we need to add a skipDrawing flag to it as well and propagate it out somehow.

Still needs tests.

[James Robinson]

Created attachment 74342 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 74342 [details]
Patch

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

Looks good overall.  A couple of comments inline.

> WebCore/ChangeLog:13
> +        The TextureManager works by providing tokens to callers that want to use a managed texture.

This part of the description should be modified to reflect the new LayerTexture wrapper object.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:313
>          m_allocatedTextureSize = requiredTextureSize;

I mention this further down as well but I think it would be better for the LayerTexture to also store the reserved size instead of keeping track of it with m_allocatedTextureSize here. You could potentially pass the requiredTextureSize to the isValid() call to test it against the already allocated size.

> WebCore/platform/graphics/chromium/LayerTexture.cpp:70
> +

I think it would be convenient for a LayerTexture to also store its size. This would simplify the layer update logic as the layer won't have to store the allocated texture size itself.

> WebCore/platform/graphics/chromium/LayerTexture.h:52
> +    void bindTexture(unsigned target);

I don't think it's necessary to pass the target as a param.  It's always going to be TEXTURE_2D.

> WebCore/platform/graphics/chromium/LayerTexture.h:53
> +    void framebufferTexture2D(unsigned long target, unsigned long attachment, unsigned long textarget, long level);

Similarly here, none of the params are really useful.

> WebCore/platform/graphics/chromium/RenderSurfaceChromium.cpp:72
> +        LOG_ERROR("RenderSurfaceChromium: Failed to create scroll shader program");

Remove the word scroll

> WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:66
> +        int renderSurfaceShaderSamplerLocation() const { return m_renderSurfaceShaderSamplerLocation; }

To stay consistent with other Shared value classes, please remove the renderSurface prefix from all members and methods.

[James Robinson]

Created attachment 74439 [details]
Patch

[James Robinson]

Addressed feedback.  I think the size part is a lot better this way - the LayerChromiums don't have to care what they have allocated in the past, they just have to ask the LayerTexture if it currently has a texture of the right size/format for the current frame in updateContentsIfDirty().

[Vangelis Kokkevis]

Comment on attachment 74439 [details]
Patch

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

Other than a bit of cleanup in the content layer, this looks good! You'll have to find a WK reviewer though to bless it.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:292
> +void ContentLayerChromium::updateTextureRect(void* pixels, const IntSize& bitmapSize, const IntSize& requiredTextureSize, const IntRect& requestedUpdateRect)

I think we don't need to pass in bitmap size as it will always be equal to the size of the requestedUpdateRect.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:303
> +    if (!m_contentsTexture->isValid(requiredTextureSize, GraphicsContext3D::RGBA)) {

I don't believe we need this test either. Since we get the pixels from our caller, we have to assume that we get enough to update the entire texture if it's not valid.  This check is already done in line 219 above.

[Vangelis Kokkevis]

Comment on attachment 74439 [details]
Patch

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

One more thing that just occurred to me.

> WebCore/platform/graphics/chromium/TextureManager.cpp:142
> +    size_t memoryRequiredBytes = memoryUseBytes(size, format);

You also need to call: layerRenderer()->checkTextureSize(size) to make sure that the texture is within the range supported by the h/w.

[James Robinson]

(In reply to comment #14)
> (From update of attachment 74439 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74439&action=review
> 
> One more thing that just occurred to me.
> 
> > WebCore/platform/graphics/chromium/TextureManager.cpp:142
> > +    size_t memoryRequiredBytes = memoryUseBytes(size, format);
> 
> You also need to call: layerRenderer()->checkTextureSize(size) to make sure that the texture is within the range supported by the h/w.

Shouldn't the caller (the *LayerChromium) do this check?  Right now the TextureManager doesn't know anything about the LayerRenderer.

[James Robinson]

(In reply to comment #13)
> (From update of attachment 74439 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74439&action=review
> 
> Other than a bit of cleanup in the content layer, this looks good! You'll have to find a WK reviewer though to bless it.
> 
> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:292
> > +void ContentLayerChromium::updateTextureRect(void* pixels, const IntSize& bitmapSize, const IntSize& requiredTextureSize, const IntRect& requestedUpdateRect)
> 
> I think we don't need to pass in bitmap size as it will always be equal to the size of the requestedUpdateRect.

True!  Removed.
> 
> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:303
> > +    if (!m_contentsTexture->isValid(requiredTextureSize, GraphicsContext3D::RGBA)) {
> 
> I don't believe we need this test either. Since we get the pixels from our caller, we have to assume that we get enough to update the entire texture if it's not valid.  This check is already done in line 219 above.

This isn't true for ImageLayerChromium - the updateRect is just the dirty rect.  I could have ImageLayerChromium::updateContentsIfDirty grow the dirty rectangle if the layer is dirty because of its LayerTexture being evicted, but that seems a little odd.

[Vangelis Kokkevis]

Comment on attachment 74439 [details]
Patch

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

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:304
>          ASSERT(bitmapSize == requiredTextureSize);

The problem is that if we ever end up growing the updateRect here then the new size won't match the size of the bitmap that "pixels" points to. As a result, texSubImage2D will either produce wrong results or read into uninitialized memory. I think that we need to check for the right texture size in ImageLayerChromium much like ContentLayerChromium does.

> WebCore/platform/graphics/chromium/TextureManager.cpp:143
> +    if (memoryRequiredBytes > m_memoryLimitBytes || !reduceMemoryToLimit(m_memoryLimitBytes - memoryRequiredBytes))

It seems more natural if texture manager does it as it's the texture manager that tries to allocate the memory for the texture.  What you could do is record the maximum supported texture size at creation time by calling:

GLC(m_context, m_context->getIntegerv(GraphicsContext3D::MAX_TEXTURE_SIZE, &m_maxTextureSize));

much like the LayerRenderer does.  As a matter of fact that call could probably be completely removed from the layer renderer once all the layer textures are provided by the texture manager.

[James Robinson]

(In reply to comment #17)
> (From update of attachment 74439 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74439&action=review
> 
> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:304
> >          ASSERT(bitmapSize == requiredTextureSize);
> 
> The problem is that if we ever end up growing the updateRect here then the new size won't match the size of the bitmap that "pixels" points to. As a result, texSubImage2D will either produce wrong results or read into uninitialized memory. I think that we need to check for the right texture size in ImageLayerChromium much like ContentLayerChromium does.

Yeah, I'll unify this logic.

> 
> > WebCore/platform/graphics/chromium/TextureManager.cpp:143
> > +    if (memoryRequiredBytes > m_memoryLimitBytes || !reduceMemoryToLimit(m_memoryLimitBytes - memoryRequiredBytes))
> 
> It seems more natural if texture manager does it as it's the texture manager that tries to allocate the memory for the texture.  What you could do is record the maximum supported texture size at creation time by calling:
> 
> GLC(m_context, m_context->getIntegerv(GraphicsContext3D::MAX_TEXTURE_SIZE, &m_maxTextureSize));
> 
> much like the LayerRenderer does.  As a matter of fact that call could probably be completely removed from the layer renderer once all the layer textures are provided by the texture manager.

Good point.

[James Robinson]

Created attachment 74596 [details]
Patch

[James Robinson]

This makes sure that the updateRect is the right size before calling updateTextureRect and adds a max texture size limit to TextureManager.

[Kenneth Russell]

Comment on attachment 74596 [details]
Patch

r+'ing based on Vangelis's review. Submitting to commit queue.

[WebKit Commit Bot]

Comment on attachment 74596 [details]
Patch

Rejecting patch 74596 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
ts/websocket/tests/workers ......
http/tests/workers .....
http/tests/xhtmlmp .
http/tests/xmlhttprequest ...........................................................................................................................................................................
http/tests/xmlhttprequest/web-apps ...............
http/tests/xmlhttprequest/workers .........
618.69s total testing time

22016 test cases (99%) succeeded
2 test cases (<1%) were new
13 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/6332009

[Kenneth Russell]

The commit queue failure above looks odd -- like DumpRenderTree silently aborted. Should we cq+ this again?

[Vangelis Kokkevis]

(In reply to comment #23)
> The commit queue failure above looks odd -- like DumpRenderTree silently aborted. Should we cq+ this again?

Yes, please.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74596 [details]:

http/tests/misc/uncacheable-script-repeated.html

Please file bugs against the tests.  These tests were authored by [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74596 [details]:

compositing/iframes/overlapped-nested-iframes.html

Please file bugs against the tests.  These tests were authored by [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 74596 [details]
Patch

Rejecting patch 74596 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
ts/websocket/tests/workers ......
http/tests/workers .....
http/tests/xhtmlmp .
http/tests/xmlhttprequest ...........................................................................................................................................................................
http/tests/xmlhttprequest/web-apps ...............
http/tests/xmlhttprequest/workers .........
537.57s total testing time

22019 test cases (99%) succeeded
2 test cases (<1%) were new
13 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/6378023

[Vangelis Kokkevis]

Created attachment 74793 [details]
Added text expectations for mac

[Kenneth Russell]

Comment on attachment 74793 [details]
Added text expectations for mac

Let's try again.

[Kenneth Russell]

Removed cq+ hoping that the commit queue hasn't picked this up already. Due to time constraints we are going to land this manually and fix up any remaining test failures.

[Vangelis Kokkevis]

Committed r72701: <http://trac.webkit.org/changeset/72701>

[James Robinson]

This patch was reverted in r72770 due to rendering issues.  It looks like the problem is that I didn't understand the clipped update path at all for content layers.  I'll try to construct a test case and update the patch ASAP.

[James Robinson]

Here's a diff to the previous patch to make it actually work:


--- a/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp
+++ b/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp
@@ -212,17 +212,17 @@ void ContentLayerChromium::updateContentsIfDirty()
             return;
         }
 
+        // If the texture needs to be reallocated then we must redraw the entire
+        // visible contents of the layer.
+        if (!m_contentsTexture || !m_contentsTexture->isValid(requiredTextureSize, GraphicsContext3D::RGBA))
+            m_dirtyRect = boundsRect;
+
         calculateClippedUpdateRect(dirtyRect, m_largeLayerDrawRect);
         if (!layerRenderer()->checkTextureSize(m_largeLayerDrawRect.size())) {
             m_skipsDraw = true;
             return;
         }
 
-        // If the texture needs to be reallocated then we must redraw the entire
-        // contents of the layer.
-        if (!m_contentsTexture || !m_contentsTexture->isValid(requiredTextureSize, GraphicsContext3D::RGBA))
-            dirtyRect = boundsRect;
-
         // If the portion of the large layer that's visible hasn't changed
         // then we don't need to update it, _unless_ its contents have changed
         // in which case we only update the dirty bits.
@@ -230,7 +230,7 @@ void ContentLayerChromium::updateContentsIfDirty()
             if (!m_dirtyRect.intersects(dirtyRect))
                 return;
             dirtyRect.intersect(IntRect(m_dirtyRect));
-            updateRect = dirtyRect;
+            updateRect = IntRect(IntPoint(0, 0), dirtyRect.size());
             requiredTextureSize = m_largeLayerDirtyRect.size();
         } else {
             m_largeLayerDirtyRect = dirtyRect;

I've manually tested this pretty carefully in the large layer case to ensure that we still render correctly if the large layer's backing texture is evicted between frames (although we render brutally slowly if we have to upload 65MB+ of texture data for each frame).  New patch incoming..

[James Robinson]

Created attachment 75878 [details]
Patch

[James Robinson]

Relative to the last patch the only changes here are (other than merging up):

- the changes in comment #33
- updating several callers of the GLC() macro so they compile if DEBUG_GL_CALLS is set to 1
- moves the new tests from LayoutTests/compositing to LayoutTests/platform/chromium/compositing.  These tests are very particular to the chromium compositor implementation and aren't generally useful for other ports.  Having the tests in LT/p/c/ also means we don't have to worry about expectations for other ports

[Kenneth Russell]

Comment on attachment 75878 [details]
Patch

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

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:233
> +            updateRect = IntRect(IntPoint(0, 0), dirtyRect.size());

I'm sure that your testing has verified the correctness of this change, but it looks like the effect is to move the updateRect to the upper-left corner. Is that the correct coordinate system transformation? Or is this supposed to create a rectangle starting at (0,0) and extending to the lower-right point of dirtyRect?

[James Robinson]

(In reply to comment #36)
> (From update of attachment 75878 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75878&action=review
> 
> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:233
> > +            updateRect = IntRect(IntPoint(0, 0), dirtyRect.size());
> 
> I'm sure that your testing has verified the correctness of this change, but it looks like the effect is to move the updateRect to the upper-left corner. Is that the correct coordinate system transformation? Or is this supposed to create a rectangle starting at (0,0) and extending to the lower-right point of dirtyRect?

This is definitely subtle.  In this function, dirtyRect is the dirty rectangle in the coordinate space of the layer and updateRect is in the coordinate space of the texture.  For large layers we always paint the dirty region of the layer (bounded by dirtyRect) into a software bitmap and then upload the entire software bitmap into a texture (bound by updateRect).  For "normal" layers the updateRect is not necessarily the entire bounds of the texture - it might be a subregion - but it's still in the coordinate space of the texture, not the layer.

To be honest I'm kind of amazed that this piece of code worked without this change - I suspect it had bugs that we just didn't catch.

[James Robinson]

(In reply to comment #37)
> (In reply to comment #36)
> > (From update of attachment 75878 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=75878&action=review
> > 
> > > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:233
> > > +            updateRect = IntRect(IntPoint(0, 0), dirtyRect.size());
> > 
> > I'm sure that your testing has verified the correctness of this change, but it looks like the effect is to move the updateRect to the upper-left corner. Is that the correct coordinate system transformation? Or is this supposed to create a rectangle starting at (0,0) and extending to the lower-right point of dirtyRect?
> 
> This is definitely subtle.  In this function, dirtyRect is the dirty rectangle in the coordinate space of the layer and updateRect is in the coordinate space of the texture.  For large layers we always paint the dirty region of the layer (bounded by dirtyRect) into a software bitmap and then upload the entire software bitmap into a texture (bound by updateRect).  For "normal" layers the updateRect is not necessarily the entire bounds of the texture - it might be a subregion - but it's still in the coordinate space of the texture, not the layer.

Actually now that I've written it out I'm not entirely sure that it's correct. Hmm...
> 
> To be honest I'm kind of amazed that this piece of code worked without this change - I suspect it had bugs that we just didn't catch.

[James Robinson]

Comment on attachment 75878 [details]
Patch

The large layer update logic isn't quite 100%.  Will fix and update.

[James Robinson]

Created attachment 76012 [details]
Patch

[James Robinson]

(In reply to comment #40)
> Created an attachment (id=76012) [details]
> Patch

The only difference between this and the previous patch is in ContentLayerChromium.cpp/h.  I rewrote the large layer handling following the design that Ken, Vangelis and I worked out on the whiteboard.  Hopefully it's a bit easier to follow now.

[Vangelis Kokkevis]

Comment on attachment 76012 [details]
Patch

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

I believe the new large layer math is right.  Just a few comments on the variable names to make sure they reflect the actual space the various entities are in.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:206
> +        IntRect visibleRectInOriginCoords = layerOriginTransform.mapRect(visibleRectInLayerCoords);

Instead of "InOriginCoords" I think you should use in "SurfaceCoords" .  This is an IntRect expressed in the target RenderSurface coordinate system.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:208
> +

m_layerCenterInSurfaceCoords

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:231
> +        IntSize layerOffsetFromRenderSurfaceCoords(visibleRectInLayerCoords.x(), visibleRectInLayerCoords.y());

I think layerOffsetFromRenderSurfaceCoords should be called visibleRectOffsetInLayerCoords as you are still in layer coords.

[James Robinson]

Created attachment 76127 [details]
with suggested renames

[Vangelis Kokkevis]

Comment on attachment 76127 [details]
with suggested renames

Looks good!  Thanks.

[Kenneth Russell]

Comment on attachment 76127 [details]
with suggested renames

Looks good to me too.

[James Robinson]

Committed r73663: <http://trac.webkit.org/changeset/73663>