Bug 88254 – [BlackBerry] Implement a top-down in-region boundary detection in InRegionScrollableArea

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Antonio Gomes]

From PRZilla:

That way we could guarantee clipping is 100% correct.

think in a scrollable iframe that has a scrollable div that has another
scrollable iframe :) and we want the visible window rect of the inner one.

[Antonio Gomes]

Created attachment 145867 [details]
(committed r119679, r=rbuis) patch

[Rob Buis]

Comment on attachment 145867 [details]
(committed r119679, r=rbuis) patch

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

Looks good overall, some questions still, would also like to see internal review.

> Source/WebKit/blackberry/Api/WebPage.cpp:4657
> +std::vector<Platform::ScrollViewBase*> WebPagePrivate::inRegionScrollableAreasForPoint(const Platform::IntPoint& point)

It seems from looking at WebPage.cpp you do not even need std:: prefix.But this does not have to be fixed in this patch.

> Source/WebKit/blackberry/Api/WebPage.cpp:4683
> +                    pushBackInRegionScrollable(validReturn, new InRegionScrollableArea(this, layer), this);

Who deletes this InRegionScrollableArea?

> Source/WebKit/blackberry/Api/WebPage.cpp:4715
> +        if (layer && layer->renderer()->isRenderView()) { // #document case

I think the RenderView is always the final layer from end to start. So could it be special cased after the for loop?

[Antonio Gomes]

> > Source/WebKit/blackberry/Api/WebPage.cpp:4683
> > +                    pushBackInRegionScrollable(validReturn, new InRegionScrollableArea(this, layer), this);
> 
> Who deletes this InRegionScrollableArea?

The client is deleting it now:

-    virtual void notifyInRegionScrollingStartingPointChanged(std::vector<Platform::ScrollViewBase>) = 0;
+    // Client is responsible for deleting the vector elements.
+    virtual void notifyInRegionScrollingStartingPointChanged(std::vector<Platform::ScrollViewBase*>) = 0;

The internal PR has the libwebview patch that deletes it now.
 
> > Source/WebKit/blackberry/Api/WebPage.cpp:4715
> > +        if (layer && layer->renderer()->isRenderView()) { // #document case
> 
> I think the RenderView is always the final layer from end to start. So could it be special cased after the for loop?

Not actually. See our parentLayer method:

4672 static RenderLayer* parentLayer(RenderLayer* layer)
4673 {
4674     ASSERT(layer);
4675     if (layer->parent())
4676         return layer->parent();
4677 
4678     RenderObject* renderer = layer->renderer();
4679     if (renderer->document() && renderer->document()->ownerElement() && renderer->document()->ownerElement()->renderer())
4680         return renderer->document()->ownerElement()->renderer()->enclosingLayer();
4681 
4682     return 0;
4683 }

in case a iframe/frame (i.e. the renderview case) is found it keeps going up cross the frame tree boundary.

[Jakob Petsovits]

Comment on attachment 145867 [details]
(committed r119679, r=rbuis) patch

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

The meat of the patch looks good to me. Some typo and style suggestions to fix for the final version maybe. Otherwise r+, only that I'm not a reviewer so this review is informal I guess.

>> Source/WebKit/blackberry/Api/WebPage.cpp:4657
>> +std::vector<Platform::ScrollViewBase*> WebPagePrivate::inRegionScrollableAreasForPoint(const Platform::IntPoint& point)
> 
> It seems from looking at WebPage.cpp you do not even need std:: prefix.But this does not have to be fixed in this patch.

I was just thinking, maybe because the InRegionScrollableAreas are now created with "new", maybe the method name should be changed to createInRegionScrollableAreasForPoint(). Makes it clearer that they need to be deleted later on.

> Source/WebKit/blackberry/Api/WebPage.cpp:4704
> +    // Pos-calculate the visible window rects in reverse hit test order so

Typo: Post-calculate
         ^

> Source/WebKit/blackberry/Api/WebPage.cpp:4707
> +            WebCore::IntPoint(), transformedViewportSize());

I was going to comment on indentation width, but if you don't use copy-constructor-like syntax then maybe one line is enough:
WebCore::IntRect recursiveClippingRect(WebCore::IntPoint::zero(), transformedViewportSize());

(Also included the ::zero() constructor for IntPoint, which had been used in InRegionScrollableArea before.)

> Source/WebKit/blackberry/ChangeLog:13
> +       window rect from the outtermost scrollable element towards the inner ones, and

Typo: outermost (only one "t")

> Source/WebKit/blackberry/ChangeLog:18
> +       we can make use of static_cast properly.

Maybe state that platform is now responsible for deleting the ScrollViewBase objects.

[Rob Buis]

Comment on attachment 145867 [details]
(committed r119679, r=rbuis) patch

R+ based on Jakob's review and Antonio's answers to my concerns.