Fix top-level context menus sorting by name

chrome/browser/extensions/context_menu_matcher.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 "base/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/extensions/context_menu_matcher.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/profiles/profile.h"
@@ skipping 80 matching lines @@
     }
     SetExtensionIcon(extension_id);
   }
 }
 
 void ContextMenuMatcher::Clear() {
   extension_item_map_.clear();
   extension_menu_models_.clear();
 }
 
+std::string ContextMenuMatcher::GetTopLevelContextMenuTitle(

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

This duplicates code in AppendExtensionItems. Can you make that function use the
logic here, so the code isn't duplicated?

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

I'm not sure about how to implement the private function and would like your
input on this.

Do you think of a function that would take 3 parameters "extension_id",
"extension*" and "items*" and sets "extension*" and "items*"  

OR something else?

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

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

I was thinking it should take an extension_id as input and return the
MenuItem::List items. There will need to be some slight logic refactoring
(around the early returns if manager->MenuItems(extension_id) is empty).

You could call this GetRelevantExtensionTopLevelItems.

+    const std::string& extension_id,
+    const string16& selection_text) {
+  ExtensionService* service =
+      extensions::ExtensionSystem::Get(profile_)->extension_service();
+  MenuManager* manager = service->menu_manager();
+  const Extension* extension = service->GetExtensionById(extension_id, false);
+
+  // Find matching items.
+  const MenuItem::List* all_items = manager->MenuItems(extension_id);
+  if (!all_items || all_items->empty())
+    // Return extension name if there are no items

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

Shouldn't this function never be called for extensions that have no items? You
ought to be able to change this to
CHECK(all_items && !all_items->empty());

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

Done.

+    return extension->name();
+  bool can_cross_incognito = service->CanCrossIncognito(extension);
+  MenuItem::List items = GetRelevantExtensionItems(*all_items,
+                                                   can_cross_incognito);
+
+  std::string title;
+
+  if (items.size() > 1 || items[0]->type() != MenuItem::NORMAL) {
+    title = extension->name();
+  } else {
+    MenuItem* item = items[0];
+    title = UTF16ToUTF8(item->TitleWithReplacement(selection_text,
+        kMaxExtensionItemTitleLength));

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

nit: if you continue function arguments onto the next line like this,
indentation should be to the open paren. Since it won't fit, start the first
argument on a new line (at +4 spaces).

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

Done.

+  }
+  return title;
+}
+
 bool ContextMenuMatcher::IsCommandIdChecked(int command_id) const {
   MenuItem* item = GetExtensionMenuItem(command_id);
   if (!item)
     return false;
   return item->checked();
 }
 
 bool ContextMenuMatcher::IsCommandIdEnabled(int command_id) const {
   MenuItem* item = GetExtensionMenuItem(command_id);
   if (!item)
@@ skipping 112 matching lines @@
   DCHECK_GE(index, 0);
 
   const SkBitmap& icon = menu_manager->GetIconForExtension(extension_id);
   DCHECK(icon.width() == gfx::kFaviconSize);
   DCHECK(icon.height() == gfx::kFaviconSize);
 
   menu_model_->SetIcon(index, gfx::Image::CreateFrom1xBitmap(icon));
 }
 
 }  // namespace extensions