Image Search: Move thumbnail JPEG encoding to the renderer.

chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc

 // Copyright 2014 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 <string>
 
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/renderer_context_menu/render_view_context_menu.h"
 #include "chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h"
 #include "chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h"
+#include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/render_messages.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "components/search_engines/template_url_data.h"
+#include "components/search_engines/template_url_service.h"
+#include "content/public/browser/browser_message_filter.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_service.h"
+#include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test_utils.h"
+#include "content/public/test/test_utils.h"
 #include "third_party/WebKit/public/web/WebContextMenuData.h"
 #include "third_party/WebKit/public/web/WebInputEvent.h"
 
 using content::WebContents;
 
 namespace {
 
 class ContextMenuBrowserTest : public InProcessBrowserTest {
  public:
   ContextMenuBrowserTest() { }
@@ skipping 246 matching lines @@
   TestRenderViewContextMenu menu(tab->GetMainFrame(), context_menu_params);
   menu.Init();
 
   // The item shouldn't be enabled in the menu.
   EXPECT_FALSE(menu.IsCommandIdEnabled(IDC_CONTENT_CONTEXT_VIEWPAGEINFO));
 
   // Ensure that viewing page info doesn't crash even if you can get to it.
   menu.ExecuteCommand(IDC_CONTENT_CONTEXT_VIEWPAGEINFO, 0);
 }
 
+const char kImageSearchURL[] = "imagesearch";
+
+void SetupImageSearchEngine(Profile* profile,
+                            net::SpawnedTestServer* test_server) {
+  static const char kShortName[] = "test";
+  static const char kKeyword[] = "test";

[Peter Kasting, 2014/12/11 20:11:29]

Nit: You could probably just pick one of these two constants and use it in both
places below (I'd pick kShortName, since having the keyword match the engine
name is common practice)

[jbroman, 2014/12/12 00:08:57]

Done.

+  static const char kSearchURL[] = "search?q={searchTerms}";
+  static const char kImageSearchPostParams[] = "thumb={google:imageThumbnail}";
+
+  TemplateURLService* model = TemplateURLServiceFactory::GetForProfile(profile);
+  ASSERT_TRUE(model);
+  ui_test_utils::WaitForTemplateURLServiceToLoad(model);
+  ASSERT_TRUE(model->loaded());
+
+  TemplateURLData data;
+  data.short_name = base::ASCIIToUTF16(kShortName);
+  data.SetKeyword(base::ASCIIToUTF16(kKeyword));
+  data.SetURL(test_server->GetURL(kSearchURL).spec());
+  data.image_url = test_server->GetURL(kImageSearchURL).spec();
+  data.image_url_post_params = kImageSearchPostParams;
+
+  // The model takes ownership of |template_url|.
+  TemplateURL* template_url = new TemplateURL(data);
+  ASSERT_TRUE(model->Add(template_url));
+  model->SetUserSelectedDefaultSearchProvider(template_url);
+}
+
+void DispatchRightClick(content::WebContents* tab, int x, int y) {

[Peter Kasting, 2014/12/11 20:11:29]

I feel like there has to be some kind of existing testing helper to do this for
us.

[jbroman, 2014/12/12 00:08:57]

Right you are. This file didn't previously use it, so I assumed it didn't exist.
Using content::SimulateMouseClickAt instead.

+  gfx::Point offset = tab->GetContainerBounds().origin();
+  blink::WebMouseEvent mouse_event;
+  mouse_event.type = blink::WebInputEvent::MouseDown;
+  mouse_event.button = blink::WebMouseEvent::ButtonRight;
+  mouse_event.x = x;
+  mouse_event.y = y;
+  mouse_event.globalX = mouse_event.x + offset.x();
+  mouse_event.globalY = mouse_event.y + offset.y();
+  tab->GetRenderViewHost()->ForwardMouseEvent(mouse_event);
+  mouse_event.type = blink::WebInputEvent::MouseUp;
+  tab->GetRenderViewHost()->ForwardMouseEvent(mouse_event);
+}
+
+class ThumbnailResponseWatcher : public content::NotificationObserver {
+ public:
+  enum QuitReason {
+    STILL_RUNNING = 0,
+    THUMBNAIL_RECEIVED,
+    RENDER_PROCESS_GONE,
+  };
+
+  class MessageFilter : public content::BrowserMessageFilter {
+   public:
+    explicit MessageFilter(ThumbnailResponseWatcher* owner)
+        : content::BrowserMessageFilter(ChromeMsgStart), owner_(owner) {}
+
+    bool OnMessageReceived(const IPC::Message& message) override {
+      if (message.type() ==
+          ChromeViewHostMsg_RequestThumbnailForContextNode_ACK::ID) {
+        content::BrowserThread::PostTask(
+            content::BrowserThread::UI, FROM_HERE,
+            base::Bind(&MessageFilter::OnRequestThumbnailForContextNodeACK,
+                       this));
+      }
+      return false;
+    }
+
+    void OnRequestThumbnailForContextNodeACK() {
+      if (owner_) {

[Peter Kasting, 2014/12/11 20:11:29]

Nit: {} unnecessary

[jbroman, 2014/12/12 00:08:57]

Done.

+        owner_->OnRequestThumbnailForContextNodeACK();
+      }
+    }
+
+    void Disown() { owner_ = nullptr; }
+
+   private:
+    ~MessageFilter() override {}
+
+    ThumbnailResponseWatcher* owner_;
+  };

[Peter Kasting, 2014/12/11 20:11:29]

Nit: DISALLOW_COPY_AND_ASSIGN (2 places)

[jbroman, 2014/12/12 00:08:57]

Done.

+
+  explicit ThumbnailResponseWatcher(content::WebContents* web_contents)
+      : message_loop_runner_(new content::MessageLoopRunner),
+        filter_(new MessageFilter(this)),
+        quit_reason_(STILL_RUNNING) {
+    notification_registrar_.Add(this,
+                                content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
+                                content::NotificationService::AllSources());
+    notification_registrar_.Add(
+        this, content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
+        content::NotificationService::AllSources());
+    web_contents->GetRenderProcessHost()->AddFilter(filter_.get());
+  }
+
+  ~ThumbnailResponseWatcher() override { filter_->Disown(); }
+
+  QuitReason Wait() WARN_UNUSED_RESULT {
+    message_loop_runner_->Run();
+    DCHECK(quit_reason_ != STILL_RUNNING);

[Peter Kasting, 2014/12/11 20:11:29]

Nit: DCHECK_NE

[jbroman, 2014/12/12 00:08:57]

Done.

+    return quit_reason_;
+  }
+
+  void Observe(int type,
+               const content::NotificationSource& source,
+               const content::NotificationDetails& details) override {
+    DCHECK(type == content::NOTIFICATION_RENDERER_PROCESS_CLOSED ||
+           type == content::NOTIFICATION_RENDERER_PROCESS_TERMINATED);
+    quit_reason_ = RENDER_PROCESS_GONE;
+    message_loop_runner_->Quit();
+  }
+
+  void OnRequestThumbnailForContextNodeACK() {
+    quit_reason_ = THUMBNAIL_RECEIVED;
+    message_loop_runner_->Quit();
+  }
+
+ private:
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+  scoped_refptr<MessageFilter> filter_;
+  content::NotificationRegistrar notification_registrar_;
+  QuitReason quit_reason_;
+};
+
+IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest, ImageSearchWithValidImage) {
+  ContextMenuNotificationObserver menu_observer(
+      IDC_CONTENT_CONTEXT_SEARCHWEBFORIMAGE);
+  ui_test_utils::WindowedTabAddedNotificationObserver tab_observer(
+      content::NotificationService::AllSources());
+  ASSERT_TRUE(test_server()->Start());
+  SetupImageSearchEngine(browser()->profile(), test_server());
+
+  // Go to a page with an image in it. The test server doesn't serve the image
+  // itself as image/png, so use a data URL to make a page containing it.
+  static const char kValidImage[] = "files/image_search/valid.png";
+  GURL valid_image(test_server()->GetURL(kValidImage));
+  GURL page("data:text/html,<img src='" + valid_image.spec() + "'>");
+  ui_test_utils::NavigateToURL(browser(), page);
+
+  // Open a context menu and click on the "search web for image" option.
+  content::WebContents* tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  DispatchRightClick(tab, 15, 15);
+
+  // The browser should open a new tab for an image search.
+  tab_observer.Wait();
+  content::WebContents* new_tab = tab_observer.GetTab();
+  content::WaitForLoadStop(new_tab);
+  EXPECT_EQ(test_server()->GetURL(kImageSearchURL), new_tab->GetURL());
+}
+
+IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest, ImageSearchWithCorruptImage) {
+  ContextMenuNotificationObserver menu_observer(
+      IDC_CONTENT_CONTEXT_SEARCHWEBFORIMAGE);
+  ASSERT_TRUE(test_server()->Start());
+  SetupImageSearchEngine(browser()->profile(), test_server());
+
+  // Go to a page with an image in it. The test server doesn't serve the image
+  // itself as image/png, so use a data URL to make a page containing it.
+  static const char kCorruptImage[] = "files/image_search/corrupt.png";
+  GURL valid_image(test_server()->GetURL(kCorruptImage));
+  GURL page("data:text/html,<img src='" + valid_image.spec() + "'>");
+  ui_test_utils::NavigateToURL(browser(), page);
+
+  // Open a context menu and click on the "search web for image" option.
+  content::WebContents* tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ThumbnailResponseWatcher watcher(tab);
+  DispatchRightClick(tab, 15, 15);

[Peter Kasting, 2014/12/11 20:11:29]

Nit: Basically all the code above here is shared with the non-corrupt image
testcase.  I suggest making the shared portions a helper function that both
tests call.

[jbroman, 2014/12/12 00:08:57]

I've extracted the common logic into a helper class (arguably this should be a
test fixture class; I'm ambivalent). It doesn't quite work as a simple function
because the ContextMenuNotificationObserver needs to last until the click has
happened (and the condition to wait for varies), and also I need to get at the
RenderProcessHost to watch for renderer crashes in the corrupt case.

So that needs two functions with a persisted ContextMenuNotificationObserver.

+
+  // This test passes if neither the renderer nor the browser crashes.
+  EXPECT_EQ(ThumbnailResponseWatcher::THUMBNAIL_RECEIVED, watcher.Wait());
+}
+
 }  // namespace