Ask for notification permission during extension installation.

Ask for notification permission during extension installation.

This will include a note about displaying notifications when installing
an extension that has the "notifications" permission.

Notifications allow the developer to display UI outside of the scope of
the extension, which is not always equally clearly attributed to the
extension showing the notification. This should not be considered a
trivial permission.

The changed .crx file requested the "notifications" permission, but
didn't use it for anything. I've swapped out for "cookies". The source
does not seem to be checked in (hosted_app/ in the folder is something
else).

TBR=benwells (for app_info_permissions_panel_unittest.cc)
BUG=583480
Committed: https://crrev.com/7d646cfee32db5f81a257a73a8a75acf03da7942
Cr-Commit-Position: refs/heads/master@{#374375}

[Peter Beverloo, 2016-02-03 12:37:32]

Patchset #1 (id:1) has been deleted

[Peter Beverloo, 2016-02-03 12:40:41]

peter@chromium.org changed reviewers:
+ dewittj@chromium.org, treib@chromium.org

[Peter Beverloo, 2016-02-03 12:40:41]

+treib for chrome_permission_message_rules.cc OWNERS
+dewittj for the notifications extensions API

There don't seem to be tests for this? I confirmed manually that the string
shows up.

http://imgur.com/G12iLmC

[Marc Treib, 2016-02-03 12:49:37]

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

[Marc Treib, 2016-02-03 12:49:37]

There are tests for some things, but not for every single permission message.
The general id->message logic is the same for all of them, so IMO there's no
need to test every individual one. If you think this deserves a test, feel free
to add one :)

For existing extensions that already have this permission, this will (in most
cases) *not* trigger a permission prompt - it'll only show up for new installs.
I guess that's intended?

Also, there are many permissions that don't result in any message. I wonder if
that was a conscious decision in this case?
+Devlin - do you know?

[Marc Treib, 2016-02-03 12:55:00]

Ah, code-wise LGTM, but please wait for Devlin's input before landing!

[Peter Beverloo, 2016-02-03 14:31:00]

Description was changed from

==========
Ask for notification permission during extension installation.

This will include a note about displaying notifications when installing
an extension that has the "notifications" permission.

BUG=583480
==========

to

==========
Ask for notification permission during extension installation.

This will include a note about displaying notifications when installing
an extension that has the "notifications" permission.

Notifications allow the developer to display UI outside of the scope of
the extension, which is not always equally clearly attributed to the
extension showing the notification. This should not be considered a
trivial permission.

The changed .crx file requested the "notifications" permission, but
didn't use it for anything. I've swapped out for "cookies". The source
does not seem to be checked in (hosted_app/ in the folder is something
else).

BUG=583480
==========

[Devlin, 2016-02-03 17:34:51]

https://codereview.chromium.org/1665563003/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1665563003/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:3971: +          Display notifications
I've looped in meacer@ (my general go-to for all permission-y things) for his
thoughts on this.  If we do decide to add a permission (which I suspect we
probably will), we might want to tweak this phrasing a bit - "Display
notifications" is kind of vague (though maybe it's sufficient?).  Let's wait to
hear back.

[Peter Beverloo, 2016-02-03 17:53:25]

On 2016/02/03 12:49:37, Marc Treib wrote:
> For existing extensions that already have this permission, this will (in most
> cases) *not* trigger a permission prompt - it'll only show up for new
installs.
> I guess that's intended?

Yes, this is my intention.

On 2016/02/03 17:34:51, Devlin wrote:
> I've looped in meacer@ (my general go-to for all permission-y things) for his
> thoughts on this.  If we do decide to add a permission (which I suspect we
> probably will), we might want to tweak this phrasing a bit - "Display
> notifications" is kind of vague (though maybe it's sufficient?).  Let's wait
to
> hear back.

The analogous infobar for Web Notifications uses the pharsing of "* wants to
send you notifications", but that also encapsulates push messaging (i.e. GCM).

Users may be familiar with that. Do we want to use "Send you notifications" here
too?

[dewittj, 2016-02-03 18:00:24]

+1 to "Send you notifications"

Concept lgtm

[dewittj, 2016-02-03 18:01:05]

I mentioned this on the bug, but we need to be sure that upgrading an installed
app will not trigger the permissions prompt (and pin apps at an old version)

[Devlin, 2016-02-03 18:07:33]

On 2016/02/03 18:01:05, dewittj wrote:
> I mentioned this on the bug, but we need to be sure that upgrading an
installed
> app will not trigger the permissions prompt (and pin apps at an old version)

The current permissions code should account for that.  We don't show increases
based on message changes, only based on actual present permissions in the
manifest (which shouldn't be altered).

[Devlin, 2016-02-08 18:47:41]

From the discussion on the bug, let's go ahead with this.  LGTM.

[Peter Beverloo, 2016-02-09 13:56:27]

Description was changed from

==========
Ask for notification permission during extension installation.

This will include a note about displaying notifications when installing
an extension that has the "notifications" permission.

Notifications allow the developer to display UI outside of the scope of
the extension, which is not always equally clearly attributed to the
extension showing the notification. This should not be considered a
trivial permission.

The changed .crx file requested the "notifications" permission, but
didn't use it for anything. I've swapped out for "cookies". The source
does not seem to be checked in (hosted_app/ in the folder is something
else).

BUG=583480
==========

to

==========
Ask for notification permission during extension installation.

This will include a note about displaying notifications when installing
an extension that has the "notifications" permission.

Notifications allow the developer to display UI outside of the scope of
the extension, which is not always equally clearly attributed to the
extension showing the notification. This should not be considered a
trivial permission.

The changed .crx file requested the "notifications" permission, but
didn't use it for anything. I've swapped out for "cookies". The source
does not seem to be checked in (hosted_app/ in the folder is something
else).

TBR=benwells (for app_info_permissions_panel_unittest.cc)
BUG=583480
==========

[Peter Beverloo, 2016-02-09 13:57:16]

peter@chromium.org changed reviewers:
+ benwells@chromium.org

[Peter Beverloo, 2016-02-09 13:57:17]

Let's do it :)

+TBR benwells for one trivial change in app_info_permissions_panel_unittest.cc.

[Peter Beverloo, 2016-02-09 13:57:49]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2016-02-09 13:57:49]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/1665563003/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-02-09 13:58:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1665563003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1665563003/60001

[commit-bot: I haz the power, 2016-02-09 15:14:35]

Description was changed from

==========
Ask for notification permission during extension installation.

This will include a note about displaying notifications when installing
an extension that has the "notifications" permission.

Notifications allow the developer to display UI outside of the scope of
the extension, which is not always equally clearly attributed to the
extension showing the notification. This should not be considered a
trivial permission.

The changed .crx file requested the "notifications" permission, but
didn't use it for anything. I've swapped out for "cookies". The source
does not seem to be checked in (hosted_app/ in the folder is something
else).

TBR=benwells (for app_info_permissions_panel_unittest.cc)
BUG=583480
==========

to

==========
Ask for notification permission during extension installation.

This will include a note about displaying notifications when installing
an extension that has the "notifications" permission.

Notifications allow the developer to display UI outside of the scope of
the extension, which is not always equally clearly attributed to the
extension showing the notification. This should not be considered a
trivial permission.

The changed .crx file requested the "notifications" permission, but
didn't use it for anything. I've swapped out for "cookies". The source
does not seem to be checked in (hosted_app/ in the folder is something
else).

TBR=benwells (for app_info_permissions_panel_unittest.cc)
BUG=583480
==========

[commit-bot: I haz the power, 2016-02-09 15:14:36]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-02-09 15:15:45]

Description was changed from

==========
Ask for notification permission during extension installation.

This will include a note about displaying notifications when installing
an extension that has the "notifications" permission.

Notifications allow the developer to display UI outside of the scope of
the extension, which is not always equally clearly attributed to the
extension showing the notification. This should not be considered a
trivial permission.

The changed .crx file requested the "notifications" permission, but
didn't use it for anything. I've swapped out for "cookies". The source
does not seem to be checked in (hosted_app/ in the folder is something
else).

TBR=benwells (for app_info_permissions_panel_unittest.cc)
BUG=583480
==========

to

==========
Ask for notification permission during extension installation.

This will include a note about displaying notifications when installing
an extension that has the "notifications" permission.

Notifications allow the developer to display UI outside of the scope of
the extension, which is not always equally clearly attributed to the
extension showing the notification. This should not be considered a
trivial permission.

The changed .crx file requested the "notifications" permission, but
didn't use it for anything. I've swapped out for "cookies". The source
does not seem to be checked in (hosted_app/ in the folder is something
else).

TBR=benwells (for app_info_permissions_panel_unittest.cc)
BUG=583480

Committed: https://crrev.com/7d646cfee32db5f81a257a73a8a75acf03da7942
Cr-Commit-Position: refs/heads/master@{#374375}
==========

[commit-bot: I haz the power, 2016-02-09 15:15:46]

Patchset 3 (id:??) landed as
https://crrev.com/7d646cfee32db5f81a257a73a8a75acf03da7942
Cr-Commit-Position: refs/heads/master@{#374375}