Bug 78448

Created attachment 126700 [details] Patch

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.

(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.

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...

(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)

(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.

(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.

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.

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

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.

Created attachment 127895 [details] Patch

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.

(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.

Created attachment 127910 [details] Patch v3

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?

(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?

(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 : )

(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.

Created attachment 128676 [details] Patch v4

Comment on attachment 128676 [details] Patch v4 r+. Please go ahead if frameLayerAbsoluteOffset is properly tested.

Comment on attachment 128676 [details] Patch v4 Sending to the cq ...

The commit-queue encountered the following flaky tests while processing attachment 128676 [details]: css3/filters/effect-invert-hw.html bug 79639 (author: cmarrin@apple.com) inspector/protocol/console-agent.html bug 79563 (authors: caseq@chromium.org, loislo@chromium.org, and pfeldman@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 128676 [details] Patch v4 Clearing flags on attachment: 128676 Committed r108963: <http://trac.webkit.org/changeset/108963>

All reviewed patches have been landed. Closing bug.