[BlackBerry] Overlays display checkerboard that doesn't resolve
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 3 Aug 2012 15:19:17 +0000 (15:19 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 3 Aug 2012 15:19:17 +0000 (15:19 +0000)
https://bugs.webkit.org/show_bug.cgi?id=93099

Patch by Arvid Nilsson <anilsson@rim.com> on 2012-08-03
Reviewed by Antonio Gomes.

The WebKit-thread overlays, like tap highlight, inspector highlight and
selection are all part of a separate graphics layer tree rooted in
WebPagePrivate::m_overlayLayer.

When LayerRenderer needs to schedule a commit to reactively render
tiles and resolve checkerboard, it does so through the root layer.
Since the overlay layer root didn't have a GraphicsLayerClient, there
was no implementation of GraphicsLayerClient::notifySyncRequired() to
call, and a commit was never scheduled, thus checkerboard never
resolved.

Fixed by adding a fallback implementation of GraphicsLayerClient in
WebPagePrivate and hooking up the overlay root to it. Also, this
implementation can be shared by the various overlays to avoide code
duplication, specifically to implement notifySyncRequired(),
showDebugBorders() and showRepaintCounter() only once.

Fixing this revealed a bug where the web page would get stuck in an
endless sequence of commits. It turned out that
WebPagePrivate::updateDelegatedOverlays() was called right in the
middle of the commit operation, after performing the webkit thread part
of the commit operation but before we continued on the compositing
thread. Since updateDelegatedOverlays() typically mutates layers, this
is very bad (layers should not be mutated mid-commit). The mutations
also cause a new commit to scheduled from within the current, which
results in an endless sequence of commits.

Fixed this latter bug by moving the updateDelegatedOverlays() call to
the beginning of the method where it can cause no harm. This is before
we mark the web page as no longer needing commit, so even if the
implementation flips the "needs commit" bit, we will immediately flip
it back and proceed with commit as usual.

PR 187458, 184377

* Api/WebPage.cpp:
(BlackBerry::WebKit::WebPagePrivate::overlayLayer):
(BlackBerry::WebKit::WebPagePrivate::commitRootLayerIfNeeded):
(WebKit):
(BlackBerry::WebKit::WebPagePrivate::notifySyncRequired):
(BlackBerry::WebKit::WebPagePrivate::showDebugBorders):
(BlackBerry::WebKit::WebPagePrivate::showRepaintCounter):
* Api/WebPage_p.h:
(WebPagePrivate):
(BlackBerry::WebKit::WebPagePrivate::notifyAnimationStarted):
(BlackBerry::WebKit::WebPagePrivate::paintContents):
* WebCoreSupport/InspectorOverlay.cpp:
(WebCore::InspectorOverlay::notifySyncRequired):
(WebCore::InspectorOverlay::showDebugBorders):
(WebCore::InspectorOverlay::showRepaintCounter):
* WebKitSupport/DefaultTapHighlight.cpp:
(BlackBerry::WebKit::DefaultTapHighlight::notifySyncRequired):
(BlackBerry::WebKit::DefaultTapHighlight::showDebugBorders):
(WebKit):
(BlackBerry::WebKit::DefaultTapHighlight::showRepaintCounter):
* WebKitSupport/DefaultTapHighlight.h:
(DefaultTapHighlight):
* WebKitSupport/SelectionOverlay.cpp:
(BlackBerry::WebKit::SelectionOverlay::notifySyncRequired):
(BlackBerry::WebKit::SelectionOverlay::showDebugBorders):
(WebKit):
(BlackBerry::WebKit::SelectionOverlay::showRepaintCounter):
* WebKitSupport/SelectionOverlay.h:
(SelectionOverlay):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@124615 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/blackberry/Api/WebPage.cpp
Source/WebKit/blackberry/Api/WebPage_p.h
Source/WebKit/blackberry/ChangeLog
Source/WebKit/blackberry/WebCoreSupport/InspectorOverlay.cpp
Source/WebKit/blackberry/WebKitSupport/DefaultTapHighlight.cpp
Source/WebKit/blackberry/WebKitSupport/DefaultTapHighlight.h
Source/WebKit/blackberry/WebKitSupport/SelectionOverlay.cpp
Source/WebKit/blackberry/WebKitSupport/SelectionOverlay.h

index 9c6026b..fef1661 100644 (file)
@@ -5623,10 +5623,8 @@ LayerRenderingResults WebPagePrivate::lastCompositingResults() const
 
 GraphicsLayer* WebPagePrivate::overlayLayer()
 {
-    // The overlay layer has no GraphicsLayerClient, it's just a container
-    // for various overlays.
     if (!m_overlayLayer)
-        m_overlayLayer = GraphicsLayer::create(0);
+        m_overlayLayer = GraphicsLayer::create(this);
 
     return m_overlayLayer.get();
 }
@@ -5745,6 +5743,10 @@ bool WebPagePrivate::commitRootLayerIfNeeded()
     if (!view)
         return false;
 
+    // This can do pretty much anything depending on the overlay,
+    // so in case it causes relayout or schedule a commit, call it early.
+    updateDelegatedOverlays();
+
     // If we sync compositing layers when a layout is pending, we may cause painting of compositing
     // layer content to occur before layout has happened, which will cause paintContents() to bail.
     if (needsLayoutRecursive(view)) {
@@ -5767,7 +5769,6 @@ bool WebPagePrivate::commitRootLayerIfNeeded()
     if (m_frameLayers && m_frameLayers->hasLayer())
         m_frameLayers->commitOnWebKitThread(scale);
 
-    updateDelegatedOverlays();
     if (m_overlayLayer)
         m_overlayLayer->platformLayer()->commitOnWebKitThread(scale);
 
@@ -5988,6 +5989,21 @@ void WebPagePrivate::setNeedsOneShotDrawingSynchronization()
     m_needsCommit = true;
     m_needsOneShotDrawingSynchronization = true;
 }
+
+void WebPagePrivate::notifySyncRequired(const GraphicsLayer*)
+{
+    scheduleRootLayerCommit();
+}
+
+bool WebPagePrivate::showDebugBorders(const GraphicsLayer*) const
+{
+    return m_page->settings()->showDebugBorders();
+}
+
+bool WebPagePrivate::showRepaintCounter(const GraphicsLayer*) const
+{
+    return m_page->settings()->showRepaintCounter();
+}
 #endif // USE(ACCELERATED_COMPOSITING)
 
 void WebPagePrivate::enterFullscreenForNode(Node* node)
index a7c0690..8b1660d 100644 (file)
@@ -25,6 +25,7 @@
 #include "InspectorOverlay.h"
 #if USE(ACCELERATED_COMPOSITING)
 #include "GLES2Context.h"
+#include "GraphicsLayerClient.h"
 #include "LayerRenderer.h"
 #include <EGL/egl.h>
 #endif
@@ -83,7 +84,12 @@ class WebPageCompositorPrivate;
 // In WebPagePrivate, the screen size is called the transformedViewportSize,
 // the viewport position is called the transformedScrollPosition,
 // and the viewport size is called the transformedActualVisibleSize.
-class WebPagePrivate : public PageClientBlackBerry, public WebSettingsDelegate, public Platform::GuardedPointerBase {
+class WebPagePrivate : public PageClientBlackBerry
+                     , public WebSettingsDelegate
+#if USE(ACCELERATED_COMPOSITING)
+                     , public WebCore::GraphicsLayerClient
+#endif
+                     , public Platform::GuardedPointerBase {
 public:
     enum ViewMode { Mobile, Desktop, FixedDesktop };
     enum LoadState { None /* on instantiation of page */, Provisional, Committed, Finished, Failed };
@@ -386,6 +392,13 @@ public:
     WebCore::LayerRenderingResults lastCompositingResults() const;
     WebCore::GraphicsLayer* overlayLayer();
 
+    // Fallback GraphicsLayerClient implementation, used for various overlay layers.
+    virtual void notifyAnimationStarted(const WebCore::GraphicsLayer*, double time) { }
+    virtual void notifySyncRequired(const WebCore::GraphicsLayer*);
+    virtual void paintContents(const WebCore::GraphicsLayer*, WebCore::GraphicsContext&, WebCore::GraphicsLayerPaintingPhase, const WebCore::IntRect& inClip) { }
+    virtual bool showDebugBorders(const WebCore::GraphicsLayer*) const;
+    virtual bool showRepaintCounter(const WebCore::GraphicsLayer*) const;
+
     // WebKit thread, plumbed through from ChromeClientBlackBerry.
     void setRootLayerWebKitThread(WebCore::Frame*, WebCore::LayerWebKitThread*);
     void setNeedsOneShotDrawingSynchronization();
index dd86e5e..81e8167 100644 (file)
@@ -1,3 +1,75 @@
+2012-08-03  Arvid Nilsson  <anilsson@rim.com>
+
+        [BlackBerry] Overlays display checkerboard that doesn't resolve
+        https://bugs.webkit.org/show_bug.cgi?id=93099
+
+        Reviewed by Antonio Gomes.
+
+        The WebKit-thread overlays, like tap highlight, inspector highlight and
+        selection are all part of a separate graphics layer tree rooted in
+        WebPagePrivate::m_overlayLayer.
+
+        When LayerRenderer needs to schedule a commit to reactively render
+        tiles and resolve checkerboard, it does so through the root layer.
+        Since the overlay layer root didn't have a GraphicsLayerClient, there
+        was no implementation of GraphicsLayerClient::notifySyncRequired() to
+        call, and a commit was never scheduled, thus checkerboard never
+        resolved.
+
+        Fixed by adding a fallback implementation of GraphicsLayerClient in
+        WebPagePrivate and hooking up the overlay root to it. Also, this
+        implementation can be shared by the various overlays to avoide code
+        duplication, specifically to implement notifySyncRequired(),
+        showDebugBorders() and showRepaintCounter() only once.
+
+        Fixing this revealed a bug where the web page would get stuck in an
+        endless sequence of commits. It turned out that
+        WebPagePrivate::updateDelegatedOverlays() was called right in the
+        middle of the commit operation, after performing the webkit thread part
+        of the commit operation but before we continued on the compositing
+        thread. Since updateDelegatedOverlays() typically mutates layers, this
+        is very bad (layers should not be mutated mid-commit). The mutations
+        also cause a new commit to scheduled from within the current, which
+        results in an endless sequence of commits.
+
+        Fixed this latter bug by moving the updateDelegatedOverlays() call to
+        the beginning of the method where it can cause no harm. This is before
+        we mark the web page as no longer needing commit, so even if the
+        implementation flips the "needs commit" bit, we will immediately flip
+        it back and proceed with commit as usual.
+
+        PR 187458, 184377
+
+        * Api/WebPage.cpp:
+        (BlackBerry::WebKit::WebPagePrivate::overlayLayer):
+        (BlackBerry::WebKit::WebPagePrivate::commitRootLayerIfNeeded):
+        (WebKit):
+        (BlackBerry::WebKit::WebPagePrivate::notifySyncRequired):
+        (BlackBerry::WebKit::WebPagePrivate::showDebugBorders):
+        (BlackBerry::WebKit::WebPagePrivate::showRepaintCounter):
+        * Api/WebPage_p.h:
+        (WebPagePrivate):
+        (BlackBerry::WebKit::WebPagePrivate::notifyAnimationStarted):
+        (BlackBerry::WebKit::WebPagePrivate::paintContents):
+        * WebCoreSupport/InspectorOverlay.cpp:
+        (WebCore::InspectorOverlay::notifySyncRequired):
+        (WebCore::InspectorOverlay::showDebugBorders):
+        (WebCore::InspectorOverlay::showRepaintCounter):
+        * WebKitSupport/DefaultTapHighlight.cpp:
+        (BlackBerry::WebKit::DefaultTapHighlight::notifySyncRequired):
+        (BlackBerry::WebKit::DefaultTapHighlight::showDebugBorders):
+        (WebKit):
+        (BlackBerry::WebKit::DefaultTapHighlight::showRepaintCounter):
+        * WebKitSupport/DefaultTapHighlight.h:
+        (DefaultTapHighlight):
+        * WebKitSupport/SelectionOverlay.cpp:
+        (BlackBerry::WebKit::SelectionOverlay::notifySyncRequired):
+        (BlackBerry::WebKit::SelectionOverlay::showDebugBorders):
+        (WebKit):
+        (BlackBerry::WebKit::SelectionOverlay::showRepaintCounter):
+        * WebKitSupport/SelectionOverlay.h:
+        (SelectionOverlay):
+
 2012-08-02  Arvid Nilsson  <anilsson@rim.com>
 
         [BlackBerry] Add default implementation of GraphicsLayerClient::contentsVisible()
index 9178432..9ee5f65 100644 (file)
@@ -24,8 +24,6 @@
 #include "FrameView.h"
 #include "GraphicsContext.h"
 #include "GraphicsLayer.h"
-#include "Page.h"
-#include "Settings.h"
 #include "WebPage_p.h"
 
 
@@ -43,9 +41,9 @@ InspectorOverlay::InspectorOverlay(BlackBerry::WebKit::WebPagePrivate* page, Ins
 }
 
 #if USE(ACCELERATED_COMPOSITING)
-void InspectorOverlay::notifySyncRequired(const GraphicsLayer*)
+void InspectorOverlay::notifySyncRequired(const GraphicsLayer* layer)
 {
-    m_webPage->scheduleRootLayerCommit();
+    m_webPage->notifySyncRequired(layer);
 }
 
 void InspectorOverlay::paintContents(const GraphicsLayer*, GraphicsContext& context, GraphicsLayerPaintingPhase, const IntRect& inClip)
@@ -57,14 +55,14 @@ void InspectorOverlay::paintContents(const GraphicsLayer*, GraphicsContext& cont
     context.restore();
 }
 
-bool InspectorOverlay::showDebugBorders(const GraphicsLayer*) const
+bool InspectorOverlay::showDebugBorders(const GraphicsLayer* layer) const
 {
-    return m_webPage->m_page->settings()->showDebugBorders();
+    return m_webPage->showDebugBorders(layer);
 }
 
-bool InspectorOverlay::showRepaintCounter(const GraphicsLayer*) const
+bool InspectorOverlay::showRepaintCounter(const GraphicsLayer* layer) const
 {
-    return m_webPage->m_page->settings()->showRepaintCounter();
+    return m_webPage->showRepaintCounter(layer);
 }
 #endif
 
index 7e06d0e..5b8eeb5 100644 (file)
@@ -110,9 +110,9 @@ void DefaultTapHighlight::hide()
         m_overlay->override()->addAnimation(fadeAnimation);
 }
 
-void DefaultTapHighlight::notifySyncRequired(const GraphicsLayer*)
+void DefaultTapHighlight::notifySyncRequired(const GraphicsLayer* layer)
 {
-    m_page->scheduleRootLayerCommit();
+    m_page->notifySyncRequired(layer);
 }
 
 void DefaultTapHighlight::paintContents(const GraphicsLayer*, GraphicsContext& c, GraphicsLayerPaintingPhase, const IntRect& /*inClip*/)
@@ -148,6 +148,16 @@ void DefaultTapHighlight::paintContents(const GraphicsLayer*, GraphicsContext& c
     c.restore();
 }
 
+bool DefaultTapHighlight::showDebugBorders(const GraphicsLayer* layer) const
+{
+    return m_page->showDebugBorders(layer);
+}
+
+bool DefaultTapHighlight::showRepaintCounter(const GraphicsLayer* layer) const
+{
+    return m_page->showRepaintCounter(layer);
+}
+
 } // namespace WebKit
 } // namespace BlackBerry
 
index 3489337..c04f637 100644 (file)
@@ -56,8 +56,8 @@ public:
     virtual void notifyAnimationStarted(const WebCore::GraphicsLayer*, double time) { }
     virtual void notifySyncRequired(const WebCore::GraphicsLayer*);
     virtual void paintContents(const WebCore::GraphicsLayer*, WebCore::GraphicsContext&, WebCore::GraphicsLayerPaintingPhase, const WebCore::IntRect& inClip);
-    virtual bool showDebugBorders(const WebCore::GraphicsLayer*) const { return false; }
-    virtual bool showRepaintCounter(const WebCore::GraphicsLayer*) const { return false; }
+    virtual bool showDebugBorders(const WebCore::GraphicsLayer*) const;
+    virtual bool showRepaintCounter(const WebCore::GraphicsLayer*) const;
 
 private:
     DefaultTapHighlight(WebPagePrivate*);
index 58fd28e..cd8c35d 100644 (file)
@@ -94,9 +94,9 @@ void SelectionOverlay::hide()
     m_page->m_webPage->removeOverlay(m_overlay.get());
 }
 
-void SelectionOverlay::notifySyncRequired(const GraphicsLayer*)
+void SelectionOverlay::notifySyncRequired(const GraphicsLayer* layer)
 {
-    m_page->scheduleRootLayerCommit();
+    m_page->notifySyncRequired(layer);
 }
 
 void SelectionOverlay::paintContents(const GraphicsLayer*, GraphicsContext& c, GraphicsLayerPaintingPhase, const IntRect& inClip)
@@ -133,6 +133,16 @@ void SelectionOverlay::paintContents(const GraphicsLayer*, GraphicsContext& c, G
     c.restore();
 }
 
+bool SelectionOverlay::showDebugBorders(const GraphicsLayer* layer) const
+{
+    return m_page->showDebugBorders(layer);
+}
+
+bool SelectionOverlay::showRepaintCounter(const GraphicsLayer* layer) const
+{
+    return m_page->showRepaintCounter(layer);
+}
+
 } // namespace WebKit
 } // namespace BlackBerry
 
index 6c8d075..be61e76 100644 (file)
@@ -52,8 +52,8 @@ public:
     virtual void notifyAnimationStarted(const WebCore::GraphicsLayer*, double time) { }
     virtual void notifySyncRequired(const WebCore::GraphicsLayer*);
     virtual void paintContents(const WebCore::GraphicsLayer*, WebCore::GraphicsContext&, WebCore::GraphicsLayerPaintingPhase, const WebCore::IntRect& inClip);
-    virtual bool showDebugBorders(const WebCore::GraphicsLayer*) const { return false; }
-    virtual bool showRepaintCounter(const WebCore::GraphicsLayer*) const { return false; }
+    virtual bool showDebugBorders(const WebCore::GraphicsLayer*) const;
+    virtual bool showRepaintCounter(const WebCore::GraphicsLayer*) const;
 
 private:
     SelectionOverlay(WebPagePrivate*);