Bug 45092 – [chromium] Accelerated Compositing: screen garbage when scrolling

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nat Duca]

Related bug: http://code.google.com/p/chromium/issues/detail?id=52519

Chrome currently uses the bulk of the software compositor code for root layer invalidations and root layer scrolling. This leads to scrolling artefacts when multiple damage rectangles survive the paint aggregation process, as is the case with fixed layers.

The proposed fix to this is to stop the compositor from piggybacking on the software rendering path for invalidation and scrolling.

[Nat Duca]

Created attachment 66418 [details]
Patch

[Nat Duca]

Created attachment 66445 [details]
Patch

[Nat Duca]

Created attachment 66447 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 66447 [details]
Patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index fad10df2a24eb70d8cc2aee9e84b229f9c0041ab..1c487a1123a87d694f63617e87a11569265bd1f2 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,31 @@
> [phone]  Nat Duca  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [chromium] Accelerated Compositing: screen garbage when scrolling
> +        https://bugs.webkit.org/show_bug.cgi?id=45092
> +
> +        Split LayerRenderChromium::drawLayers into several different
> +        functions, responsible for preparing the backbuffer, updating the
> +        root texture, compositing and performing the final
> +        swapbuffers. This are then used by the new
> +        WebViewImpl::paintComposited rendering path.
> +
> +        * platform/graphics/chromium/LayerChromium.cpp:
> +        (WebCore::LayerChromium::setBounds):
> +        (WebCore::LayerChromium::setFrame):
> +        (WebCore::LayerChromium::setNeedsDisplay):
> +        (WebCore::LayerChromium::resetNeedsDisplay):
> +        * platform/graphics/chromium/LayerChromium.h:
> +        (WebCore::LayerChromium::dirtyRect):
> +        * platform/graphics/chromium/LayerRendererChromium.cpp:
> +        (WebCore::LayerRendererChromium::setRootLayerCanvasSize):
> +        (WebCore::LayerRendererChromium::prepareToDrawLayers):
> +        (WebCore::LayerRendererChromium::updateRootLayerTextureRect):
> +        (WebCore::LayerRendererChromium::drawLayers):
> +        (WebCore::LayerRendererChromium::present):
> +        * platform/graphics/chromium/LayerRendererChromium.h:
> +
>  2010-09-02  Eric Seidel  <[e-mail]>
>  
>          Reviewed by Dimitri Glazkov.
> diff --git a/WebCore/platform/graphics/chromium/LayerChromium.cpp b/WebCore/platform/graphics/chromium/LayerChromium.cpp
> index 6519f1ff2ba1826f2e285962b0381712a46e9dcc..92ec49b5cc7f7ff8b637b6d30a124a14dc0bc3bb 100644
> --- a/WebCore/platform/graphics/chromium/LayerChromium.cpp
> +++ b/WebCore/platform/graphics/chromium/LayerChromium.cpp
> @@ -299,10 +299,15 @@ void LayerChromium::setBounds(const IntSize& size)
>      if (m_bounds == size)
>          return;
>  
> +    bool firstResize = !m_bounds.width() && !m_bounds.height() && size.width() && size.height();
> +    
>      m_bounds = size;
>      m_backingStoreSize = size;
>  
> -    setNeedsCommit();
> +    if (firstResize)
> +        setNeedsDisplay(FloatRect(0, 0, m_bounds.width(), m_bounds.height()));
> +    else
> +        setNeedsCommit();
>  }
>  
>  void LayerChromium::setFrame(const FloatRect& rect)
> @@ -311,7 +316,7 @@ void LayerChromium::setFrame(const FloatRect& rect)
>        return;
>  
>      m_frame = rect;
> -    setNeedsCommit();
> +    setNeedsDisplay(FloatRect(0, 0, m_bounds.width(), m_bounds.height()));
>  }
>  
>  const LayerChromium* LayerChromium::rootLayer() const
> @@ -353,7 +358,6 @@ void LayerChromium::setNeedsDisplay(const FloatRect& dirtyRect)
>      m_contentsDirty = true;
>  
>      m_dirtyRect.unite(dirtyRect);
> -
>      setNeedsCommit();
>  }
>  
> @@ -363,6 +367,12 @@ void LayerChromium::setNeedsDisplay()
>      m_contentsDirty = true;
>  }
>  
> +void LayerChromium::resetNeedsDisplay()
> +{
> +    m_dirtyRect = FloatRect();
> +    m_contentsDirty = false;
> +}
> +
>  void LayerChromium::toGLMatrix(float* flattened, const TransformationMatrix& m)
>  {
>      flattened[0] = m.m11();
> diff --git a/WebCore/platform/graphics/chromium/LayerChromium.h b/WebCore/platform/graphics/chromium/LayerChromium.h
> index ba1508844ba35b3451f82cd589a653976521a08c..2d225daf5a2071babfebd6a7cf3cc05e26b39086 100644
> --- a/WebCore/platform/graphics/chromium/LayerChromium.h
> +++ b/WebCore/platform/graphics/chromium/LayerChromium.h
> @@ -112,6 +112,8 @@ public:
>  
>      void setNeedsDisplay(const FloatRect& dirtyRect);
>      void setNeedsDisplay();
> +    const FloatRect& dirtyRect() const { return m_dirtyRect; }
> +    void resetNeedsDisplay();
>  
>      void setNeedsDisplayOnBoundsChange(bool needsDisplay) { m_needsDisplayOnBoundsChange = needsDisplay; }
>  
> diff --git a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> index 50338d2d5f7324e5a119c509190dfef8698b1769..af60c8edfa77bbdea3edba8e2e5fb3b68b2f7b25 100644
> --- a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> +++ b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> @@ -109,6 +109,7 @@ void LayerRendererChromium::debugGLCall(const char* command, const char* file, i
>  // Creates a canvas and an associated graphics context that the root layer will
>  // render into.
>  void LayerRendererChromium::setRootLayerCanvasSize(const IntSize& size)
> +
Please remove empty line
>  {
>      if (size == m_rootLayerCanvasSize)
>          return;
> @@ -152,8 +153,8 @@ void LayerRendererChromium::useShader(unsigned programId)
>  
>  // Updates the contents of the root layer texture that fall inside the updateRect
>  // and re-composits all sublayers.
> -void LayerRendererChromium::drawLayers(const IntRect& updateRect, const IntRect& visibleRect,
> -                                       const IntRect& contentRect, const IntPoint& scrollPosition)
> +void LayerRendererChromium::prepareToDrawLayers(const IntRect& visibleRect, const IntRect& contentRect, 
> +                                                const IntPoint& scrollPosition)
>  {
>      ASSERT(m_hardwareCompositing);
>  
> @@ -180,10 +181,11 @@ void LayerRendererChromium::drawLayers(const IntRect& updateRect, const IntRect&
>      // The GL viewport covers the entire visible area, including the scrollbars.
>      GLC(glViewport(0, 0, visibleRectWidth, visibleRectHeight));
>  
> +
Please remove newly added blank line. Here and below.

>      // Bind the common vertex attributes used for drawing all the layers.
>      LayerChromium::prepareForDraw(layerSharedValues());
> +    
>  
> -    // FIXME: These calls can be made once, when the compositor context is initialized.

The FIXME comment is still valid

>      GLC(glDisable(GL_DEPTH_TEST));
>      GLC(glDisable(GL_CULL_FACE));
>      GLC(glDepthFunc(GL_LEQUAL));
> @@ -193,14 +195,12 @@ void LayerRendererChromium::drawLayers(const IntRect& updateRect, const IntRect&
>          m_scrollPosition = scrollPosition;
>  
>      IntPoint scrollDelta = toPoint(scrollPosition - m_scrollPosition);
> -    // Scroll only when the updateRect contains pixels for the newly uncovered region to avoid flashing.
> -    if ((scrollDelta.x() && updateRect.width() >= abs(scrollDelta.x()) && updateRect.height() >= contentRect.height())
> -        || (scrollDelta.y() && updateRect.height() >= abs(scrollDelta.y()) && updateRect.width() >= contentRect.width())) {
> +    // Scroll the backbuffer
> +    if (scrollDelta.x() || scrollDelta.y()) {
>          // Scrolling works as follows: We render a quad with the current root layer contents
>          // translated by the amount the page has scrolled since the last update and then read the
>          // pixels of the content area (visible area excluding the scroll bars) back into the
> -        // root layer texture. The newly exposed area is subesquently filled as usual with
> -        // the contents of the updateRect.
> +        // root layer texture. The newly exposed area will be filled by a subsequent drawLayersIntoRect call
>          TransformationMatrix scrolledLayerMatrix;
>  #if PLATFORM(SKIA)
>          float scaleFactor = 1.0f;
> @@ -230,43 +230,64 @@ void LayerRendererChromium::drawLayers(const IntRect& updateRect, const IntRect&
>          // no need to copy framebuffer pixels back into the texture.
>          m_scrollPosition = scrollPosition;
>      }
> +    
> +    // Translate all the composited layers by the scroll position.
> +    TransformationMatrix matrix;
> +    matrix.translate3d(-m_scrollPosition.x(), -m_scrollPosition.y(), 0);
>  
> -    // FIXME: The following check should go away when the compositor renders independently from its own thread.
> -    // Ignore a 1x1 update rect at (0, 0) as that's used a way to kick off a redraw for the compositor.
> -    if (!(!updateRect.x() && !updateRect.y() && updateRect.width() == 1 && updateRect.height() == 1)) {
> -        // Update the root layer texture.
> -        ASSERT((updateRect.x() + updateRect.width() <= m_rootLayerTextureWidth)
> -               && (updateRect.y() + updateRect.height() <= m_rootLayerTextureHeight));
> +    // Traverse the layer tree and update the layer transforms.
> +    float opacity = 1;
> +    const Vector<RefPtr<LayerChromium> >& sublayers = m_rootLayer->getSublayers();
> +    size_t i;
> +    for (i = 0; i < sublayers.size(); i++)
> +        updateLayersRecursive(sublayers[i].get(), matrix, opacity);
> +}
>  
> +void LayerRendererChromium::updateRootLayerTextureRect(const IntRect& updateRect)
> +{
> +    ASSERT(m_hardwareCompositing);
> +
> +    if (!m_rootLayer)
> +        return;
> +
> +    // Update the root layer texture.
> +    ASSERT((updateRect.x() + updateRect.width() <= m_rootLayerTextureWidth)
> +           && (updateRect.y() + updateRect.height() <= m_rootLayerTextureHeight));

You can use IntRect::right() and IntRect::bottom() to get the coordinates of the extremes of the rect.

> +    
>  #if PLATFORM(SKIA)
> -        // Get the contents of the updated rect.
> -        const SkBitmap bitmap = m_rootLayerCanvas->getDevice()->accessBitmap(false);
> -        int rootLayerWidth = bitmap.width();
> -        int rootLayerHeight = bitmap.height();
> -        ASSERT(rootLayerWidth == updateRect.width() && rootLayerHeight == updateRect.height());

I'm not sure I follow here.  Is the updateRect supposed to match the root layer size? I thought update rects can be smaller. Shouldn't rootLayerWidth == m_rootLayerTextureWidth?  You are asserting a different condition in the lines above.

> -        void* pixels = bitmap.getPixels();
> -
> -        // Copy the contents of the updated rect to the root layer texture.
> -        GLC(glTexSubImage2D(GL_TEXTURE_2D, 0, updateRect.x(), updateRect.y(), updateRect.width(), updateRect.height(), GL_RGBA, GL_UNSIGNED_BYTE, pixels));

Don't you need to bind the root layer texture before calling this? 

> +    // Get the contents of the updated rect.
> +    const SkBitmap bitmap = m_rootLayerCanvas->getDevice()->accessBitmap(false);
> +    int rootLayerWidth = bitmap.width();
> +    int rootLayerHeight = bitmap.height();
> +    ASSERT(rootLayerWidth == updateRect.width() && rootLayerHeight == updateRect.height());
> +    void* pixels = bitmap.getPixels();
> +    // Copy the contents of the updated rect to the root layer texture.
> +    GLC(glTexSubImage2D(GL_TEXTURE_2D, 0, updateRect.x(), updateRect.y(), updateRect.width(), updateRect.height(), GL_RGBA, GL_UNSIGNED_BYTE, pixels));
>  #elif PLATFORM(CG)
> -        // Get the contents of the updated rect.
> -        ASSERT(static_cast<int>(CGBitmapContextGetWidth(m_rootLayerCGContext.get())) == updateRect.width() && static_cast<int>(CGBitmapContextGetHeight(m_rootLayerCGContext.get())) == updateRect.height());
> -        void* pixels = m_rootLayerBackingStore.data();
> -
> -        // Copy the contents of the updated rect to the root layer texture.
> -        // The origin is at the lower left in Core Graphics' coordinate system. We need to correct for this here.
> -        GLC(glTexSubImage2D(GL_TEXTURE_2D, 0,
> -                            updateRect.x(), m_rootLayerTextureHeight - updateRect.y() - updateRect.height(),
> -                            updateRect.width(), updateRect.height(),
> -                            GL_RGBA, GL_UNSIGNED_BYTE, pixels));
> +    // Get the contents of the updated rect.
> +    // ASSERT(static_cast<int>(CGBitmapContextGetWidth(m_rootLayerCGContext.get())) == updateRect.width() && static_cast<int>(CGBitmapContextGetHeight(m_rootLayerCGContext.get())) == updateRect.height());
> +    void* pixels = m_rootLayerBackingStore.data();

If this ASSERT is not necessary, please remove rather than commenting out.
> +
> +    // Copy the contents of the updated rect to the root layer texture.
> +    // The origin is at the lower left in Core Graphics' coordinate system. We need to correct for this here.
> +    GLC(glTexSubImage2D(GL_TEXTURE_2D, 0,
> +                        updateRect.x(), m_rootLayerTextureHeight - updateRect.y() - updateRect.height(),
> +                        updateRect.width(), updateRect.height(),
> +                        GL_RGBA, GL_UNSIGNED_BYTE, pixels));
>  #else
>  #error "Need to implement for your platform."
>  #endif
> -    }
> +}
>  
> +void LayerRendererChromium::drawLayers(const IntRect& visibleRect, const IntRect& contentRect)
> +{
> +    ASSERT(m_hardwareCompositing);
> +    
>      glClearColor(0, 0, 1, 1);
>      glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
> -
> +    
> +    GLC(glBindTexture(GL_TEXTURE_2D, m_rootLayerTextureId));
> +    
>      // Render the root layer using a quad that takes up the entire visible area of the window.
>      // We reuse the shader program used by ContentLayerChromium.
>      const ContentLayerChromium::SharedValues* contentLayerValues = contentLayerSharedValues();
> @@ -290,22 +311,13 @@ void LayerRendererChromium::drawLayers(const IntRect& updateRect, const IntRect&
>      GLC(glEnable(GL_BLEND));
>      GLC(glBlendFunc(GL_ONE, GL_ONE_MINUS_SRC_ALPHA));
>  
> -    // Translate all the composited layers by the scroll position.
> -    TransformationMatrix matrix;
> -    matrix.translate3d(-m_scrollPosition.x(), -m_scrollPosition.y(), 0);
> -
> -    // Traverse the layer tree and update the layer transforms.
> -    float opacity = 1;
> -    const Vector<RefPtr<LayerChromium> >& sublayers = m_rootLayer->getSublayers();
> -    size_t i;
> -    for (i = 0; i < sublayers.size(); i++)
> -        updateLayersRecursive(sublayers[i].get(), matrix, opacity);
> -
> -    m_rootVisibleRect = visibleRect;
> -
>      // Enable scissoring to avoid rendering composited layers over the scrollbars.
>      GLC(glEnable(GL_SCISSOR_TEST));
>      FloatRect scissorRect(contentRect);
> +
> +    // Set the rootVisibleRect --- used by subsequent drawLayers calls
> +    m_rootVisibleRect = visibleRect;
> +
>      // The scissorRect should not include the scroll offset.
>      scissorRect.move(-m_scrollPosition.x(), -m_scrollPosition.y());
>      scissorToRect(scissorRect);
> @@ -316,13 +328,17 @@ void LayerRendererChromium::drawLayers(const IntRect& updateRect, const IntRect&
>      GLC(glStencilMask(0));
>  
>      // Traverse the layer tree one more time to draw the layers.
> -    for (i = 0; i < sublayers.size(); i++)
> +    const Vector<RefPtr<LayerChromium> >& sublayers = m_rootLayer->getSublayers();
> +    for (size_t i = 0; i < sublayers.size(); i++)
>          drawLayersRecursive(sublayers[i].get(), scissorRect);
>  
>      GLC(glDisable(GL_SCISSOR_TEST));
> +}
>  
> +void LayerRendererChromium::present()
> +{
> +    // We're done! Time to swapbuffers!
>      m_gles2Context->swapBuffers();
> -
>      m_needsDisplay = false;
>  }
>  
> diff --git a/WebCore/platform/graphics/chromium/LayerRendererChromium.h b/WebCore/platform/graphics/chromium/LayerRendererChromium.h
> index 8f44afebd0a2c7b475d04ba48723f1cdc5455091..2f9edef67c233ffb7c02d43517de5b754701b128 100644
> --- a/WebCore/platform/graphics/chromium/LayerRendererChromium.h
> +++ b/WebCore/platform/graphics/chromium/LayerRendererChromium.h
> @@ -61,9 +61,17 @@ public:
>      LayerRendererChromium(PassOwnPtr<GLES2Context> gles2Context);
>      ~LayerRendererChromium();
>  
> -    // Updates the contents of the root layer that fall inside the updateRect and recomposites
> -    // all the layers.
> -    void drawLayers(const IntRect& updateRect, const IntRect& visibleRect, const IntRect& contentRect, const IntPoint& scrollPosition);
> +    // updates size of root texture, if needed, and scrolls the backbuffer
> +    void prepareToDrawLayers(const IntRect& visibleRect, const IntRect& contentRect, const IntPoint& scrollPosition);
> +    
> +    // updates a rectangle within the root layer texture
> +    void updateRootLayerTextureRect(const IntRect& updateRect);
> +
> +    // draws the current layers onto the backbuffer
> +    void drawLayers(const IntRect& visibleRect, const IntRect& contentRect);
> +
> +    // puts backbufeffer onscreen

Typo: backbuffer

> +    void present();
>  
>      void setRootLayer(PassRefPtr<LayerChromium> layer) { m_rootLayer = layer; }
>      LayerChromium* rootLayer() { return m_rootLayer.get(); }
> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> index 527e8efccae39045fd985ded3bf20d7166afc69d..59509251a1e367266ed20c7fab2e70aed3457cda 100644
> --- a/WebKit/chromium/ChangeLog
> +++ b/WebKit/chromium/ChangeLog
> @@ -1,3 +1,48 @@
> [phone]  Nat Duca  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [chromium] Accelerated Compositing: screen garbage when scrolling
> +        https://bugs.webkit.org/show_bug.cgi?id=45092
> +
> +        Introduce a new API on WebWidget for painting with accelerated
> +        compositing that allows the compositor to properly distingiush
> +        scrolling, invalidation and repainting from one another. The key
> +        change is that in accelerated rendering case, invalidates and
> +        scrolling pass directly to the compositor, rather than passing up
> +        to the client as was the case in the software path. For
> +        accelerated rendering, the previous paint() method is replaced by
> +        a pair of methods: paintComposited and
> +        invalidateCompositedRootLayerRect.
> +
> +
> +        * public/WebWidget.h:
> +        * public/WebWidgetClient.h:
> +        (WebKit::WebWidgetClient::scheduleDeferredComposite):
> +        * src/ChromeClientImpl.cpp:
> +        (WebKit::ChromeClientImpl::invalidateContentsAndWindow):
> +        (WebKit::ChromeClientImpl::scroll):
> +        * src/WebPopupMenuImpl.cpp:
> +        (WebKit::WebPopupMenuImpl::paint):
> +        (WebKit::WebPopupMenuImpl::invalidateCompositedRootLayerRect):
> +        (WebKit::WebPopupMenuImpl::paintComposited):
> +        * src/WebPopupMenuImpl.h:
> +        * src/WebViewImpl.cpp:
> +        (WebKit::WebViewImpl::resize):
> +        (WebKit::WebViewImpl::paint):
> +        (WebKit::WebViewImpl::setRootGraphicsLayer):
> +        (WebKit::WebViewImpl::setIsAcceleratedCompositingActive):
> +        (WebKit::WebViewImpl::updateRootLayerContents):
> +        (WebKit::WebViewImpl::setRootLayerNeedsDisplay):
> +        (WebKit::WebViewImpl::invalidateRootLayerRect):
> +        (WebKit::WebViewImpl::scrollRootLayer):
> +        (WebKit::WebViewImpl::invalidateCompositedRootLayerRect):
> +        (WebKit::WebViewImpl::paintComposited):
> +        * src/WebViewImpl.h:
> +        * tests/PopupMenuTest.cpp:
> +        (WebKit::TestWebWidget::invalidateCompositedRootLayerRect):
> +        (WebKit::TestWebWidget::paintComposited):
> +
>  2010-09-02  Ilya Sherman  <[e-mail]>
>  
>          Reviewed by Eric Seidel.
> diff --git a/WebKit/chromium/public/WebWidget.h b/WebKit/chromium/public/WebWidget.h
> index 5c9f54e79366b83afee7c0e1e3a3288c34412ca9..da690a63edc3d3ca3a104e9b8774c3a2e23b783a 100644
> --- a/WebKit/chromium/public/WebWidget.h
> +++ b/WebKit/chromium/public/WebWidget.h
> @@ -59,15 +59,28 @@ public:
>      // Called to layout the WebWidget.  This MUST be called before Paint,
>      // and it may result in calls to WebWidgetClient::didInvalidateRect.
>      virtual void layout() = 0;
> -
> -    // Called to paint the specified region of the WebWidget onto the given
> -    // canvas.  You MUST call Layout before calling this method.  It is
> -    // okay to call paint multiple times once layout has been called,
> -    // assuming no other changes are made to the WebWidget (e.g., once
> -    // events are processed, it should be assumed that another call to
> -    // layout is warranted before painting again).
> -    virtual void paint(WebCanvas*, const WebRect&) = 0;
> -
> +    
> +    // Called to paint the specified viewport of the WebWidget 
> +    // onto the specified canvas at (viewport.x,viewport.y). You MUST call

Does viewport specify the region of the WebWidget that will be grabbed or the place where it's
going to end up on the canvas? The comment seems to indicate both but I'm not sure that's correct.

> +    // Layout before calling this method.  It is okay to call paint
> +    // multiple times once layout has been called, assuming no other
> +    // changes are made to the WebWidget (e.g., once events are
> +    // processed, it should be assumed that another call to layout is
> +    // warranted before painting again).
> +    virtual void paint(WebCanvas*, const WebRect& viewport) = 0;
> +    
> +    // Triggers compositing of the current layers onto the screen.
> +    // The finish argument controls whether the compositor will waiting for the
> +    // GPU to finish rendering before returning. You MUST call Layout
> +    // before calling this method, for the same reasons described in
> +    // the paint method above.
> +    virtual void composite(bool finish) = 0;
> +
> +    // Called to inform the WebWidget of a change in native widget settings.
> +    // Implementors that cache rendered copies of widgets need to re-render
> +    // on receiving this message
> +    virtual void themeChanged() = 0;
> +    
>      // Called to inform the WebWidget of an input event.  Returns true if
>      // the event has been processed, false otherwise.
>      virtual bool handleInputEvent(const WebInputEvent&) = 0;
> diff --git a/WebKit/chromium/public/WebWidgetClient.h b/WebKit/chromium/public/WebWidgetClient.h
> index bd7bd6a74fcbaf371df3e9a6e410bedbf3592320..b3ee9f56fd1147787b2e46925f0de451d9ac451b 100644
> --- a/WebKit/chromium/public/WebWidgetClient.h
> +++ b/WebKit/chromium/public/WebWidgetClient.h
> @@ -45,7 +45,10 @@ class WebWidgetClient {
>  public:
>      // Called when a region of the WebWidget needs to be re-painted.
>      virtual void didInvalidateRect(const WebRect&) { }
> -
> +    
> +    // Called when a call to WebWidget::composite is required
> +    virtual void scheduleComposite() { }
> +    
>      // Called when a region of the WebWidget, given by clipRect, should be
>      // scrolled by the specified dx and dy amounts.
>      virtual void didScrollRect(int dx, int dy, const WebRect& clipRect) { }
> diff --git a/WebKit/chromium/src/ChromeClientImpl.cpp b/WebKit/chromium/src/ChromeClientImpl.cpp
> index e6f14007180d8d3bf6128b130dc28abaaec1f90a..2fda8e68e614ff36817b4c38badfb1d31160248d 100644
> --- a/WebKit/chromium/src/ChromeClientImpl.cpp
> +++ b/WebKit/chromium/src/ChromeClientImpl.cpp
> @@ -492,8 +492,16 @@ void ChromeClientImpl::invalidateContentsAndWindow(const IntRect& updateRect, bo
>  {
>      if (updateRect.isEmpty())
>          return;
> -    if (m_webView->client())
> -        m_webView->client()->didInvalidateRect(updateRect);
> +#if USE(ACCELERATED_COMPOSITING)
> +    if (!m_webView->isAcceleratedCompositingActive()) {
> +#endif
> +        if (m_webView->client())
> +            m_webView->client()->didInvalidateRect(updateRect);
> +#if USE(ACCELERATED_COMPOSITING)
> +    } else {
> +        m_webView->invalidateRootLayerRect(updateRect);
> +    }
> +#endif
>  }
>  
>  void ChromeClientImpl::invalidateContentsForSlowScroll(const IntRect& updateRect, bool immediate)
> @@ -507,11 +515,19 @@ void ChromeClientImpl::scroll(
>      const IntRect& clipRect)
>  {
>      m_webView->hidePopups();
> -    if (m_webView->client()) {
> -        int dx = scrollDelta.width();
> -        int dy = scrollDelta.height();
> -        m_webView->client()->didScrollRect(dx, dy, clipRect);
> +#if USE(ACCELERATED_COMPOSITING)
> +    if (!m_webView->isAcceleratedCompositingActive()) {
> +#endif
> +        if (m_webView->client()) {
> +            int dx = scrollDelta.width();
> +            int dy = scrollDelta.height();
> +            m_webView->client()->didScrollRect(dx, dy, clipRect);
> +        }
> +#if USE(ACCELERATED_COMPOSITING)
> +    } else {
> +        m_webView->scrollRootLayer(scrollDelta, clipRect);
>      }
> +#endif
>  }
>  
>  IntPoint ChromeClientImpl::screenToWindow(const IntPoint&) const
> diff --git a/WebKit/chromium/src/WebPopupMenuImpl.cpp b/WebKit/chromium/src/WebPopupMenuImpl.cpp
> index 75d6cc143824b7f59291029b427ce920c23e4539..717ff82e25e5cf4acd03283683a608ad7847705a 100644
> --- a/WebKit/chromium/src/WebPopupMenuImpl.cpp
> +++ b/WebKit/chromium/src/WebPopupMenuImpl.cpp
> @@ -155,12 +155,12 @@ void WebPopupMenuImpl::layout()
>  {
>  }
>  
> -void WebPopupMenuImpl::paint(WebCanvas* canvas, const WebRect& rect)
> +void WebPopupMenuImpl::paint(WebCanvas* canvas, const WebRect& viewport)
>  {
>      if (!m_widget)
>          return;
>  
> -    if (!rect.isEmpty()) {
> +    if (!viewport.isEmpty()) {
>  #if WEBKIT_USING_CG
>          GraphicsContext gc(canvas);
>  #elif WEBKIT_USING_SKIA
> @@ -170,10 +170,19 @@ void WebPopupMenuImpl::paint(WebCanvas* canvas, const WebRect& rect)
>  #else
>          notImplemented();
>  #endif
> -        m_widget->paint(&gc, rect);
> +        m_widget->paint(&gc, viewport);
>      }
>  }
>  
> +void WebPopupMenuImpl::themeChanged()
> +{
> +    notImplemented();
> +}
> +void WebPopupMenuImpl::composite(bool finish)
> +{
> +    notImplemented();
> +}
> +
>  bool WebPopupMenuImpl::handleInputEvent(const WebInputEvent& inputEvent)
>  {
>      if (!m_widget)
> diff --git a/WebKit/chromium/src/WebPopupMenuImpl.h b/WebKit/chromium/src/WebPopupMenuImpl.h
> index edbb4ab50d416b03b7bd1a5fce800556091ea227..221ba038f2c4c736d677fabfa322e051f904264c 100644
> --- a/WebKit/chromium/src/WebPopupMenuImpl.h
> +++ b/WebKit/chromium/src/WebPopupMenuImpl.h
> @@ -63,6 +63,8 @@ public:
>      virtual void resize(const WebSize&);
>      virtual void layout();
>      virtual void paint(WebCanvas* canvas, const WebRect& rect);
> +    virtual void themeChanged();
> +    virtual void composite(bool finish);
>      virtual bool handleInputEvent(const WebInputEvent&);
>      virtual void mouseCaptureLost();
>      virtual void setFocus(bool enable);
> diff --git a/WebKit/chromium/src/WebViewImpl.cpp b/WebKit/chromium/src/WebViewImpl.cpp
> index 4b129d694b1f112f27b21ae724db67a68a74e02a..8a48442fc9d5d63a8297aead124347b0cee745d5 100644
> --- a/WebKit/chromium/src/WebViewImpl.cpp
> +++ b/WebKit/chromium/src/WebViewImpl.cpp
> @@ -909,7 +909,12 @@ void WebViewImpl::resize(const WebSize& newSize)
>  
>      if (m_client) {
>          WebRect damagedRect(0, 0, m_size.width, m_size.height);
> -        m_client->didInvalidateRect(damagedRect);
> +        if (isAcceleratedCompositingActive()) {

I don't have the code handy but I seem to remember that isAcceleratedCompositingActive() is defined only if USE(ACCELERATED_COMPOSTING) is.

> +#if USE(ACCELERATED_COMPOSITING)
> +            invalidateRootLayerRect(damagedRect);
> +#endif
> +        } else 
> +            m_client->didInvalidateRect(damagedRect);
>      }
>  
>  #if OS(DARWIN)
> @@ -943,33 +948,17 @@ void WebViewImpl::layout()
>  
>  void WebViewImpl::paint(WebCanvas* canvas, const WebRect& rect)
>  {
> -
> +    if (isAcceleratedCompositingActive()) {

Same comment as above.  Should isAcceleratedCompositingActive stay inside the #if ? 

>  #if USE(ACCELERATED_COMPOSITING)
> -    if (!isAcceleratedCompositingActive()) {
> +        invalidateRootLayerRect(rect);
> +        doComposite(canvas, false);
>  #endif
> +    } else {
> +        // TODO, if accelerated, redirect to paint composited loop

In WebKit TODO -> FIXME

>          WebFrameImpl* webframe = mainFrameImpl();
>          if (webframe)
>              webframe->paint(canvas, rect);
> -#if USE(ACCELERATED_COMPOSITING)
> -    } else {
> -        // Draw the contents of the root layer.
> -        updateRootLayerContents(rect);
> -
> -        WebFrameImpl* webframe = mainFrameImpl();
> -        if (!webframe)
> -            return;
> -        FrameView* view = webframe->frameView();
> -        if (!view)
> -            return;
> -
> -        // The visibleRect includes scrollbars whereas the contentRect doesn't.
> -        IntRect visibleRect = view->visibleContentRect(true);
> -        IntRect contentRect = view->visibleContentRect(false);
> -
> -        // Ask the layer compositor to redraw all the layers.
> -        m_layerRenderer->drawLayers(rect, visibleRect, contentRect, IntPoint(view->scrollX(), view->scrollY()));
>      }
> -#endif
>  }
>  
>  // FIXME: m_currentInputEvent should be removed once ChromeClient::show() can
> @@ -2105,9 +2094,17 @@ bool WebViewImpl::tabsToLinks() const
>  #if USE(ACCELERATED_COMPOSITING)
>  void WebViewImpl::setRootGraphicsLayer(WebCore::PlatformLayer* layer)
>  {
> +    bool wasActive = m_isAcceleratedCompositingActive;
>      setIsAcceleratedCompositingActive(layer ? true : false);
>      if (m_layerRenderer)
>          m_layerRenderer->setRootLayer(layer);
> +    if (wasActive != m_isAcceleratedCompositingActive) {
> +        WebRect damagedRect(0, 0, m_size.width, m_size.height);
> +        if (m_isAcceleratedCompositingActive) 
> +            invalidateRootLayerRect(damagedRect);
> +        else
> +            m_client->didInvalidateRect(damagedRect);
> +    }
>  }
>  
>  void WebViewImpl::setIsAcceleratedCompositingActive(bool active)
> @@ -2119,10 +2116,6 @@ void WebViewImpl::setIsAcceleratedCompositingActive(bool active)
>          m_layerRenderer = LayerRendererChromium::create(getOnscreenGLES2Context());
>          if (m_layerRenderer->hardwareCompositing()) {
>              m_isAcceleratedCompositingActive = true;
> -            
> -            // Force a redraw the entire view so that the compositor gets the entire view,
> -            // rather than just the currently-dirty subset.
> -            m_client->didInvalidateRect(IntRect(0, 0, m_size.width, m_size.height));
>          } else {
>              m_layerRenderer.clear();
>              m_isAcceleratedCompositingActive = false;
> @@ -2138,12 +2131,6 @@ void WebViewImpl::updateRootLayerContents(const WebRect& rect)
>      if (!isAcceleratedCompositingActive())
>          return;
>  
> -    // FIXME: The accelerated compositing path invalidates a 1x1 rect at (0, 0)
> -    // in order to get the renderer to ask the compositor to redraw. This is only
> -    // temporary until we get the compositor to render directly from its own thread.
> -    if (!rect.x && !rect.y && rect.width == 1 && rect.height == 1)
> -        return;
> -
>      WebFrameImpl* webframe = mainFrameImpl();
>      if (!webframe)
>          return;
> @@ -2195,21 +2182,174 @@ void WebViewImpl::updateRootLayerContents(const WebRect& rect)
>  
>  void WebViewImpl::setRootLayerNeedsDisplay()
>  {
> -    // FIXME: For now we're posting a repaint event for the entire page which is an overkill.
> -    if (WebFrameImpl* webframe = mainFrameImpl()) {
> -        if (FrameView* view = webframe->frameView()) {
> -            // FIXME: Temporary hack to invalidate part of the page so that we get called to render
> -            //        again.
> -            IntRect visibleRect = view->visibleContentRect(true);
> -            m_client->didInvalidateRect(IntRect(0, 0, 1, 1));
> +    m_client->scheduleComposite();
> +    if (m_layerRenderer)
> +        m_layerRenderer->setNeedsDisplay();
> +}
> +
> +// WebViewImpl method
> +void WebViewImpl::scrollRootLayer(const WebSize& scrollDelta, const WebRect& clipRect)
> +{
> +    ASSERT(m_layerRenderer);
> +    // Compute the damage rect in viewport space
> +    // Should only be scrolling in one direction at a time.
> +    ASSERT(!(scrollDelta.width && scrollDelta.height));
> +    WebFrameImpl* webframe = mainFrameImpl();
> +    if (!webframe)
> +        return;
> +    FrameView* view = webframe->frameView();
> +    if (!view)
> +        return;
> +    
> +    FloatRect damagedRect;
> +    // Put the viewportRect in _layer space_ prior to the scroll since the logic
> +    // below emits damage in the space "before" scrolling
> +    // Using layer space for damage rects allows us to intermix invalidates with scrolls
> +    IntRect contentRect = view->visibleContentRect(false);
> +    FloatRect viewportRect(contentRect.x(),
> +                           contentRect.y(),
> +                           contentRect.width(), contentRect.height());
> +    
> +    // Compute the region we will expose by scrolling, and paint that into a
> +    // shared memory section.
> +    if (scrollDelta.width) {
> +        float dx = (float)scrollDelta.width;
> +        damagedRect.setY(viewportRect.y());
> +        damagedRect.setHeight(viewportRect.height());
> +        if (dx > 0) {
> +            damagedRect.setX(viewportRect.x());
> +            damagedRect.setWidth(dx);
> +        } else {

Is it not possible that you could have a scrollDelta with non-zero width _and_ height? 
> +            damagedRect.setX(viewportRect.right() + dx);
> +            damagedRect.setWidth(-dx);
> +        }
> +    } else {
> +        float dy = (float)scrollDelta.height;
> +        damagedRect.setX(viewportRect.x());
> +        damagedRect.setWidth(viewportRect.width());
> +        if (dy > 0) {
> +            damagedRect.setY(viewportRect.y());
> +            damagedRect.setHeight(dy);
> +        } else {
> +            damagedRect.setY(viewportRect.bottom() + dy);
> +            damagedRect.setHeight(-dy);
>          }
>      }
> +    
> +    // TODO: add a smarter damage aggregation logic? Right now, LayerChromium does simple union-ing
> +    IntRect iDamagedRect(damagedRect);
> +    iDamagedRect.unite(m_scrollDamage);
> +    m_scrollDamage = iDamagedRect;
> +    setRootLayerNeedsDisplay();
> +}
> +void WebViewImpl::invalidateRootLayerRect(const WebRect& rect)
> +{
> +    // rect is in viewport space. Convert to layer space
> +    // so that invalidations and scroll invalidations play well with one-another
> +    ASSERT(m_layerRenderer);
> +    
> +    WebFrameImpl* webframe = mainFrameImpl();
> +    if (!webframe)
> +        return;
> +    FrameView* view = webframe->frameView();
> +    if (!view)
> +        return;
>  
> -    if (m_layerRenderer)
> -        m_layerRenderer->setNeedsDisplay();
> +    FloatRect lsRect(rect.x + view->scrollX(),
> +                      rect.y + view->scrollY(),
> +                      rect.width, rect.height);
> +    
> +    // TODO: add a smarter damage aggregation logic? Right now, LayerChromium does simple union-ing
> +    m_layerRenderer->rootLayer()->setNeedsDisplay(lsRect);
>  }
>  #endif // USE(ACCELERATED_COMPOSITING)
>  
> +// WebWidet method
> +void WebViewImpl::themeChanged()
> +{
> +    if (isAcceleratedCompositingActive()) {
> +#if USE(ACCELERATED_COMPOSITING)
> +        m_layerRenderer->rootLayer()->setNeedsDisplay();
> +        setRootLayerNeedsDisplay();
> +#endif
> +    } else {
> +        WebRect damagedRect(0, 0, m_size.width, m_size.height);
> +        m_client->didInvalidateRect(damagedRect);
> +    }
> +}
> +
> +void WebViewImpl::composite(bool finish)
> +{
> +#if USE(ACCELERATED_COMPOSITING)
> +    doComposite(0, finish);
> +#endif
> +}
> +#if USE(ACCELERATED_COMPOSITING)
> +void WebViewImpl::doComposite(WebCanvas* canvas, bool finish)
> +{
> +    ASSERT(isAcceleratedCompositingActive());
> +    WebFrameImpl* webframe = mainFrameImpl();
> +    if (!webframe)
> +        return;
> +    FrameView* view = webframe->frameView();
> +    if (!view)
> +        return;
> +
> +    // The visibleRect includes scrollbars whereas the contentRect doesn't.
> +    IntRect visibleRect = view->visibleContentRect(true);
> +    IntRect contentRect = view->visibleContentRect(false);
> +    IntRect viewport    = IntRect(0, 0, m_size.width, m_size.height);
> +
> +    // Give the compoisitor a chance to setup/resize the root texture handle and perform scrolling
> +    m_layerRenderer->prepareToDrawLayers(visibleRect, contentRect, IntPoint(view->scrollX(), view->scrollY()));
> +
> +    // Draw the contents of the root layer
> +    Vector<FloatRect> damageRects;
> +    damageRects.append(FloatRect(m_scrollDamage.x, m_scrollDamage.y, m_scrollDamage.width, m_scrollDamage.height));
> +    damageRects.append(m_layerRenderer->rootLayer()->dirtyRect());
> +    for (size_t i = 0; i < damageRects.size(); ++i) {
> +        // the damage rects for the root layer is in layer space [e.g. unscrolled.]
> +        // convert from layer space to viewport space
> +        const FloatRect ssRect = damageRects[i];
> +        FloatRect rect(ssRect.x() - view->scrollX(),
> +                       ssRect.y() - view->scrollY(),
> +                       ssRect.width(), ssRect.height());
> +        
> +        // intersect this rectangle with the viewport
> +        rect.intersect(viewport);
> +        
> +        
> +        // now render it
> +        WebRect wrect(rect.x(), rect.y(), rect.width(), rect.height());
> +        if (wrect.width && wrect.height) {
> +            updateRootLayerContents(wrect);
> +            m_layerRenderer->updateRootLayerTextureRect(wrect);
> +        }
> +    }
> +    m_layerRenderer->rootLayer()->resetNeedsDisplay();
> +    m_scrollDamage = WebRect();
> +        
> +    // Draw the actual layers...
> +    m_layerRenderer->drawLayers(visibleRect, contentRect);
> +    m_layerRenderer->drawLayers(visibleRect, contentRect);
> +
> +    // readback into the canvas, if requested
> +    // FIXME Insert wjmaclean's readback code here for webkit bug 44127
> +    
> +    // finish if requested
> +    // FIXME: handle finish flag
> +    
> +    // we're done... put result onscreen
> +    m_layerRenderer->present();
> +
> +    // tell the browser that the rendering is done ---
> +    // TODO: this is being called too early, we need to defer calling this 
> +    //       until the swapbuffers itself has executed on the GPU.
> +    // m_client->onCompositeComplete();
> +}
> +#endif
> +
> +
>  PassOwnPtr<GLES2Context> WebViewImpl::getOnscreenGLES2Context()
>  {
>      return GLES2Context::create(GLES2ContextInternal::create(gles2Context(), false));
> diff --git a/WebKit/chromium/src/WebViewImpl.h b/WebKit/chromium/src/WebViewImpl.h
> index c29612123f4514c94dd7d970c73114a4beeec355..bcd4f46d87f1a5fc3d6fd64f35a92dbdc6960286 100644
> --- a/WebKit/chromium/src/WebViewImpl.h
> +++ b/WebKit/chromium/src/WebViewImpl.h
> @@ -34,6 +34,7 @@
>  #include "WebGLES2Context.h"
>  #include "WebNavigationPolicy.h"
>  #include "WebPoint.h"
> +#include "WebRect.h"
>  #include "WebSize.h"
>  #include "WebString.h"
>  #include "WebView.h"
> @@ -92,6 +93,8 @@ public:
>      virtual void resize(const WebSize&);
>      virtual void layout();
>      virtual void paint(WebCanvas*, const WebRect&);
> +    virtual void themeChanged();
> +    virtual void composite(bool finish);
>      virtual bool handleInputEvent(const WebInputEvent&);
>      virtual void mouseCaptureLost();
>      virtual void setFocus(bool enable);
> @@ -322,6 +325,9 @@ public:
>      }
>  
>  #if USE(ACCELERATED_COMPOSITING)
> +    void doComposite(WebCanvas*, bool finish);
> +    void scrollRootLayer(const WebSize& scrollDelta, const WebRect& clipRect);
> +    void invalidateRootLayerRect(const WebRect&);
>      void setRootLayerNeedsDisplay();
>      void setRootGraphicsLayer(WebCore::PlatformLayer*);
>  #endif
> @@ -510,6 +516,7 @@ private:
>      RefPtr<WebCore::Node> m_mouseCaptureNode;
>  
>  #if USE(ACCELERATED_COMPOSITING)
> +    WebRect m_scrollDamage;
>      OwnPtr<WebCore::LayerRendererChromium> m_layerRenderer;
>      bool m_isAcceleratedCompositingActive;
>  #endif
> diff --git a/WebKit/chromium/tests/PopupMenuTest.cpp b/WebKit/chromium/tests/PopupMenuTest.cpp
> index 50319af23b0a92f44c9158efb3e6461823981d10..aef29cf2d04124b4c87ef07a2e5f385096782b68 100644
> --- a/WebKit/chromium/tests/PopupMenuTest.cpp
> +++ b/WebKit/chromium/tests/PopupMenuTest.cpp
> @@ -128,6 +128,8 @@ public:
>      virtual void resize(const WebSize&) { }
>      virtual void layout() { }
>      virtual void paint(WebCanvas*, const WebRect&) { }
> +    virtual void themeChanged() { }
> +    virtual void composite(bool finish) { }
>      virtual bool handleInputEvent(const WebInputEvent&) { return true; }
>      virtual void mouseCaptureLost() { }
>      virtual void setFocus(bool) { }

[Nat Duca]

>> -        ASSERT(rootLayerWidth == updateRect.width() && rootLayerHeight == updateRect.height());
> I'm not sure I follow here.  Is the updateRect supposed to match the root layer size? I thought update rects
> can be smaller. Shouldn't rootLayerWidth == m_rootLayerTextureWidth?  You are asserting a different condition
> in the lines above.

Oops, bad variable naming. What should be asserted here is that the canvas containing the update bitmap should match the updateRect that we were handed. I renamed these variables to be bitmapWidth/etc to make the intent clearer.

> Don't you need to bind the root layer texture before calling this? 
Good point.


>> +    // ASSERT(static_cast<int>(CGBitmapContextGetWidth(m_rootLayerCGContext.get())) == updateRect.width() && static_cast<int>(CGBitmapContextGetHeight(m_rootLayerCGContext.get())) == updateRect.height());
>> +    void* pixels = m_rootLayerBackingStore.data();

>If this ASSERT is not necessary, please remove rather than commenting out.
This assert came from the existing code --- I'm simply moved this function rather than added the assertion.
I suggest we leave it there for the time being --- it looks like it is analogous to the SKIA "check the bitmap is the correct size" function.




> Does viewport specify the region of the WebWidget that will be grabbed or the place where it's
> going to end up on the canvas? The comment seems to indicate both but I'm not sure that's correct.
Updated my comments to be clearer on this. Net/net, the rectangle "viewport" on WebWidget will be drawn to viewport.x,viewport.y on the provided canvas. Put another way, the viewport will *not* be translated to the origin during drawing.


> I don't have the code handy but I seem to remember that isAcceleratedCompositingActive() is defined only if USE(ACCELERATED_COMPOSTING) is.
Thankfully, it is available at all times. I actually think this really helpful since it allows us to avoid monstrosities like:
#if USE(ACCELERATED_COMPOSITING)
if(isAcceleratedCompositingEnabled()) {
} else {
#endif
  ...
#if USE(ACCELERATED_COMPOSITING)
}
#endif



>Is it not possible that you could have a scrollDelta with non-zero width _and_ height? 
Great catch. I will invalidate in this case.

[Nat Duca]

Created attachment 66522 [details]
Patch

[Nat Duca]

Created attachment 66524 [details]
Patch

[Nat Duca]

Created attachment 66535 [details]
Patch

Remove extra drawLayers call. Oops

[Vangelis Kokkevis]

Comment on attachment 66535 [details]
Patch

Looks real good!
A couple of high level comments/questions:
1. What is it that prevents us from moving all the dirty and scroll-rect logic down to the compositor?  Is it that the root layer doesn't know how to draw itself?  Because if that's the only reason, it can probably be fixed.
2. Remember that comments need to be complete sentences, starting with a capital letter and ending in a period. It takes some getting used to but really improves consistency in the file. 

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

> WebKit/chromium/src/WebPopupMenuImpl.cpp:158
> +void WebPopupMenuImpl::paint(WebCanvas* canvas, const WebRect& viewport)
nit: Is there a good reason to change the parameter name?  If yes, then you'll probably want to change in the header as well for consistency.

> WebKit/chromium/src/WebViewImpl.cpp:2189
> +// WebViewImpl method
nit: is this comment necessary?

> WebKit/chromium/src/WebViewImpl.cpp:2211
> +    // below emits damage in the space "before" scrolling
Add a period at the end of this sentence and the one below.

> WebKit/chromium/src/WebViewImpl.cpp:2212
> +    // Using layer space for damage rects allows us to intermix invalidates with scrolls
missing period

> WebKit/chromium/src/WebViewImpl.cpp:2243
> +    // TODO: add a smarter damage aggregation logic? Right now, LayerChromium does simple union-ing
nit: In WebKit TODO's are FIXME
Add a period at the end of the sentence.

> WebKit/chromium/src/WebViewImpl.cpp:2252
> +    // so that invalidations and scroll invalidations play well with one-another
Add a period at the end of the sentence.

> WebKit/chromium/src/WebViewImpl.cpp:2266
> +    // TODO: add a smarter damage aggregation logic? Right now, LayerChromium does simple union-ing
Add a period at the end of the sentence.

> WebKit/chromium/src/WebViewImpl.cpp:2307
> +    // Give the compoisitor a chance to setup/resize the root texture handle and perform scrolling
typo: compositor
Add a period at the end of the sentence.

> WebKit/chromium/src/WebViewImpl.cpp:2339
> +    // readback into the canvas, if requested
Capitalize and add a period at the end of the sentence.

> WebKit/chromium/src/WebViewImpl.cpp:2343
> +    // FIXME: handle finish flag
Capitalize and add a period at the end of the sentence.

> WebKit/chromium/src/WebViewImpl.cpp:2345
> +    // we're done... put result onscreen
Capitalize and add a period at the end of the sentence.

> WebKit/chromium/src/WebViewImpl.cpp:2348
> +    // tell the browser that the rendering is done ---
Capitalize and add a period at the end of the sentence.

> WebKit/chromium/src/WebViewImpl.cpp:2349
> +    // TODO: this is being called too early, we need to defer calling this 
TODO->FIXME

[Nat Duca]

All good points. Uploading new patch...

With regard to placement of the root layer logic:
Option 1) push it into LayerRendererChromium --- same functions would exist, but they wouldn't be polluting WebViewImpl
Option 2) try to get root layers drawing as regular ContentLayerChromium's, then remove the root layer special case
Option 3) land the changes as is in order to fix the scrolling bugs, then try for option 2

Any preferences?

[Nat Duca]

Created attachment 66768 [details]
Patch

Fixes based on latest review.

[Darin Fisher (:fishd, Google)]

Comment on attachment 66768 [details]
Patch

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

I only reviewed the bits in WebKit/chromium for now.

> WebKit/chromium/public/WebWidget.h:62
> +    
nit: it looks like there is some extra whitespace here

> WebKit/chromium/public/WebWidget.h:70
> +    virtual void paint(WebCanvas*, const WebRect& viewport) = 0;
nit: viewport -> viewPort

> WebKit/chromium/public/WebWidget.h:71
> +    
and here

> WebKit/chromium/public/WebWidget.h:73
> +    // The finish argument controls whether the compositor will waiting for the
nit: "will waiting" -> "will wait"

> WebKit/chromium/public/WebWidget.h:78
> +
and here

> WebKit/chromium/public/WebWidget.h:79
> +    // Called to inform the WebWidget of a change in native widget settings.
nit: "change in native widget settings" -> "change in theme"

> WebKit/chromium/public/WebWidgetClient.h:49
> +    // Called when a call to WebWidget::composite is required
nit: please end this comment with a period.  nit: please move scheduleComposite
down below didScrollRect since didInvalidateRect and didScrollRect really belong
together.

> WebKit/chromium/src/ChromeClientImpl.cpp:529
> +        m_webView->scrollRootLayer(scrollDelta, clipRect);
nit: it seems like scrollRootLayer should also end with "Rect" to match the style of invalidateRootLayerRect.

> WebKit/chromium/src/WebPopupMenuImpl.cpp:180
> +}
nit: please add a new line between method definitions

> WebKit/chromium/src/WebPopupMenuImpl.h:66
> +    virtual void themeChanged();
nit: the order of method declarations here should match the order in which they are declared in WebWidget.h

> WebKit/chromium/src/WebViewImpl.cpp:915
> +        if (isAcceleratedCompositingActive()) {
nit: elsewhere you enclose the isAcceleratedCompositingActive() call within #if USE(ACCELERATED_COMPOSITING).  might be good to be consistent with that.

> WebKit/chromium/src/WebViewImpl.cpp:956
> +        invalidateRootLayerRect(rect);
why do we need to invalidate here?  it seems strange to invalidate during painting.

> WebKit/chromium/src/WebViewImpl.cpp:957
> +        doComposite(canvas, false);
in the case where this paint method is being called to capture a thumbnail
for the new tab page in chromium, shouldn't we avoid the present step?

i hadn't considered this issue when we talked about these API interfaces
before.

would it be meaningful to split up the composite step from the present
step?

> WebKit/chromium/src/WebViewImpl.cpp:2109
> +        WebRect damagedRect(0, 0, m_size.width, m_size.height);
nit: better to use IntRect for internal usage.  WebRect should just be
used at the interface level when passing things across the interface.

> WebKit/chromium/src/WebViewImpl.cpp:2192
> +    m_client->scheduleComposite();
perhaps we should re-order these?  imagine if the implementor of
scheduleComposite did something evil like call WebWidget::close!
in general, you should be extra defensive when you call out to
foreign code.

> WebKit/chromium/src/WebViewImpl.cpp:2197
> +void WebViewImpl::scrollRootLayer(const WebSize& scrollDelta, const WebRect& clipRect)
nit: recommend using IntSize and IntRect here

> WebKit/chromium/src/WebViewImpl.cpp:2222
> +                           contentRect.width(), contentRect.height());
nit: seems odd to line wrap all params except the last.  either wrap all or none?

> WebKit/chromium/src/WebViewImpl.cpp:2225
> +    // shared memory section.
nit: this comment mentions shared memory.  i don't see the painting step down below.

> WebKit/chromium/src/WebViewImpl.cpp:2227
> +        float dx = (float)scrollDelta.width;
nit: prefer C++ style static_cast<float> over C-style cast

> WebKit/chromium/src/WebViewImpl.cpp:2251
> +    IntRect iDamagedRect(damagedRect);
this is another reason to pass IntRect to this method instead of WebRect.

you could just write m_scrollDamage.unite(damagedRect), right?

> WebKit/chromium/src/WebViewImpl.cpp:2255
> +}
nit: add new line between method definitions

> WebKit/chromium/src/WebViewImpl.cpp:2256
> +void WebViewImpl::invalidateRootLayerRect(const WebRect& rect)
nit: we generally prefer to use WebCore types instead of API types for internal functions.
type conversion should happen on the barrier of the API.  WebRect only exists as a means to
hide IntRect (and the WebCore namespace) from the embedder (Chromium).

> WebKit/chromium/src/WebViewImpl.cpp:2262
> +    WebFrameImpl* webframe = mainFrameImpl();
it would probably be a bit more direct to go page()->mainFrame()->view() to get to the main FrameView.  in this case, you only need to null check page().

> WebKit/chromium/src/WebViewImpl.cpp:2269
> +    FloatRect lsRect(rect.x + view->scrollX(),
nit: lsRect is a mysterious name.  maybe contentRect would be better since you are taking scroll offset into account?

maybe you should be using ScrollView::windowToContents for this conversion?

(I'm concerned that all of this logic is only correct for the top-most FrameView, but we can worry about IFRAMEs in a separate bug.)

> WebKit/chromium/src/WebViewImpl.cpp:2288
> +        m_client->didInvalidateRect(damagedRect);
you could also call FrameView::invalidate() on the main FrameView.  that should be equivalent, but at least it would allow any future code that intercepts invalidations made on a FrameView to notice.

> WebKit/chromium/src/WebViewImpl.cpp:2302
> +    WebFrameImpl* webframe = mainFrameImpl();
nit: same comment about getting the FrameView from page()->mainFrame()->view().

> WebKit/chromium/src/WebViewImpl.cpp:2312
> +    IntRect viewport    = IntRect(0, 0, m_size.width, m_size.height);
nit: viewport -> viewPort and it is also not conventional in webkit style to line up the "="s like that.

> WebKit/chromium/src/WebViewImpl.cpp:2319
> +    damageRects.append(FloatRect(m_scrollDamage.x, m_scrollDamage.y, m_scrollDamage.width, m_scrollDamage.height));
if m_scrollDamage were stored as IntRect, then conversion to FloatRect would be implicit.

> WebKit/chromium/src/WebViewImpl.cpp:2322
> +        // the damage rects for the root layer is in layer space [e.g. unscrolled.]
nit: "the damage rects..." -> "The damage rects..."

> WebKit/chromium/src/WebViewImpl.cpp:2324
> +        const FloatRect ssRect = damageRects[i];
nit: prefer a more meaningful name than ssRect.  i'm not sure what the "ss" prefix means.

this looks like it could be the output of ScrollView::contentsToWindow.  maybe you should call the variable visibleDamage?  after intersecting with the viewPort that would seem to describe what it is.

> WebKit/chromium/src/WebViewImpl.cpp:2328
> +        
nit: avoid unnecessary whitespace.  blank lines should not have extra whitespace.

> WebKit/chromium/src/WebViewImpl.cpp:2334
> +        WebRect wrect(rect.x(), rect.y(), rect.width(), rect.height());
avoiding use of WebRect for internal interfaces should help eliminate the need for this WebRect temporary.

> WebKit/chromium/src/WebViewImpl.cpp:2346
> +    // Readback into the canvas, if requested.
nit: perhaps the readback step can be factored out into a helper function?

> WebKit/chromium/src/WebViewImpl.cpp:2356
> +    // FIXME: this is being called too early, we need to defer calling this 
isn't this what handling the finish flag will mean?  is this FIXME redundant with that FIXME?

> WebKit/chromium/src/WebViewImpl.h:96
> +    virtual void themeChanged();
nit: list methods here in the same order they are declared in WebWidget.h

> WebKit/chromium/src/WebViewImpl.h:520
> +    WebRect m_scrollDamage;
this should be stored as IntRect

> WebKit/chromium/tests/PopupMenuTest.cpp:131
> +    virtual void themeChanged() { }
nit: list methods here in the same order they are declared in WebWidget.h

[Vangelis Kokkevis]

(In reply to comment #10)
> All good points. Uploading new patch...
> 
> With regard to placement of the root layer logic:
> Option 1) push it into LayerRendererChromium --- same functions would exist, but they wouldn't be polluting WebViewImpl
> Option 2) try to get root layers drawing as regular ContentLayerChromium's, then remove the root layer special case
> Option 3) land the changes as is in order to fix the scrolling bugs, then try for option 2
> 
> Any preferences?

Definitely Option #3.  I was just curious how close we are to moving this logic to the compositor. I think longer term we should do #2 but be smart about update rects (not do a blind union like composited layers do now) so that we don't end up redrawing more than we need to.

[Nat Duca]

I suspect we're pretty near to #3. The two gaps are (1) merging the updateLayerContents logic with the RootLayer equivalent and (2) figuring out what to do with scrolling on child rects.

I am hopeful that chasing our IFrame bugs will end up being a forcing function for fusing rootLayer with ContentLayer.

[Nat Duca]

> it seems like scrollRootLayer should also end with "Rect" to match the style of invalidateRootLayerRect.

Interesting thought. The one difference is that scrollLayer isn't actually sending a rectangle. I personally lean away from adding a rect suffix, but I'm ok either way. :)


> nit: elsewhere you enclose the isAcceleratedCompositingActive() call within #if USE(ACCELERATED_COMPOSITING).  might be good to be consistent with that.
The rule I'm [trying] to follow is as follows...

If the code depends on accelerated compositing being ON, but does not itself use any code that is conditional on USE(ACCELERATED_COMPOSITING), I use isAcceleratedCompositingActive. E.g. render_widget.cc::DoDeferredUpdate's uses isAccelerated because the methods it dispatches to are always present.

In contrast, if the code uses functions or variables that are conditional on USE(ACCELERATED_COMPOSITING), I enclose the just the "dependent" part of the function in a USE(ACCELERERATED_COMPOSITING) test, leaving the isAccelerated test outside. E.g.:
  if(isAcceleratedCompositing()) {
#if USE(ACCELERATED_COMPOSITING)
    someCompositorOnlyFunction();
#endif
  } else {
    // software path
  }
To me at least, this seemed better than:
#if USE(ACCELERATED_COMPOSITING)
  if(isAcceleratedCompositing()) {
    someCompositorOnlyFunction();
  } else {
#endif
    // software path
#if USE(ACCELERATED_COMPOSITING)
  }
#endif

I can change approach if there are strong opinions. :)


> WebKit/chromium/src/WebViewImpl.cpp:956
> +        invalidateRootLayerRect(rect);
> why do we need to invalidate here?  it seems strange to invalidate during painting.
I have no clue! Deleted! :)

> in the case where this paint method is being called to capture a thumbnail
> for the new tab page in chromium, shouldn't we avoid the present step?

Good idea. I'll push the present() that is in the doComposite flow up to the ::paint function.
The placeholder for James' readback code will also move up to ::paint.



> WebViewImpl::themeChanged:  you could also call FrameView::invalidate() on the main FrameView
I switched over to this code. Afaik, I can call this directly, and it will route back to the view if needed. Does this seem right?




>> WebKit/chromium/src/WebViewImpl.cpp:2356
>> +    // FIXME: this is being called too early, we need to defer calling this 
>isn't this what handling the finish flag will mean?  is this FIXME redundant with that FIXME?
Yes it is. Deleted.

[Nat Duca]

Created attachment 66815 [details]
Patch

Fixes based on Darin's comments.

[WebKit Review Bot]

Attachment 66815 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebViewImpl.cpp:970:  Missing space before ( in if(  [whitespace/parens] [5]
WebKit/chromium/src/WebViewImpl.cpp:2207:  Missing space before ( in if(  [whitespace/parens] [5]
WebKit/chromium/src/WebViewImpl.cpp:2294:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nat Duca]

Created attachment 66828 [details]
Patch

Oops, Stylebot.

[Nat Duca]

Created attachment 66951 [details]
Patch

Changes to allow this patch to land without disrupting the downstream 'old' render_widget.

[Vangelis Kokkevis]

Comment on attachment 66828 [details]
Patch

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

Just a couple of random nits on my side.

> WebCore/platform/graphics/chromium/LayerRendererChromium.h:64
> +    // updates size of root texture, if needed, and scrolls the backbuffer
Descriptions need to start with a capital letter and end in a period. Here and below.

> WebKit/chromium/src/WebViewImpl.cpp:2169
> +    // Compute the region we will expose by scrolling We use the
Missing period after "scrolling"

> WebKit/chromium/src/WebViewImpl.cpp:2204
> +    // so that invalidations and scroll invalidations play well with one-another.
this comment should probably move down, to right above the point where you do the actual conversion.

[Nat Duca]

Created attachment 66956 [details]
Patch

Git pain.

[Nat Duca]

Created attachment 66958 [details]
Patch

Changes based on vangelis' comments

[Vangelis Kokkevis]

Comment on attachment 66956 [details]
Patch

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

Looks good!

> WebKit/chromium/ChangeLog:35
> [phone]  Nat Duca  <[e-mail]>
Changelog now has two entries :)

[Nat Duca]

Created attachment 66961 [details]
Patch

Remove double changelog.

[Darin Fisher (:fishd, Google)]

(In reply to comment #15)
> > it seems like scrollRootLayer should also end with "Rect" to match the style of invalidateRootLayerRect.
> 
> Interesting thought. The one difference is that scrollLayer isn't actually sending a rectangle. I personally lean away from adding a rect suffix, but I'm ok either way. :)

Well, there is the clipping rectangle.  It is "scrolling the root layer within the given rectangle."


> > nit: elsewhere you enclose the isAcceleratedCompositingActive() call within #if USE(ACCELERATED_COMPOSITING).  might be good to be consistent with that.
> The rule I'm [trying] to follow is as follows...
> 
> If the code depends on accelerated compositing being ON, but does not itself use any code that is conditional on USE(ACCELERATED_COMPOSITING), I use isAcceleratedCompositingActive. E.g. render_widget.cc::DoDeferredUpdate's uses isAccelerated because the methods it dispatches to are always present.
> 
> In contrast, if the code uses functions or variables that are conditional on USE(ACCELERATED_COMPOSITING), I enclose the just the "dependent" part of the function in a USE(ACCELERERATED_COMPOSITING) test, leaving the isAccelerated test outside. E.g.:
>   if(isAcceleratedCompositing()) {
> #if USE(ACCELERATED_COMPOSITING)
>     someCompositorOnlyFunction();
> #endif
>   } else {
>     // software path
>   }
> To me at least, this seemed better than:
> #if USE(ACCELERATED_COMPOSITING)
>   if(isAcceleratedCompositing()) {
>     someCompositorOnlyFunction();
>   } else {
> #endif
>     // software path
> #if USE(ACCELERATED_COMPOSITING)
>   }
> #endif
> 
> I can change approach if there are strong opinions. :)

OK, that's fine.  Thanks for the explanation.


> > WebKit/chromium/src/WebViewImpl.cpp:956
> > +        invalidateRootLayerRect(rect);
> > why do we need to invalidate here?  it seems strange to invalidate during painting.
> I have no clue! Deleted! :)
> 
> > in the case where this paint method is being called to capture a thumbnail
> > for the new tab page in chromium, shouldn't we avoid the present step?
> 
> Good idea. I'll push the present() that is in the doComposite flow up to the ::paint function.
> The placeholder for James' readback code will also move up to ::paint.

OK


> > WebViewImpl::themeChanged:  you could also call FrameView::invalidate() on the main FrameView
> I switched over to this code. Afaik, I can call this directly, and it will route back to the view if needed. Does this seem right?

Yes.

[Darin Fisher (:fishd, Google)]

Comment on attachment 66961 [details]
Patch

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

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:275
> +    // ASSERT(static_cast<int>(CGBitmapContextGetWidth(m_rootLayerCGContext.get())) == updateRect.width() && static_cast<int>(CGBitmapContextGetHeight(m_rootLayerCGContext.get())) == updateRect.height());
is there a reason for commenting out this assertion?  should it just be deleted instead?

> WebKit/chromium/ChangeLog:15
> +        a pair of methods: paintComposited and
this ChangeLog needs to be revised to correspond to the actual name
of the methods.  you should probably just regenerate it completely
as the list of methods below is also out of date.

> WebKit/chromium/src/WebViewImpl.cpp:956
> +        doComposite(canvas);
it's not clear why doComposite needs a canvas parameter if wjmaclean's
readback code is going to go below here.

> WebKit/chromium/src/WebViewImpl.cpp:2311
> +void WebViewImpl::doComposite(WebCanvas* canvas)
i'm pretty sure this canvas parameter can be deleted now.

Looking great overall.  Just these few minor issues remaining.

[Nat Duca]

New patch coming with Darin's suggestions. ScrollRootLayer -> ScrollRootLayerRect, changelog fixes, doComposite has no arguments and accidentally-commented out assert all fixed.

[Nat Duca]

Created attachment 67089 [details]
Patch

[Nat Duca]

Created attachment 67094 [details]
I fail at changelogs.

[Nat Duca]

Created attachment 67096 [details]
I fail at git as well.

[Darin Fisher (:fishd, Google)]

Comment on attachment 67096 [details]
I fail at git as well.

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

> WebKit/chromium/src/ChromeClientImpl.cpp:502
> +    } else {
nit: no angle brackets here

> WebKit/chromium/src/ChromeClientImpl.cpp:528
> +    } else {
nit: no angle brackets here

R=me otherwise.  I'll fix those up and commit this patch manually.

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/67244