Improve messaging for shared modules on chrome://extensions

Improve messaging for shared modules on chrome://extensions

List the dependent extensions under the shared module and change message
from generic policy message to be more specific for shared modules
to explain why the extension cannot be disabled or uninstalled.

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

[elijahtaylor1, 2014-07-17 18:41:33]

PTAL

Please see private issue crbug.com/394837 for screenshots of this implementation
(private because of extensions shown).

Is there any useful testing I should write for the changes to
chrome://extensions?  I verified I didn't create any JS errors/exceptions but
didn't see an obvious testing framework for this code.

[not at google - send to devlin, 2014-07-17 19:26:49]

Antony I'll assume you're taking this?

[asargent_no_longer_on_chrome, 2014-07-17 19:40:07]

On 2014/07/17 19:26:49, kalman wrote:
> Antony I'll assume you're taking this?

Yep, I'll take it

[asargent_no_longer_on_chrome, 2014-07-17 19:40:35]

lgtm w/ a question/comment/suggestion

https://codereview.chromium.org/403593002/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/403593002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:10208: +          Installed because of
dependent extension(s).
I think this might be a little hard for an average user to understand. Would
something like this be any better?

"Installed because other extensions require it (turn on developer mode to see
which ones)"


We might want to get Sriram's opinion, and/or brainstorm with someone on the UI
team like ainsle@. One thing we might consider is instead of only showing the
details in developer mode, instead showing the details in a tooltip on hover or
click or something, like: "Provides shared resources to other extensions.
<link>which ones?</link>" where hovering or clicking on the "which ones" text
will show you the names (not just ids) of the dependent extensions. 

Anyhow, it's probably better to get *something* in though, so I would be fine
with an imperfect version going in for now which we can update as we get better
ideas.

[elijahtaylor1, 2014-07-17 20:30:05]

@kalman: I think I will still need OWNERS approval from you for
chrome/browser/resource/extensions if you could take a look.

https://codereview.chromium.org/403593002/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/403593002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:10208: +          Installed because of
dependent extension(s).
After we chatted offline, I changed the behavior to now show the dependency list
always.  There are probably better UI treatments we should pursue in the future,
particularly to hide the full list (which eventually could grow quite large)
behind a user action.

On 2014/07/17 19:40:35, Antony Sargent wrote:
> I think this might be a little hard for an average user to understand. Would
> something like this be any better?
> 
> "Installed because other extensions require it (turn on developer mode to see
> which ones)"
> 
> 
> We might want to get Sriram's opinion, and/or brainstorm with someone on the
UI
> team like ainsle@. One thing we might consider is instead of only showing the
> details in developer mode, instead showing the details in a tooltip on hover
or
> click or something, like: "Provides shared resources to other extensions.
> <link>which ones?</link>" where hovering or clicking on the "which ones" text
> will show you the names (not just ids) of the dependent extensions. 
> 
> Anyhow, it's probably better to get *something* in though, so I would be fine
> with an imperfect version going in for now which we can update as we get
better
> ideas. 
>

[not at google - send to devlin, 2014-07-17 20:44:53]

ok

https://codereview.chromium.org/403593002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/403593002/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:316: if
(extension.is_shared_module && extension.dependents.length > 0) {
seems like this should be isSharedModule not is_shared_module

and similar question re the extension.dependents.length check. it seems very
similar to this "managed" concept but they're treated differently? like you
shouldn't be able to uninstall a shared module that has any dependent
extensions.

https://codereview.chromium.org/403593002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/403593002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:233:
extension->is_shared_module() && extension->from_webstore();
why does being from the webstore make a difference?

and why this term 'managed'? I thought that shared modules were basically always
managed, and the term 'managed' here is actually confusing because it usually
refers to policy.

https://codereview.chromium.org/403593002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:276:
scoped_ptr<ExtensionSet> dependents =
nit: can we call this 'dependentExtensions' not 'dependents' because
'dependents' is getting me all confused that it sounds like 'dependence' which
is the opposite way around.

(same for the extension data property)

[elijahtaylor1, 2014-07-17 21:45:30]

Thanks for the review, PTAL!

https://codereview.chromium.org/403593002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/403593002/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:316: if
(extension.is_shared_module && extension.dependents.length > 0) {
On 2014/07/17 20:44:52, kalman wrote:
> seems like this should be isSharedModule not is_shared_module
> 
> and similar question re the extension.dependents.length check. it seems very
> similar to this "managed" concept but they're treated differently? like you
> shouldn't be able to uninstall a shared module that has any dependent
> extensions.

Hmm, this makes a lot of sense.  We should never have a shared module in the
list that doesn't have dependent extensions if it was CWS installed (because it
will be auto uninstalled), and we probably want to avoid letting even developers
who side-loaded or unpacked loaded an extension if another extension depends on
it.

https://codereview.chromium.org/403593002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/403593002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:233:
extension->is_shared_module() && extension->from_webstore();
On 2014/07/17 20:44:52, kalman wrote:
> why does being from the webstore make a difference?
> 
> and why this term 'managed'? I thought that shared modules were basically
always
> managed, and the term 'managed' here is actually confusing because it usually
> refers to policy.

based on your other comment, I'm getting rid of this variable entirely

https://codereview.chromium.org/403593002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:276:
scoped_ptr<ExtensionSet> dependents =
On 2014/07/17 20:44:52, kalman wrote:
> nit: can we call this 'dependentExtensions' not 'dependents' because
> 'dependents' is getting me all confused that it sounds like 'dependence' which
> is the opposite way around.
> 
> (same for the extension data property)

Done.

[not at google - send to devlin, 2014-07-17 22:03:41]

lgtm

https://codereview.chromium.org/403593002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/403593002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:271:
scoped_ptr<ExtensionSet> dependentExtensions =
depentent_extensions

https://codereview.chromium.org/403593002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:306: } else if
(extension->is_shared_module() && extension->from_webstore()) {
remove the from_webstore check here?

[not at google - send to devlin, 2014-07-17 22:03:57]

https://codereview.chromium.org/403593002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/403593002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:271:
scoped_ptr<ExtensionSet> dependentExtensions =
On 2014/07/17 22:03:41, kalman wrote:
> depentent_extensions

by which I mean dependent_extensions

[elijahtaylor1, 2014-07-17 23:28:04]

https://codereview.chromium.org/403593002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/403593002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:271:
scoped_ptr<ExtensionSet> dependentExtensions =
On 2014/07/17 22:03:57, kalman wrote:
> On 2014/07/17 22:03:41, kalman wrote:
> > depentent_extensions
> 
> by which I mean dependent_extensions


Arg, too much JS/C++ context switching :)

https://codereview.chromium.org/403593002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:306: } else if
(extension->is_shared_module() && extension->from_webstore()) {
On 2014/07/17 22:03:41, kalman wrote:
> remove the from_webstore check here?

This I would like to keep because it controls the messaging to the user about
why/from-where the extension was installed.  If it's not from CWS (which implies
it was automatic) then I don't want to message the user/developer.

[elijahtaylor1, 2014-07-17 23:28:14]

The CQ bit was checked by elijahtaylor@chromium.org

[commit-bot: I haz the power, 2014-07-17 23:30:26]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/elijahtaylor@chromium.org/403593002/60001

[not at google - send to devlin, 2014-07-17 23:30:35]

https://codereview.chromium.org/403593002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/403593002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:306: } else if
(extension->is_shared_module() && extension->from_webstore()) {
On 2014/07/17 23:28:04, elijahtaylor1 wrote:
> On 2014/07/17 22:03:41, kalman wrote:
> > remove the from_webstore check here?
> 
> This I would like to keep because it controls the messaging to the user about
> why/from-where the extension was installed.  If it's not from CWS (which
implies
> it was automatic) then I don't want to message the user/developer.

all this sets is location text, it can't hurt to always set it. it's weird
seeing the from webstore check - but if you must, you should add a comment to
explain why.

[elijahtaylor1, 2014-07-17 23:36:10]

The CQ bit was unchecked by elijahtaylor@chromium.org

[elijahtaylor1, 2014-07-18 00:22:31]

https://codereview.chromium.org/403593002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/403593002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:306: } else if
(extension->is_shared_module() && extension->from_webstore()) {
On 2014/07/17 23:30:35, kalman wrote:
> On 2014/07/17 23:28:04, elijahtaylor1 wrote:
> > On 2014/07/17 22:03:41, kalman wrote:
> > > remove the from_webstore check here?
> > 
> > This I would like to keep because it controls the messaging to the user
about
> > why/from-where the extension was installed.  If it's not from CWS (which
> implies
> > it was automatic) then I don't want to message the user/developer.
> 
> all this sets is location text, it can't hurt to always set it. it's weird
> seeing the from webstore check - but if you must, you should add a comment to
> explain why.

eh, ok, I don't feel too strongly about this.  Removed

[elijahtaylor1, 2014-07-18 00:22:42]

The CQ bit was checked by elijahtaylor@chromium.org

[commit-bot: I haz the power, 2014-07-18 00:25:23]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/elijahtaylor@chromium.org/403593002/10...

[commit-bot: I haz the power, 2014-07-18 05:35:15]

Change committed as 284010