Image Search: Move thumbnail JPEG encoding to the renderer.

chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc

Index: chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
diff --git a/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc b/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
index 518ea4ba48e4a91090a43bac842b5d820cfc4052..26a7ce8fe9b1abca57df2968a54ffc82fa14dffe 100644
--- a/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
+++ b/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
@@ -4,6 +4,8 @@
 
 #include <string>
 
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
@@ -12,16 +14,24 @@
 #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"
 
@@ -288,4 +298,183 @@ IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest, ViewPageInfoWithNoEntry) {
   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));

[Nico, 2014/12/12 18:43:32]

Do you want to return true in this branch?

[jbroman, 2014/12/12 18:47:03]

IIUC, this will block it from being received by CoreTabHelper. I don't have any
particular reason to want to treat it as "handled", and in fact if CoreTabHelper
/were/ to crash upon receiving the message, that would be a good crash to detect
(though it's not what this test is looking for).

I'll add a "return true" if you think it's helpful, but I don't think it's
necessary.

[Nico, 2014/12/12 18:49:06]

Ok, sounds good. Maybe add an "// Not returning true because…" comment, since
this looks like it could be an omission to a clueless reader like me. (or keep
as is if you think a comment is overkill)

+      }
+      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(STILL_RUNNING, quit_reason_);
+    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 SearchByImageBrowserTest : public InProcessBrowserTest {
+ protected:
+  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));
+    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.
+    menu_observer_.reset(new ContextMenuNotificationObserver(
+        IDC_CONTENT_CONTEXT_SEARCHWEBFORIMAGE));
+    content::WebContents* tab =
+        browser()->tab_strip_model()->GetActiveWebContents();
+    content::SimulateMouseClickAt(tab, 0, blink::WebMouseEvent::ButtonRight,
+                                  gfx::Point(15, 15));
+  }
+
+  GURL GetImageSearchURL() {
+    static const char kImageSearchURL[] = "imagesearch";
+    return test_server()->GetURL(kImageSearchURL);
+  }
+
+ private:
+  void SetupImageSearchEngine() {
+    static const char kShortName[] = "test";

[Nico, 2014/12/12 18:43:32]

(note to future self: I was wondering if we usually says static on const
char[]s. "[chromium-dev] Non-static const char arrays?" answers this. With clang
it doesn't make a difference, but gcc produces better code with static. For
details, see that thread)

+    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 = GetImageSearchURL().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 TearDownInProcessBrowserTestFixture() override {
+    menu_observer_.reset();
+  }
+
+  scoped_ptr<ContextMenuNotificationObserver> menu_observer_;
+};
+
+IN_PROC_BROWSER_TEST_F(SearchByImageBrowserTest, ImageSearchWithValidImage) {
+  static const char kValidImage[] = "files/image_search/valid.png";
+  SetupAndLoadImagePage(kValidImage);
+
+  ui_test_utils::WindowedTabAddedNotificationObserver tab_observer(
+      content::NotificationService::AllSources());
+  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(GetImageSearchURL(), new_tab->GetURL());
+}
+
+IN_PROC_BROWSER_TEST_F(SearchByImageBrowserTest, ImageSearchWithCorruptImage) {
+  static const char kCorruptImage[] = "files/image_search/corrupt.png";
+  SetupAndLoadImagePage(kCorruptImage);
+
+  content::WebContents* tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ThumbnailResponseWatcher watcher(tab->GetRenderProcessHost());
+  AttemptImageSearch();
+
+  // The browser should receive a response from the renderer, because the
+  // renderer should not crash.
+  EXPECT_EQ(ThumbnailResponseWatcher::THUMBNAIL_RECEIVED, watcher.Wait());
+}
+
 }  // namespace