[Extensions] Remove the prompt to re-enable app when visiting its site

[Extensions] Remove the prompt to re-enable app when visiting its site

We currently prompt the user to re-enable a disabled hosted app if they
visit the corresponding site. However, this can be really annoying, and
confusing, since we don't offer much context on the choice, and don't
persist the preference.

This is offering relatively little value, and the site (if it wanted)
could detect and inform the user that the app wasn't enabled, and give
them sufficiently more context.

However, we should keep this when the user visits a chrome-extension
url - this is a much stronger signal, and we can't display anything else
anyway (whereas visiting a hosted app's site still shows the site).

This was never tested, so add some tests.

BUG=678631
Review-Url: https://codereview.chromium.org/2621953003
Cr-Commit-Position: refs/heads/master@{#443439}
Committed: https://chromium.googlesource.com/chromium/src/+/4b184ca093fba8aabfccdba8391c3676815beed9

[Devlin, 2017-01-10 16:23:00]

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

[commit-bot: I haz the power, 2017-01-10 16:23:34]

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-10 17:39:52]

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

[commit-bot: I haz the power, 2017-01-10 17:39:53]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-01-10 21:36:34]

Description was changed from

==========
removed

BUG=
==========

to

==========
[Extensions] Remove the prompt to re-enable app when visiting its site

We currently prompt the user to re-enable a disabled app if they visit
the corresponding site. However, this can be really annoying, and
confusing, since we don't offer much context on the choice, and don't
persist the preference.

This is offering relatively little value, and the site (if it wanted)
could detect and inform the user that the app wasn't enabled, and give
them sufficiently more context.

Remove the associated code. Apparently, this was never tested, so
nothing there to remove.

BUG=678631
==========

[Devlin, 2017-01-10 21:36:44]

rdevlin.cronin@chromium.org changed reviewers:
+ lazyboy@chromium.org

[lazyboy, 2017-01-11 00:33:30]

https://codereview.chromium.org/2621953003/diff/1/chrome/browser/extensions/n...
File chrome/browser/extensions/navigation_observer.cc (left):

https://codereview.chromium.org/2621953003/diff/1/chrome/browser/extensions/n...
chrome/browser/extensions/navigation_observer.cc:75: if
(extension_prefs->DidExtensionEscalatePermissions(extension->id())) {
So the re-enable prompt we were thinking of probably is this one:
if extension A is disabled due to permission increase, then navigating to its
resource through omnibox shows this re-enable prompt (I just double checked). So
we will lose that with this CL.

Not sure there's any value in keeping this behavior. But I just wanted to make
sure we are aware of that if we are removing it.
Otherwise, replace the GetExtensionOrAppByURL call above to just use
nav_entry->GetURL()->host() as extension ID. And add a TODO with a bug to
consider removing that as well.

[Devlin, 2017-01-12 17:25:36]

Description was changed from

==========
[Extensions] Remove the prompt to re-enable app when visiting its site

We currently prompt the user to re-enable a disabled app if they visit
the corresponding site. However, this can be really annoying, and
confusing, since we don't offer much context on the choice, and don't
persist the preference.

This is offering relatively little value, and the site (if it wanted)
could detect and inform the user that the app wasn't enabled, and give
them sufficiently more context.

Remove the associated code. Apparently, this was never tested, so
nothing there to remove.

BUG=678631
==========

to

==========
[Extensions] Remove the prompt to re-enable app when visiting its site

We currently prompt the user to re-enable a disabled hosted app if they
visit the corresponding site. However, this can be really annoying, and
confusing, since we don't offer much context on the choice, and don't
persist the preference.

This is offering relatively little value, and the site (if it wanted)
could detect and inform the user that the app wasn't enabled, and give
them sufficiently more context.

However, we should keep this when the user visits a chrome-extension
url - this is a much stronger signal, and we can't display anything else
anyway (whereas visiting a hosted app's site still shows the site).

This was never tested, so add some tests.

BUG=678631
==========

[Devlin, 2017-01-12 17:25:44]

https://codereview.chromium.org/2621953003/diff/1/chrome/browser/extensions/n...
File chrome/browser/extensions/navigation_observer.cc (left):

https://codereview.chromium.org/2621953003/diff/1/chrome/browser/extensions/n...
chrome/browser/extensions/navigation_observer.cc:75: if
(extension_prefs->DidExtensionEscalatePermissions(extension->id())) {
On 2017/01/11 00:33:30, lazyboy wrote:
> So the re-enable prompt we were thinking of probably is this one:
> if extension A is disabled due to permission increase, then navigating to its
> resource through omnibox shows this re-enable prompt (I just double checked).
So
> we will lose that with this CL.
> 
> Not sure there's any value in keeping this behavior. But I just wanted to make
> sure we are aware of that if we are removing it.
> Otherwise, replace the GetExtensionOrAppByURL call above to just use
> nav_entry->GetURL()->host() as extension ID. And add a TODO with a bug to
> consider removing that as well.

Ah, you're quite right.  Let's keep it for chrome-extension urls.  Sadly, this
meant a lot more work, since now I actually have to test it :P

[lazyboy, 2017-01-12 21:22:02]

Thanks for writing the test! One typo I think.

https://codereview.chromium.org/2621953003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/navigation_observer.cc (right):

https://codereview.chromium.org/2621953003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/navigation_observer.cc:99: if
(!extension_prefs->DidExtensionEscalatePermissions(extension->id())) {
Shouldn't this be inverted?

[Devlin, 2017-01-12 23:00:51]

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

[Devlin, 2017-01-12 23:01:04]

https://codereview.chromium.org/2621953003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/navigation_observer.cc (right):

https://codereview.chromium.org/2621953003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/navigation_observer.cc:99: if
(!extension_prefs->DidExtensionEscalatePermissions(extension->id())) {
On 2017/01/12 21:22:01, lazyboy wrote:
> Shouldn't this be inverted?

Whoops, yes.  I had originally changed it to early return
if (!DidEscalate())
  return;

but decided it wasn't worth the blame history - and apparently didn't revert it
fully.  Fixed.

[commit-bot: I haz the power, 2017-01-12 23:01:40]

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

[lazyboy, 2017-01-12 23:02:20]

lgtm

[commit-bot: I haz the power, 2017-01-13 00:20:28]

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

[commit-bot: I haz the power, 2017-01-13 00:20:29]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-01-13 01:21:15]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2017-01-13 01:22:30]

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-13 01:35:48]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484270475216340,
"parent_rev": "cb827f61f1bab8042b5f7593b250e705910d9c55", "commit_rev":
"4b184ca093fba8aabfccdba8391c3676815beed9"}

[commit-bot: I haz the power, 2017-01-13 01:36:17]

Description was changed from

==========
[Extensions] Remove the prompt to re-enable app when visiting its site

We currently prompt the user to re-enable a disabled hosted app if they
visit the corresponding site. However, this can be really annoying, and
confusing, since we don't offer much context on the choice, and don't
persist the preference.

This is offering relatively little value, and the site (if it wanted)
could detect and inform the user that the app wasn't enabled, and give
them sufficiently more context.

However, we should keep this when the user visits a chrome-extension
url - this is a much stronger signal, and we can't display anything else
anyway (whereas visiting a hosted app's site still shows the site).

This was never tested, so add some tests.

BUG=678631
==========

to

==========
[Extensions] Remove the prompt to re-enable app when visiting its site

We currently prompt the user to re-enable a disabled hosted app if they
visit the corresponding site. However, this can be really annoying, and
confusing, since we don't offer much context on the choice, and don't
persist the preference.

This is offering relatively little value, and the site (if it wanted)
could detect and inform the user that the app wasn't enabled, and give
them sufficiently more context.

However, we should keep this when the user visits a chrome-extension
url - this is a much stronger signal, and we can't display anything else
anyway (whereas visiting a hosted app's site still shows the site).

This was never tested, so add some tests.

BUG=678631

Review-Url: https://codereview.chromium.org/2621953003
Cr-Commit-Position: refs/heads/master@{#443439}
Committed:
https://chromium.googlesource.com/chromium/src/+/4b184ca093fba8aabfccdba8391c...
==========

[commit-bot: I haz the power, 2017-01-13 01:36:18]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4b184ca093fba8aabfccdba8391c...