Move save/generate password options to the top of the context menu

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 <stddef.h>
 
 #include <algorithm>
 #include <set>
@@ skipping 1449 matching lines @@
       l10n_util::GetStringUTF16(IDS_CONTENT_CONTEXT_OPENLINKWITH_CONFIGURE));
 
   menu_model_.AddSubMenu(
       IDC_CONTENT_CONTEXT_OPENLINKWITH,
       l10n_util::GetStringUTF16(IDS_CONTENT_CONTEXT_OPENLINKWITH),
       &protocol_handler_submenu_model_);
 }
 
 void RenderViewContextMenu::AppendPasswordItems() {
   bool separator_added = false;
+  int menu_position = 0;
   if (password_manager::ForceSavingExperimentEnabled()) {
-    menu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
+    menu_model_.InsertItemWithStringIdAt(menu_position,

[lazyboy, 2017/02/14 19:26:41]

instead of using mix of menu_position, ++menu_position and menu_position+1,
could we always use menu_position++ as parameter?

However, I feel like this better be avoided to explicitly specify position 0 for
these kind of items b/c it will encourage multiple group items to fight for a
spot. And it will be harder to understand by reading the code where a group will
end up in. Any group adding items later can steal the spot 0.

If we moved AppendPasswordItems at the top of RenderViewContextMenu::InitMenu()
after calling Base::InitMenu, would that be enough? Currently only custom items
and toolkit delegate items will come before AppendPasswordItems

[edwardjung, 2017/02/14 20:17:21]

Of course. yes this is much simpler and removes the need for inserting at
specific indexes. Thanks!

I tweaked the logic for displaying the separator. 

+                                         IDC_CONTENT_CONTEXT_FORCESAVEPASSWORD,
+                                         IDS_CONTENT_CONTEXT_FORCESAVEPASSWORD);
+    menu_model_.InsertSeparatorAt(++menu_position, ui::NORMAL_SEPARATOR);
     separator_added = true;
-    menu_model_.AddItemWithStringId(IDC_CONTENT_CONTEXT_FORCESAVEPASSWORD,
-                                    IDS_CONTENT_CONTEXT_FORCESAVEPASSWORD);
   }
   if (password_manager::ManualPasswordGenerationEnabled()) {
+    menu_model_.InsertItemWithStringIdAt(menu_position,
+                                         IDC_CONTENT_CONTEXT_GENERATEPASSWORD,
+                                         IDS_CONTENT_CONTEXT_GENERATEPASSWORD);
     if (!separator_added)
-      menu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
-    menu_model_.AddItemWithStringId(IDC_CONTENT_CONTEXT_GENERATEPASSWORD,
-                                    IDS_CONTENT_CONTEXT_GENERATEPASSWORD);
+      menu_model_.InsertSeparatorAt(menu_position + 1, ui::NORMAL_SEPARATOR);
   }
 }
 
 // Menu delegate functions -----------------------------------------------------
 
 bool RenderViewContextMenu::IsCommandIdEnabled(int id) const {
   {
     bool enabled = false;
     if (RenderViewContextMenuBase::IsCommandIdKnown(id, &enabled))
       return enabled;
@@ skipping 1033 matching lines @@
 void RenderViewContextMenu::PluginActionAt(
     const gfx::Point& location,
     const WebPluginAction& action) {
   source_web_contents_->GetRenderViewHost()->
       ExecutePluginActionAtLocation(location, action);
 }
 
 Browser* RenderViewContextMenu::GetBrowser() const {
   return chrome::FindBrowserWithWebContents(embedder_web_contents_);
 }