SupportsUserData and manifest handlers for Extension; use them for the Omnibox API.

SupportsUserData and manifest handlers for Extension; use them for the Omnibox API.

Note that manifest handlers aren't available in the renderer (there seems to be no place to register them). At least for Omnibox, the renderer doesn't need to parse the manifest field, but this won't be true everywhere.
Pull a bunch of Omnibox API stuff out of ExtensionService.

BUG=159265
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172638

[Yoyo Zhou, 2012-12-06 19:58:53]

I think I found ChromeContentRendererClient as the place to put registration in
the renderer. With that I'll probably move the manifest handling part of
omnibox_api to chrome/common/extensions/api/omnibox, if that seems reasonable.

[Matt Perry, 2012-12-07 01:22:58]

Neat, LGTM.

If we don't need the omnibox API in the renderer, why not just not parse it at
all there? Seems reasonable to only bother parsing the stuff that needs parsing.

https://chromiumcodereview.appspot.com/11446034/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right):

https://chromiumcodereview.appspot.com/11446034/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/omnibox/omnibox_api.cc:99: return false;
leaking |info|. use scoped_ptr

https://chromiumcodereview.appspot.com/11446034/diff/2001/chrome/common/exten...
File chrome/common/extensions/manifest_handler.h (right):

https://chromiumcodereview.appspot.com/11446034/diff/2001/chrome/common/exten...
chrome/common/extensions/manifest_handler.h:22: // Attempts to parse the
manifest value.
This comment seems to have wandered away from home.

https://chromiumcodereview.appspot.com/11446034/diff/2001/chrome/common/exten...
chrome/common/extensions/manifest_handler.h:38: void
RegisterManifestHandler(const std::string& key, ManifestHandler* handler);
Make these static methods of ManifestHandler?

[Yoyo Zhou, 2012-12-07 02:13:32]

I went and moved the common parts to common. The renderer still doesn't know
about them. Let me know if you think this is better, or if I should move them
back.

https://chromiumcodereview.appspot.com/11446034/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right):

https://chromiumcodereview.appspot.com/11446034/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/omnibox/omnibox_api.cc:99: return false;
On 2012/12/07 01:22:59, Matt Perry wrote:
> leaking |info|. use scoped_ptr

Done.

https://chromiumcodereview.appspot.com/11446034/diff/2001/chrome/common/exten...
File chrome/common/extensions/manifest_handler.h (right):

https://chromiumcodereview.appspot.com/11446034/diff/2001/chrome/common/exten...
chrome/common/extensions/manifest_handler.h:22: // Attempts to parse the
manifest value.
On 2012/12/07 01:22:59, Matt Perry wrote:
> This comment seems to have wandered away from home.

Found its way back.

https://chromiumcodereview.appspot.com/11446034/diff/2001/chrome/common/exten...
chrome/common/extensions/manifest_handler.h:38: void
RegisterManifestHandler(const std::string& key, ManifestHandler* handler);
On 2012/12/07 01:22:59, Matt Perry wrote:
> Make these static methods of ManifestHandler?

Sure.

[Matt Perry, 2012-12-07 20:20:02]

lgtm

[commit-bot: I haz the power, 2012-12-07 21:25:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/11446034/5002

[commit-bot: I haz the power, 2012-12-07 21:25:31]

Failed to apply patch for chrome/browser/profiles/profile_dependency_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/profiles/profile_dependency_manager.cc
  Hunk #1 succeeded at 21 with fuzz 2 (offset 2 lines).
  Hunk #2 FAILED at 226.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/profiles/profile_dependency_manager.cc.rej

Patch:       chrome/browser/profiles/profile_dependency_manager.cc
Index: chrome/browser/profiles/profile_dependency_manager.cc
diff --git a/chrome/browser/profiles/profile_dependency_manager.cc
b/chrome/browser/profiles/profile_dependency_manager.cc
index
282d5a8d30a3aa248331c5840bac41cb142d9433..e64ffe0788f96a1c5152927a2ab1a1f2267f1e20
100644
--- a/chrome/browser/profiles/profile_dependency_manager.cc
+++ b/chrome/browser/profiles/profile_dependency_manager.cc
@@ -19,6 +19,7 @@
 #include
"chrome/browser/extensions/api/discovery/suggested_links_registry_factory.h"
 #include "chrome/browser/extensions/api/idle/idle_manager_factory.h"
 #include "chrome/browser/extensions/api/management/management_api_factory.h"
+#include "chrome/browser/extensions/api/omnibox/omnibox_api_factory.h"
 #include "chrome/browser/extensions/api/processes/processes_api_factory.h"
 #include
"chrome/browser/extensions/api/tab_capture/tab_capture_registry_factory.h"
 #include "chrome/browser/extensions/app_restore_service_factory.h"
@@ -225,6 +226,7 @@ void ProfileDependencyManager::AssertFactoriesBuilt() {
   extensions::CommandServiceFactory::GetInstance();
   extensions::ExtensionSystemFactory::GetInstance();
   extensions::IdleManagerFactory::GetInstance();
+  extensions::OmniboxAPIFactory::GetInstance();
   extensions::ProcessesAPIFactory::GetInstance();
   extensions::SuggestedLinksRegistryFactory::GetInstance();
   extensions::TabCaptureRegistryFactory::GetInstance();

[Yoyo Zhou, 2012-12-07 21:44:31]

sky: please review gyp changes, c/b/ui, c/b/search_engines. It's mostly
refactoring.
rlp: please review c/b/profiles.

[rpetterson, 2012-12-07 22:02:58]

c/b/profiles LGTM

[Yoyo Zhou, 2012-12-07 22:24:21]

+nileshagrawal: I'd like to get your opinion on what to do in case
enable_extensions == 0. There's an error because omnibox_handler.h is under
chrome/common/extensions/api and thus not compiled in:

http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...

I could add it to the exceptions in chrome_common.gypi so that it *is* compiled,
or I could #ifdef out the offending code in template_url_service. I think I
would prefer the former. Does that sound reasonable to you?

[nilesh, 2012-12-07 23:11:47]

On 2012/12/07 22:24:21, Yoyo Zhou wrote:
> +nileshagrawal: I'd like to get your opinion on what to do in case
> enable_extensions == 0. There's an error because omnibox_handler.h is under
> chrome/common/extensions/api and thus not compiled in:
> 
>
http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...
> 
> I could add it to the exceptions in chrome_common.gypi so that it *is*
compiled,
> or I could #ifdef out the offending code in template_url_service. I think I
> would prefer the former. Does that sound reasonable to you?

My first preference would be to #ifdef out the calls to RegisterExtensionKeyword
and UnregisterExtensionKeyword (in extension_service.cc and
template_url_service.cc). That way we can leave the methods as is and the linker
should be happy. It is not ideal that we have to add ENABLE_EXTENSIONS in
template_url_service but this is because it maintains a list of
pending_extension_ids_ (and is extension aware in general)

Even better, we can call RegisterExtensionKeyword with id, name and
omnibox_keyword (instead of Extension*) and let TemplateUrlService maintain a
list of 3-tuples. That way it does not need to query GetExtensionById
(ExtensionSystem::Get(profile_)->extension_service()->GetExtensionById) to get
all the info later (when it is loaded), unless it really needs to query the
ExtensionService for the latest info.

This way we remove a lot of extensions awareness from this class. WDYT?

[sky, 2012-12-07 23:44:04]

LGTM

[Yoyo Zhou, 2012-12-08 00:27:44]

On 2012/12/07 23:11:47, nileshagrawal1 wrote:
> Even better, we can call RegisterExtensionKeyword with id, name and
> omnibox_keyword (instead of Extension*) and let TemplateUrlService maintain a
> list of 3-tuples. That way it does not need to query GetExtensionById
> (ExtensionSystem::Get(profile_)->extension_service()->GetExtensionById) to get
> all the info later (when it is loaded), unless it really needs to query the
> ExtensionService for the latest info.
> 
> This way we remove a lot of extensions awareness from this class. WDYT?

Actually, you're right - TemplateUrlService has no business knowing about
extensions either. And PendingExtensionIDs aren't its problem either. I moved
all this over to OmniboxAPI.

I also re-enabled some of the OmniboxAPI API tests because it looks like they
were only disabled on account of chromeos gtk, which no longer exists.

mpcomplete and sky, sorry, but could you take another look?

[Matt Perry, 2012-12-08 00:46:26]

lgtm

https://chromiumcodereview.appspot.com/11446034/diff/8031/chrome/browser/exte...
File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right):

https://chromiumcodereview.appspot.com/11446034/diff/8031/chrome/browser/exte...
chrome/browser/extensions/api/omnibox/omnibox_api.cc:199:
pending_extensions_.erase(i);
pendings_extensions_.erase(extension) should work too.

[Yoyo Zhou, 2012-12-08 00:58:18]

https://chromiumcodereview.appspot.com/11446034/diff/8031/chrome/browser/exte...
File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right):

https://chromiumcodereview.appspot.com/11446034/diff/8031/chrome/browser/exte...
chrome/browser/extensions/api/omnibox/omnibox_api.cc:199:
pending_extensions_.erase(i);
On 2012/12/08 00:46:26, Matt Perry wrote:
> pendings_extensions_.erase(extension) should work too.

It doesn't work for std::list. But it should have been a std::set anyway, so I
changed it to be.

[Yoyo Zhou, 2012-12-08 02:06:15]

FYI, according to test failures, it appears that I'll need to switch to a
thread-safe version of SupportsUserData instead. I'll plan on adding that to
base in a separate CL before this one can be ready.

[Yoyo Zhou, 2012-12-11 22:42:43]

After discussion with jam, I added the SupportsUserData-alike to Extension
directly.

Matt, another look?

(Sorry about the rebase, but the changes since the last patch are only to
extension.{h,cc} and omnibox_handler.{h,cc}.)

[Matt Perry, 2012-12-11 23:27:56]

Nice and clean. I like it. LGTM

[commit-bot: I haz the power, 2012-12-12 00:13:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/11446034/18001

[commit-bot: I haz the power, 2012-12-12 03:10:57]

Retried try job too often on linux_chromeos for step(s) aura_unittests,
base_unittests, browser_tests, cacheinvalidation_unittests, check_deps,
chromeos_unittests, content_browsertests, content_unittests, crypto_unittests,
dbus_unittests, device_unittests, gpu_unittests, interactive_ui_tests,
ipc_tests, jingle_unittests, media_unittests, net_unittests, ppapi_unittests,
printing_unittests, sandbox_linux_unittests, sql_unittests, sync_unit_tests,
unit_tests

[commit-bot: I haz the power, 2012-12-12 17:23:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/11446034/18001

[commit-bot: I haz the power, 2012-12-12 19:32:59]

Change committed as 172638