Extend contextMenus API to support browser/page actions

chrome/browser/extensions/menu_manager_factory.cc

 // Copyright 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 "chrome/browser/extensions/menu_manager_factory.h"
 
 #include "chrome/browser/extensions/menu_manager.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/extension_system_provider.h"
 #include "extensions/browser/extensions_browser_client.h"
 
 namespace extensions {
 
 // static
 MenuManager* MenuManagerFactory::GetForProfile(
     Profile* profile) {
   return static_cast<MenuManager*>(
       GetInstance()->GetServiceForBrowserContext(profile, true));
 }
 
 // static
 MenuManagerFactory* MenuManagerFactory::GetInstance() {
   return Singleton<MenuManagerFactory>::get();
 }
 
+// static
+KeyedService* MenuManagerFactory::BuildInstanceFor(

[Devlin, 2014/07/24 21:51:28]

If you're gonna do this, why not just make BuildServiceInstanceFor public, or
(better yet) expose a ForTest() method so people get errors if they try to use
it in real code?

[gpdavis, 2014/07/28 21:08:22]

BuildServiceInstanceFor is an overridden non-static method, and BuildInstanceFor
must be static, so there's some conflict (unless I'm misunderstanding
something).

In what way can we ensure that a ForTest() method is only run in a testing
environment?  I'm a bit confused about what this method would be.  The only
thing I could find was this:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

but Associate is called in the code I've already added
(GetServiceForBrowserContext calls it).  Maybe a little explanation about what's
bad about the way I've implemented this would help steer me in the right
direction.

[Yoyo Zhou, 2014/07/28 23:23:25]

We have presubmits that warn about the use of methods named "...ForTesting" in
non-test code.

The goal here is not to have methods on the factory that can be accidentally
misused. Basically, it's better to define the method that creates the
MenuManager for tests in the test itself, even if your creating the MenuManager
looks a bit redundant with what's in here. (Sorry, in my earlier comment, I
hadn't realized we had ServiceIsNULLWhileTesting = true.) Here's a simpler
example of how one can use SetTestingFactoryAndUse:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

+    content::BrowserContext* context) {
+  Profile* profile = Profile::FromBrowserContext(context);
+  return new MenuManager(profile, ExtensionSystem::Get(profile)->state_store());
+}
+
 MenuManagerFactory::MenuManagerFactory()
     : BrowserContextKeyedServiceFactory(
         "MenuManager",
         BrowserContextDependencyManager::GetInstance()) {
   DependsOn(ExtensionsBrowserClient::Get()->GetExtensionSystemFactory());
 }
 
 MenuManagerFactory::~MenuManagerFactory() {}
 
 KeyedService* MenuManagerFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
-  Profile* profile = Profile::FromBrowserContext(context);
-  return new MenuManager(
-      profile,
-      ExtensionSystem::Get(profile)->state_store());
+  return BuildInstanceFor(context);
 }
 
 content::BrowserContext* MenuManagerFactory::GetBrowserContextToUse(
     content::BrowserContext* context) const {
   return ExtensionsBrowserClient::Get()->GetOriginalContext(context);
 }
 
 bool MenuManagerFactory::ServiceIsCreatedWithBrowserContext() const {
   return true;
 }
 
 bool MenuManagerFactory::ServiceIsNULLWhileTesting() const {
   return true;
 }
 
 }  // namespace extensions