[webkit-changes] [125678] trunk/Source/WebKit/blackberry

Wed, 15 Aug 2012 09:05:31 -0700

Title: [125678] trunk/Source/WebKit/blackberry
Revision
125678
Author
[email protected]
Date
2012-08-15 09:05:56 -0700 (Wed, 15 Aug 2012)

Log Message

Robust-fy the LayerWebKitThread ownership with InRegionScroller
https://bugs.webkit.org/show_bug.cgi?id=93983
PR #191737

Reviewed by Yong Li.
Patch by Antonio Gomes <[email protected]>

Patch changes the way we currently keep track of the active scrollable area
objects: before, we acquired the scrollable areas and just passed them in a vector up
to the client, copying it over and over again. Also, it was a client responsability to
delete stuff (BAD!).
Now, we keep track of vector within InRegionScroller, as a class member, which allows us to
avoid copies (in follow up patch), and control until when these objects outlive.

Patch also changes InRegionScrollableArea to "retptr" the composited layer
associated to it (if any). This ensure we have a non-null scrollable element always.

As mentioned, InRegionScroller is now responsible for deleting and vector of scrollable areas.

Internally reviewed by Arvid Nilsson.

* Api/InRegionScroller.cpp:
(WebKit):
(BlackBerry::WebKit::InRegionScrollerPrivate::reset): Method is now responsible for
deleting the tracked scrollable areas.
(BlackBerry::WebKit::InRegionScrollerPrivate::calculateInRegionScrollableAreasForPoint):
Renamed from 'inRegionScrollableAreasForPoint'. It was changed in order to store the
scrollable area objects instead of just pass a copy of them up to the client.
(BlackBerry::WebKit::InRegionScrollerPrivate::activeInRegionScrollableAreas): Getter.
(BlackBerry::WebKit::InRegionScrollerPrivate::pushBackInRegionScrollable): It was
promoted to a class method instead of a local helper.
* Api/InRegionScroller_p.h:
(WebKit):
(InRegionScrollerPrivate):
* Api/WebPage.cpp:
(BlackBerry::WebKit::WebPagePrivate::setScrollOriginPoint): Adjustments needed due to the
above changed.
* WebKitSupport/InRegionScrollableArea.cpp:
(BlackBerry::WebKit::InRegionScrollableArea::~InRegionScrollableArea): Clear up the cached layer.
(WebKit):
(BlackBerry::WebKit::InRegionScrollableArea::InRegionScrollableArea):
* WebKitSupport/InRegionScrollableArea.h:
(InRegionScrollableArea):

Modified Paths

Diff

Modified: trunk/Source/WebKit/blackberry/Api/InRegionScroller.cpp (125677 => 125678)


--- trunk/Source/WebKit/blackberry/Api/InRegionScroller.cpp	2012-08-15 15:56:28 UTC (rev 125677)
+++ trunk/Source/WebKit/blackberry/Api/InRegionScroller.cpp	2012-08-15 16:05:56 UTC (rev 125678)
@@ -45,7 +45,6 @@
 static RenderLayer* parentLayer(RenderLayer*);
 static Node* enclosingLayerNode(RenderLayer*);
 static bool isNonRenderViewFixedPositionedContainer(RenderLayer*);
-static void pushBackInRegionScrollable(std::vector<Platform::ScrollViewBase*>&, InRegionScrollableArea*, InRegionScrollerPrivate*);
 
 InRegionScroller::InRegionScroller(WebPagePrivate* webPagePrivate)
     : d(new InRegionScrollerPrivate(webPagePrivate))
@@ -92,6 +91,10 @@
 void InRegionScrollerPrivate::reset()
 {
     setNode(0);
+
+    for (size_t i = 0; i < m_activeInRegionScrollableAreas.size(); ++i)
+        delete m_activeInRegionScrollableAreas[i];
+    m_activeInRegionScrollableAreas.clear();
 }
 
 bool InRegionScrollerPrivate::hasNode() const
@@ -143,15 +146,14 @@
     return scrollNodeRecursively(node(), delta);
 }
 
-std::vector<Platform::ScrollViewBase*> InRegionScrollerPrivate::inRegionScrollableAreasForPoint(const WebCore::IntPoint& point)
+void InRegionScrollerPrivate::calculateInRegionScrollableAreasForPoint(const WebCore::IntPoint& point)
 {
-    std::vector<Platform::ScrollViewBase*> validReturn;
-    std::vector<Platform::ScrollViewBase*> emptyReturn;
+    ASSERT(m_activeInRegionScrollableAreas.empty());
 
     HitTestResult result = m_webPage->m_mainFrame->eventHandler()->hitTestResultAtPoint(m_webPage->mapFromViewportToContents(point), false /*allowShadowContent*/);
     Node* node = result.innerNonSharedNode();
     if (!node || !node->renderer())
-        return emptyReturn;
+        return;
 
     RenderLayer* layer = node->renderer()->enclosingLayer();
     do {
@@ -160,37 +162,39 @@
         if (renderer->isRenderView()) {
             if (RenderView* renderView = toRenderView(renderer)) {
                 FrameView* view = renderView->frameView();
-                if (!view)
-                    return emptyReturn;
+                if (!view) {
+                    reset();
+                    return;
+                }
 
                 if (canScrollInnerFrame(view->frame())) {
-                    pushBackInRegionScrollable(validReturn, new InRegionScrollableArea(m_webPage, layer), this);
+                    pushBackInRegionScrollable(new InRegionScrollableArea(m_webPage, layer));
                     continue;
                 }
             }
         } else if (canScrollRenderBox(layer->renderBox())) {
-            pushBackInRegionScrollable(validReturn, new InRegionScrollableArea(m_webPage, layer), this);
+            pushBackInRegionScrollable(new InRegionScrollableArea(m_webPage, layer));
             continue;
         }
 
         // If we run into a fix positioned layer, set the last scrollable in-region object
         // as not able to propagate scroll to its parent scrollable.
-        if (isNonRenderViewFixedPositionedContainer(layer) && validReturn.size()) {
-            Platform::ScrollViewBase* end = validReturn.back();
+        if (isNonRenderViewFixedPositionedContainer(layer) && m_activeInRegionScrollableAreas.size()) {
+            Platform::ScrollViewBase* end = m_activeInRegionScrollableAreas.back();
             end->setCanPropagateScrollingToEnclosingScrollable(false);
         }
 
     } while (layer = parentLayer(layer));
 
-    if (validReturn.empty())
-        return emptyReturn;
+    if (m_activeInRegionScrollableAreas.empty())
+        return;
 
     // Post-calculate the visible window rects in reverse hit test order so
     // we account for all and any clipping rects.
     WebCore::IntRect recursiveClippingRect(WebCore::IntPoint::zero(), m_webPage->transformedViewportSize());
 
-    std::vector<Platform::ScrollViewBase*>::reverse_iterator rend = validReturn.rend();
-    for (std::vector<Platform::ScrollViewBase*>::reverse_iterator rit = validReturn.rbegin(); rit != rend; ++rit) {
+    std::vector<Platform::ScrollViewBase*>::reverse_iterator rend = m_activeInRegionScrollableAreas.rend();
+    for (std::vector<Platform::ScrollViewBase*>::reverse_iterator rit = m_activeInRegionScrollableAreas.rbegin(); rit != rend; ++rit) {
 
         InRegionScrollableArea* curr = static_cast<InRegionScrollableArea*>(*rit);
         RenderLayer* layer = curr->layer();
@@ -220,8 +224,11 @@
             recursiveClippingRect = visibleWindowRect;
         }
     }
+}
 
-    return validReturn;
+const std::vector<Platform::ScrollViewBase*>& InRegionScrollerPrivate::activeInRegionScrollableAreas() const
+{
+    return m_activeInRegionScrollableAreas;
 }
 
 bool InRegionScrollerPrivate::setLayerScrollPosition(RenderLayer* layer, const IntPoint& scrollPosition)
@@ -431,16 +438,15 @@
     return o->isOutOfFlowPositioned() && o->style()->position() == FixedPosition;
 }
 
-static void pushBackInRegionScrollable(std::vector<Platform::ScrollViewBase*>& vector, InRegionScrollableArea* scrollableArea, InRegionScrollerPrivate* scroller)
+void InRegionScrollerPrivate::pushBackInRegionScrollable(InRegionScrollableArea* scrollableArea)
 {
-    ASSERT(scroller);
     ASSERT(!scrollableArea->isNull());
 
     scrollableArea->setCanPropagateScrollingToEnclosingScrollable(!isNonRenderViewFixedPositionedContainer(scrollableArea->layer()));
-    vector.push_back(scrollableArea);
-    if (vector.size() == 1) {
+    m_activeInRegionScrollableAreas.push_back(scrollableArea);
+    if (m_activeInRegionScrollableAreas.size() == 1) {
         // FIXME: Use RenderLayer::renderBox()->node() instead?
-        scroller->setNode(enclosingLayerNode(scrollableArea->layer()));
+        setNode(enclosingLayerNode(scrollableArea->layer()));
     }
 }

Modified: trunk/Source/WebKit/blackberry/Api/InRegionScroller_p.h (125677 => 125678)


--- trunk/Source/WebKit/blackberry/Api/InRegionScroller_p.h	2012-08-15 15:56:28 UTC (rev 125677)
+++ trunk/Source/WebKit/blackberry/Api/InRegionScroller_p.h	2012-08-15 16:05:56 UTC (rev 125678)
@@ -35,6 +35,7 @@
 namespace BlackBerry {
 namespace WebKit {
 
+class InRegionScrollableArea;
 class WebPagePrivate;
 
 class InRegionScrollerPrivate {
@@ -53,19 +54,23 @@
     bool setScrollPositionCompositingThread(unsigned camouflagedLayer, const WebCore::IntPoint& scrollPosition);
     bool setScrollPositionWebKitThread(unsigned camouflagedLayer, const WebCore::IntPoint& scrollPosition);
 
-    std::vector<Platform::ScrollViewBase*> inRegionScrollableAreasForPoint(const WebCore::IntPoint&);
+    void calculateInRegionScrollableAreasForPoint(const WebCore::IntPoint&);
+    const std::vector<Platform::ScrollViewBase*>& activeInRegionScrollableAreas() const;
 
     WebPagePrivate* m_webPage;
 
 private:
     bool setLayerScrollPosition(WebCore::RenderLayer*, const WebCore::IntPoint& scrollPosition);
 
+    void pushBackInRegionScrollable(InRegionScrollableArea*);
+
     // Obsolete codepath.
     bool scrollNodeRecursively(WebCore::Node*, const WebCore::IntSize& delta);
     bool scrollRenderer(WebCore::RenderObject*, const WebCore::IntSize& delta);
     void adjustScrollDelta(const WebCore::IntPoint& maxOffset, const WebCore::IntPoint& currentOffset, WebCore::IntSize& delta) const;
 
     RefPtr<WebCore::Node> m_inRegionScrollStartingNode;
+    std::vector<Platform::ScrollViewBase*> m_activeInRegionScrollableAreas;
 };
 
 }

Modified: trunk/Source/WebKit/blackberry/Api/WebPage.cpp (125677 => 125678)


--- trunk/Source/WebKit/blackberry/Api/WebPage.cpp	2012-08-15 15:56:28 UTC (rev 125677)
+++ trunk/Source/WebKit/blackberry/Api/WebPage.cpp	2012-08-15 16:05:56 UTC (rev 125678)
@@ -4177,7 +4177,9 @@
     if (!m_hasInRegionScrollableAreas)
         return;
 
-    m_client->notifyInRegionScrollingStartingPointChanged(m_inRegionScroller->d->inRegionScrollableAreasForPoint(point));
+    m_inRegionScroller->d->calculateInRegionScrollableAreasForPoint(point);
+    if (!m_inRegionScroller->d->activeInRegionScrollableAreas().empty())
+        m_client->notifyInRegionScrollingStartingPointChanged(m_inRegionScroller->d->activeInRegionScrollableAreas());
 }
 
 void WebPage::setScrollOriginPoint(const Platform::IntPoint& point)

Modified: trunk/Source/WebKit/blackberry/Api/WebPageClient.h (125677 => 125678)


--- trunk/Source/WebKit/blackberry/Api/WebPageClient.h	2012-08-15 15:56:28 UTC (rev 125677)
+++ trunk/Source/WebKit/blackberry/Api/WebPageClient.h	2012-08-15 16:05:56 UTC (rev 125678)
@@ -100,7 +100,6 @@
 
     virtual void notifyRunLayoutTestsFinished() = 0;
 
-    // Client is responsible for deleting the vector elements.
     virtual void notifyInRegionScrollingStartingPointChanged(std::vector<Platform::ScrollViewBase*>) = 0;
 
     virtual void notifyDocumentOnLoad() = 0;

Modified: trunk/Source/WebKit/blackberry/ChangeLog (125677 => 125678)


--- trunk/Source/WebKit/blackberry/ChangeLog	2012-08-15 15:56:28 UTC (rev 125677)
+++ trunk/Source/WebKit/blackberry/ChangeLog	2012-08-15 16:05:56 UTC (rev 125678)
@@ -1,3 +1,48 @@
+2012-08-14  Antonio Gomes  <[email protected]>
+
+        [BlackBerry] Robust-fy the LayerWebKitThread ownership with InRegionScroller
+        https://bugs.webkit.org/show_bug.cgi?id=93983
+        PR #191737
+
+        Reviewed by Yong Li.
+
+        Patch changes the way we currently keep track of the active scrollable area
+        objects: before, we acquired the scrollable areas and just passed them in a vector up
+        to the client, copying it over and over again. Also, it was a client responsability to
+        delete stuff (BAD!).
+        Now, we keep track of vector within InRegionScroller, as a class member, which allows us to
+        avoid copies (in follow up patch), and control until when these objects outlive.
+
+        Patch also changes InRegionScrollableArea to "retptr" the composited layer
+        associated to it (if any). This ensure we have a non-null scrollable element always.
+
+        As mentioned, InRegionScroller is now responsible for deleting and vector of scrollable areas.
+
+        Internally reviewed by Arvid Nilsson.
+
+        * Api/InRegionScroller.cpp:
+        (WebKit):
+        (BlackBerry::WebKit::InRegionScrollerPrivate::reset): Method is now responsible for
+        deleting the tracked scrollable areas.
+        (BlackBerry::WebKit::InRegionScrollerPrivate::calculateInRegionScrollableAreasForPoint):
+        Renamed from 'inRegionScrollableAreasForPoint'. It was changed in order to store the
+        scrollable area objects instead of just pass a copy of them up to the client.
+        (BlackBerry::WebKit::InRegionScrollerPrivate::activeInRegionScrollableAreas): Getter.
+        (BlackBerry::WebKit::InRegionScrollerPrivate::pushBackInRegionScrollable): It was
+        promoted to a class method instead of a local helper.
+        * Api/InRegionScroller_p.h:
+        (WebKit):
+        (InRegionScrollerPrivate):
+        * Api/WebPage.cpp:
+        (BlackBerry::WebKit::WebPagePrivate::setScrollOriginPoint): Adjustments needed due to the
+        above changed.
+        * WebKitSupport/InRegionScrollableArea.cpp:
+        (BlackBerry::WebKit::InRegionScrollableArea::~InRegionScrollableArea): Clear up the cached layer.
+        (WebKit):
+        (BlackBerry::WebKit::InRegionScrollableArea::InRegionScrollableArea):
+        * WebKitSupport/InRegionScrollableArea.h:
+        (InRegionScrollableArea):
+
 2012-08-15  Nima Ghanavatian  <[email protected]>
 
         [BlackBerry] Check for valid field focus before processing a spellcheck request

Modified: trunk/Source/WebKit/blackberry/WebKitSupport/InRegionScrollableArea.cpp (125677 => 125678)


--- trunk/Source/WebKit/blackberry/WebKitSupport/InRegionScrollableArea.cpp	2012-08-15 15:56:28 UTC (rev 125677)
+++ trunk/Source/WebKit/blackberry/WebKitSupport/InRegionScrollableArea.cpp	2012-08-15 16:05:56 UTC (rev 125678)
@@ -40,6 +40,10 @@
 {
 }
 
+InRegionScrollableArea::~InRegionScrollableArea()
+{
+}
+
 InRegionScrollableArea::InRegionScrollableArea(WebPagePrivate* webPage, RenderLayer* layer)
     : m_webPage(webPage)
     , m_layer(layer)
@@ -89,7 +93,8 @@
         if (m_layer->usesCompositedScrolling()) {
             m_supportsCompositedScrolling = true;
             ASSERT(m_layer->backing()->hasScrollingLayer());
-            m_cachedCompositedScrollableLayer = reinterpret_cast<unsigned>(m_layer->backing()->scrollingLayer()->platformLayer());
+            m_camouflagedCompositedScrollableLayer = reinterpret_cast<unsigned>(m_layer->backing()->scrollingLayer()->platformLayer());
+            m_cachedCompositedScrollableLayer = m_layer->backing()->scrollingLayer()->platformLayer();
         }
 
         m_overscrollLimitFactor = 0.0;

Modified: trunk/Source/WebKit/blackberry/WebKitSupport/InRegionScrollableArea.h (125677 => 125678)


--- trunk/Source/WebKit/blackberry/WebKitSupport/InRegionScrollableArea.h	2012-08-15 15:56:28 UTC (rev 125677)
+++ trunk/Source/WebKit/blackberry/WebKitSupport/InRegionScrollableArea.h	2012-08-15 16:05:56 UTC (rev 125678)
@@ -24,6 +24,7 @@
 #include <interaction/ScrollViewBase.h>
 
 namespace WebCore {
+class LayerWebKitThread;
 class RenderLayer;
 }
 
@@ -37,6 +38,7 @@
 
     InRegionScrollableArea();
     InRegionScrollableArea(WebPagePrivate*, WebCore::RenderLayer*);
+    virtual ~InRegionScrollableArea();
 
     void setVisibleWindowRect(const WebCore::IntRect&);
     Platform::IntRect visibleWindowRect() const;
@@ -46,6 +48,9 @@
 private:
     WebPagePrivate* m_webPage;
     WebCore::RenderLayer* m_layer;
+
+    RefPtr<WebCore::LayerWebKitThread> m_cachedCompositedScrollableLayer;
+
     bool m_hasWindowVisibleRectCalculated;
 };
 

_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to