Split extensions::MenuManager instance out from ExtensionService.

Split extensions::MenuManager instance out from ExtensionService.

This is now its own BCKS. There was no need for it to be in
ExtensionService.

TBR=sky@chromium.org
BUG=162530
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235120

[miket_OOO, 2013-11-13 16:52:18]

LGTM unless my comment is valid.

https://codereview.chromium.org/64953004/diff/110001/chrome/browser/extension...
File chrome/browser/extensions/menu_manager_factory.cc (right):

https://codereview.chromium.org/64953004/diff/110001/chrome/browser/extension...
chrome/browser/extensions/menu_manager_factory.cc:56: return true;
Is this a TODO? Seems this would be false or else have some actual logic in it.
(This isn't a testing class, so this implementation of this method should be
false, it seems.)

[James Cook, 2013-11-13 17:04:30]

Drive-by comments

https://codereview.chromium.org/64953004/diff/110001/chrome/browser/extension...
File chrome/browser/extensions/menu_manager.h (right):

https://codereview.chromium.org/64953004/diff/110001/chrome/browser/extension...
chrome/browser/extensions/menu_manager.h:251: explicit MenuManager(Profile*
profile, StateStore* store_);
nit: no need for explicit

https://codereview.chromium.org/64953004/diff/110001/extensions/browser/proce...
File extensions/browser/process_manager.cc (left):

https://codereview.chromium.org/64953004/diff/110001/extensions/browser/proce...
extensions/browser/process_manager.cc:796: // TODO(yoz): This cleanup code
belongs in the MenuManager.
Thanks for cleaning this up!

[benwells, 2013-11-14 01:44:42]

In the interest of getting stuff landed, I'm going to put this in the queue now.
miket: let me know if you disagree and I can fix up with a subsequent CL. Note I
used the same Null while testing thing for the ExtensionToolbarModel to prevent
crashing in tests.

https://codereview.chromium.org/64953004/diff/110001/chrome/browser/extension...
File chrome/browser/extensions/menu_manager.h (right):

https://codereview.chromium.org/64953004/diff/110001/chrome/browser/extension...
chrome/browser/extensions/menu_manager.h:251: explicit MenuManager(Profile*
profile, StateStore* store_);
On 2013/11/13 17:04:31, James Cook wrote:
> nit: no need for explicit

oops, done.

https://codereview.chromium.org/64953004/diff/110001/chrome/browser/extension...
File chrome/browser/extensions/menu_manager_factory.cc (right):

https://codereview.chromium.org/64953004/diff/110001/chrome/browser/extension...
chrome/browser/extensions/menu_manager_factory.cc:56: return true;
On 2013/11/13 16:52:18, miket wrote:
> Is this a TODO? Seems this would be false or else have some actual logic in
it.
> (This isn't a testing class, so this implementation of this method should be
> false, it seems.)

Tried to chat about this. I don't understand your comment, or perhaps i don't
understand how this should be used as well as you.

My understanding is that this is used to remove BCKS's from unit tests that
don't need to always be there. Without it, as ServiceIsCreatedWithBrowserContext
is true, this would be created with every new profile in unit tests. I can't see
a reason for that - the less we have in unit tests the better. If a test
requires it, it can still call MenuManager::Get explicitly.

[benwells, 2013-11-14 01:46:19]

+tbr sky for refactoring fallout

[commit-bot: I haz the power, 2013-11-14 01:48:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/64953004/230001

[commit-bot: I haz the power, 2013-11-14 02:46:26]

Retried try job too often on mac for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2013-11-14 03:05:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/64953004/230001

[commit-bot: I haz the power, 2013-11-14 11:14:21]

Change committed as 235120

[miket_OOO, 2013-11-14 23:07:31]

https://codereview.chromium.org/64953004/diff/110001/chrome/browser/extension...
File chrome/browser/extensions/menu_manager_factory.cc (right):

https://codereview.chromium.org/64953004/diff/110001/chrome/browser/extension...
chrome/browser/extensions/menu_manager_factory.cc:56: return true;
I understand now. I thought it was more of a getter-for-testing kind of method,
but now that I see the usage elsewhere in the codebase, I get that it's a
runtime signal telling test code how to use it efficiently. Still LGTM!