Fix the position of context menu for BrowserPlugins inside OOPIF

chrome/browser/site_per_process_interactive_browsertest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #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"
 #include "ui/display/screen.h"
 #include "url/gurl.h"
 
 class SitePerProcessInteractiveBrowserTest : public InProcessBrowserTest {
  public:
   SitePerProcessInteractiveBrowserTest() {}
   ~SitePerProcessInteractiveBrowserTest() override {}
@@ skipping 733 matching lines @@
                                           "document.pointerLockElement)",
                                           &mouse_locked));
   EXPECT_TRUE(mouse_locked);
   EXPECT_TRUE(main_frame->GetView()->IsMouseLocked());
 
   EXPECT_TRUE(ExecuteScript(main_frame,
                             "document.querySelector('iframe').parentNode."
                             "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 observers a WebContents for a navigation to an extension scheme to

[Charlie Reis, 2016/12/13 19:20:49]

nit: observes

[EhsanK, 2016/12/13 20:32:00]

Done.

+// start.
+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 DidStartNavigation(content::NavigationHandle* handle) override {

[EhsanK, 2016/12/13 01:36:27]

DidEndNavigation will cause test failures on win_chromium_x64_rel_ng.(seems to
be timing out).

[Charlie Reis, 2016/12/13 19:20:49]

I assume you mean DidFinishNavigation?

Anyway, that's concerning.  It's unusual to only wait for the navigation to
start, and it looks like the wrong thing to do below.  We should figure out why
the navigation never finished on Windows, or whatever is going wrong.

[EhsanK, 2016/12/13 20:32:00]

Yes.
Hmm...apart from the issue of the potential failure in navigation, which I will
dig into more, I still think waiting for navigation start is more correct. Our
intention here is not to navigate to the extension but rather determine the
first point in time after the guest is attached. Attaching, is the first point
in time where we can rely on the position of context menu. Navigation start
happens right after attach.
Yes. Agreed. Unfortunately it does not repro locally and only on one of the bots
so far. I will keep poking at it.

[Charlie Reis, 2016/12/15 23:07:19]

I'm not thrilled with using navigation start as the way to listen for attach,
since those are two semantically different things.  Is there no way to directly
listen for attach?

If not, maybe we can do this, but this code should be much more explicit about
the fact that it's waiting for the guest to attach and not for the extension
page to be ready.  The current name doesn't reflect that, for example.
This would be the better way to go from my perspective, but I suppose I'm ok
with proceeding if the test really doesn't care about whether the extension page
has committed or not.

+    if (!handle->GetURL().SchemeIs(extensions::kExtensionScheme))
+      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 BrowserPluing

[Charlie Reis, 2016/12/13 19:20:49]

nit: BrowserPlugin

[EhsanK, 2016/12/13 20:32:00]

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.
+  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.type = type;
+    host->ForwardMouseEvent(event);
+  };
+
+  // Before sending the mouse clicks, make sure the extension navigation is
+  // complete. This means the BrowserPlugin is attached and the

[Charlie Reis, 2016/12/13 19:20:49]

But it's not complete, given how the observer is written.  The extension page
hasn't committed yet, which means that the context menu actions below could
happen on the initial blank page in the BrowserPlugin, and thus be testing the
wrong thing.

[EhsanK, 2016/12/13 20:32:00]

Sorry. 'complete' is incorrect. And we are probably sending the click to the
partially loaded extension or the empty page. 
Hmm...I don't quite agree with the last point since we are only testing the
context menu position. As long as the BP is there and handles input and knows
its window rect we should be fine. WDYT?

[Charlie Reis, 2016/12/15 23:07:19]

I would be worried about that leading to fragile or flaky tests, if sometimes
the context menu happens on the blank page and others on the extension page.  I
thought it might even fail if the extension commit dismissed the about:blank
context menu, but (surprisingly) that doesn't seem to happen.

Maybe it's ok for now, if we make it very clear that the test can't assume the
extension page has committed.  (I just don't want people to add things to the
test assuming it will happen on the extension page.)

[EhsanK, 2017/01/25 18:48:50]

Coming back to this CL I am finding myself agreeing with your comment more (we
should be using an API which is meaningful).

That being said, this test has issues on Windows 7 and they are probably not
related to either of Finish and Start navigation events. It so happened that the
test was flaking for some other reason and I presumed it has to do with Finish
and Start (since it happened to flake on using finish only but that was by
chance).

I could finally repro this reliably on a VM and noticed that the test fails at
navigating the child frame. I will have to investigate this. Hopefully 1) that
is the only issue with the test, and 2) I can find the resolution soon.

+  // BrowserPluginGuest has updated window rects for the guest.
+  navigation_observer.Wait();
+
+  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);
+}