Make page popups work under OOPIF

third_party/WebKit/Source/web/ChromeClientImpl.cpp

Index: third_party/WebKit/Source/web/ChromeClientImpl.cpp
diff --git a/third_party/WebKit/Source/web/ChromeClientImpl.cpp b/third_party/WebKit/Source/web/ChromeClientImpl.cpp
index b099c8b0037714bcd3463ba62248f777e0b2a660..1b699f7646552d34556bb4feccb27593bd33e887 100644
--- a/third_party/WebKit/Source/web/ChromeClientImpl.cpp
+++ b/third_party/WebKit/Source/web/ChromeClientImpl.cpp
@@ -507,11 +507,21 @@ void ChromeClientImpl::scheduleAnimation(Widget* widget)
     }
 }
 
-IntRect ChromeClientImpl::viewportToScreen(const IntRect& rectInViewport) const
+IntRect ChromeClientImpl::viewportToScreen(const IntRect& rectInViewport, const Widget* widget) const

[bokan, 2016/03/18 15:27:36]

It seems like an awkward thing to have to pass in a FrameView to convert from an
unrelated viewport rect to an unrelated screen rect. Is there any way
ChromeClientImpl can figure out the local frame root of the current process on
its own?

[kenrb, 2016/03/18 17:46:10]

The trouble is that there can be many local frame roots in the current process,
such as if a page creates 2 out-of-process iframes that have the same origin as
each other.

[bokan, 2016/03/18 19:25:42]

Ah, I see. ChromeClient still seems like an odd place for this function in the
OOPIF world. Perhaps FrameHost would be a better place for it? Anyway, I suppose
it doesn't matter much right now and you have a better intuition for where it
might/might not belong so I'm fine with this.

 {
     WebRect screenRect(rectInViewport);
 
-    if (m_webView->client()) {
+    ASSERT(widget->isFrameView());

[bokan, 2016/03/18 15:27:36]

Just make the parameter a FrameView*?

[kenrb, 2016/03/18 17:46:10]

I think it's a layering violation for HostWindow.h (in platform) to include a
file from core.

[bokan, 2016/03/18 19:25:42]

Ah, yah, you're right. Though that seems to imply that Widget is too broad but
changing that would be a  larger refactoring...

+    const FrameView* view = toFrameView(widget);
+    LocalFrame* frame = view->frame().localFrameRoot();
+
+    if (WebLocalFrameImpl::fromFrame(frame) && WebLocalFrameImpl::fromFrame(frame)->frameWidget()) {

[bokan, 2016/03/18 15:27:36]

Seems like the only difference in these branches is who's client you're using.
You could make this more clear by conditionally assigning into a
WebWidgetClient* and then the rest of the code can be outside the conditional
block.

[kenrb, 2016/03/18 17:46:10]

Done.

+        WebFrameWidgetImpl* frameWidget = toWebFrameWidgetImpl(WebLocalFrameImpl::fromFrame(frame)->frameWidget());
+        frameWidget->client()->convertViewportToWindow(&screenRect);
+        WebRect windowRect = frameWidget->client()->windowRect();
+        screenRect.x += windowRect.x;
+        screenRect.y += windowRect.y;
+    } else if (m_webView->client()) {
         m_webView->client()->convertViewportToWindow(&screenRect);
         WebRect windowRect = m_webView->client()->windowRect();
         screenRect.x += windowRect.x;