Clean up RenderViewContextMenu a bit.

chrome/browser/renderer_context_menu/render_view_context_menu.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 "chrome/browser/renderer_context_menu/render_view_context_menu.h"
 
 #include <algorithm>
 #include <set>
 #include <utility>
 
@@ skipping 259 matching lines @@
 void EscapeAmpersands(base::string16* text) {
   base::ReplaceChars(*text, base::ASCIIToUTF16("&"), base::ASCIIToUTF16("&&"),
                      text);
 }
 
 // Returns the preference of the profile represented by the |context|.
 PrefService* GetPrefs(content::BrowserContext* context) {
   return user_prefs::UserPrefs::Get(context);
 }
 
-bool custom_id_ranges_initialized = false;
+bool ExtensionPatternMatch(const extensions::URLPatternSet& patterns,
+                           const GURL& url) {
+  // No patterns means no restriction, so that implicitly matches.
+  if (patterns.is_empty())
+    return true;
+  return patterns.MatchesURL(url);
+}
+
+const GURL& GetDocumentURL(const content::ContextMenuParams& params) {
+  return params.frame_url.is_empty() ? params.page_url : params.frame_url;
+}
+
+content::Referrer CreateSaveAsReferrer(
+    const content::ContextMenuParams& params) {
+  const GURL& url = params.link_url;
+  const GURL& referring_url = GetDocumentURL(params);
+  return content::Referrer::SanitizeForRequest(
+      url,
+      content::Referrer(referring_url.GetAsReferrer(), params.referrer_policy));
+}
+
+bool g_custom_id_ranges_initialized = false;

[lazyboy, 2014/08/17 15:23:28]

I think we should keep it as is since this is not a global, rather a file level
var,
oshima@ made this w/o g_ prefix for this reason..

[Lei Zhang, 2014/08/18 22:56:25]

File-level globals are still globals.

[lazyboy, 2014/08/21 18:24:56]

OK.

+
+const int kSpellcheckRadioGroup = 1;
 
 }  // namespace
 
 // static
 bool RenderViewContextMenu::IsDevToolsURL(const GURL& url) {
   return url.SchemeIs(content::kChromeDevToolsScheme);
 }
 
 // static
 bool RenderViewContextMenu::IsInternalResourcesURL(const GURL& url) {
   if (!url.SchemeIs(content::kChromeUIScheme))
     return false;
   return url.host() == chrome::kChromeUISyncResourcesHost;
 }
 
-static const int kSpellcheckRadioGroup = 1;
-
 RenderViewContextMenu::RenderViewContextMenu(
     content::RenderFrameHost* render_frame_host,
     const content::ContextMenuParams& params)
     : RenderViewContextMenuBase(render_frame_host, params),
       extension_items_(browser_context_,
                        this,
                        &menu_model_,
                        base::Bind(MenuItemMatchesParams, params_)),
       protocol_handler_submenu_model_(this),
       protocol_handler_registry_(
           ProtocolHandlerRegistryFactory::GetForBrowserContext(GetProfile())) {
-  if (!custom_id_ranges_initialized) {
-    custom_id_ranges_initialized = true;
+  if (!g_custom_id_ranges_initialized) {
+    g_custom_id_ranges_initialized = true;
     SetContentCustomCommandIdRange(IDC_CONTENT_CONTEXT_CUSTOM_FIRST,
                                    IDC_CONTENT_CONTEXT_CUSTOM_LAST);
   }
   set_content_type(ContextMenuContentTypeFactory::Create(
       source_web_contents_, params));
 }
 
 RenderViewContextMenu::~RenderViewContextMenu() {
 }
 
 // Menu construction functions -------------------------------------------------
 
-static bool ExtensionPatternMatch(const extensions::URLPatternSet& patterns,
-                                  const GURL& url) {
-  // No patterns means no restriction, so that implicitly matches.
-  if (patterns.is_empty())
-    return true;
-  return patterns.MatchesURL(url);
-}
-
 // static
 bool RenderViewContextMenu::ExtensionContextAndPatternMatch(
     const content::ContextMenuParams& params,
-    MenuItem::ContextList contexts,
+    const MenuItem::ContextList& contexts,

[lazyboy, 2014/08/17 15:23:28]

FYI, ContextList provides a copy constructor, this probably was a copy (of a
uint32) for that reason.
I don't know the performance implication of either, can you provide a quick
explanation of this change?

[Lei Zhang, 2014/08/18 22:56:25]

I don't see a reason to make a copy. Also, when we pass by reference, it should
be const.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere...

     const extensions::URLPatternSet& target_url_patterns) {
   const bool has_link = !params.link_url.is_empty();
   const bool has_selection = !params.selection_text.empty();
   const bool in_frame = !params.frame_url.is_empty();
 
   if (contexts.Contains(MenuItem::ALL) ||
       (has_selection && contexts.Contains(MenuItem::SELECTION)) ||
       (params.is_editable && contexts.Contains(MenuItem::EDITABLE)) ||
       (in_frame && contexts.Contains(MenuItem::FRAME)))
     return true;
@@ skipping 29 matching lines @@
   // other contexts apply (except for FRAME, which is included in PAGE for
   // backwards compatibility).
   if (!has_link && !has_selection && !params.is_editable &&
       params.media_type == WebContextMenuData::MediaTypeNone &&
       contexts.Contains(MenuItem::PAGE))
     return true;
 
   return false;
 }
 
-static const GURL& GetDocumentURL(const content::ContextMenuParams& params) {
-  return params.frame_url.is_empty() ? params.page_url : params.frame_url;
-}
-
 // static
 bool RenderViewContextMenu::MenuItemMatchesParams(
     const content::ContextMenuParams& params,
     const extensions::MenuItem* item) {
   bool match = ExtensionContextAndPatternMatch(params, item->contexts(),
                                                item->target_url_patterns());
   if (!match)
     return false;
 
   const GURL& document_url = GetDocumentURL(params);
@@ skipping 430 matching lines @@
 
   base::TrimWhitespace(params_.selection_text, base::TRIM_ALL,
                        &params_.selection_text);
   if (params_.selection_text.empty())
     return;
 
   base::ReplaceChars(params_.selection_text, AutocompleteMatch::kInvalidChars,
                      base::ASCIIToUTF16(" "), &params_.selection_text);
 
   AutocompleteMatch match;
-  AutocompleteClassifierFactory::GetForProfile(GetProfile())
-      ->Classify(params_.selection_text,
-                 false,
-                 false,
-                 metrics::OmniboxEventProto::INVALID_SPEC,
-                 &match,
-                 NULL);
+  AutocompleteClassifierFactory::GetForProfile(GetProfile())->Classify(
+      params_.selection_text,
+      false,
+      false,
+      metrics::OmniboxEventProto::INVALID_SPEC,
+      &match,
+      NULL);
   selection_navigation_url_ = match.destination_url;
   if (!selection_navigation_url_.is_valid())
     return;
 
   base::string16 printable_selection_text = PrintableSelectionText();
   EscapeAmpersands(&printable_selection_text);
 
   if (AutocompleteMatch::IsSearchType(match.type)) {
     const TemplateURL* const default_provider =
         TemplateURLServiceFactory::GetForProfile(GetProfile())
@@ skipping 387 matching lines @@
       NOTREACHED();
       return false;
   }
 }
 
 bool RenderViewContextMenu::IsCommandIdChecked(int id) const {
   if (RenderViewContextMenuBase::IsCommandIdChecked(id))
     return true;
 
   // See if the video is set to looping.
-  if (id == IDC_CONTENT_CONTEXT_LOOP) {
-    return (params_.media_flags &
-            WebContextMenuData::MediaLoop) != 0;
-  }
+  if (id == IDC_CONTENT_CONTEXT_LOOP)
+    return (params_.media_flags & WebContextMenuData::MediaLoop) != 0;
 
-  if (id == IDC_CONTENT_CONTEXT_CONTROLS) {
-    return (params_.media_flags &
-            WebContextMenuData::MediaControls) != 0;
-  }
+  if (id == IDC_CONTENT_CONTEXT_CONTROLS)
+    return (params_.media_flags & WebContextMenuData::MediaControls) != 0;
 
   // Extension items.
   if (ContextMenuMatcher::IsExtensionsCustomCommandId(id))
     return extension_items_.IsCommandIdChecked(id);
 
   return false;
 }
 
 void RenderViewContextMenu::ExecuteCommand(int id, int event_flags) {
   RenderViewContextMenuBase::ExecuteCommand(id, event_flags);
   if (command_executed_)
     return;
   command_executed_ = true;
 
   RenderFrameHost* render_frame_host = GetRenderFrameHost();
 
   // Process extension menu items.
   if (ContextMenuMatcher::IsExtensionsCustomCommandId(id)) {
     extension_items_.ExecuteCommand(id, source_web_contents_, params_);
     return;
   }
 
   if (id >= IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_FIRST &&
       id <= IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_LAST) {
     ProtocolHandlerRegistry::ProtocolHandlerList handlers =
         GetHandlersForLinkUrl();
-    if (handlers.empty()) {
+    if (handlers.empty())
       return;
-    }
+
     content::RecordAction(
         UserMetricsAction("RegisterProtocolHandler.ContextMenu_Open"));
     int handlerIndex = id - IDC_CONTENT_CONTEXT_PROTOCOL_HANDLER_FIRST;
     WindowOpenDisposition disposition =
         ForceNewTabDispositionFromEventFlags(event_flags);
-    OpenURL(
-        handlers[handlerIndex].TranslateUrl(params_.link_url),
-        params_.frame_url.is_empty() ? params_.page_url : params_.frame_url,
-        disposition, content::PAGE_TRANSITION_LINK);
+    OpenURL(handlers[handlerIndex].TranslateUrl(params_.link_url),
+            GetDocumentURL(params_),
+            disposition,
+            content::PAGE_TRANSITION_LINK);
     return;
   }
 
   switch (id) {
     case IDC_CONTENT_CONTEXT_OPENLINKNEWTAB: {
       Browser* browser =
           chrome::FindBrowserWithWebContents(source_web_contents_);
-      OpenURL(
-          params_.link_url,
-          params_.frame_url.is_empty() ? params_.page_url : params_.frame_url,
-          !browser || browser->is_app() ?
+      OpenURL(params_.link_url,
+              GetDocumentURL(params_),
+              !browser || browser->is_app() ?
                   NEW_FOREGROUND_TAB : NEW_BACKGROUND_TAB,
-          content::PAGE_TRANSITION_LINK);
+              content::PAGE_TRANSITION_LINK);
       break;
     }
     case IDC_CONTENT_CONTEXT_OPENLINKNEWWINDOW:
-      OpenURL(
-          params_.link_url,
-          params_.frame_url.is_empty() ? params_.page_url : params_.frame_url,
-          NEW_WINDOW, content::PAGE_TRANSITION_LINK);
+      OpenURL(params_.link_url,
+              GetDocumentURL(params_),
+              NEW_WINDOW,
+              content::PAGE_TRANSITION_LINK);
       break;
 
     case IDC_CONTENT_CONTEXT_OPENLINKOFFTHERECORD:
       OpenURL(params_.link_url, GURL(), OFF_THE_RECORD,
               content::PAGE_TRANSITION_LINK);
       break;
 
     case IDC_CONTENT_CONTEXT_SAVELINKAS: {
       RecordDownloadSource(DOWNLOAD_INITIATED_BY_CONTEXT_MENU);
       const GURL& url = params_.link_url;
-      const GURL& referring_url =
-          params_.frame_url.is_empty() ? params_.page_url : params_.frame_url;
-      content::Referrer referrer = content::Referrer::SanitizeForRequest(
-          url,
-          content::Referrer(referring_url.GetAsReferrer(),
-                            params_.referrer_policy));
+      content::Referrer referrer = CreateSaveAsReferrer(params_);
       DownloadManager* dlm =
           BrowserContext::GetDownloadManager(browser_context_);
       scoped_ptr<DownloadUrlParameters> dl_params(
           DownloadUrlParameters::FromWebContents(source_web_contents_, url));
       dl_params->set_referrer(referrer);
       dl_params->set_referrer_encoding(params_.frame_charset);
       dl_params->set_suggested_name(params_.suggested_filename);
       dl_params->set_prompt(true);
       dlm->DownloadUrl(dl_params.Pass());
       break;
     }
 
     case IDC_CONTENT_CONTEXT_SAVEAVAS:
     case IDC_CONTENT_CONTEXT_SAVEIMAGEAS: {
       if (params_.media_type == WebContextMenuData::MediaTypeCanvas) {
         source_web_contents_->GetRenderViewHost()->SaveImageAt(
           params_.x, params_.y);
       } else {
         // TODO(zino): We can use SaveImageAt() like a case of canvas.
         RecordDownloadSource(DOWNLOAD_INITIATED_BY_CONTEXT_MENU);
-        const GURL& url = params_.src_url;
-        const GURL& referring_url =
-            params_.frame_url.is_empty() ? params_.page_url : params_.frame_url;
-        content::Referrer referrer = content::Referrer::SanitizeForRequest(
-            url,
-            content::Referrer(referring_url.GetAsReferrer(),
-                              params_.referrer_policy));
+        const GURL& url = params_.link_url;
+        content::Referrer referrer = CreateSaveAsReferrer(params_);
         source_web_contents_->SaveFrame(url, referrer);
       }
       break;
     }
 
     case IDC_CONTENT_CONTEXT_COPYLINKLOCATION:
       WriteURLToClipboard(params_.unfiltered_link_url);
       break;
 
     case IDC_CONTENT_CONTEXT_COPYIMAGELOCATION:
     case IDC_CONTENT_CONTEXT_COPYAVLOCATION:
       WriteURLToClipboard(params_.src_url);
       break;
 
     case IDC_CONTENT_CONTEXT_COPYIMAGE:
       CopyImageAt(params_.x, params_.y);
       break;
 
     case IDC_CONTENT_CONTEXT_SEARCHWEBFORIMAGE:
       GetImageThumbnailForSearch();
       break;
 
     case IDC_CONTENT_CONTEXT_OPENIMAGENEWTAB:
     case IDC_CONTENT_CONTEXT_OPENAVNEWTAB:
-      OpenURL(
-          params_.src_url,
-          params_.frame_url.is_empty() ? params_.page_url : params_.frame_url,
-          NEW_BACKGROUND_TAB, content::PAGE_TRANSITION_LINK);
+      OpenURL(params_.src_url,
+              GetDocumentURL(params_),
+              NEW_BACKGROUND_TAB,
+              content::PAGE_TRANSITION_LINK);
       break;
 
     case IDC_CONTENT_CONTEXT_PLAYPAUSE: {
       bool play = !!(params_.media_flags & WebContextMenuData::MediaPaused);
       if (play) {
         content::RecordAction(UserMetricsAction("MediaContextMenu_Play"));
       } else {
         content::RecordAction(UserMetricsAction("MediaContextMenu_Pause"));
       }
       MediaPlayerActionAt(gfx::Point(params_.x, params_.y),
@@ skipping 30 matching lines @@
           WebMediaPlayerAction(
               WebMediaPlayerAction::Controls,
               !IsCommandIdChecked(IDC_CONTENT_CONTEXT_CONTROLS)));
       break;
 
     case IDC_CONTENT_CONTEXT_ROTATECW:
       content::RecordAction(
       UserMetricsAction("PluginContextMenu_RotateClockwise"));
       PluginActionAt(
           gfx::Point(params_.x, params_.y),
-          WebPluginAction(
-              WebPluginAction::Rotate90Clockwise,
-              true));
+          WebPluginAction(WebPluginAction::Rotate90Clockwise, true));
       break;
 
     case IDC_CONTENT_CONTEXT_ROTATECCW:
       content::RecordAction(
       UserMetricsAction("PluginContextMenu_RotateCounterclockwise"));
       PluginActionAt(
           gfx::Point(params_.x, params_.y),
-          WebPluginAction(
-              WebPluginAction::Rotate90Counterclockwise,
-              true));
+          WebPluginAction(WebPluginAction::Rotate90Counterclockwise, true));
       break;
 
     case IDC_BACK:
       source_web_contents_->GetController().GoBack();
       break;
 
     case IDC_FORWARD:
       source_web_contents_->GetController().GoForward();
       break;
 
@@ skipping 323 matching lines @@
   source_web_contents_->GetRenderViewHost()->
       ExecuteMediaPlayerActionAtLocation(location, action);
 }
 
 void RenderViewContextMenu::PluginActionAt(
     const gfx::Point& location,
     const WebPluginAction& action) {
   source_web_contents_->GetRenderViewHost()->
       ExecutePluginActionAtLocation(location, action);
 }