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/macros.h"
 #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);
 }
 
+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_)
+        owner_->OnRequestThumbnailForContextNodeACK();
+    }
+
+    void Disown() { owner_ = nullptr; }
+
+   private:
+    ~MessageFilter() override {}
+
+    ThumbnailResponseWatcher* owner_;
+
+    DISALLOW_COPY_AND_ASSIGN(MessageFilter);
+  };
+
+  explicit ThumbnailResponseWatcher(
+      content::RenderProcessHost* render_process_host)
+      : message_loop_runner_(new content::MessageLoopRunner),
+        filter_(new MessageFilter(this)),
+        quit_reason_(STILL_RUNNING) {
+    notification_registrar_.Add(
+        this, content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
+        content::Source<content::RenderProcessHost>(render_process_host));
+    notification_registrar_.Add(
+        this, content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
+        content::Source<content::RenderProcessHost>(render_process_host));
+    render_process_host->AddFilter(filter_.get());
+  }
+
+  ~ThumbnailResponseWatcher() override { filter_->Disown(); }
+
+  QuitReason Wait() WARN_UNUSED_RESULT {
+    message_loop_runner_->Run();
+    DCHECK_NE(quit_reason_, STILL_RUNNING);

[Peter Kasting, 2014/12/12 00:35:51]

Nit: (expected, actual)

[jbroman, 2014/12/12 01:10:46]

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_;
+
+  DISALLOW_COPY_AND_ASSIGN(ThumbnailResponseWatcher);
+};
+
+// Maintains image search test state. In particular, note that |menu_observer_|
+// must live until the right-click completes asynchronously.
+class ImageSearchTestHelper {
+ public:

[Peter Kasting, 2014/12/12 00:35:51]

Nit: I think all these can be protected

[jbroman, 2014/12/12 01:10:46]

If this actually were a fixture class (which raises the questions "should this
have a separate fixture class? should it be in a separate file? what should that
file be called?"), then these could be protected.

At the moment, it isn't, so the test class needs these members to be public.

[Peter Kasting, 2014/12/12 01:14:45]

Oh duh, somehow I failed to process your comments about that last time.

Honestly I think this should probably be a fixture class.  You can leave it
declared/defined right here.  That basically means we'd move the stuff from
SetupAndLoadImagePage() to SetUp(), and we wouldn't need to pass in the args to
the constructor because that class could call the relevant accessors directly.

It doesn't seem like there would be much difference beyond those things, so
AFAICT, using a fixture class would mostly just mean a slight reduction in the
amount of code here.

[jbroman, 2014/12/12 01:58:24]

Done. It still needs the filename argument (and you can't pass arguments to
SetUp AFAIK), but done nonetheless.

+  static const char kImageSearchURL[];
+
+  ImageSearchTestHelper(net::SpawnedTestServer* test_server, Browser* browser)
+      : menu_observer_(IDC_CONTENT_CONTEXT_SEARCHWEBFORIMAGE),
+        test_server_(test_server),
+        browser_(browser) {}
+
+  void SetupAndLoadImagePage(const std::string& image_path) {
+    // The test server must start first, so that we know the port that the test
+    // server is using.
+    ASSERT_TRUE(test_server_->Start());
+    SetupImageSearchEngine();
+
+    // Go to a page with an image in it. The test server doesn't serve the image
+    // with the right MIME type, so use a data URL to make a page containing it.
+    GURL image_url(test_server_->GetURL(image_path));

[Peter Kasting, 2014/12/12 00:35:51]

Nit: I would probably inline this into the next statement, but up to you.

[jbroman, 2014/12/12 01:10:46]

still doesn't fit on one line, though, so I mildly prefer to give the temporary
a name.

+    GURL page("data:text/html,<img src='" + image_url.spec() + "'>");
+    ui_test_utils::NavigateToURL(browser_, page);
+  }
+
+  void AttemptImageSearch() {
+    // Right-click where the image should be.
+    // |menu_observer_| will cause the search-by-image menu item to be clicked.
+    content::WebContents* tab =
+        browser_->tab_strip_model()->GetActiveWebContents();
+    content::SimulateMouseClickAt(tab, 0, blink::WebMouseEvent::ButtonRight,
+                                  gfx::Point(15, 15));
+  }
+
+ private:
+  void SetupImageSearchEngine() {
+    static const char kShortName[] = "test";
+    static const char kSearchURL[] = "search?q={searchTerms}";
+    static const char kImageSearchPostParams[] =
+        "thumb={google:imageThumbnail}";
+
+    TemplateURLService* model =
+        TemplateURLServiceFactory::GetForProfile(browser_->profile());
+    ASSERT_TRUE(model);
+    ui_test_utils::WaitForTemplateURLServiceToLoad(model);
+    ASSERT_TRUE(model->loaded());
+
+    TemplateURLData data;
+    data.short_name = base::ASCIIToUTF16(kShortName);
+    data.SetKeyword(data.short_name);
+    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);
+  }
+
+  ContextMenuNotificationObserver menu_observer_;
+  net::SpawnedTestServer* test_server_;
+  Browser* browser_;
+
+  DISALLOW_COPY_AND_ASSIGN(ImageSearchTestHelper);
+};
+
+const char ImageSearchTestHelper::kImageSearchURL[] = "imagesearch";
+
+IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest, ImageSearchWithValidImage) {
+  static const char kValidImage[] = "files/image_search/valid.png";
+  ImageSearchTestHelper helper(test_server(), browser());
+  helper.SetupAndLoadImagePage(kValidImage);
+
+  ui_test_utils::WindowedTabAddedNotificationObserver tab_observer(
+      content::NotificationService::AllSources());
+  helper.AttemptImageSearch();
+
+  // 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(ImageSearchTestHelper::kImageSearchURL),
+            new_tab->GetURL());
+}
+
+IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest, ImageSearchWithCorruptImage) {
+  static const char kCorruptImage[] = "files/image_search/corrupt.png";
+  ImageSearchTestHelper helper(test_server(), browser());
+  helper.SetupAndLoadImagePage(kCorruptImage);
+
+  content::WebContents* tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ThumbnailResponseWatcher watcher(tab->GetRenderProcessHost());
+  helper.AttemptImageSearch();
+
+  // The browser should receive a response from the renderer, because the
+  // renderer should not crash.
+  EXPECT_EQ(ThumbnailResponseWatcher::THUMBNAIL_RECEIVED, watcher.Wait());
+}
+
 }  // namespace