[Extensions Toolbar] Have all extensions in the toolbar show a context menu

extensions/common/extension.cc

 // Copyright (c) 2013 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 "extensions/common/extension.h"
 
 #include "base/base64.h"
 #include "base/basictypes.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/i18n/rtl.h"
 #include "base/logging.h"
 #include "base/memory/singleton.h"
 #include "base/stl_util.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "base/version.h"
 #include "components/crx_file/id_util.h"
 #include "content/public/common/url_constants.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/error_utils.h"
+#include "extensions/common/feature_switch.h"
 #include "extensions/common/manifest.h"
 #include "extensions/common/manifest_constants.h"
 #include "extensions/common/manifest_handler.h"
 #include "extensions/common/manifest_handlers/permissions_parser.h"
 #include "extensions/common/permissions/permission_set.h"
 #include "extensions/common/permissions/permissions_data.h"
 #include "extensions/common/permissions/permissions_info.h"
 #include "extensions/common/switches.h"
 #include "extensions/common/url_pattern.h"
 #include "net/base/filename_util.h"
@@ skipping 231 matching lines @@
 GURL Extension::GetBaseURLFromExtensionId(const std::string& extension_id) {
   return GURL(std::string(extensions::kExtensionScheme) +
               url::kStandardSchemeSeparator + extension_id + "/");
 }
 
 bool Extension::ShowConfigureContextMenus() const {
   // Don't show context menu for component extensions. We might want to show
   // options for component extension button but now there is no component
   // extension with options. All other menu items like uninstall have
   // no sense for component extensions.
+  // We change this rule if the redesign is enabled, because it's just weird to
+  // have a random button that has no context menu when the rest do.
   return location() != Manifest::COMPONENT &&
-         location() != Manifest::EXTERNAL_COMPONENT;
+         location() != Manifest::EXTERNAL_COMPONENT &&
+         !FeatureSwitch::extension_action_redesign()->IsEnabled();

[Finnur, 2015/02/13 14:49:59]

But what does the menu show for component extensions?

Does it suggest the user can uninstall a component extension?
Do the basic component extensions have meaningful names?
Can it at any point in time lead the user to a 404 on the webstore (by clicking
on the name)?
Does the Inspect Popup entry work?
Does the Options entry work?

[Devlin, 2015/02/13 18:06:54]

Good questions, all!

So, first off, the only component extension that has an action is Stars (we
won't, by default, show component extensions in the toolbar, because unlike
other extensions with no actions, they're basically an implementation detail of
chrome).
No.  Just like policy-enabled extensions, the "Remove from Chrome" option will
be disabled, because the user can't modify the settings. That said, this one
probably warrants a unit test.  Added.
Right now, this is just stars - which I think has the official name of "Bookmark
Manager" (which is meaningful, I think).
We link to the extension's "homepage" by clicking on the name. If the extension
doesn't specify a (valid) homepage in the manifest, we default to the store, if
it updates via the gallery.  If it doesn't, then we don't link to anything (this
happens for, e.g., unpacked extensions).  This is actually an interesting
question for Stars, which *does* have a webstore extension, but I'm not sure if
the one uploaded in the webstore and the default-component one are the same...
in any case, we should never 404.
Yep (as long as it has a popup).
Yep (as long as it has an options page).

[Finnur, 2015/02/16 11:39:51]

I'd be surprised if they are different, but in any case -- this link is just to
learn more about it, and as long as the description (and all that) still
applies, it wouldn't be a huge deal if there is a mismatch in what's on the
store vs. locally.

[Finnur, 2015/02/16 11:39:51]

Hah! I made the same mistake you did when I reviewed this and thought it was
correct. 

However, isn't this a bit more readable?

// Normally we don't show a context menu for component actions, but when
// re-design is enabled we show them in the toolbar at times, and it is
// weird to have a random button that has no context menu when the rest do.
if (location() == Manifest::COMPONENT ||
    location() == Manifest::EXTERNAL_COMPONENT)
  return FeatureSwitch::extension_action_redesign()->IsEnabled();

return true;

[Devlin, 2015/02/17 18:40:41]

Yes, that is more readable. :)  Done.

 }
 
 bool Extension::OverlapsWithOrigin(const GURL& origin) const {
   if (url() == origin)
     return true;
 
   if (web_extent().is_empty())
     return false;
 
   // Note: patterns and extents ignore port numbers.
@@ skipping 485 matching lines @@
 
 UpdatedExtensionPermissionsInfo::UpdatedExtensionPermissionsInfo(
     const Extension* extension,
     const PermissionSet* permissions,
     Reason reason)
     : reason(reason),
       extension(extension),
       permissions(permissions) {}
 
 }   // namespace extensions