Make page popups work under OOPIF

third_party/WebKit/Source/web/ChromeClientImpl.cpp

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies).
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
@@ skipping 489 matching lines @@
     if (WebLocalFrameImpl::fromFrame(frame) && WebLocalFrameImpl::fromFrame(frame)->frameWidget()) {
         WebLocalFrameImpl::fromFrame(frame)->frameWidget()->scheduleAnimation();
     } else {
         // TODO(lfg): We need to keep this for now because we still have some
         // WebViews who don't have a WebViewFrameWidget. This should be
         // removed once the WebViewFrameWidget refactor is complete.
         m_webView->scheduleAnimation();
     }
 }
 
-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;
         screenRect.y += windowRect.y;
     }
     return screenRect;
 }
 
 float ChromeClientImpl::windowToViewportScalar(const float scalarValue) const
 {
@@ skipping 533 matching lines @@
     if (m_webView->pageImportanceSignals())
         m_webView->pageImportanceSignals()->setIssuedNonGetFetchFromScript();
 }
 
 PassOwnPtr<WebFrameScheduler> ChromeClientImpl::createFrameScheduler()
 {
     return m_webView->scheduler()->createFrameScheduler().release();
 }
 
 } // namespace blink