Fix the position of context menu for BrowserPlugins inside OOPIF

chrome/browser/site_per_process_interactive_browsertest.cc

Index: chrome/browser/site_per_process_interactive_browsertest.cc
diff --git a/chrome/browser/site_per_process_interactive_browsertest.cc b/chrome/browser/site_per_process_interactive_browsertest.cc
index de70dde96691548580e283698860815035defd1a..bb1d476bca479feb2d800ffe3db7af7e1d21b2ec 100644
--- a/chrome/browser/site_per_process_interactive_browsertest.cc
+++ b/chrome/browser/site_per_process_interactive_browsertest.cc
@@ -4,6 +4,7 @@
 
 #include "base/command_line.h"
 #include "base/strings/string_number_conversions.h"
+#include "chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/exclusive_access/fullscreen_controller_test.h"
@@ -11,13 +12,19 @@
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/interactive_test_utils.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "components/guest_view/browser/guest_view_manager_delegate.h"
+#include "components/guest_view/browser/test_guest_view_manager.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/render_widget_host.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
+#include "extensions/browser/api/extensions_api_client.h"
+#include "extensions/common/constants.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "ui/display/display.h"
@@ -771,3 +778,131 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessInteractiveBrowserTest,
                             "removeChild(document.querySelector('iframe'))"));
   EXPECT_FALSE(main_frame->GetView()->IsMouseLocked());
 }
+
+// Base test class for interactive tests which load and test PDF files.
+class SitePerProcessInteractivePDFTest
+    : public SitePerProcessInteractiveBrowserTest {
+ public:
+  SitePerProcessInteractivePDFTest() : test_guest_view_manager_(nullptr) {}
+  ~SitePerProcessInteractivePDFTest() override {}
+
+  void SetUpOnMainThread() override {
+    SitePerProcessInteractiveBrowserTest::SetUpOnMainThread();
+    guest_view::GuestViewManager::set_factory_for_testing(&factory_);
+    test_guest_view_manager_ = static_cast<guest_view::TestGuestViewManager*>(
+        guest_view::GuestViewManager::CreateWithDelegate(
+            browser()->profile(),
+            extensions::ExtensionsAPIClient::Get()
+                ->CreateGuestViewManagerDelegate(browser()->profile())));
+  }
+
+ protected:
+  guest_view::TestGuestViewManager* test_guest_view_manager() const {
+    return test_guest_view_manager_;
+  }
+
+ private:
+  guest_view::TestGuestViewManagerFactory factory_;
+  guest_view::TestGuestViewManager* test_guest_view_manager_;
+
+  DISALLOW_COPY_AND_ASSIGN(SitePerProcessInteractivePDFTest);
+};
+
+// This class observes a WebContents for a navigation to an extension scheme to
+// start.

[Charlie Reis, 2017/01/25 21:27:02]

s/start/finish/

[EhsanK, 2017/01/26 15:47:03]

Thanks! Done.

+class NavigationToExtensionSchemeObserver
+    : public content::WebContentsObserver {
+ public:
+  explicit NavigationToExtensionSchemeObserver(content::WebContents* contents)
+      : content::WebContentsObserver(contents),
+        extension_loaded_(contents->GetLastCommittedURL().SchemeIs(
+            extensions::kExtensionScheme)) {}
+
+  void Wait() {
+    if (extension_loaded_)
+      return;
+    message_loop_runner_ = new content::MessageLoopRunner();
+    message_loop_runner_->Run();
+  }
+
+ private:
+  void DidFinishNavigation(content::NavigationHandle* handle) override {
+    if (!handle->GetURL().SchemeIs(extensions::kExtensionScheme))

[Charlie Reis, 2017/01/25 21:27:02]

Let's also return if !handle->HasCommitted() or
navigation_handle->IsErrorPage(), just to make the test more robust.

[EhsanK, 2017/01/26 15:47:03]

Thanks! Done.

+      return;
+    extension_loaded_ = true;
+    message_loop_runner_->Quit();
+  }
+
+  bool extension_loaded_;
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(NavigationToExtensionSchemeObserver);
+};
+
+// This test loads a PDF inside an OOPIF and then verifies that context menu
+// shows up at the correct position.
+IN_PROC_BROWSER_TEST_F(SitePerProcessInteractivePDFTest,
+                       ContextMenuPositionForEmbeddedPDFInCrossOriginFrame) {
+  // Navigate to a page with an <iframe>.
+  GURL main_url(embedded_test_server()->GetURL("a.com", "/iframe.html"));
+  ui_test_utils::NavigateToURL(browser(), main_url);
+
+  // Initially, no guests are created.
+  EXPECT_EQ(0U, test_guest_view_manager()->num_guests_created());
+
+  // Navigate subframe to a cross-site page with an embedded PDF.
+  content::WebContents* active_web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  GURL frame_url =
+      embedded_test_server()->GetURL("b.com", "/page_with_embedded_pdf.html");
+
+  // Ensure the page finishes loading without crashing.
+  EXPECT_TRUE(NavigateIframeToURL(active_web_contents, "test", frame_url));
+
+  // Wait until the guest contents for PDF is created.
+  content::WebContents* guest_contents =
+      test_guest_view_manager()->WaitForSingleGuestCreated();
+
+  // Observe navigations in guest to find out when navigation to the (PDF)
+  // extension starts. It will be used as an indicator that BrowserPlugin

[Charlie Reis, 2017/01/25 21:27:02]

s/starts/commits/

[EhsanK, 2017/01/26 15:47:03]

Thanks! Done.

+  // has attached.
+  NavigationToExtensionSchemeObserver navigation_observer(guest_contents);
+
+  // Change the position of the <iframe> inside the page.
+  EXPECT_TRUE(ExecuteScript(active_web_contents,
+                            "document.querySelector('iframe').style ="
+                            " 'margin-left: 100px; margin-top: 100px;';"));
+
+  // Send a right click to the embedder frame and observe the context menu.

[Charlie Reis, 2017/01/25 21:27:02]

nit: Phrase this as declaring a lambda for it, since it sounds like we're
actually doing that here, but it doesn't happen until later.

Or even better, move this declaration down just below the uses, since there's no
need to declare it before doing the navigation_observer.Wait().  (It's a bit
confusing to have it up here.)

[EhsanK, 2017/01/26 15:47:03]

Thanks!
Done.
Done.

+  auto send_right_mouse_event = [](content::RenderWidgetHost* host, int x,
+                                   int y, blink::WebInputEvent::Type type) {
+    blink::WebMouseEvent event;
+    event.x = x;
+    event.y = y;
+    event.button = blink::WebMouseEvent::Button::Right;
+    event.setType(type);
+    host->ForwardMouseEvent(event);
+  };
+
+  // Before sending the mouse clicks, we need to make sure the BrowserPlugin has
+  // attached, which happens before navigating the guest to the PDF extension.
+  // When attached, the window rects are updated and the context menu position
+  // can be properly calculated.
+  navigation_observer.Wait();

[Charlie Reis, 2017/01/25 21:27:02]

Any reason this needs to be after the ExecuteScript on line 872?  I would expect
them to be somewhat unrelated, and that it might be clearer to put this first
(to have fewer things happening at the same time).

[EhsanK, 2017/01/26 15:47:03]

Good point and good catch actually. I tried moving navigation wait before the
script (which is changing the position of the frame). That made the test flake
since the frame was sometimes confused about its position (was assuming the
original position at (0, 0)). The way it was before was giving the frame the
chance to update its coordinates in time.

That being said, I think the right order of things to happen is to 
1) Navigate main page
2) change the position of the frame
3) navigate the frame
4) wait for extension navigation to end
5) test context menu.

This is the way the test is written now. WDYT?

[Charlie Reis, 2017/01/26 20:50:41]

Yes, I like this new order-- thanks!  (Changing the position between navigating
the frame and waiting for the frame navigation to finish would likely have been
flaky, but doing the position change before the frame navigation sounds right.)

+
+  content::RenderWidgetHostView* child_view =
+      ChildFrameAt(active_web_contents->GetMainFrame(), 0)->GetView();
+
+  ContextMenuWaiter menu_waiter(content::NotificationService::AllSources());
+  send_right_mouse_event(child_view->GetRenderWidgetHost(), 10, 20,
+                         blink::WebInputEvent::MouseDown);
+  send_right_mouse_event(child_view->GetRenderWidgetHost(), 10, 20,
+                         blink::WebInputEvent::MouseUp);
+  menu_waiter.WaitForMenuOpenAndClose();
+
+  gfx::Point point_in_root_window =
+      child_view->TransformPointToRootCoordSpace(gfx::Point(10, 20));
+
+  EXPECT_EQ(point_in_root_window.x(), menu_waiter.params().x);
+  EXPECT_EQ(point_in_root_window.y(), menu_waiter.params().y);
+}