Bug 78448 – [BlackBerry] Upstream accelerated compositing helper class

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Leo Yang]

class FrameLayers in the BlackBerry webkit porting is designed to manage attachments of different frames.

[Leo Yang]

Created attachment 126700 [details]
Patch

[Rob Buis]

Comment on attachment 126700 [details]
Patch

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

Looks good.

> Source/WebKit/blackberry/WebKitSupport/FrameLayers.h:39
> +// FIXME: with the exception of the rootLayerWebKitThread() method

Can you improve above, so we have proper sentences with periods at end? I guess you need two separate sentences here.

[Leo Yang]

(In reply to comment #2)
> (From update of attachment 126700 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126700&action=review
> 
> Looks good.
> 
> > Source/WebKit/blackberry/WebKitSupport/FrameLayers.h:39
> > +// FIXME: with the exception of the rootLayerWebKitThread() method
> 
> Can you improve above, so we have proper sentences with periods at end? I guess you need two separate sentences here.

Thank you. I'll fix it before landing.

[Antonio Gomes]

Comment on attachment 126700 [details]
Patch

could you check this class is really needed, still?

I changed it so that both iframe and frame take the webcore code path. Could you please confirm before landing this. It should not be needed anymore...

[Leo Yang]

(In reply to comment #4)
> (From update of attachment 126700 [details])
> could you check this class is really needed, still?
> 
> I changed it so that both iframe and frame take the webcore code path. Could you please confirm before landing this. It should not be needed anymore...

It's still being used in void WebPagePrivate::setRootLayerWebKitThread(Frame* frame, LayerWebKitThread* layer)

[Antonio Gomes]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 126700 [details] [details])
> > could you check this class is really needed, still?
> > 
> > I changed it so that both iframe and frame take the webcore code path. Could you please confirm before landing this. It should not be needed anymore...
> 
> It's still being used in void WebPagePrivate::setRootLayerWebKitThread(Frame* frame, LayerWebKitThread* layer)

this is ever being called? it should not since both frames and iframe take another codepath.

[Leo Yang]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 126700 [details] [details] [details])
> > > could you check this class is really needed, still?
> > > 
> > > I changed it so that both iframe and frame take the webcore code path. Could you please confirm before landing this. It should not be needed anymore...
> > 
> > It's still being used in void WebPagePrivate::setRootLayerWebKitThread(Frame* frame, LayerWebKitThread* layer)
> 
> this is ever being called? it should not since both frames and iframe take another codepath.

OK, I'm holding it and will check if it's ever be called.

[Leo Yang]

Comment on attachment 126700 [details]
Patch

Obsolete the patch. FrameLayers does be used, but it only contain root layer even if there are AC subframes. This is because upstream take care of the layers for subframes. I'll refactor internally to evaluate if FrameLayers is needed.

[Nima Ghanavatian]

*** Bug 78606 has been marked as a duplicate of this bug. ***

[Leo Yang]

Antonio, after revisit the code. I found that the FrameLayers should still be there. Look at RenderLayerCompositor::shouldPropagateCompositingToEnclosingFrame(), it's possible that this function returns false for top-level frame. Per my understanding, top-level frame isn't always main frame because it may be a top-level frame for a sub tree. Windonws and Chromium porting also maintain something like a tree structure for layers attached via ChromeClient.

[Leo Yang]

Created attachment 127895 [details]
Patch

[Antonio Gomes]

Comment on attachment 127895 [details]
Patch

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

> Source/WebKit/blackberry/WebKitSupport/FrameLayers.cpp:57
> +static FloatSize frameLayerAbsoluteOffset(Frame* frame)
> +{
> +    FloatSize result;
> +    ASSERT(frame);
> +    const ScrollView* scrollView = frame->view();
> +    if (!scrollView)
> +        return result;
> +
> +    while (const ScrollView* parentScrollView = scrollView->parent()) {
> +        if (scrollView->isFrameView()) {
> +            RenderPart* renderer = static_cast<const FrameView*>(scrollView)->frame()->ownerRenderer();
> +            if (!renderer)
> +                return result;
> +            FloatPoint point = FloatPoint(renderer->borderLeft() + renderer->paddingLeft(), renderer->borderTop() + renderer->paddingTop());
> +            point = renderer->localToAbsolute(point);
> +            result += FloatSize(point.x(), point.y());
> +        } else
> +            result += FloatSize(scrollView->x(), scrollView->y());
> +        scrollView = parentScrollView;
> +    }
> +    return result;
> +}

how is this different than ScrollView::windowToContents and contentsToWindow methods?

> Source/WebKit/blackberry/WebKitSupport/FrameLayers.h:57
> +    bool isRootLayerMainFrameLayer() const
> +    {
> +        return m_rootLayer && !m_rootGraphicsLayer;
> +    }

we should be consistent if we want the

xxx { return abc; } in one line

or

xxx
{
    return abc();
}

> Source/WebKit/blackberry/WebKitSupport/FrameLayers.h:64
> +    typedef HashMap<WebCore::Frame*, WebCore::LayerWebKitThread*> FrameLayerMap;

lets move this typedef declaration a few lines down to be closer to where it is used.

[Leo Yang]

(In reply to comment #12)
> (From update of attachment 127895 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127895&action=review
> 
> > Source/WebKit/blackberry/WebKitSupport/FrameLayers.cpp:57
> > +static FloatSize frameLayerAbsoluteOffset(Frame* frame)
> > +{
> > +    FloatSize result;
> > +    ASSERT(frame);
> > +    const ScrollView* scrollView = frame->view();
> > +    if (!scrollView)
> > +        return result;
> > +
> > +    while (const ScrollView* parentScrollView = scrollView->parent()) {
> > +        if (scrollView->isFrameView()) {
> > +            RenderPart* renderer = static_cast<const FrameView*>(scrollView)->frame()->ownerRenderer();
> > +            if (!renderer)
> > +                return result;
> > +            FloatPoint point = FloatPoint(renderer->borderLeft() + renderer->paddingLeft(), renderer->borderTop() + renderer->paddingTop());
> > +            point = renderer->localToAbsolute(point);
> > +            result += FloatSize(point.x(), point.y());
> > +        } else
> > +            result += FloatSize(scrollView->x(), scrollView->y());
> > +        scrollView = parentScrollView;
> > +    }
> > +    return result;
> > +}
> 
> how is this different than ScrollView::windowToContents and contentsToWindow methods?
This is to convert a position of a frame which is in its parent's content coordination to the top frame's content coordination. It will not take scroll position into account.

> 
> > Source/WebKit/blackberry/WebKitSupport/FrameLayers.h:57
> > +    bool isRootLayerMainFrameLayer() const
> > +    {
> > +        return m_rootLayer && !m_rootGraphicsLayer;
> > +    }
> 
> we should be consistent if we want the
> 
> xxx { return abc; } in one line
> 
> or
> 
> xxx
> {
>     return abc();
> }
> 
OK.

> > Source/WebKit/blackberry/WebKitSupport/FrameLayers.h:64
> > +    typedef HashMap<WebCore::Frame*, WebCore::LayerWebKitThread*> FrameLayerMap;
> 
> lets move this typedef declaration a few lines down to be closer to where it is used.
Sure.

[Leo Yang]

Created attachment 127910 [details]
Patch v3

[Antonio Gomes]

Comment on attachment 127910 [details]
Patch v3

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

> Source/WebKit/blackberry/WebKitSupport/FrameLayers.cpp:56
> +    while (const ScrollView* parentScrollView = scrollView->parent()) {
> +        if (scrollView->isFrameView()) {
> +            RenderPart* renderer = static_cast<const FrameView*>(scrollView)->frame()->ownerRenderer();
> +            if (!renderer)
> +                return result;
> +            FloatPoint point = FloatPoint(renderer->borderLeft() + renderer->paddingLeft(), renderer->borderTop() + renderer->paddingTop());
> +            point = renderer->localToAbsolute(point);
> +            result += FloatSize(point.x(), point.y());
> +        } else
> +            result += FloatSize(scrollView->x(), scrollView->y());
> +        scrollView = parentScrollView;
> +    }
> +    return result;

m_webPage->mainFrame()->view()->windowToContents(frame->view()->contentsToWindow(IntPoint::zero()));

something like this?

[Leo Yang]

(In reply to comment #15)
> (From update of attachment 127910 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127910&action=review
> 
> > Source/WebKit/blackberry/WebKitSupport/FrameLayers.cpp:56
> > +    while (const ScrollView* parentScrollView = scrollView->parent()) {
> > +        if (scrollView->isFrameView()) {
> > +            RenderPart* renderer = static_cast<const FrameView*>(scrollView)->frame()->ownerRenderer();
> > +            if (!renderer)
> > +                return result;
> > +            FloatPoint point = FloatPoint(renderer->borderLeft() + renderer->paddingLeft(), renderer->borderTop() + renderer->paddingTop());
> > +            point = renderer->localToAbsolute(point);
> > +            result += FloatSize(point.x(), point.y());
> > +        } else
> > +            result += FloatSize(scrollView->x(), scrollView->y());
> > +        scrollView = parentScrollView;
> > +    }
> > +    return result;
> 
> m_webPage->mainFrame()->view()->windowToContents(frame->view()->contentsToWindow(IntPoint::zero()));
> 
> something like this?
If the frame is not in the tree of the main frame, will the result be different?

[Antonio Gomes]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 127910 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=127910&action=review
> > 
> > > Source/WebKit/blackberry/WebKitSupport/FrameLayers.cpp:56
> > > +    while (const ScrollView* parentScrollView = scrollView->parent()) {
> > > +        if (scrollView->isFrameView()) {
> > > +            RenderPart* renderer = static_cast<const FrameView*>(scrollView)->frame()->ownerRenderer();
> > > +            if (!renderer)
> > > +                return result;
> > > +            FloatPoint point = FloatPoint(renderer->borderLeft() + renderer->paddingLeft(), renderer->borderTop() + renderer->paddingTop());
> > > +            point = renderer->localToAbsolute(point);
> > > +            result += FloatSize(point.x(), point.y());
> > > +        } else
> > > +            result += FloatSize(scrollView->x(), scrollView->y());
> > > +        scrollView = parentScrollView;
> > > +    }
> > > +    return result;
> > 
> > m_webPage->mainFrame()->view()->windowToContents(frame->view()->contentsToWindow(IntPoint::zero()));
> > 
> > something like this?
> If the frame is not in the tree of the main frame, will the result be different?

Not sure if I understood your question. Anyways, you tell me : )

[Leo Yang]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (From update of attachment 127910 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=127910&action=review
> > > 
> > > > Source/WebKit/blackberry/WebKitSupport/FrameLayers.cpp:56
> > > > +    while (const ScrollView* parentScrollView = scrollView->parent()) {
> > > > +        if (scrollView->isFrameView()) {
> > > > +            RenderPart* renderer = static_cast<const FrameView*>(scrollView)->frame()->ownerRenderer();
> > > > +            if (!renderer)
> > > > +                return result;
> > > > +            FloatPoint point = FloatPoint(renderer->borderLeft() + renderer->paddingLeft(), renderer->borderTop() + renderer->paddingTop());
> > > > +            point = renderer->localToAbsolute(point);
> > > > +            result += FloatSize(point.x(), point.y());
> > > > +        } else
> > > > +            result += FloatSize(scrollView->x(), scrollView->y());
> > > > +        scrollView = parentScrollView;
> > > > +    }
> > > > +    return result;
> > > 
> > > m_webPage->mainFrame()->view()->windowToContents(frame->view()->contentsToWindow(IntPoint::zero()));
> > > 
> > > something like this?
> > If the frame is not in the tree of the main frame, will the result be different?
> 
> Not sure if I understood your question. Anyways, you tell me : )

If the frame is not in the subtree of main frame, frameLayerAbsoluteOffset() will return the offset in the top-level frame (not main frame) coordination. But I don't know if m_webPage->mainFrame()->view()->windowToContents(frame->view()->contentsToWindow(IntPoint::zero())) behaves in the same way.

[Leo Yang]

Created attachment 128676 [details]
Patch v4

[Antonio Gomes]

Comment on attachment 128676 [details]
Patch v4

r+. Please go ahead if frameLayerAbsoluteOffset is properly tested.

[Leo Yang]

Comment on attachment 128676 [details]
Patch v4

Sending to the cq ...

[WebKit Review Bot]

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

css3/filters/effect-invert-hw.html bug 79639 (author: [e-mail])
inspector/protocol/console-agent.html bug 79563 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

Comment on attachment 128676 [details]
Patch v4

Clearing flags on attachment: 128676

Committed r108963: <http://trac.webkit.org/changeset/108963>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.