Add an extension bubble explaining which extensions are in dev mode.

Add an extension bubble explaining which extensions are in dev mode. 

In order to accomplish this without too much code duplication I refactored the common code of the newly added SuspiciousExtension bubble into a base class ExtensionMessageBubbleController and a derived class (SuspiciousExtensionBubbleController) to accomodate a new type of bubble (DevModeBubbleController). I also generalized the BubbleView code to just display whatever the controller tells it (and removed mentions of SuspiciousExtensions from it). 

The new controller has an action button (Disable) so new functionality was added to accomodate. 

BUG=TBD

[Finnur, 2013-11-28 15:12:46]

Scott, can you take care of:

The rename (and generalization) of:
chrome/browser/ui/views/extensions/suspicious_extension_bubble_view.h  
chrome/browser/ui/views/extensions/suspicious_extension_bubble_view.cc  
... to:
chrome/browser/ui/views/extensions/extension_message_bubble_view.h  
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc  

... and changes in:
chrome/browser/ui/views/toolbar/browser_action_view.cc 
chrome/browser/ui/views/toolbar/browser_actions_container.h  
chrome/browser/ui/views/toolbar/toolbar_view.cc  

Ben:

Refactor of:
chrome/browser/extensions/suspicious_extension_bubble.h     
chrome/browser/extensions/suspicious_extension_bubble_controller.h     
chrome/browser/extensions/suspicious_extension_bubble_controller.cc   
chrome/browser/extensions/suspicious_extension_bubble_controller_unittest.cc  
... to:
chrome/browser/extensions/extension_message_bubble.h  
chrome/browser/extensions/extension_message_bubble_controller.h  
chrome/browser/extensions/extension_message_bubble_controller.cc  
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc 

And new class:
chrome/browser/extensions/dev_mode_bubble_controller.h  
chrome/browser/extensions/dev_mode_bubble_controller.cc

... and changes in:
chrome/app/theme/theme_resources.grd 
chrome/browser/extensions/extension_prefs.h  
chrome/browser/extensions/extension_prefs.cc  
chrome/chrome_browser_extensions.gypi  
chrome/chrome_browser_ui.gypi  
chrome/chrome_tests_unit.gypi  
chrome/common/url_constants.h  
chrome/common/url_constants.cc  
extensions/common/extension.h  
extensions/common/extension.cc

[sky, 2013-12-02 16:26:44]

Side by side diffs didn't make it. Could you reupload?

[Finnur, 2013-12-02 18:10:51]

How about now?

[sky, 2013-12-02 21:56:06]

Sorry, one more. Can you play with --similarity in hopes of
extension_message_bubble_view.cc being seen as a move/copy and not a new file?

[sky, 2013-12-02 22:28:03]

LGTM

[not at google - send to devlin, 2013-12-04 17:25:12]

It would be easier to review this if you did the file move + slight refactor in
a separate CL then added the developer mode feature afterwards. Is that
reasonable? It's hard to see what I can ignore and what I can't.

Also this patch doesn't apply cleanly on tip. I was trying to patch it in to do
some grepping:

error: patch failed: chrome/browser/extensions/suspicious_extension_bubble.h:1
error: chrome/browser/extensions/suspicious_extension_bubble.h: patch does not
apply
error: patch failed:
chrome/browser/extensions/suspicious_extension_bubble_controller_unittest.cc:1
error:
chrome/browser/extensions/suspicious_extension_bubble_controller_unittest.cc:
patch does not apply
error: patch failed:
chrome/browser/extensions/suspicious_extension_bubble_controller.cc:61
Falling back to three-way merge...
Applied patch to
'chrome/browser/extensions/suspicious_extension_bubble_controller.cc' with
conflicts.
error: patch failed: chrome/chrome_tests_unit.gypi:2588
Falling back to three-way merge...
Applied patch to 'chrome/chrome_tests_unit.gypi' cleanly.
Failed to apply the patch

https://codereview.chromium.org/95133002/diff/70001/chrome/browser/extensions...
File chrome/browser/extensions/extension_message_bubble.h (right):

https://codereview.chromium.org/95133002/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/extension_message_bubble.h:23: public:
nit: I find this block hard to distinguish between method names. could you
separate each with a blank line?

https://codereview.chromium.org/95133002/diff/70001/chrome/browser/extensions...
File chrome/browser/extensions/extension_message_bubble_controller.h (right):

https://codereview.chromium.org/95133002/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/extension_message_bubble_controller.h:23:
ExtensionMessageBubbleController(ExtensionListType type, Profile* profile);
Having discrete "list types" like this makes this class less reusable; anybody
creating a new bubble type (which I think is quite likely going forward) will
need to modify code that they shouldn't care about.

See comment below about the delegate.

https://codereview.chromium.org/95133002/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/extension_message_bubble_controller.h:56: virtual void
PerformAction() = 0;
the more typical idiom is to define a Delegate for this class which has these
methods; ExtensionMessageBubbleController::Delegate. Looking at the code which
is divergent on |type_|, you'd also have things like LogUMA and GetTitle or
whatever.

https://codereview.chromium.org/95133002/diff/70001/chrome/browser/extensions...
File chrome/browser/extensions/extension_prefs.h (right):

https://codereview.chromium.org/95133002/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.h:308: void
SetDevModeAcknowledged(const std::string& extension_id, bool value);
why are we storing this in preferences? Isn't the point of this that we don't
store such things in preferences, and we show it on every startup?

https://codereview.chromium.org/95133002/diff/70001/extensions/common/extensi...
File extensions/common/extension.h (right):

https://codereview.chromium.org/95133002/diff/70001/extensions/common/extensi...
extensions/common/extension.h:263: bool IsDevModeExtension() const;
I think this belongs in extension_util.h if anywhere. I'm not sure that the
model should care about this though, it's an interpretation of the data that
this is loaded in "developer mode". Could it go in one of the controller classes
instead?

[Finnur, 2013-12-05 23:55:42]

Patch synced to trunk and uploaded with 40% similarity. Should work against
trunk now. The view has changed little enough to show nice diff, but the
controller classes are significantly different with the delegate changes, so I
think they warrant a complete review.

[Finnur, 2013-12-05 23:55:54]

https://codereview.chromium.org/95133002/diff/70001/chrome/browser/extensions...
File chrome/browser/extensions/extension_prefs.h (right):

https://codereview.chromium.org/95133002/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.h:308: void
SetDevModeAcknowledged(const std::string& extension_id, bool value);
Indeed. Done.

https://codereview.chromium.org/95133002/diff/70001/extensions/common/extensi...
File extensions/common/extension.h (right):

https://codereview.chromium.org/95133002/diff/70001/extensions/common/extensi...
extensions/common/extension.h:263: bool IsDevModeExtension() const;
Sure. Done.

[not at google - send to devlin, 2013-12-10 01:21:39]

Sorry for the vague comments again, I don't seem able to page this whole change
in at once and I hope that making it a more clearly separated architecture will
help.

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/extension...
File chrome/browser/extensions/extension_message_bubble_controller.cc (right):

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/extension...
chrome/browser/extensions/extension_message_bubble_controller.cc:39: bool
ExtensionMessageBubbleController::HasExtensionList() {
HasExtensionList is confusing given it's actually based on whether the bubble
should be shown. Perhaps ShouldShow()?

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/extension...
chrome/browser/extensions/extension_message_bubble_controller.cc:56: return
!extension_list_.empty();
A lot of the behaviour in this class seems to rely on HasExtensionList() being
called first? That doesn't seem right. Do you want to implement lazy
initialization of that extension list? I would add a method like
GetExtensionList() which initializes the extension list if it hasn't been
already (it would need a separate bool I suppose, to distinguish from there
being no extensions).

The call to LogExtensionCount would go in there.

and this method would look like

bool ShouldShow() {
  if (has_notified_) return false;
  has_notified_ = true;
  return !GetExtensionList().empty();
}

though perhaps it should be the view that decides whether not having any
extensions implies showing it anyway. Small point.

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/extension...
File chrome/browser/extensions/extension_message_bubble_controller.h (right):

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/extension...
chrome/browser/extensions/extension_message_bubble_controller.h:37: virtual
const ExtensionIdList& GetExtensionIdList() const = 0;
is this used?

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/extension...
chrome/browser/extensions/extension_message_bubble_controller.h:54: virtual bool
ShouldShowExtensionList() const = 0;
this seems to always return true, also, it doesn't seem so necessary given
HasExtensionList, also it doesn't appear to be used.

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/extension...
File
chrome/browser/extensions/suspicious_extension_bubble_controller_delegate.cc
(right):

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/extension...
chrome/browser/extensions/suspicious_extension_bubble_controller_delegate.cc:61:
controller_->Show(bubble);
see comment in extension_message_bubble_view.cc, the delegates shouldn't be
implementing any publicly available functionality. The Show/Has/etc methods
should only appear on the Controller.

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

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:83:
SuspiciousExtensionBubbleControllerDelegate* suspicious_extensions =
These Delegates are supposed to be implementation details of the Controllers,
not actually exposed as part of the functional interface. I.e. I would expect to
see this accessing a controller for the suspicious extensions then calling
Show() on it. This would prevent those odd methods on the Delegates which just
call through to the Controller.

This raises the problem of where to put the ::Get(Profile*) method. IIRC you
need the per-profile singleton-ness to enforce that the bubble only gets shown
once per launch? For that perhaps you could implement that within the Delegate
by maintaining a LazyInstance<ProfileSet> where ProfileSet contains a
set<Profile*>.

Incidentally, this whole method is a bit suspect. This MaybeShow is now actually
responsible for showing *two* views and (see below) that's subtle and perhaps it
should go in that Controller so that it can be tested.

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:91: return;
so what is supposed to happen when there are both suspicious extensions and it's
in dev mode? It looks like the first time MaybeShow is called the suspicious
bubble is shown, the second time the dev mode bubble is shown.

just checking that's what you intended.

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:121: break; 
// Found a good candidate.
all this logic should be in the Delegate implementation for dev mode bubble so
that it can be tested.

[Finnur, 2013-12-11 23:12:17]

OK, Ben, here's the latest version.

I tried to create a clean patch (using the default upload --similarity value of
50) to see if I could get a clean patch that can be applied to the local tree. 

It didn't work for me, but the bots so far seem to be happy with it so maybe it
will work for you. Worth a shot.
https://codereview.chromium.org/98363008

I include it purely for your convenience and it would probably make sense to not
add comments on that CL but keep the comments here.

Thanks!

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/extension...
File chrome/browser/extensions/extension_message_bubble_controller.cc (right):

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/extension...
chrome/browser/extensions/extension_message_bubble_controller.cc:39: bool
ExtensionMessageBubbleController::HasExtensionList() {
On 2013/12/10 01:21:39, kalman wrote:
> HasExtensionList is confusing given it's actually based on whether the bubble
> should be shown. Perhaps ShouldShow()?

Done.

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/extension...
chrome/browser/extensions/extension_message_bubble_controller.cc:56: return
!extension_list_.empty();
The class already has a GetExtensionList method, but I added a private
FetchExtensionList() which is used internally for lazy initializing.

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/extension...
File chrome/browser/extensions/extension_message_bubble_controller.h (right):

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/extension...
chrome/browser/extensions/extension_message_bubble_controller.h:37: virtual
const ExtensionIdList& GetExtensionIdList() const = 0;
It was used in the last CL but no longer. Removed.

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/extension...
chrome/browser/extensions/extension_message_bubble_controller.h:54: virtual bool
ShouldShowExtensionList() const = 0;
It is used in the view.

The decision has been going back and forth on whether we want to show the full
extension list in the dev mode bubble. Now the word on the street is that we do
not want that, so this is now not always returning false.

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/extension...
File
chrome/browser/extensions/suspicious_extension_bubble_controller_delegate.cc
(right):

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/extension...
chrome/browser/extensions/suspicious_extension_bubble_controller_delegate.cc:61:
controller_->Show(bubble);
These functions are now gone.

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

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:83:
SuspiciousExtensionBubbleControllerDelegate* suspicious_extensions =
The call-through methods are gone. As for the two bubbles showing, I refer to
our in-person chat and the comment above.

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:91: return;
Yes. That is exactly what I intended. See comment at the top of the function.

https://codereview.chromium.org/95133002/diff/230001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:121: break; 
// Found a good candidate.
This is becoming kind of a moot point because the UX designer wants to force the
dev mode extensions to be visible and not in the overflow, so I'll need to rip
this out anyway (in my next changelist). For now, I'd like to land this as-is so
that I can get this changelist out of the way. :)

[not at google - send to devlin, 2013-12-12 18:21:37]

Ok, I understand this change now, thanks for bearing with me. A bunch of
comments but that's all I have.

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

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/extension...
chrome/browser/extensions/dev_mode_bubble_controller.cc:67: const std::string&
extension_id) {
I think generally methods should take Extensions not extension IDs since it
saves a lot of boilerplate in grabbing the ExtensionService, querying it for the
extension ID, then dealing with it not existing.

If this were passed an extension the the whole method would be just "return
IsDevModeExtension(extension)" (in fact that is probably what the funciton
should do anyway).

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/extension...
chrome/browser/extensions/dev_mode_bubble_controller.cc:84:
service_->DisableExtension(list[i], Extension::DISABLE_USER_ACTION);
don't you want to disable the extensions on Chrome startup, not on bubble show?
The bubble show is purely informative?

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/extension...
chrome/browser/extensions/dev_mode_bubble_controller.cc:123: return
GetExtensionList();
calling from the Delegate back into the Controller is an inversion of what a
Delegate is supposed to be; I guess that's another reason not to make this class
extend Controller. See comment in the view.

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

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/extension...
chrome/browser/extensions/dev_mode_bubble_controller.h:24: public
ExtensionMessageBubbleController,
The only reason to extend ExtensionMessageBubbleController here is so that you
can pass it around as a Controller, right? And the only reason you need to do
that is because you need to make sure they only show once per profile, so you
make them singletons on profile?

It would be really nice to implement that some other way, because having this
extend both the Controller *and* its delegate is a pretty weird pattern. It's
also a bit inflexible (basically means that every Delegate needs to be
ProfileKeyed or at least have lifetime tied to something else). This is what I
would hope works:

- Make ExtensionMessageBubbleController take *ownership* of its Delegate
- Make ExtensionMessageBubbleView take *ownership* of its Controller
- To implement profile-singleton-ness implement it per-Delegate, that is, pull
ShouldShow implementation out of the Controller and into the delegate. The
controller's ShouldShow doesn't actually do very much.

Is there some reason that wouldn't work? ShouldShow would be pretty trivial to
implement:

#include "base/lazy_instance.h"
base::LazyInstance<std::set<Profile*> > g_shown_for_profiles =
    LAZY_INSTANCE_INITIALIZER;
bool Delegate::ShouldShow() {
  // both Delegates would have this exact implementation, but future
  // Delegates might not, and that would be flexible.
  return g_shown_for_profiles.Get().count(profile_) && !GetExtensions().empty();
}

I think this would be a pretty mechanical change.

Actually - since GetExtensions() will hopefully go away, if ShouldShow is put on
the Delegate then it won't be able to take that into account. Perhaps ShouldShow
needs to stay on the Controller but you need a second method like
ShouldShowForDelegate or something on these Delegate methods.

Eh, a little more complicated, but even that would be a lot simpler than this
ProfileKeyedAPI approach.

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/extension...
File chrome/browser/extensions/extension_message_bubble_controller.cc (right):

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/extension...
chrome/browser/extensions/extension_message_bubble_controller.cc:94:
delegate_->PerformAction(extension_list_);
see comment below about GetOrCreateExtensionList(); presumably you'd want to use
it here?

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/extension...
chrome/browser/extensions/extension_message_bubble_controller.cc:125: for
(ExtensionIdList::const_iterator it = extension_list_.begin();
and here?

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/extension...
chrome/browser/extensions/extension_message_bubble_controller.cc:130: void
ExtensionMessageBubbleController::FetchExtensionList() {
if this were GetOrCreateExtensionList() which returns the extension list if
initialized, otherwise creates it, then you wouldn't need the "if
(!initialized_)" checks where FetchExtensionList is called from.

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

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:76: void
ExtensionMessageBubbleView::MaybeShow(
This MaybeShow code doesn't really belong here.

ExtensionMessageBubbleView is a non-specific class that knows how to render a
bubble. Having MaybeShow defined here implies that it knows about all types of
instances of itself, and furthermore, how they interact with each other.

I think basically moving this whole function into toolbar_view is a decent
enough way to address this. That would also simplify this method since you
wouldn't need to dig the toolbar view back out of the browser (though for that
matter, you could pass it in here anyway).

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:82: // the
next startup. That way, we're not too spammy with the bubbles.
it's not next startup, it's next time this method is called (since after this
method is called once, suspicious_extensions->ShouldShow() will return false),
and this method might be called from anywhere in theory (unless you move it into
toolbar_view), though in any case, there is a toolbar view constructed per
window I presume so really this is "next time there is a new window"?

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:102:
browser_view->GetToolbarView()->browser_actions();
see comment above; you could at least pass the toolbar view (or the browser
actions container) directly into this method, but I think that moving MaybeShow
would make even more sense here (it could be an instance method on toolbar_view
and you could use "this->browser_actions()" there).

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:112:
dev_mode_extensions->extension_id_list();
can you make dev_mode_extensions return an ExtensionSet rather than a list of
IDs? that would simplify this logic even further; and the less logic in the
views the better.

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:243:
std::vector<string16> suspicious = delegate_->GetExtensions();
use controller->GetExtensionList() here?

oh, you don't hold onto the controller... so maybe just hold onto the controller
and (though it's a bit odd; less odd) have a delegate() method on the controller

so that would change things like:
- delegate_->ShouldShowExtensionList() ->
controller_->delegate()->ShouldShowExtensionList()
- delegate_->GetMessageBody() -> controller_->delegate()->GetMessageBody()

etc.

[Finnur, 2013-12-12 20:25:12]

I've made some of the changes requested (see comments below). For the ones I've
|not| responded to, those are structural changes that I'd like to address in a
followup changelist. Looking the timeline, if I don't get this in today then
this is at risk for slipping. I need some time to test and potentially polish
(by EOD tomorrow) because the schedule for the weekend (with the party, packing
and my flight) doesn't give me much chance to work on this over the weekend.

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

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/extension...
chrome/browser/extensions/dev_mode_bubble_controller.cc:67: const std::string&
extension_id) {
Generally, yes. However, in this case, we want the implementors to decide for
themselves whether to include disabled extensions when they pull them from the
service. If the base class decides, it will have to pull disabled ones, because
the SuspiciousExtension bubble only cares about those. But the dev mode bubble
has no reason to highlight disabled extensions, so we'd have to replace the
boilerplate code for pulling the extension with boilerplate code to check if the
extension is disabled. In other words, I don't think we get much benefit.

However... more worryingly is that the next person who comes in and implements a
new bubble might not realize they are getting passed an extension* to a disabled
extension so I worry about bugs creeping in.

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/extension...
chrome/browser/extensions/dev_mode_bubble_controller.cc:84:
service_->DisableExtension(list[i], Extension::DISABLE_USER_ACTION);
Not the DevMode bubble. It is meant to highlight dev mode extensions and ask the
user what to do with them. There's a disable button in that bubble, which
corresponds to this action.

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/extension...
File chrome/browser/extensions/extension_message_bubble_controller.cc (right):

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/extension...
chrome/browser/extensions/extension_message_bubble_controller.cc:94:
delegate_->PerformAction(extension_list_);
On 2013/12/12 18:21:38, kalman wrote:
> see comment below about GetOrCreateExtensionList(); presumably you'd want to
use
> it here?

Done.

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/extension...
chrome/browser/extensions/extension_message_bubble_controller.cc:125: for
(ExtensionIdList::const_iterator it = extension_list_.begin();
On 2013/12/12 18:21:38, kalman wrote:
> and here?

Done.

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/extension...
chrome/browser/extensions/extension_message_bubble_controller.cc:130: void
ExtensionMessageBubbleController::FetchExtensionList() {
On 2013/12/12 18:21:38, kalman wrote:
> if this were GetOrCreateExtensionList() which returns the extension list if
> initialized, otherwise creates it, then you wouldn't need the "if
> (!initialized_)" checks where FetchExtensionList is called from.

Done.

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

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:76: void
ExtensionMessageBubbleView::MaybeShow(
This logic doesn't belong in either place really, it belongs in some framework
that controls the priority of the bubbles and makes sure we show them in an
orderly fashion. A framework we currently need but don't have.

The toolbar view is a convenient hook to show the bubbles, but I think the
toolbar view folks would hold the opposite view (that the logic of what bubble
is shown belongs to the bubble code and not the toolbar).

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:82: // the
next startup. That way, we're not too spammy with the bubbles.
Yeah, it is next startup/next window. Updated comment to reflect.

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:102:
browser_view->GetToolbarView()->browser_actions();
Added toolbar_view param.

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:112:
dev_mode_extensions->extension_id_list();
Same argument applies here as with ShouldShowExtension(Extension*/Extension
ID)). We'd have to pass disabled extensions as well, which could lead to subtle
bugs.

[not at google - send to devlin, 2013-12-12 21:17:18]

lgtm, thanks for offering to do a follow-up.

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

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/extension...
chrome/browser/extensions/dev_mode_bubble_controller.cc:67: const std::string&
extension_id) {
On 2013/12/12 20:25:12, Finnur wrote:
> Generally, yes. However, in this case, we want the implementors to decide for
> themselves whether to include disabled extensions when they pull them from the
> service. If the base class decides, it will have to pull disabled ones,
because
> the SuspiciousExtension bubble only cares about those. But the dev mode bubble
> has no reason to highlight disabled extensions, so we'd have to replace the
> boilerplate code for pulling the extension with boilerplate code to check if
the
> extension is disabled. In other words, I don't think we get much benefit.
> 
> However... more worryingly is that the next person who comes in and implements
a
> new bubble might not realize they are getting passed an extension* to a
disabled
> extension so I worry about bugs creeping in.

Ok that makes sense.

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/extension...
File chrome/browser/extensions/extension_message_bubble_controller.h (right):

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/extension...
chrome/browser/extensions/extension_message_bubble_controller.h:69: const
ExtensionIdList& extension_id_list() const {
having this method and GetExtensionList is confusing, particuarly since this one
only works under certain circumstances. Jiust have GetExtensionList.

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

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:76: void
ExtensionMessageBubbleView::MaybeShow(
On 2013/12/12 20:25:12, Finnur wrote:
> This logic doesn't belong in either place really, it belongs in some framework
> that controls the priority of the bubbles and makes sure we show them in an
> orderly fashion. A framework we currently need but don't have.
> 
> The toolbar view is a convenient hook to show the bubbles, but I think the
> toolbar view folks would hold the opposite view (that the logic of what bubble
> is shown belongs to the bubble code and not the toolbar).

Only the toolbar view knows that the "suspicious bubble" and the "dev mode
bubble" should be shown from it. In other cases maybe it's the hotdog menu, or
an infobar, or the back button, or the omnibox, so in terms of *constructing*
the view it should be the toolbar doing that, not here. Reusable views like this
shouldn't care where they're attached.

However, you're right that the actual logic to decide when to show toolbars
doesn't belong in either place, insofar as it's not a view concern. A sensible
way to implement this would probably be to make the Controllers aware of each
other in some way, so that the dev mode controller returns false from ShouldShow
if the suspicious bubble should show.

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:112:
dev_mode_extensions->extension_id_list();
On 2013/12/12 20:25:12, Finnur wrote:
> Same argument applies here as with ShouldShowExtension(Extension*/Extension
> ID)). We'd have to pass disabled extensions as well, which could lead to
subtle
> bugs.

Makes sense. You could include the Extension::State of the extension, and I
think that'd be nicer in this case, but still a little bit arbitrary.

See comment about the extension_id_list() function though. I didn't make it
before because I was hoping the method would entirely change, but just call
GetExtensionList() here.

https://codereview.chromium.org/95133002/diff/340026/chrome/browser/extension...
File chrome/browser/extensions/extension_message_bubble_controller.cc (right):

https://codereview.chromium.org/95133002/diff/340026/chrome/browser/extension...
chrome/browser/extensions/extension_message_bubble_controller.cc:45: return
!list->empty();
this method can be shorter if you do the has_notified assign first, then "return
!GetOrCreateExtensionList()->empty();" directly

[Finnur, 2013-12-13 21:29:48]

Thanks for the feedback. I'll follow this up with another CL to address some of
the structural changes and please note that I'm closing this issue and checking
it in in another CL (https://codereview.chromium.org/114153003/) as --similarity
changes always seem to prevent my CLs from applying cleanly on the bots).

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/extension...
File chrome/browser/extensions/extension_message_bubble_controller.h (right):

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/extension...
chrome/browser/extensions/extension_message_bubble_controller.h:69: const
ExtensionIdList& extension_id_list() const {
GetExtensionList is a convenience method to get at the names of the extensions.
This function, however, gives you the IDs. If I remove this function I can't get
at the extensions (I'd only have their name) so I have to have an ID function. I
considered removing the other one (GetExtensionList) but it encapsulates logic
and boiler plate that we'd have to sprinkle around at the call sites instead...

So, I ended up moving this function into the .cc file and making sure it always
works.

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

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:76: void
ExtensionMessageBubbleView::MaybeShow(
I thought about that, but this gets messy as you add more bubbles. If we make
the controllers aware of each other then you need to update n code sites in code
you don't care about every time you add bubble n+1.

Bottom line: I think we can't fix this in a way everyone will be happy with
until we have some outside framework that controls the bubble priority and the
orderly showing of them.

https://codereview.chromium.org/95133002/diff/480001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:112:
dev_mode_extensions->extension_id_list();
GetExtensionList returns names, not IDs. Changed it to GetExtensionIdList.

https://codereview.chromium.org/95133002/diff/340026/chrome/browser/extension...
File chrome/browser/extensions/extension_message_bubble_controller.cc (right):

https://codereview.chromium.org/95133002/diff/340026/chrome/browser/extension...
chrome/browser/extensions/extension_message_bubble_controller.cc:45: return
!list->empty();
Good point.