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..ca61ac12d2380fd0a16e8b57e372b8fbb9a87105 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,7 @@
 
 #include <string>
 
+#include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
@@ -12,16 +13,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 +297,188 @@ 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));
+      }
+      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