Simplify managing visibility of extensions in settings.

Simplify managing visibility of extensions in settings.

Summary of behavior:
- chrome.management API will expose everything which is not component
  including themes and hosted apps (no change). It will never expose
  components, even with --show-component-extensions-options.
- chrome://extensions will show the same as before this CL, so no
  themes, sometimes hosted documents (only if unpacked), etc (no
  change). What changed is that we will show all components in
  chrome://extensions if --show-component-extensions-options is passed.
  Before we would only show those not hidden from the app launcher
  (sic).

We have too many methods for determining visibility, and they were
contradicting each other. Eg. ShouldNotBeVisible() which is documented
as whether it should be visible *anywhere* can return true, while
ShouldDisplayInAppLauncher could return false in the same time.

In different places we use different of the above methods causing
inconsistent behavior, eg. component extensions would be shown
in chrome://extensions when --show-component-extensions-options is
used, but excluding component extensions which are hidden from
launcher.

Finally, the logic in methods for determining visibility was
too complex, and as a result it was difficult to say whether
something will be visible in some parts of Chrome UI or not.

This CL removes the umbrella ShouldNotBeVisible() method, and introduces
ShouldExposeInManagementAPI.

Also, callers have been updated to use either
ShouldDisplayInExtensionSettings or ShouldDisplayInAppLancher or
ShouldDisplayInNewTabPage or ShouldExposeInManagementAPI depending on
what is the caller.

Ideally, all of the Should* should rather be moved to the callers, as
the callers better know what to display or show, rather than the
Extension class. However, for consistency ShouldExposeInManagementAPI
was added.

Finally, this CL simplifies the logic for determining visibility
by assuming that by default everything is visible, and filtering
out things which we want to hide (no more return true/false every
second line).

BUG=680429
TEST=Confirm that all component extensions are visible in
     chrome://extensions when --show-component-extensions-options is
     passed. Also, confirm there are no regressions, so no component
     extensions shown in chrome://extensions by default. Also,
     confirm that chrome.management API doesn't regress by using
     the "Chrome Apps & Extensions Developer Tool".
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2645863004
Cr-Commit-Position: refs/heads/master@{#446543}
Committed: https://chromium.googlesource.com/chromium/src/+/79fc3e73cfaa2b0b3b51d9e0176f5463cc7bb565

[mtomasz, 2017-01-20 01:32:35]

Description was changed from

==========
Simplify managing visibility of extensions in settings.

We have too many methods for determining visibility, and they were
contradicting each other. Eg. ShouldNotBeVisible() which is documented
as whether it should be visible *anywhere* can return true, while
ShouldDisplayInAppLauncher could return false in the same time.

In different places we use different of the above methods causing
inconsistent behavior, eg. component extensions would be shown
in chrome://extensions when --show-component-extensions-options is
used, but excluding component extensions which are hidden from
launcher.

Finally, the logic in methods for determining visibility was
too complex, and as a result it was difficult to say whether
something will be visible in some parts of Chrome UI or not.

This CL removes the umbrella ShouldNotBeVisible() method, and updates
callers to use either ShouldDisplayInExtensionSettings or
ShouldDisplayInAppLancher or ShouldDisplayInNewTabPage, depending on
what the caller is doing.

Finally, this CL simplifies the logic for determining visibility
by assuming that by default everything is visible, and filtering
out things which we want to hide (no more return true/false every
second line).

BUG=680429
TEST=Confirm that all component extensions are visible in
     chrome://extensions when --show-component-extensions-options is
     passed. Also, confirm there are no regressions, so no component
     extensions shown in chrome://extensions by default. Also,
     confirm that chrome.management API doesn't regress by using
     the "Chrome Apps & Extensions Developer Tool".
==========

to

==========
Simplify managing visibility of extensions in settings.

We have too many methods for determining visibility, and they were
contradicting each other. Eg. ShouldNotBeVisible() which is documented
as whether it should be visible *anywhere* can return true, while
ShouldDisplayInAppLauncher could return false in the same time.

In different places we use different of the above methods causing
inconsistent behavior, eg. component extensions would be shown
in chrome://extensions when --show-component-extensions-options is
used, but excluding component extensions which are hidden from
launcher.

Finally, the logic in methods for determining visibility was
too complex, and as a result it was difficult to say whether
something will be visible in some parts of Chrome UI or not.

This CL removes the umbrella ShouldNotBeVisible() method, and updates
callers to use either ShouldDisplayInExtensionSettings or
ShouldDisplayInAppLancher or ShouldDisplayInNewTabPage, depending on
what the caller is doing.

Finally, this CL simplifies the logic for determining visibility
by assuming that by default everything is visible, and filtering
out things which we want to hide (no more return true/false every
second line).

BUG=680429
TEST=Confirm that all component extensions are visible in
     chrome://extensions when --show-component-extensions-options is
     passed. Also, confirm there are no regressions, so no component
     extensions shown in chrome://extensions by default. Also,
     confirm that chrome.management API doesn't regress by using
     the "Chrome Apps & Extensions Developer Tool".
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[mtomasz, 2017-01-20 01:34:20]

The CQ bit was checked by mtomasz@chromium.org to run a CQ dry run

[mtomasz, 2017-01-20 01:34:21]

Description was changed from

==========
Simplify managing visibility of extensions in settings.

We have too many methods for determining visibility, and they were
contradicting each other. Eg. ShouldNotBeVisible() which is documented
as whether it should be visible *anywhere* can return true, while
ShouldDisplayInAppLauncher could return false in the same time.

In different places we use different of the above methods causing
inconsistent behavior, eg. component extensions would be shown
in chrome://extensions when --show-component-extensions-options is
used, but excluding component extensions which are hidden from
launcher.

Finally, the logic in methods for determining visibility was
too complex, and as a result it was difficult to say whether
something will be visible in some parts of Chrome UI or not.

This CL removes the umbrella ShouldNotBeVisible() method, and updates
callers to use either ShouldDisplayInExtensionSettings or
ShouldDisplayInAppLancher or ShouldDisplayInNewTabPage, depending on
what the caller is doing.

Finally, this CL simplifies the logic for determining visibility
by assuming that by default everything is visible, and filtering
out things which we want to hide (no more return true/false every
second line).

BUG=680429
TEST=Confirm that all component extensions are visible in
     chrome://extensions when --show-component-extensions-options is
     passed. Also, confirm there are no regressions, so no component
     extensions shown in chrome://extensions by default. Also,
     confirm that chrome.management API doesn't regress by using
     the "Chrome Apps & Extensions Developer Tool".
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Simplify managing visibility of extensions in settings.

We have too many methods for determining visibility, and they were
contradicting each other. Eg. ShouldNotBeVisible() which is documented
as whether it should be visible *anywhere* can return true, while
ShouldDisplayInAppLauncher could return false in the same time.

In different places we use different of the above methods causing
inconsistent behavior, eg. component extensions would be shown
in chrome://extensions when --show-component-extensions-options is
used, but excluding component extensions which are hidden from
launcher.

Finally, the logic in methods for determining visibility was
too complex, and as a result it was difficult to say whether
something will be visible in some parts of Chrome UI or not.

This CL removes the umbrella ShouldNotBeVisible() method, and updates
callers to use either ShouldDisplayInExtensionSettings or
ShouldDisplayInAppLancher or ShouldDisplayInNewTabPage, depending on
what the caller is doing.

Finally, this CL simplifies the logic for determining visibility
by assuming that by default everything is visible, and filtering
out things which we want to hide (no more return true/false every
second line).

BUG=680429
TEST=Confirm that all component extensions are visible in
     chrome://extensions when --show-component-extensions-options is
     passed. Also, confirm there are no regressions, so no component
     extensions shown in chrome://extensions by default. Also,
     confirm that chrome.management API doesn't regress by using
     the "Chrome Apps & Extensions Developer Tool".
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[mtomasz, 2017-01-20 01:34:21]

mtomasz@chromium.org changed reviewers:
+ rdcronin@google.com

[mtomasz, 2017-01-20 01:34:29]

@rdcronin: PTAL. Thanks!

[commit-bot: I haz the power, 2017-01-20 01:35:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mtomasz, 2017-01-20 01:36:29]

The CQ bit was checked by mtomasz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 01:37:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 02:23:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 02:23:48]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mtomasz, 2017-01-20 07:17:50]

mtomasz@chromium.org changed reviewers:
- rdcronin@google.com

[mtomasz, 2017-01-23 02:47:07]

The CQ bit was checked by mtomasz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 02:47:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mtomasz, 2017-01-23 02:53:54]

mtomasz@chromium.org changed reviewers:
+ lazyboy@chromium.org

[mtomasz, 2017-01-23 02:54:10]

@lazyboy: PTAL. Thanks!

[commit-bot: I haz the power, 2017-01-23 04:19:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 04:19:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mtomasz, 2017-01-23 05:13:57]

The CQ bit was checked by mtomasz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 05:14:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 06:04:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 06:05:00]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-01-23 22:22:26]

Thanks. Here's what I got: 1) and 2a/2b) below in the comments are the two
changes that would visible, right? Also if so, I'd highlight them in the CL
description, right in the beginning for clarity.

Also, I'm not sure I totally understand the hotword issue you mention in the
bug: if sync disabled the component (external?) extension of hotword, currently
there are no ways get to them from UI and this CL would ensure
--show-component.. flag would expose them in chrome://settings?

https://codereview.chromium.org/2645863004/diff/40001/extensions/common/exten...
File extensions/common/extension.cc (left):

https://codereview.chromium.org/2645863004/diff/40001/extensions/common/exten...
extensions/common/extension.cc:393: if (is_app() &&
!ShouldDisplayInAppLauncher() && !ShouldDisplayInNewTabPage())
2) Apps that didn't specify to show themselves in launcher/NTP: a) management
API would have not see them before, but now they do. b) same for
chrome://settings

https://codereview.chromium.org/2645863004/diff/40001/extensions/common/exten...
File extensions/common/extension.cc (right):

https://codereview.chromium.org/2645863004/diff/40001/extensions/common/exten...
extensions/common/extension.cc:382: return
!extensions::Manifest::IsComponentLocation(location());
1) So management api won't see component extensions anymore even if one has
passed --show-component.. flag, right? I'd highlight that fact in the CL
description, possibly at the very beginning after the subject line.

[mtomasz, 2017-01-24 00:29:18]

Description was changed from

==========
Simplify managing visibility of extensions in settings.

We have too many methods for determining visibility, and they were
contradicting each other. Eg. ShouldNotBeVisible() which is documented
as whether it should be visible *anywhere* can return true, while
ShouldDisplayInAppLauncher could return false in the same time.

In different places we use different of the above methods causing
inconsistent behavior, eg. component extensions would be shown
in chrome://extensions when --show-component-extensions-options is
used, but excluding component extensions which are hidden from
launcher.

Finally, the logic in methods for determining visibility was
too complex, and as a result it was difficult to say whether
something will be visible in some parts of Chrome UI or not.

This CL removes the umbrella ShouldNotBeVisible() method, and updates
callers to use either ShouldDisplayInExtensionSettings or
ShouldDisplayInAppLancher or ShouldDisplayInNewTabPage, depending on
what the caller is doing.

Finally, this CL simplifies the logic for determining visibility
by assuming that by default everything is visible, and filtering
out things which we want to hide (no more return true/false every
second line).

BUG=680429
TEST=Confirm that all component extensions are visible in
     chrome://extensions when --show-component-extensions-options is
     passed. Also, confirm there are no regressions, so no component
     extensions shown in chrome://extensions by default. Also,
     confirm that chrome.management API doesn't regress by using
     the "Chrome Apps & Extensions Developer Tool".
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Simplify managing visibility of extensions in settings.

Summary of behavior:
- chrome.management API will expose everything which is not component
  including themes and hosted apps (no change). It will never expose
  components, even with --show-component-extensions-options.
- chrome://extensions will show the same as before this CL, so no
  themes, sometimes hosted documents (only if unpacked), etc (no
  change). What changed is that we will show all components in
  chrome://extensions if --show-component-extensions-options is passed.
  Before we would only show those not hidden from the app launcher
  (sic).

We have too many methods for determining visibility, and they were
contradicting each other. Eg. ShouldNotBeVisible() which is documented
as whether it should be visible *anywhere* can return true, while
ShouldDisplayInAppLauncher could return false in the same time.

In different places we use different of the above methods causing
inconsistent behavior, eg. component extensions would be shown
in chrome://extensions when --show-component-extensions-options is
used, but excluding component extensions which are hidden from
launcher.

Finally, the logic in methods for determining visibility was
too complex, and as a result it was difficult to say whether
something will be visible in some parts of Chrome UI or not.

This CL removes the umbrella ShouldNotBeVisible() method, and introduces
ShouldExposeInManagementAPI.

Also, callers have been updated to use either
ShouldDisplayInExtensionSettings or ShouldDisplayInAppLancher or
ShouldDisplayInNewTabPage or ShouldExposeInManagementAPI depending on
what is the caller.

Ideally, all of the Should* should rather be moved to the callers, as
the callers better know what to display or show, rather than the
Extension class. However, for consistency ShouldExposeInManagementAPI
was added.

Finally, this CL simplifies the logic for determining visibility
by assuming that by default everything is visible, and filtering
out things which we want to hide (no more return true/false every
second line).

BUG=680429
TEST=Confirm that all component extensions are visible in
     chrome://extensions when --show-component-extensions-options is
     passed. Also, confirm there are no regressions, so no component
     extensions shown in chrome://extensions by default. Also,
     confirm that chrome.management API doesn't regress by using
     the "Chrome Apps & Extensions Developer Tool".
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[mtomasz, 2017-01-24 00:43:09]

On 2017/01/23 22:22:26, lazyboy wrote:
> Thanks. Here's what I got: 1) and 2a/2b) below in the comments are the two
> changes that would visible, right? Also if so, I'd highlight them in the CL
> description, right in the beginning for clarity.

Done.

> 
> Also, I'm not sure I totally understand the hotword issue you mention in the
> bug: if sync disabled the component (external?) extension of hotword,
currently
> there are no ways get to them from UI and this CL would ensure
> --show-component.. flag would expose them in chrome://settings?

Almost. Hotword would always be shown, but ZIP unpacker wouldn't, as it's hidden
from the launcher in it's manifest. Hotword was mentioned in crbug.com to
justify the other CL - crrev.com/2647783003. The other CL reenables all external
component extensions in the profile resetter. If we didn't have hotword, then we
could just prevent disabling any external component extensions in extension
service, instead of the profile resetter recovery code.


> 
>
https://codereview.chromium.org/2645863004/diff/40001/extensions/common/exten...
> File extensions/common/extension.cc (left):
> 
>
https://codereview.chromium.org/2645863004/diff/40001/extensions/common/exten...
> extensions/common/extension.cc:393: if (is_app() &&
> !ShouldDisplayInAppLauncher() && !ShouldDisplayInNewTabPage())
> 2) Apps that didn't specify to show themselves in launcher/NTP: a) management
> API would have not see them before, but now they do. b) same for
> chrome://settings
> 
>
https://codereview.chromium.org/2645863004/diff/40001/extensions/common/exten...
> File extensions/common/extension.cc (right):
> 
>
https://codereview.chromium.org/2645863004/diff/40001/extensions/common/exten...
> extensions/common/extension.cc:382: return
> !extensions::Manifest::IsComponentLocation(location());
> 1) So management api won't see component extensions anymore even if one has
> passed --show-component.. flag, right? I'd highlight that fact in the CL
> description, possibly at the very beginning after the subject line.

[mtomasz, 2017-01-26 08:38:30]

https://codereview.chromium.org/2645863004/diff/40001/extensions/common/exten...
File extensions/common/extension.cc (left):

https://codereview.chromium.org/2645863004/diff/40001/extensions/common/exten...
extensions/common/extension.cc:393: if (is_app() &&
!ShouldDisplayInAppLauncher() && !ShouldDisplayInNewTabPage())
On 2017/01/23 22:22:26, lazyboy wrote:
> 2) Apps that didn't specify to show themselves in launcher/NTP: a) management
> API would have not see them before, but now they do. b) same for
> chrome://settings

Correct. I believe this was a bug. Hiding from launcher doesn't mean that the
item should be hidden from management API or from chrome://extensions. These
things are completely separate.

https://codereview.chromium.org/2645863004/diff/40001/extensions/common/exten...
File extensions/common/extension.cc (right):

https://codereview.chromium.org/2645863004/diff/40001/extensions/common/exten...
extensions/common/extension.cc:382: return
!extensions::Manifest::IsComponentLocation(location());
On 2017/01/23 22:22:26, lazyboy wrote:
> 1) So management api won't see component extensions anymore even if one has
> passed --show-component.. flag, right? I'd highlight that fact in the CL
> description, possibly at the very beginning after the subject line.

Correct. --show-component-extensions-options refers to chrome://extensions so no
reason to affect the management API. Added explanation in the CL description.

[lazyboy, 2017-01-26 18:20:22]

thanks, lgtm.

[mtomasz, 2017-01-27 00:34:33]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2017-01-27 00:35:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 02:34:25]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485477273730300,
"parent_rev": "623b452d81af15a58f04dd81775867c653a3e4a3", "commit_rev":
"79fc3e73cfaa2b0b3b51d9e0176f5463cc7bb565"}

[commit-bot: I haz the power, 2017-01-27 02:34:53]

Description was changed from

==========
Simplify managing visibility of extensions in settings.

Summary of behavior:
- chrome.management API will expose everything which is not component
  including themes and hosted apps (no change). It will never expose
  components, even with --show-component-extensions-options.
- chrome://extensions will show the same as before this CL, so no
  themes, sometimes hosted documents (only if unpacked), etc (no
  change). What changed is that we will show all components in
  chrome://extensions if --show-component-extensions-options is passed.
  Before we would only show those not hidden from the app launcher
  (sic).

We have too many methods for determining visibility, and they were
contradicting each other. Eg. ShouldNotBeVisible() which is documented
as whether it should be visible *anywhere* can return true, while
ShouldDisplayInAppLauncher could return false in the same time.

In different places we use different of the above methods causing
inconsistent behavior, eg. component extensions would be shown
in chrome://extensions when --show-component-extensions-options is
used, but excluding component extensions which are hidden from
launcher.

Finally, the logic in methods for determining visibility was
too complex, and as a result it was difficult to say whether
something will be visible in some parts of Chrome UI or not.

This CL removes the umbrella ShouldNotBeVisible() method, and introduces
ShouldExposeInManagementAPI.

Also, callers have been updated to use either
ShouldDisplayInExtensionSettings or ShouldDisplayInAppLancher or
ShouldDisplayInNewTabPage or ShouldExposeInManagementAPI depending on
what is the caller.

Ideally, all of the Should* should rather be moved to the callers, as
the callers better know what to display or show, rather than the
Extension class. However, for consistency ShouldExposeInManagementAPI
was added.

Finally, this CL simplifies the logic for determining visibility
by assuming that by default everything is visible, and filtering
out things which we want to hide (no more return true/false every
second line).

BUG=680429
TEST=Confirm that all component extensions are visible in
     chrome://extensions when --show-component-extensions-options is
     passed. Also, confirm there are no regressions, so no component
     extensions shown in chrome://extensions by default. Also,
     confirm that chrome.management API doesn't regress by using
     the "Chrome Apps & Extensions Developer Tool".
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Simplify managing visibility of extensions in settings.

Summary of behavior:
- chrome.management API will expose everything which is not component
  including themes and hosted apps (no change). It will never expose
  components, even with --show-component-extensions-options.
- chrome://extensions will show the same as before this CL, so no
  themes, sometimes hosted documents (only if unpacked), etc (no
  change). What changed is that we will show all components in
  chrome://extensions if --show-component-extensions-options is passed.
  Before we would only show those not hidden from the app launcher
  (sic).

We have too many methods for determining visibility, and they were
contradicting each other. Eg. ShouldNotBeVisible() which is documented
as whether it should be visible *anywhere* can return true, while
ShouldDisplayInAppLauncher could return false in the same time.

In different places we use different of the above methods causing
inconsistent behavior, eg. component extensions would be shown
in chrome://extensions when --show-component-extensions-options is
used, but excluding component extensions which are hidden from
launcher.

Finally, the logic in methods for determining visibility was
too complex, and as a result it was difficult to say whether
something will be visible in some parts of Chrome UI or not.

This CL removes the umbrella ShouldNotBeVisible() method, and introduces
ShouldExposeInManagementAPI.

Also, callers have been updated to use either
ShouldDisplayInExtensionSettings or ShouldDisplayInAppLancher or
ShouldDisplayInNewTabPage or ShouldExposeInManagementAPI depending on
what is the caller.

Ideally, all of the Should* should rather be moved to the callers, as
the callers better know what to display or show, rather than the
Extension class. However, for consistency ShouldExposeInManagementAPI
was added.

Finally, this CL simplifies the logic for determining visibility
by assuming that by default everything is visible, and filtering
out things which we want to hide (no more return true/false every
second line).

BUG=680429
TEST=Confirm that all component extensions are visible in
     chrome://extensions when --show-component-extensions-options is
     passed. Also, confirm there are no regressions, so no component
     extensions shown in chrome://extensions by default. Also,
     confirm that chrome.management API doesn't regress by using
     the "Chrome Apps & Extensions Developer Tool".
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2645863004
Cr-Commit-Position: refs/heads/master@{#446543}
Committed:
https://chromium.googlesource.com/chromium/src/+/79fc3e73cfaa2b0b3b51d9e0176f...
==========

[commit-bot: I haz the power, 2017-01-27 02:34:55]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/79fc3e73cfaa2b0b3b51d9e0176f...