Add new permission string for extensions that override new tab pages.

Add new permission string for extensions that override new tab pages.

BUG=695285, 280619
Review-Url: https://codereview.chromium.org/2950203002
Cr-Commit-Position: refs/heads/master@{#488042}
Committed: https://chromium.googlesource.com/chromium/src/+/9cf4246e762313b1588c164ee2b8dfd40549815b

[robertshield, 2017-06-21 21:48:57]

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

[commit-bot: I haz the power, 2017-06-21 21:49:13]

Dry run: CQ is trying da patch.

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

[robertshield, 2017-06-21 22:01:27]

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

[robertshield, 2017-06-21 22:01:27]

Hey Devlin,

  I suspect this might be missing bits / tests, the permission ordering might be
wrong and the string could probably use some tweaking, but other than that,
could you let me know if this is generally structurally sound? It does (at
least!) result in the desired string showing up in the extension install dialog.

Thanks!
Robert

[commit-bot: I haz the power, 2017-06-21 22:23:50]

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

[commit-bot: I haz the power, 2017-06-21 22:23:51]

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_...)

[Devlin, 2017-06-21 22:51:02]

On 2017/06/21 22:01:27, robertshield wrote:
> Hey Devlin,
> 
>   I suspect this might be missing bits / tests, the permission ordering might
be
> wrong and the string could probably use some tweaking, but other than that,
> could you let me know if this is generally structurally sound? It does (at
> least!) result in the desired string showing up in the extension install
dialog.
> 
> Thanks!
> Robert

This looks pretty reasonable, but does need tests.  In particular, one thing is
that we need to make sure we don't disable existing NTP extensions that are
added.  I think that, as this is now, it may, so we might need to add logic to
address that.

[robertshield, 2017-06-29 20:36:11]

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

[commit-bot: I haz the power, 2017-06-29 20:36:33]

Dry run: CQ is trying da patch.

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

[robertshield, 2017-06-29 20:44:15]

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

[commit-bot: I haz the power, 2017-06-29 20:44:43]

Dry run: CQ is trying da patch.

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

[robertshield, 2017-06-29 20:45:47]

On 2017/06/21 22:51:02, Devlin wrote:
> On 2017/06/21 22:01:27, robertshield wrote:
> > Hey Devlin,
> > 
> >   I suspect this might be missing bits / tests, the permission ordering
might
> be
> > wrong and the string could probably use some tweaking, but other than that,
> > could you let me know if this is generally structurally sound? It does (at
> > least!) result in the desired string showing up in the extension install
> dialog.
> > 
> > Thanks!
> > Robert
> 
> This looks pretty reasonable, but does need tests.  In particular, one thing
is
> that we need to make sure we don't disable existing NTP extensions that are
> added.  I think that, as this is now, it may, so we might need to add logic to
> address that.

Thanks Devlin! 

I tweaked this such that the new API permission doesn't get considered when
computing permission changes and added a browser_test to verify. Please could
you take a look and let me know if there are other tests that would be
appropriate here (especially thinking about other upgrade scenarios that the
current test doesn't cover).

[commit-bot: I haz the power, 2017-06-29 21:22:39]

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

[commit-bot: I haz the power, 2017-06-29 21:22:40]

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

[robertshield, 2017-06-30 04:38:54]

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

[commit-bot: I haz the power, 2017-06-30 04:39:08]

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-06-30 05:55:10]

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

[commit-bot: I haz the power, 2017-06-30 05:55:11]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-07-05 15:45:02]

Overall, looks pretty good.  One last question in the test.

Also, have you tested this locally with the flow of
- Start with chrome N
- Add an NTP extension
- Upgrade to chrome N+1
- Ensure NTP extension is still there and hasn't been disabled

where chrome N does not have the NTP permission and N+1 does?

https://codereview.chromium.org/2950203002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_loading_browsertest.cc (right):

https://codereview.chromium.org/2950203002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_loading_browsertest.cc:132:
ASSERT_NE(nullptr, new_tab_extension);
optional: I think ASSERT_TRUE(new_tab_extension) is more common, but either way
works.

https://codereview.chromium.org/2950203002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_loading_browsertest.cc:139: const GURL
test_link_from_NTP =
nit: ignore capitals in variable names, so just |test_link_from_ntp|.

https://codereview.chromium.org/2950203002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_loading_browsertest.cc:156: // the extension
is still present and not disabled and that it now has the
Is this desired?  It means that if I don't want the user to see that I change
the NTP, I could have them install the extension at v1 and then upgrade to v2 to
silently swap out the NTP for all existing users.  I assume this is just to
allow for the migration process?

[robertshield, 2017-07-18 19:46:09]

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

[robertshield, 2017-07-18 19:46:11]

Thanks for the comments! 

On 2017/07/05 15:45:02, Devlin wrote:
> Overall, looks pretty good.  One last question in the test.
> 
> Also, have you tested this locally with the flow of
> - Start with chrome N
> - Add an NTP extension
> - Upgrade to chrome N+1
> - Ensure NTP extension is still there and hasn't been disabled

I ran the test from M59 to trunk (currently M61) with this patch and the
permission prompt did not appear, as desired.

> 
> where chrome N does not have the NTP permission and N+1 does?
> 
>
https://codereview.chromium.org/2950203002/diff/60001/chrome/browser/extensio...
> File chrome/browser/extensions/extension_loading_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2950203002/diff/60001/chrome/browser/extensio...
> chrome/browser/extensions/extension_loading_browsertest.cc:132:
> ASSERT_NE(nullptr, new_tab_extension);
> optional: I think ASSERT_TRUE(new_tab_extension) is more common, but either
way
> works.
> 
>
https://codereview.chromium.org/2950203002/diff/60001/chrome/browser/extensio...
> chrome/browser/extensions/extension_loading_browsertest.cc:139: const GURL
> test_link_from_NTP =
> nit: ignore capitals in variable names, so just |test_link_from_ntp|.
> 
>
https://codereview.chromium.org/2950203002/diff/60001/chrome/browser/extensio...
> chrome/browser/extensions/extension_loading_browsertest.cc:156: // the
extension
> is still present and not disabled and that it now has the
> Is this desired?  It means that if I don't want the user to see that I change
> the NTP, I could have them install the extension at v1 and then upgrade to v2
to
> silently swap out the NTP for all existing users.  I assume this is just to
> allow for the migration process?

[robertshield, 2017-07-18 19:46:20]

https://codereview.chromium.org/2950203002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_loading_browsertest.cc (right):

https://codereview.chromium.org/2950203002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_loading_browsertest.cc:132:
ASSERT_NE(nullptr, new_tab_extension);
On 2017/07/05 15:45:02, Devlin wrote:
> optional: I think ASSERT_TRUE(new_tab_extension) is more common, but either
way
> works.

Done.

https://codereview.chromium.org/2950203002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_loading_browsertest.cc:139: const GURL
test_link_from_NTP =
On 2017/07/05 15:45:02, Devlin wrote:
> nit: ignore capitals in variable names, so just |test_link_from_ntp|.

Done.

https://codereview.chromium.org/2950203002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_loading_browsertest.cc:156: // the extension
is still present and not disabled and that it now has the
On 2017/07/05 15:45:02, Devlin wrote:
> Is this desired?  It means that if I don't want the user to see that I change
> the NTP, I could have them install the extension at v1 and then upgrade to v2
to
> silently swap out the NTP for all existing users.  I assume this is just to
> allow for the migration process?

Yes, it's intended for migration. The intent is not to re-prompt users of
existing ntp override extensions so this does allow exactly the strategy you
describe. 

In practice updating an extension that does not replace the NTP to start doing
so is likely a policy violation so may be addressable through other means.

[commit-bot: I haz the power, 2017-07-18 19:46:29]

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-07-18 21:32:34]

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

[commit-bot: I haz the power, 2017-07-18 21:32:35]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-07-18 22:27:27]

lgtm!

https://codereview.chromium.org/2950203002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_loading_browsertest.cc (right):

https://codereview.chromium.org/2950203002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_loading_browsertest.cc:112: const char
manifest_template[] =
nit: POD-constant-style-naming: kManifestTemplate

https://codereview.chromium.org/2950203002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_loading_browsertest.cc:119: "  'background':
{"
Do we need a background at all?

https://codereview.chromium.org/2950203002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_loading_browsertest.cc:157: // new API
permission.
nit: duplicate your TODO to update this?

[robertshield, 2017-07-19 20:26:15]

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

[commit-bot: I haz the power, 2017-07-19 20:26:23]

Dry run: CQ is trying da patch.

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

[robertshield, 2017-07-19 21:02:57]

Thanks!

https://codereview.chromium.org/2950203002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_loading_browsertest.cc (right):

https://codereview.chromium.org/2950203002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_loading_browsertest.cc:112: const char
manifest_template[] =
On 2017/07/18 22:27:26, Devlin wrote:
> nit: POD-constant-style-naming: kManifestTemplate

Done.

https://codereview.chromium.org/2950203002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_loading_browsertest.cc:119: "  'background':
{"
On 2017/07/18 22:27:27, Devlin wrote:
> Do we need a background at all?

Done.

https://codereview.chromium.org/2950203002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_loading_browsertest.cc:157: // new API
permission.
On 2017/07/18 22:27:27, Devlin wrote:
> nit: duplicate your TODO to update this?

Done.

[commit-bot: I haz the power, 2017-07-19 22:18:12]

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

[commit-bot: I haz the power, 2017-07-19 22:18:13]

Dry run: This issue passed the CQ dry run.

[robertshield, 2017-07-19 23:38:32]

The CQ bit was checked by robertshield@chromium.org

[robertshield, 2017-07-19 23:38:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2950203002/#ps100001
(title: "Devlin's last feedback.")

[commit-bot: I haz the power, 2017-07-19 23:38:50]

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-07-19 23:44:11]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1500507512589850,
"parent_rev": "24a36414f5f13bc3036a440154a3dee4c73b3978", "commit_rev":
"275a07c6ea50481ae0856016b42354c2692533c1"}

[commit-bot: I haz the power, 2017-07-19 23:44:34]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1500507512589850,
"parent_rev": "48620dd3340c07313f34e9e5a05fdfa8a6c4c770", "commit_rev":
"9cf4246e762313b1588c164ee2b8dfd40549815b"}

[commit-bot: I haz the power, 2017-07-19 23:44:49]

Description was changed from

==========
Add new permission string for extensions that override new tab pages.

BUG=695285,280619
==========

to

==========
Add new permission string for extensions that override new tab pages.

BUG=695285,280619

Review-Url: https://codereview.chromium.org/2950203002
Cr-Commit-Position: refs/heads/master@{#488042}
Committed:
https://chromium.googlesource.com/chromium/src/+/9cf4246e762313b1588c164ee2b8...
==========

[commit-bot: I haz the power, 2017-07-19 23:44:52]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/9cf4246e762313b1588c164ee2b8...