Refactor the extension message bubbles.

Refactor the extension message bubbles.

- Get rid of ProfileKeyedApi inheritance from the controllers.
- Keep track of Profile list with base::LazyInstance.
- Extract the Delegate interface out of the Controller.
- Get rid of GetExtensions() function from the delegate interface.
- Change the ownership model. Now: View owns Controller who owns Delegate.

BUG=328441
R=kalman@chromium.org
TBR=sky

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

[Finnur, 2014-01-10 16:24:39]

Let me know if you think more is needed.

[not at google - send to devlin, 2014-01-10 21:28:43]

lgtm with a bunch of comments, thanks for following this up!

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/extension...
File chrome/browser/extensions/dev_mode_bubble_controller.cc (right):

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/extension...
chrome/browser/extensions/dev_mode_bubble_controller.cc:51: :
service_(extensions::ExtensionSystem::Get(profile)->extension_service()) {
perhaps just pass in the ExtensionService directly, rather than the whole
Profile?

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/extension...
File chrome/browser/extensions/dev_mode_bubble_controller.h (right):

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/extension...
chrome/browser/extensions/dev_mode_bubble_controller.h:21: class
DevModeBubbleDelegate
you could move this entire class into an anonymous namespace right? the only
thing is the call in chrome/browser/ui/views/toolbar/browser_action_view.cc but
you could move that function onto the controller.

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/extension...
chrome/browser/extensions/dev_mode_bubble_controller.h:74: void
ClearProfileListForTesting();
this can be static? it may seem a little arbitrary - but it's odd seeing a
controller be constructed in a test then immediately cleared. having it static
would make it clear why.

btw given the name says "ForTest" you could make it public and not worry about
the friend-ing of DevModeControllerTest.

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/extension...
File chrome/browser/extensions/suspicious_extension_bubble_controller.cc
(right):

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/extension...
chrome/browser/extensions/suspicious_extension_bubble_controller.cc:36: :
service_(extensions::ExtensionSystem::Get(profile)->extension_service()) {
likewise, only need prefs not whole profile

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/extension...
File chrome/browser/extensions/suspicious_extension_bubble_controller.h (right):

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/extension...
chrome/browser/extensions/suspicious_extension_bubble_controller.h:20: class
SuspiciousExtensionBubbleDelegate
same comments here as in the other file (move this into the other file, public +
static testing function)

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_message_bubble_view.cc
(right):

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:76: void
ExtensionMessageBubbleView::MaybeShow(
i still think it's odd to have this on extension_message_bubble_view rather than
where the MaybeShow is called from, but, i won't object at this point :)

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:90: // the
view.
i think this comment, particularly after the scoped_ptr change, is self
explanatory

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:92:
suspicious_extensions.release();
when the View takes a scoped_ptr<controller> this would be .get() and when
constructing the view suspicious_extensions.Pass()

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:137: // the
view.
likewise x2

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_message_bubble_view.h (right):

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.h:49:
ExtensionMessageBubbleController* controller);
take a scoped_ptr to indicate ownership

[Finnur, 2014-01-13 15:36:32]

Thanks for the feedback, both now and during last round. Very helpful.

I've addressed all review comments and uploaded. I'll TBR sky@ for trivial
changes in my Views classes and check in via CQ.

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/extension...
File chrome/browser/extensions/dev_mode_bubble_controller.cc (right):

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/extension...
chrome/browser/extensions/dev_mode_bubble_controller.cc:51: :
service_(extensions::ExtensionSystem::Get(profile)->extension_service()) {
On 2014/01/10 21:28:43, kalman wrote:
> perhaps just pass in the ExtensionService directly, rather than the whole
> Profile?

Done.

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/extension...
File chrome/browser/extensions/dev_mode_bubble_controller.h (right):

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/extension...
chrome/browser/extensions/dev_mode_bubble_controller.h:21: class
DevModeBubbleDelegate
On 2014/01/10 21:28:43, kalman wrote:
> you could move this entire class into an anonymous namespace right? the only
> thing is the call in chrome/browser/ui/views/toolbar/browser_action_view.cc
but
> you could move that function onto the controller.

Done.

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/extension...
chrome/browser/extensions/dev_mode_bubble_controller.h:74: void
ClearProfileListForTesting();
On 2014/01/10 21:28:43, kalman wrote:
> this can be static? it may seem a little arbitrary - but it's odd seeing a
> controller be constructed in a test then immediately cleared. having it static
> would make it clear why.
> 
> btw given the name says "ForTest" you could make it public and not worry about
> the friend-ing of DevModeControllerTest.

Done.

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/extension...
File chrome/browser/extensions/suspicious_extension_bubble_controller.cc
(right):

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/extension...
chrome/browser/extensions/suspicious_extension_bubble_controller.cc:36: :
service_(extensions::ExtensionSystem::Get(profile)->extension_service()) {
On 2014/01/10 21:28:43, kalman wrote:
> likewise, only need prefs not whole profile

Done.

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/extension...
File chrome/browser/extensions/suspicious_extension_bubble_controller.h (right):

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/extension...
chrome/browser/extensions/suspicious_extension_bubble_controller.h:20: class
SuspiciousExtensionBubbleDelegate
On 2014/01/10 21:28:43, kalman wrote:
> same comments here as in the other file (move this into the other file, public
+
> static testing function)

Done.

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_message_bubble_view.cc
(right):

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:76: void
ExtensionMessageBubbleView::MaybeShow(
Yup. I believe we are in agreement. Someone wants to add a third type of bubble
(listing admin force-installed extensions that continuously crash) and that
necessitates some queuing mechanism for the bubbles (because both the DevMode
bubble and that bubble want to always show and we don't want multiple bubbles
popping up at once). This code should therefore, when that is implemented, be
moved there.

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:90: // the
view.
On 2014/01/10 21:28:43, kalman wrote:
> i think this comment, particularly after the scoped_ptr change, is self
> explanatory 

Done.

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:92:
suspicious_extensions.release();
On 2014/01/10 21:28:43, kalman wrote:
> when the View takes a scoped_ptr<controller> this would be .get() and when
> constructing the view suspicious_extensions.Pass()

Done.

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:137: // the
view.
On 2014/01/10 21:28:43, kalman wrote:
> likewise x2

Done.

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_message_bubble_view.h (right):

https://codereview.chromium.org/134103002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.h:49:
ExtensionMessageBubbleController* controller);
On 2014/01/10 21:28:43, kalman wrote:
> take a scoped_ptr to indicate ownership

Done.

[Finnur, 2014-01-14 11:29:46]

Committed patchset #3 manually as r244671 (presubmit successful).