Fix top-level context menus sorting by name

chrome/browser/tab_contents/render_view_context_menu.cc

 // Copyright (c) 2012 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/tab_contents/render_view_context_menu.h"
 
 #include <algorithm>
 #include <set>
 #include <utility>
 
@@ skipping 346 matching lines @@
 }
 
 void RenderViewContextMenu::AppendAllExtensionItems() {
   extension_items_.Clear();
   ExtensionService* service =
       extensions::ExtensionSystem::Get(profile_)->extension_service();
   if (!service)
     return;  // In unit-tests, we may not have an ExtensionService.
   MenuManager* menu_manager = service->menu_manager();
 
+  string16 printable_selection_text = PrintableSelectionText();
+  // Escape "&" as "&&".
+  for (size_t position = printable_selection_text.find('&');

[Yoyo Zhou, 2013/02/19 19:00:50]

This is more complex than necessary. Use ReplaceChars - see
https://chromiumcodereview.appspot.com/12294022/
(but that one is *also* more complex than necessary, see my comment).

[François Beaufort, 2013/02/20 11:32:06]

I added a TODO to commit this when the other code review is committed to avoid
conflict.

On 2013/02/19 19:00:50, Yoyo Zhou wrote:

+       position != string16::npos;
+       position = printable_selection_text.find('&', position + 2))
+    printable_selection_text.insert(position, 1, '&');
+
   // Get a list of extension id's that have context menu items, and sort it by

[Yoyo Zhou, 2013/02/19 19:00:50]

delete 'it'

[François Beaufort, 2013/02/20 11:32:06]

Done.

-  // the extension's name.
+  // the top level context menu title of the extension.
   std::set<std::string> ids = menu_manager->ExtensionIds();
   std::vector<std::pair<std::string, std::string> > sorted_ids;
   for (std::set<std::string>::iterator i = ids.begin(); i != ids.end(); ++i) {
     const Extension* extension = service->GetExtensionById(*i, false);
     // Platform apps have their context menus created directly in
     // AppendPlatformAppItems.

[Yoyo Zhou, 2013/02/19 19:00:50]

Does AppendPlatformAppItems have the same sorting bug as in the case of
extensions? You don't need to fix it in this CL, but it may be worthwhile to
file a bug for it.

[François Beaufort, 2013/02/20 11:32:06]

From what I can read, there is no sorting involved in AppendPlatformAppItems
since Context Menus are created in order by the owner of the App.
See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

On 2013/02/19 19:00:50, Yoyo Zhou wrote:

-    if (extension && !extension->is_platform_app())
+    if (extension && !extension->is_platform_app()) {
+      std::string menu_title = extension_items_.GetTopLevelContextMenuTitle(*i,

[Yoyo Zhou, 2013/02/19 19:00:50]

nit: put *i on the next line. You can put it on the same line as
printable_selection_text.

[François Beaufort, 2013/02/20 11:32:06]

Done.

+          printable_selection_text);
       sorted_ids.push_back(
-          std::pair<std::string, std::string>(extension->name(), *i));
+          std::pair<std::string, std::string>(menu_title, *i));
+    }
   }
   // TODO(asargent) - See if this works properly for i18n names (bug 32363).
   std::sort(sorted_ids.begin(), sorted_ids.end());
 
   if (sorted_ids.empty())
     return;
 
   int index = 0;
+  // TODO(fbeaufort): Where to move this UMA now?

[Yoyo Zhou, 2013/02/19 19:00:50]

I think it's okay to leave it here.

[François Beaufort, 2013/02/20 11:32:06]

Done.

   base::TimeTicks begin = base::TimeTicks::Now();
   std::vector<std::pair<std::string, std::string> >::const_iterator i;
-  for (i = sorted_ids.begin();
-       i != sorted_ids.end(); ++i) {
-    string16 printable_selection_text = PrintableSelectionText();
-    // Escape "&" as "&&".
-    for (size_t position = printable_selection_text.find('&');
-         position != string16::npos;
-         position = printable_selection_text.find('&', position + 2))
-      printable_selection_text.insert(position, 1, '&');
-
+  for (i = sorted_ids.begin(); i != sorted_ids.end(); ++i)
     extension_items_.AppendExtensionItems(i->second, printable_selection_text,

[Yoyo Zhou, 2013/02/19 19:00:50]

I don't see how this can behave equivalently to the old version.
printable_selection_text never changes so it seems that you would append many
copies of the same text.

[François Beaufort, 2013/02/20 11:32:06]

I'm not sure about what you mean here. Before, printable_selection_text was
defined for each Extension in the loop.
But now, we define it at the very top of this function and call
AppendExtensionItems with it. 
Does that make sense?

On 2013/02/19 19:00:50, Yoyo Zhou wrote:

[Yoyo Zhou, 2013/03/06 20:18:58]

No. I'm not sure of this, but I believe PrintableSelectionText() returns a
different value each time it's called.

                                           &index);
-  }
   UMA_HISTOGRAM_TIMES("Extensions.ContextMenus_BuildTime",
                       base::TimeTicks::Now() - begin);
   UMA_HISTOGRAM_COUNTS("Extensions.ContextMenus_ItemCount", index);
 }
 
 void RenderViewContextMenu::InitMenu() {
   chrome::ViewType view_type = chrome::GetViewType(source_web_contents_);
   if (view_type == chrome::VIEW_TYPE_APP_SHELL) {
     AppendPlatformAppItems();
     return;
@@ skipping 1462 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);
 }