Add menu option for disabling app notifications

Add menu option for disabling app notifications

This should only show up for apps which have the appNotifications permission
in their manifest.

BUG=106268
TEST=Install an app with the appNotifications permission - the icon on the
NTP should get a 'Disable Notifications' context menu entry which if checked
will keep the notification bubbles from appearing over the app's icon.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113111

[asargent_no_longer_on_chrome, 2011-12-03 05:27:32]

finnur - please review all changes
estade - please review NTP stuff

[Finnur, 2011-12-05 16:15:15]

http://codereview.chromium.org/8785016/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8785016/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.cc:1450: id,
extension_sync_data.notifications_disabled());
Wait... Isn't one possibility here that app notifications is disabled for one
id, but extension sync data is enabled, so we flip the enabled bit for this id
to true?

Also, nit: Multi-line if clause, needs braces.

http://codereview.chromium.org/8785016/diff/1/chrome/browser/resources/ntp4/a...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/8785016/diff/1/chrome/browser/resources/ntp4/a...
chrome/browser/resources/ntp4/apps_page.js:139: var notificationsDisabled =
app.appData_.notifications_disabled;
The underscore in appData_ denotes private, you shouldn't access it directly.
You'll see elsewhere in this class we use the accessor (same name, but without
the underscore) and you should do that here (and elsewhere in this change). In
other words: 

app.appData   and not...
app.appData_

http://codereview.chromium.org/8785016/diff/1/chrome/browser/resources/ntp4/a...
chrome/browser/resources/ntp4/apps_page.js:172: if (!currentlyDisabled &&
app.currentBubbleShowing_) {
same problem here, _ means private.

[asargent_no_longer_on_chrome, 2011-12-05 17:42:39]

Addressed comments. Please look again (Hoping to land this today to make M17
cutoff)

Thanks!

http://codereview.chromium.org/8785016/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8785016/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.cc:1450: id,
extension_sync_data.notifications_disabled());
On 2011/12/05 16:15:16, Finnur wrote:
> Wait... Isn't one possibility here that app notifications is disabled for one
> id, but extension sync data is enabled, so we flip the enabled bit for this id
> to true?

|extension_sync_data| is specific to a particular extension (it's an object
encapsulating the state we push around via sync like enabled/disabled, allow in
incognito, etc.). Note that we grab |id| from it up above in this function.
 

> 
> Also, nit: Multi-line if clause, needs braces.
> 

Done.

http://codereview.chromium.org/8785016/diff/1/chrome/browser/resources/ntp4/a...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/8785016/diff/1/chrome/browser/resources/ntp4/a...
chrome/browser/resources/ntp4/apps_page.js:139: var notificationsDisabled =
app.appData_.notifications_disabled;
On 2011/12/05 16:15:16, Finnur wrote:
> The underscore in appData_ denotes private, you shouldn't access it directly.
> You'll see elsewhere in this class we use the accessor (same name, but without
> the underscore) and you should do that here (and elsewhere in this change). In
> other words: 
> 
> app.appData   and not...
> app.appData_

Done.

http://codereview.chromium.org/8785016/diff/1/chrome/browser/resources/ntp4/a...
chrome/browser/resources/ntp4/apps_page.js:172: if (!currentlyDisabled &&
app.currentBubbleShowing_) {
On 2011/12/05 16:15:16, Finnur wrote:
> same problem here, _ means private.

Done.

[Finnur, 2011-12-05 17:49:59]

http://codereview.chromium.org/8785016/diff/1/chrome/browser/resources/ntp4/a...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/8785016/diff/1/chrome/browser/resources/ntp4/a...
chrome/browser/resources/ntp4/apps_page.js:172: if (!currentlyDisabled &&
app.currentBubbleShowing_) {
I was referring to currentBubbleShowing_ ...

 On 2011/12/05 16:15:16, Finnur wrote:
> same problem here, _ means private.

[asargent_no_longer_on_chrome, 2011-12-05 18:21:05]

http://codereview.chromium.org/8785016/diff/1/chrome/browser/resources/ntp4/a...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/8785016/diff/1/chrome/browser/resources/ntp4/a...
chrome/browser/resources/ntp4/apps_page.js:172: if (!currentlyDisabled &&
app.currentBubbleShowing_) {
On 2011/12/05 17:50:00, Finnur wrote:
> I was referring to currentBubbleShowing_ ...
> 
>  On 2011/12/05 16:15:16, Finnur wrote:
> > same problem here, _ means private.
> 

Oops, fixed. I added a method called removeBubble instead of directly
manipulating currentBubbleShowing. here.

[Finnur, 2011-12-05 18:24:22]

lgtm

[Evan Stade, 2011-12-05 22:04:16]

http://codereview.chromium.org/8785016/diff/9001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/8785016/diff/9001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:8957: Disable Notifications
this should be sentence case on windows, and title case on mac/linux. But it
seems we don't do this for the rest of the menu items in this particular context
menu. In lieu of fixing the entire app menu, you should at least keep it
consistent (i.e. make it sentence case).

http://codereview.chromium.org/8785016/diff/9001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/8785016/diff/9001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:171: var currentlyDisabled =
app.appData.notifications_disabled;
could you instead do

var disabled = this.disableNotifications_.checked;

this seems less error prone

http://codereview.chromium.org/8785016/diff/9001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:172: if (!currentlyDisabled) {
no curlies.

also, what is the advantage of this check?

[asargent_no_longer_on_chrome, 2011-12-05 23:09:14]

http://codereview.chromium.org/8785016/diff/9001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/8785016/diff/9001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:8957: Disable Notifications
On 2011/12/05 22:04:16, Evan Stade wrote:
> this should be sentence case on windows, and title case on mac/linux. But it
> seems we don't do this for the rest of the menu items in this particular
context
> menu. In lieu of fixing the entire app menu, you should at least keep it
> consistent (i.e. make it sentence case).

Fixed to sentence case for now - I've filed crbug.com/106475 as a follow-up bug
about title case on Mac/Linux.

http://codereview.chromium.org/8785016/diff/9001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/8785016/diff/9001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:171: var currentlyDisabled =
app.appData.notifications_disabled;
On 2011/12/05 22:04:16, Evan Stade wrote:
> could you instead do
> 
> var disabled = this.disableNotifications_.checked;
> 
> this seems less error prone

Done.

http://codereview.chromium.org/8785016/diff/9001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:172: if (!currentlyDisabled) {
On 2011/12/05 22:04:16, Evan Stade wrote:
> no curlies.
> 
> also, what is the advantage of this check?

Good point - if we're toggling from disabled to enabled, there shouldn't be a
bubble anyway. Removed the check.

[Evan Stade, 2011-12-05 23:19:32]

http://codereview.chromium.org/8785016/diff/4002/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/8785016/diff/4002/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:8957: Disable notifications
sorry I didn't think of this prior, but it seems better to keep action strings
positive, i.e. this could be "Allow notifications" and its meaning inverted of
course. Perhaps best to get a UI lead to weigh in.

http://codereview.chromium.org/8785016/diff/4002/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/8785016/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:173:
app.appData.notifications_disabled = !disabled;
as it stands, isn't this backwards

http://codereview.chromium.org/8785016/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:175: [app.appData.id,
app.appData.notifications_disabled]);
nit: s/app.appData.notifications_disabled/disabled/

[asargent_no_longer_on_chrome, 2011-12-06 00:14:55]

http://codereview.chromium.org/8785016/diff/4002/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/8785016/diff/4002/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:8957: Disable notifications
On 2011/12/05 23:19:32, Evan Stade wrote:
> sorry I didn't think of this prior, but it seems better to keep action strings
> positive, i.e. this could be "Allow notifications" and its meaning inverted of
> course. Perhaps best to get a UI lead to weigh in.

Actually just a little while ago today Glen chimed in on a thread suggesting the
same thing, with the verb of either "Enable" or "Allow". I need to pin down
which of the two we want, but would like to proceed with this CL for now. (BTW,
this entire feature is going to be gated with a webstore service with just
trusted testers for a while - the messages that show up get sent to this service
and delivered through the sync channel. So we just want to get the functionality
into the client for M17 so we can dogfood a while and make incremental
improvements based on dogfooding).

I have a separate CL getting the strings in for the translation cutoff
(http://codereview.chromium.org/8801032/), so we can do a patch to switch if we
want.

http://codereview.chromium.org/8785016/diff/4002/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/8785016/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:173:
app.appData.notifications_disabled = !disabled;
On 2011/12/05 23:19:32, Evan Stade wrote:
> as it stands, isn't this backwards

Nope, this event handler toggles the sense of the "disabled" flag, so it's
supposed to change from true to false. 

I did a slight refactor of these lines of code to try to make this more clear.

http://codereview.chromium.org/8785016/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:175: [app.appData.id,
app.appData.notifications_disabled]);
On 2011/12/05 23:19:32, Evan Stade wrote:
> nit: s/app.appData.notifications_disabled/disabled/

fixed by refactor

[Evan Stade, 2011-12-06 01:27:08]

lgtm

http://codereview.chromium.org/8785016/diff/4002/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/8785016/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:173:
app.appData.notifications_disabled = !disabled;
On 2011/12/06 00:14:55, Antony Sargent wrote:
> On 2011/12/05 23:19:32, Evan Stade wrote:
> > as it stands, isn't this backwards
> 
> Nope, this event handler toggles the sense of the "disabled" flag, so it's
> supposed to change from true to false. 
> 
> I did a slight refactor of these lines of code to try to make this more clear.

ah, I think the confusion is about whether checked refers to before or after the
click.

[commit-bot: I haz the power, 2011-12-06 01:35:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asargent@chromium.org/8785016/6003