New post-sideload UI:

New post-sideload UI:
- External extensions are installed initially disabled.
- Global error appears in menu, which will show an install dialog when clicked.

TBR=sky@chromium.org
BUG=137469, 131510
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162380

[Matt Perry, 2012-10-13 00:33:18]

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_install_prompt.cc (right):

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_install_prompt.cc:453: show_dialog_callback_
=  // @@@MP
I'll remove this once http://codereview.chromium.org/11131002/ lands.

[Matt Perry, 2012-10-13 00:34:33]

I'm still working on a browsertest, but wanted to get this out sooner due to
time pressure.

[Jeffrey Yasskin, 2012-10-13 21:17:34]

Didn't get all the way through, but here are some initial comments.

http://codereview.chromium.org/11150002/diff/18/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/11150002/diff/18/chrome/app/generated_resource...
chrome/app/generated_resources.grd:4622: Add "<ph
name="EXTENSION_NAME">$1<ex>Gmail Checker</ex></ph>"?
It's interesting that the extension's name appears in the opposite one of title
vs. heading for normal vs external installs.

http://codereview.chromium.org/11150002/diff/18/chrome/app/generated_resource...
chrome/app/generated_resources.grd:5325: New extension added (<ph
name="EXTENSION_NAME">$1<ex>Babylon Toolbar</ex></ph>)
It would be nice to present a count instead of a single name if multiple
extensions were installed.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_install_prompt.cc (right):

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_install_prompt.cc:96: 0,  //
TODO(mpcomplete): Do we need this for external install UI?
I'd assume external extensions can ask for oauth permissions just like
user-installed ones, and that we should warn just the same, but I can't find the
docs on user-installed oauth permission requests, so I can't tell for sure.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_install_prompt.cc:456: LoadImageIfNeeded();
Wow, that's a subtle name for the "go show the dialog" function.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_install_prompt.cc:522: if (!icon_.empty()) {
Not related to this CL, but !icon_.empty() seems to be the opposite condition
from the one mentioned by the comment on the previous line.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_install_prompt.cc:545: prompt_type_ ==
INLINE_INSTALL_PROMPT ||
Do you need to include EXTERNAL_INSTALL_PROMPT in this list if you decide not to
show oauth permissions?

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_install_prompt.h (right):

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_install_prompt.h:237: virtual void
ConfirmExternalInstall(Delegate* delegate,
Comment this function. It might make sense to refactor the comments for the
Confirm*() functions so they have less duplicated text, after which this
function might not need its own comment.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:1830: if (!extension) {
extension is definitely NULL here.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:1849: //
UpdateExternalExtensionAlert may get called multiple times). But that
Won't it get called for different extensions in that case?

[Jeffrey Yasskin, 2012-10-15 00:43:59]

The rest of my comments:

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:857: void
ExtensionService::EnableExtension(const std::string& extension_id) {
Is there a comment somewhere saying that this function is called in response to
user actions instead of some other way of enabling extensions?

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:2200:
system_->management_policy()->MustRemainEnabled(extension, NULL);
Seems odd to override the management policy with the external install check. If
that's correct, could you update the comment to cover the combination of
conditions?

Would it make sense to extract the |initial_enable| calculation into a helper
function?

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
File chrome/browser/extensions/external_install_ui.cc (right):

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/external_install_ui.cc:44: public
base::RefCountedThreadSafe<ExternalInstallDialogDelegate> {
Please don't use refcounting to manage the lifetime of objects unless it's
absolutely necessary, and if it is, comment why so we'll know when we can remove
it.

In this case, I think the object is singly-owned by a sequence of callbacks in
the ExtensionInstallPrompt. If you give the object a "go" method that takes a
completion callback that both InstallUIProceed() and InstallUIAbort() call, you
can have ShowExternalInstallDialog() pass in a "delete the object" callback, and
put the allocation information all in one place. Worst case, you can keep the
self-owning semantics and have Proceed() and Abort() run `delete this;`.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/external_install_ui.cc:140: // TODO(yoz): create error
at startup for disabled extensions.
Is this comment correct for this file?

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/external_install_ui.cc:159: return true;
Since this is a .cc file, I'd probably inline all of the 1-line methods.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/external_install_ui.cc:238: user_response_ = REENABLE;
Naively, this makes it look like if the external system removes and recreates
the extension, it'll become acknowledged? Comment why this doesn't get around
the user-notification system.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/external_install_ui.cc:247: // Globals
--------------------------------------------------------------------
Do you mean "public interface"?

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
File chrome/browser/extensions/external_install_ui.h (right):

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/external_install_ui.h:15: bool
AddExternalInstallError(ExtensionService* service,
Please document these functions.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/pen...
File chrome/browser/extensions/pending_extension_manager.cc (right):

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/pen...
chrome/browser/extensions/pending_extension_manager.cc:117: const std::string&
id,
Revert this file?

[Matt Perry, 2012-10-15 19:13:38]

http://codereview.chromium.org/11150002/diff/18/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/11150002/diff/18/chrome/app/generated_resource...
chrome/app/generated_resources.grd:4622: Add "<ph
name="EXTENSION_NAME">$1<ex>Gmail Checker</ex></ph>"?
On 2012/10/13 21:17:35, Jeffrey Yasskin wrote:
> It's interesting that the extension's name appears in the opposite one of
title
> vs. heading for normal vs external installs.

Agreed. I'm just copying the spec, which I've now shared with you.

http://codereview.chromium.org/11150002/diff/18/chrome/app/generated_resource...
chrome/app/generated_resources.grd:5325: New extension added (<ph
name="EXTENSION_NAME">$1<ex>Babylon Toolbar</ex></ph>)
On 2012/10/13 21:17:35, Jeffrey Yasskin wrote:
> It would be nice to present a count instead of a single name if multiple
> extensions were installed.

That should be a pretty occurrence. When it does happen, we just queue them up
and show them one at a time.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_install_prompt.cc (right):

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_install_prompt.cc:545: prompt_type_ ==
INLINE_INSTALL_PROMPT ||
On 2012/10/13 21:17:35, Jeffrey Yasskin wrote:
> Do you need to include EXTERNAL_INSTALL_PROMPT in this list if you decide not
to
> show oauth permissions?

Good catch. Done.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_install_prompt.h (right):

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_install_prompt.h:237: virtual void
ConfirmExternalInstall(Delegate* delegate,
On 2012/10/13 21:17:35, Jeffrey Yasskin wrote:
> Comment this function. It might make sense to refactor the comments for the
> Confirm*() functions so they have less duplicated text, after which this
> function might not need its own comment.

Done.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:857: void
ExtensionService::EnableExtension(const std::string& extension_id) {
On 2012/10/15 00:43:59, Jeffrey Yasskin wrote:
> Is there a comment somewhere saying that this function is called in response
to
> user actions instead of some other way of enabling extensions?

I'm not sure what you're asking. EnableExtension is called from a variety of
places, whenever we need to enable the extension.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:1830: if (!extension) {
On 2012/10/13 21:17:35, Jeffrey Yasskin wrote:
> extension is definitely NULL here.

You can never be too sure... :)

Fixed.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:1849: //
UpdateExternalExtensionAlert may get called multiple times). But that
On 2012/10/13 21:17:35, Jeffrey Yasskin wrote:
> Won't it get called for different extensions in that case?

Not necessarily. This method is called both on startup (1) and each time an
install finishes (2). I can't remember the details now, but there was a
particular order that resulted in this getting called twice: once when only
extensionA is installed, then again with extensionA+extensionB. If we end up
choosing to display extensionB the second time, then the first call for
extensionA was an overcount.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:2200:
system_->management_policy()->MustRemainEnabled(extension, NULL);
On 2012/10/15 00:43:59, Jeffrey Yasskin wrote:
> Seems odd to override the management policy with the external install check.
If
> that's correct, could you update the comment to cover the combination of
> conditions?

You're right - management policy should win out here.

> Would it make sense to extract the |initial_enable| calculation into a helper
> function?

Done.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
File chrome/browser/extensions/external_install_ui.cc (right):

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/external_install_ui.cc:44: public
base::RefCountedThreadSafe<ExternalInstallDialogDelegate> {
On 2012/10/15 00:43:59, Jeffrey Yasskin wrote:
> Please don't use refcounting to manage the lifetime of objects unless it's
> absolutely necessary, and if it is, comment why so we'll know when we can
remove
> it.
> 
> In this case, I think the object is singly-owned by a sequence of callbacks in
> the ExtensionInstallPrompt. If you give the object a "go" method that takes a
> completion callback that both InstallUIProceed() and InstallUIAbort() call,
you
> can have ShowExternalInstallDialog() pass in a "delete the object" callback,
and
> put the allocation information all in one place. Worst case, you can keep the
> self-owning semantics and have Proceed() and Abort() run `delete this;`.

This file is a copy+paste of extension_disabled_ui.cc with modifications. (It's
too bad it didn't show up as a file move in rietveld). Do you mind if I leave
this as a TODO, as I plan on going back and merging these two files again?

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/external_install_ui.cc:140: // TODO(yoz): create error
at startup for disabled extensions.
On 2012/10/15 00:43:59, Jeffrey Yasskin wrote:
> Is this comment correct for this file?

Oops, removed.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/external_install_ui.cc:238: user_response_ = REENABLE;
On 2012/10/15 00:43:59, Jeffrey Yasskin wrote:
> Naively, this makes it look like if the external system removes and recreates
> the extension, it'll become acknowledged? Comment why this doesn't get around
> the user-notification system.

Done.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/external_install_ui.cc:247: // Globals
--------------------------------------------------------------------
On 2012/10/15 00:43:59, Jeffrey Yasskin wrote:
> Do you mean "public interface"?

Done.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
File chrome/browser/extensions/external_install_ui.h (right):

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/external_install_ui.h:15: bool
AddExternalInstallError(ExtensionService* service,
On 2012/10/15 00:43:59, Jeffrey Yasskin wrote:
> Please document these functions.

Done.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/ext...
chrome/browser/extensions/external_install_ui.h:20: void
ShowExternalInstallDialog(ExtensionService* service,
I made this a static function in the .cc, since it doesn't need to be public.

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/pen...
File chrome/browser/extensions/pending_extension_manager.cc (right):

http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/pen...
chrome/browser/extensions/pending_extension_manager.cc:117: const std::string&
id,
On 2012/10/15 00:43:59, Jeffrey Yasskin wrote:
> Revert this file?

Done.

[Matt Perry, 2012-10-16 21:09:50]

-jyasskin
+yoz

Latest version wraps the feature in a command-line flag. Also, I've fixed some
broken tests (by disabling the flag) and added/changed a couple unit tests to
test the feature.

[Matt Perry, 2012-10-16 21:15:31]

sail: can you please review global_error/*

[sail, 2012-10-16 21:26:32]

http://codereview.chromium.org/11150002/diff/15001/chrome/browser/ui/global_e...
File chrome/browser/ui/global_error/global_error.h (right):

http://codereview.chromium.org/11150002/diff/15001/chrome/browser/ui/global_e...
chrome/browser/ui/global_error/global_error.h:23: NUM_SEVERITY,
It seems like this isn't used anywhere. Can we remove it?
In general, it's better not to have this. This allows us to have switch
statements without having to add a NOTREACHED() clause.

[Matt Perry, 2012-10-16 21:36:31]

http://codereview.chromium.org/11150002/diff/15001/chrome/browser/ui/global_e...
File chrome/browser/ui/global_error/global_error.h (right):

http://codereview.chromium.org/11150002/diff/15001/chrome/browser/ui/global_e...
chrome/browser/ui/global_error/global_error.h:23: NUM_SEVERITY,
On 2012/10/16 21:26:32, sail wrote:
> It seems like this isn't used anywhere. Can we remove it?
> In general, it's better not to have this. This allows us to have switch
> statements without having to add a NOTREACHED() clause.

Done.

[sail, 2012-10-16 21:42:58]

global_error/* LGTM

[Yoyo Zhou, 2012-10-16 23:28:05]

First pass of comments - I still have tests to look at.

http://codereview.chromium.org/11150002/diff/18/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/11150002/diff/18/chrome/app/generated_resource...
chrome/app/generated_resources.grd:5325: New extension added (<ph
name="EXTENSION_NAME">$1<ex>Babylon Toolbar</ex></ph>)
On 2012/10/15 19:13:38, Matt Perry wrote:
> On 2012/10/13 21:17:35, Jeffrey Yasskin wrote:
> > It would be nice to present a count instead of a single name if multiple
> > extensions were installed.
> 
> That should be a pretty occurrence. When it does happen, we just queue them up
> and show them one at a time.

If this is

http://codereview.chromium.org/11150002/diff/15001/chrome/app/chrome_command_...
File chrome/app/chrome_command_ids.h (right):

http://codereview.chromium.org/11150002/diff/15001/chrome/app/chrome_command_...
chrome/app/chrome_command_ids.h:189: #define IDC_EXTERNAL_EXTENSION_ALERT   
40035
This should not be here - it's in the middle of a FIRST...LAST range of menu
IDs. (DEV_TOOLS_TOGGLE is also in a wrong place.) It should come after the last
one defined here.

http://codereview.chromium.org/11150002/diff/15001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/11150002/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:1848: if
(extension_prefs_->IncrementAcknowledgePromptCount(extension->id()) >=
Seems like this could be done when the error is shown.

http://codereview.chromium.org/11150002/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:2733: if
(extensions::FeatureSwitch::external_extension_prompt()->IsEnabled()) {
What do you think of making this switch still available on ChromeOS, just
disabled by default? Or is that pointless?

http://codereview.chromium.org/11150002/diff/15001/chrome/common/chrome_switc...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/11150002/diff/15001/chrome/common/chrome_switc...
chrome/common/chrome_switches.cc:701: const char kExternalExtensionPrompt[]     
 = "external-extension-prompt";
naming nit: I would like prompt-for-external-extensions better.

http://codereview.chromium.org/11150002/diff/15003/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/11150002/diff/15003/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:1844: // Note: this can sometimes
overcount if there are multiple external
This would only overcount if AddExternalInstallError succeeds again after the
first time. See comment in AddExternalInstallError.

If you do make this change, then the logic for calling
RemoveExternalInstallError would change too. But actually, I think it's wrong
right now: suppose you get UpdateExternalExtensionAlert called twice at startup,
and the extension you find is the same; then the first time you'd add the error,
and the second time you'd remove it.

http://codereview.chromium.org/11150002/diff/15003/chrome/browser/extensions/...
File chrome/browser/extensions/external_install_ui.cc (right):

http://codereview.chromium.org/11150002/diff/15003/chrome/browser/extensions/...
chrome/browser/extensions/external_install_ui.cc:180: return kMenuCommandId;
nit: could be inlined.

http://codereview.chromium.org/11150002/diff/15003/chrome/browser/extensions/...
chrome/browser/extensions/external_install_ui.cc:269: return false;  // already
showing
Would you want this if the error was already showing for any extension? As per
the comment on extension_service.cc, if you returned false as long as any error
was showing, you wouldn't even overcount. And it's not clear that there's any
benefit to potentially changing which extension is being alerted about.

[Matt Perry, 2012-10-17 00:02:44]

http://codereview.chromium.org/11150002/diff/15001/chrome/app/chrome_command_...
File chrome/app/chrome_command_ids.h (right):

http://codereview.chromium.org/11150002/diff/15001/chrome/app/chrome_command_...
chrome/app/chrome_command_ids.h:189: #define IDC_EXTERNAL_EXTENSION_ALERT   
40035
On 2012/10/16 23:28:05, Yoyo Zhou wrote:
> This should not be here - it's in the middle of a FIRST...LAST range of menu
> IDs. (DEV_TOOLS_TOGGLE is also in a wrong place.) It should come after the
last
> one defined here.

Done.

http://codereview.chromium.org/11150002/diff/15001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/11150002/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:1848: if
(extension_prefs_->IncrementAcknowledgePromptCount(extension->id()) >=
On 2012/10/16 23:28:05, Yoyo Zhou wrote:
> Seems like this could be done when the error is shown.

Ignored as discussed in person.

http://codereview.chromium.org/11150002/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:2733: if
(extensions::FeatureSwitch::external_extension_prompt()->IsEnabled()) {
On 2012/10/16 23:28:05, Yoyo Zhou wrote:
> What do you think of making this switch still available on ChromeOS, just
> disabled by default? Or is that pointless?

I thought about that, but IMO this switch should be temporary. It'll be easier
to rip out the switch if we leave the ChromeOS check separate.

http://codereview.chromium.org/11150002/diff/15001/chrome/common/chrome_switc...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/11150002/diff/15001/chrome/common/chrome_switc...
chrome/common/chrome_switches.cc:701: const char kExternalExtensionPrompt[]     
 = "external-extension-prompt";
On 2012/10/16 23:28:05, Yoyo Zhou wrote:
> naming nit: I would like prompt-for-external-extensions better.

Done.

http://codereview.chromium.org/11150002/diff/15003/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/11150002/diff/15003/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:1844: // Note: this can sometimes
overcount if there are multiple external
On 2012/10/16 23:28:05, Yoyo Zhou wrote:
> This would only overcount if AddExternalInstallError succeeds again after the
> first time. See comment in AddExternalInstallError.
> 
> If you do make this change, then the logic for calling
> RemoveExternalInstallError would change too. But actually, I think it's wrong
> right now: suppose you get UpdateExternalExtensionAlert called twice at
startup,
> and the extension you find is the same; then the first time you'd add the
error,
> and the second time you'd remove it.

The "else RemoveExternalInstallError" clause runs only if extension is NULL.
(It's an else for the outer if, not the inner one).

http://codereview.chromium.org/11150002/diff/15003/chrome/browser/extensions/...
File chrome/browser/extensions/external_install_ui.cc (right):

http://codereview.chromium.org/11150002/diff/15003/chrome/browser/extensions/...
chrome/browser/extensions/external_install_ui.cc:269: return false;  // already
showing
On 2012/10/16 23:28:05, Yoyo Zhou wrote:
> Would you want this if the error was already showing for any extension? As per
> the comment on extension_service.cc, if you returned false as long as any
error
> was showing, you wouldn't even overcount. And it's not clear that there's any
> benefit to potentially changing which extension is being alerted about.

You're right. I had myself convinced that we needed this in order to switch from
one error to the next, but I can just add a RemoveGlobalError in Observe()
above. Fixed.

[Yoyo Zhou, 2012-10-17 00:23:55]

LGTM, only nits

http://codereview.chromium.org/11150002/diff/15003/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/11150002/diff/15003/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:1844: // Note: this can sometimes
overcount if there are multiple external
On 2012/10/17 00:02:44, Matt Perry wrote:
> On 2012/10/16 23:28:05, Yoyo Zhou wrote:
> > This would only overcount if AddExternalInstallError succeeds again after
the
> > first time. See comment in AddExternalInstallError.
> > 
> > If you do make this change, then the logic for calling
> > RemoveExternalInstallError would change too. But actually, I think it's
wrong
> > right now: suppose you get UpdateExternalExtensionAlert called twice at
> startup,
> > and the extension you find is the same; then the first time you'd add the
> error,
> > and the second time you'd remove it.
> 
> The "else RemoveExternalInstallError" clause runs only if extension is NULL.
> (It's an else for the outer if, not the inner one).

Ah, misread.

http://codereview.chromium.org/11150002/diff/26002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/11150002/diff/26002/chrome/browser/extensions/...
chrome/browser/extensions/extension_service_unittest.cc:5579: // Should return
false, meaning there aren't any extensions that the user
comment belongs 1 line lower

http://codereview.chromium.org/11150002/diff/26002/chrome/browser/extensions/...
File chrome/browser/extensions/external_install_ui.cc (right):

http://codereview.chromium.org/11150002/diff/26002/chrome/browser/extensions/...
chrome/browser/extensions/external_install_ui.cc:257: // This call deletes us
(via Add/RemoveExternalInstallError).
This comment seems to be no longer accurate.

[Matt Perry, 2012-10-17 00:35:20]

http://codereview.chromium.org/11150002/diff/26002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/11150002/diff/26002/chrome/browser/extensions/...
chrome/browser/extensions/extension_service_unittest.cc:5579: // Should return
false, meaning there aren't any extensions that the user
On 2012/10/17 00:23:55, Yoyo Zhou wrote:
> comment belongs 1 line lower

Done.

http://codereview.chromium.org/11150002/diff/26002/chrome/browser/extensions/...
File chrome/browser/extensions/external_install_ui.cc (right):

http://codereview.chromium.org/11150002/diff/26002/chrome/browser/extensions/...
chrome/browser/extensions/external_install_ui.cc:257: // This call deletes us
(via Add/RemoveExternalInstallError).
On 2012/10/17 00:23:55, Yoyo Zhou wrote:
> This comment seems to be no longer accurate.

Done.

[Matt Perry, 2012-10-17 00:36:24]

sky: OWNERS review for chrome/browser/ui

[commit-bot: I haz the power, 2012-10-17 01:05:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpcomplete@chromium.org/11150002/27002

[Jeffrey Yasskin, 2012-10-17 07:08:55]

http://codereview.chromium.org/11150002/diff/15001/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/11150002/diff/15001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:5322: New extension added (<ph
name="EXTENSION_NAME">$1<ex>Babylon Toolbar</ex></ph>)
On 2012/10/15 19:13:38, Matt Perry wrote:
> On 2012/10/13 21:17:35, Jeffrey Yasskin wrote:
> > It would be nice to present a count instead of a single name if multiple
> > extensions were installed.
> 
> That should be a pretty occurrence. When it does happen, we just queue them up
> and show them one at a time.

Should be rare, and it's certainly fair to change the spelling in a subsequent
change if the UI folks agree with me.

http://codereview.chromium.org/11150002/diff/15001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/11150002/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:858: void
ExtensionService::EnableExtension(const std::string& extension_id) {
On 2012/10/15 19:13:38, Matt Perry wrote:
> On 2012/10/15 00:43:59, Jeffrey Yasskin wrote:
> > Is there a comment somewhere saying that this function is called in response
> to
> > user actions instead of some other way of enabling extensions?
> 
> I'm not sure what you're asking. EnableExtension is called from a variety of
> places, whenever we need to enable the extension.

I'm pointing out that this will definitely call AcknowledgeExternalExtension(),
so if EnableExtension is called in cases where the user wasn't asked, it's going
to incorrectly save that the user has agreed to run the extension. It would be
nice to have some comment that this can't happen.

http://codereview.chromium.org/11150002/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:1846: //
UpdateExternalExtensionAlert may get called multiple times). But that
On 2012/10/15 19:13:38, Matt Perry wrote:
> On 2012/10/13 21:17:35, Jeffrey Yasskin wrote:
> > Won't it get called for different extensions in that case?
> 
> Not necessarily. This method is called both on startup (1) and each time an
> install finishes (2). I can't remember the details now, but there was a
> particular order that resulted in this getting called twice: once when only
> extensionA is installed, then again with extensionA+extensionB. If we end up
> choosing to display extensionB the second time, then the first call for
> extensionA was an overcount.

Since this only increments the count for the extension it chooses to install,
I'm not seeing how it could overcount. Could you figure that out, and include it
in the comment, or weaken the comment to just say that we don't care about any
possible overcount, but not that there definitely is one?

http://codereview.chromium.org/11150002/diff/15001/chrome/browser/extensions/...
File chrome/browser/extensions/external_install_ui.cc (right):

http://codereview.chromium.org/11150002/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/external_install_ui.cc:47: public
base::RefCountedThreadSafe<ExternalInstallDialogDelegate> {
On 2012/10/15 19:13:38, Matt Perry wrote:
> On 2012/10/15 00:43:59, Jeffrey Yasskin wrote:
> > Please don't use refcounting to manage the lifetime of objects unless it's
> > absolutely necessary, and if it is, comment why so we'll know when we can
> remove
> > it.
> > 
> > In this case, I think the object is singly-owned by a sequence of callbacks
in
> > the ExtensionInstallPrompt. If you give the object a "go" method that takes
a
> > completion callback that both InstallUIProceed() and InstallUIAbort() call,
> you
> > can have ShowExternalInstallDialog() pass in a "delete the object" callback,
> and
> > put the allocation information all in one place. Worst case, you can keep
the
> > self-owning semantics and have Proceed() and Abort() run `delete this;`.
> 
> This file is a copy+paste of extension_disabled_ui.cc with modifications.
(It's
> too bad it didn't show up as a file move in rietveld). Do you mind if I leave
> this as a TODO, as I plan on going back and merging these two files again?

Ah, ok.

[Jeffrey Yasskin, 2012-10-17 07:10:09]

But LGTM from me too. The comment fixes aren't that important.

[commit-bot: I haz the power, 2012-10-17 11:16:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpcomplete@chromium.org/11150002/27002

[commit-bot: I haz the power, 2012-10-17 13:18:06]

Change committed as 162380

[sky, 2012-10-17 14:11:18]

LGTM

[Matt Perry, 2012-10-17 18:38:17]

http://codereview.chromium.org/11150002/diff/15001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/11150002/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:1846: //
UpdateExternalExtensionAlert may get called multiple times). But that
On 2012/10/17 07:08:56, Jeffrey Yasskin wrote:
> On 2012/10/15 19:13:38, Matt Perry wrote:
> > On 2012/10/13 21:17:35, Jeffrey Yasskin wrote:
> > > Won't it get called for different extensions in that case?
> > 
> > Not necessarily. This method is called both on startup (1) and each time an
> > install finishes (2). I can't remember the details now, but there was a
> > particular order that resulted in this getting called twice: once when only
> > extensionA is installed, then again with extensionA+extensionB. If we end up
> > choosing to display extensionB the second time, then the first call for
> > extensionA was an overcount.
> 
> Since this only increments the count for the extension it chooses to install,
> I'm not seeing how it could overcount. Could you figure that out, and include
it
> in the comment, or weaken the comment to just say that we don't care about any
> possible overcount, but not that there definitely is one?

FYI I removed this comment entirely after restructuring the code so that this
doesn't overcount anymore.