Use suggested filename for "Save Link As"

chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc

Index: chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc
diff --git a/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc b/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc
index ef42c55e5c1d9c054a3420b72d1123bc6451f35d..73b54ffb92dea0f65ec133562ad7ee719fe072b5 100644
--- a/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc
+++ b/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc
@@ -6,13 +6,15 @@
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop.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 "content/public/browser/notification_service.h"
+#include "content/public/test/test_utils.h"
 
 ContextMenuNotificationObserver::ContextMenuNotificationObserver(
     int command_to_execute)
-    : command_to_execute_(command_to_execute) {
+    : command_to_execute_(command_to_execute), menu_visible_(false) {
   registrar_.Add(this,
                  chrome::NOTIFICATION_RENDER_VIEW_CONTEXT_MENU_SHOWN,
                  content::NotificationService::AllSources());
@@ -27,12 +29,13 @@ void ContextMenuNotificationObserver::Observe(
     const content::NotificationDetails& details) {
   switch (type) {
     case chrome::NOTIFICATION_RENDER_VIEW_CONTEXT_MENU_SHOWN: {
-      RenderViewContextMenu* context_menu =
-          content::Source<RenderViewContextMenu>(source).ptr();
+      menu_visible_ = true;
+      context_menu_ = content::Source<RenderViewContextMenu>(source).ptr();

[lazyboy, 2014/06/24 01:15:04]

Instead of saving the raw pointer of RVContextMenu, just copy
context_menu_->params() to a member.
That way you don't have to worry whether context_menu_ is valid ptr anymore or
not (Once you show another context menu, this would become invalid iiuc).

[Nikhil, 2014/06/24 10:39:51]

Done.

       base::MessageLoop::current()->PostTask(
           FROM_HERE,
           base::Bind(&ContextMenuNotificationObserver::ExecuteCommand,
-                     base::Unretained(this), context_menu));
+                     base::Unretained(this),
+                     context_menu_));
       break;
     }
 
@@ -41,8 +44,34 @@ void ContextMenuNotificationObserver::Observe(
   }
 }
 
+void ContextMenuNotificationObserver::WaitForMenu() {
+  content::WindowedNotificationObserver menu_observer(
+      chrome::NOTIFICATION_RENDER_VIEW_CONTEXT_MENU_SHOWN,
+      content::NotificationService::AllSources());
+  if (!menu_visible_)
+    menu_observer.Wait();
+  menu_visible_ = false;
+}
+
+base::string16 ContextMenuNotificationObserver::GetSuggestedFilename() {
+  return context_menu_->params().suggested_filename;
+}
+
 void ContextMenuNotificationObserver::ExecuteCommand(
     RenderViewContextMenu* context_menu) {
-  context_menu->ExecuteCommand(command_to_execute_, 0);
-  context_menu->Cancel();
+  switch (command_to_execute_) {
+    case IDC_CONTENT_CONTEXT_OPENLINKNEWTAB: {

[lazyboy, 2014/06/24 01:15:04]

The class is named more generic as ContextMenUNotificationObserver but is doing
very special focused things for NEWTAB and SAVELINKAS.
I'd extract the SaveLinkAs to separate class, possibly extending from
ContextMenuNOtificationObserver, to sth like SaveLinkAsContextMenuObserver or
DownloadContextMenuObserver. Then we can have our own virtual ExecuteCommand
implementation there.
Otherwise this class does not remain generic enough to be used by any test that
requires observer context menu.

[Nikhil, 2014/06/24 10:39:51]

Done.

+      context_menu->ExecuteCommand(command_to_execute_, 0);
+      context_menu->Cancel();
+      break;
+    }
+    case IDC_CONTENT_CONTEXT_SAVELINKAS: {
+      // Don't start download.
+      context_menu->Cancel();
+      break;
+    }
+
+    default:
+      NOTREACHED();
+  }
 }