mac: Allows extensions to hide the bookmark page menu item.

chrome/browser/ui/bookmarks/bookmark_utils.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/ui/bookmarks/bookmark_utils.h"
 
 #include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_number_conversions.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/extensions/api/commands/command_service.h"
+#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/search.h"
 #include "chrome/browser/ui/app_list/app_list_util.h"
 #include "chrome/browser/ui/bookmarks/bookmark_editor.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/simple_message_box.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "components/user_prefs/user_prefs.h"
 #include "content/public/browser/web_contents.h"
+#include "extensions/browser/extension_system.h"
+#include "extensions/common/extension_set.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "net/base/net_util.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace chrome {
 
 int num_bookmark_urls_before_prompting = 15;
 
 namespace {
@@ skipping 251 matching lines @@
   return chrome::IsInstantExtendedAPIEnabled() && !profile->IsOffTheRecord();
 }
 
 bool ShouldShowAppsShortcutInBookmarkBar(
   Profile* profile,
   chrome::HostDesktopType host_desktop_type) {
   return IsAppsShortcutEnabled(profile, host_desktop_type) &&
       profile->GetPrefs()->GetBoolean(prefs::kShowAppsShortcutInBookmarkBar);
 }
 
-BookmarkShortcutDisposition GetBookmarkShortcutDisposition(
-    const extensions::CommandService* command_service,
-    const extensions::Extension* extension) {
-  if (command_service->OverridesBookmarkShortcut(extension))
-    return BOOKMARK_SHORTCUT_DISPOSITION_OVERRIDDEN;
+BookmarkShortcutDisposition GetBookmarkShortcutDisposition(Profile* profile) {
+  extensions::CommandService* command_service =
+      extensions::CommandService::Get(profile);
 
-  return extensions::CommandService::RemovesBookmarkShortcut(extension) ?
-      BOOKMARK_SHORTCUT_DISPOSITION_REMOVED :
-      BOOKMARK_SHORTCUT_DISPOSITION_UNCHANGED;
+  const extensions::ExtensionSet* extension_set =
+      extensions::ExtensionSystem::Get(profile)
+          ->extension_service()

[Robert Sesek, 2014/02/26 18:05:13]

It's more common to put the pointer indirection operator on the preceding line:

https://code.google.com/p/chromium/codesearch#search/&q=%5E%5Cs%2B-%3E%20file...
https://code.google.com/p/chromium/codesearch#search/&q=%5C-%3E$%20file:src/c...

[erikchen, 2014/02/26 19:58:23]

Done. I was using the output of clang-format, fyi.
Also, your links don't load. :(

On 2014/02/26 18:05:13, rsesek wrote:

[Robert Sesek, 2014/02/26 20:30:00]

Weird, Rietveld double-percent-escapes them. It's was something like 160
instances of on new line vs 1200 on previous line. 

+          ->extensions();

[Robert Sesek, 2014/02/26 18:05:13]

This will fit on the previous line.

[erikchen, 2014/02/26 19:58:23]

ditto

On 2014/02/26 18:05:13, rsesek wrote:

+  for (extensions::ExtensionSet::const_iterator i = extension_set->begin();

[erikchen, 2014/02/26 01:08:19]

The extension proposal:
https://docs.google.com/a/chromium.org/document/d/1C2Mle92O9uGlji5y5gGDM5tNJ_...

is unclear about what to do if multiple extensions set the flag to override the
bookmark menu item. I take the simplest approach, which is to use disposition of
the first extension with the given flag. Feel free to suggest a different
approach.

[Robert Sesek, 2014/02/26 18:05:13]

This approach is probably fine, but you may want to check with the people
designing this feature if the behavior should be better defined.

[Mike Wittman, 2014/02/26 18:29:05]

The extension keybinding behavior is that the first extension to request the
shortcut gets it. So we don't need to care about which extension asks for the
override as long as one does.

I think the proper logic here should be something like:

if any extension overrides the bookmark shortcut
  return BOOKMARK_SHORTCUT_DISPOSITION_OVERRIDDEN
else if any extension removes the bookmark shortcut
  return BOOKMARK_SHORTCUT_DISPOSITION_REMOVED
else
  return BOOKMARK_SHORTCUT_DISPOSITION_UNCHANGED

[erikchen, 2014/02/26 19:58:23]

Done.

+       i != extension_set->end();
+       ++i) {
+    if (command_service->OverridesBookmarkShortcut(*i))
+      return BOOKMARK_SHORTCUT_DISPOSITION_OVERRIDDEN;
+
+    if (extensions::CommandService::RemovesBookmarkShortcut(*i))
+      return BOOKMARK_SHORTCUT_DISPOSITION_REMOVED;
+  }
+  return BOOKMARK_SHORTCUT_DISPOSITION_UNCHANGED;
+}
+
+bool ShouldShowBookmarkPageMenuItem(Profile* profile) {
+  return GetBookmarkShortcutDisposition(profile) !=
+         BOOKMARK_SHORTCUT_DISPOSITION_REMOVED;
 }
 
 }  // namespace chrome