Migrate the notification permission to the new common permission classes.

Migrate the notification permission to the new common permission classes.

Move the profile related methods to its own class.

BUG=392145
TBR=vasilii,miket

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285130

[Miguel Garcia, 2014-07-09 18:45:24]

Initial non owner review

[Peter Beverloo, 2014-07-10 11:29:39]

Thanks for the patch! There are a number of architectural concerns which we'll
have to resolve, and various test targets will currently be broken because of
these changes.

Nonetheless, the overall direction looks really good, and it allows for some
nice optimizations in the future :-).

https://codereview.chromium.org/376253005/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/376253005/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:2042: const PermissionRequestID
id(render_frame_host->GetProcess()->GetID(),
Can we name this request_id? There's a lot of Ids in this code.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:2050: true, /* user gesture
always true for notifications */
This is not true anymore, and we'll have to pass through whether the permission
request was caused by a user gesture from the renderer.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:2829: base::Closure callback,
bool result) {
Having |result| be here makes me happy :-). This allows us to eliminate yet
another call in RenderFrameHostImpl.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/376253005/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.h:298: // TODO(beverloo) remove
once the notification callback returns
s/beverloo/peter/. Elsewhere too.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.h:331:
base::WeakPtrFactory<ChromeContentBrowserClient> weak_factory_;
#include "base/memory/weak_ptr.h"

(IWYU)

https://codereview.chromium.org/376253005/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/permission_bubble_request_impl.cc (right):

https://codereview.chromium.org/376253005/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/permission_bubble_request_impl.cc:38: break;
nit: in permission_context_base.cc you add *_NOTIFICATIONS at the end of the
switch, in this file you add the defines at the beginning of the switch.
Consistency (probably in order of the enum's value declarations) would be nice.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/permission_bubble_request_impl.cc:56: message_id
= IDS_NOTIFICATION_PERMISSIONS;
IDS_NOTIFICATION_PERMISSIONS has <ph name="site">,
IDS_MIDI_SYSEX_INFOBAR_QUESTION has <ph name="url"> and
IDS_PUSH_MESSAGES_PERMISSION_QUESTION has <ph name="URL">.

Do these mean anything? Did you confirm that the origin of the permission
request got put in the bubble?

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_infobar_delegate.cc
(right):

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_infobar_delegate.cc:5:
#include <chrome/browser/notifications/desktop_notification_infobar_delegate.h>
Includes of source files should use quotes, not brackets. Also, please have a
blank line between the header file belonging to this .cc and other includes.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_infobar_delegate.cc:40: :
PermissionInfobarDelegate(controller, id, requesting_frame),
nit: four space indent.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_infobar_delegate.h
(right):

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_infobar_delegate.h:16: //
Returns the infobar if it was successfully added.
Nit: continue on the previous line.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_infobar_delegate.h:30: //
ConfirmInfoBarDelegate:
s/ConfirmInfoBarDelegate:/PermissionInfobarDelegate implementation./

I see that PermissionInfobarDelegate inherits from ConfirmInfoBarDelegate, which
actually defines these methods, but referring to a implementation detail of
another class will surely make this comment obsolete at some point in the
future.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_service.cc (left):

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:256:
UMA_HISTOGRAM_COUNTS("NotificationPermissionRequest.Ignored", 1);
We seem to lose this UMA coverage, but I've heard chatter of "the UMA changes".
Could you please list the patches which this one depends on?

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:477: void
DesktopNotificationService::ResetAllOrigins() {
This is still used in notification_browsertest.cc.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:196: void
DesktopNotificationService::GrantPermission(const GURL& origin) {
You remove all calls to DesktopNotificationService::GrantPermission and
DesktopNotificationService::DenyPermission from production code (in favor of
managing this in MessageCenterSettingsController), but there still are some call
sites in test code.

In order to reduce duplication, either the tests should be updated and this
method should be removed, or MessageCenterSettingsController should continue
calling these two methods.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_service.h (right):

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.h:22: #include
"components/keyed_service/core/keyed_service.h"
We don't have to include keyed_service.h directly anymore since it's not being
used in this header file (and we get it implicitly by including
permission_context_base.h).

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/message_center_settings_controller.cc (right):

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
chrome/browser/notifications/message_center_settings_controller.cc:314: void
MessageCenterSettingsController::SetNotifierEnabled(
The MessageCenterSettingsController is the settings provider for the
MessageCenterNotificationManager, which we use as the UI Manager on the desktop
platforms.

On Android, however, we have the NotificationUIManagerAndroid, since we won't be
using the message center. This means that Android won't actually be able to use
the MessageCenterSettingsController, and that we thus would have to duplicate a
lot of the content setting code.

Would it make sense for the settings controller to live before the abstraction,
e.g. in call-sites of DesktopNotificationService before it delegates to the UI
managers?

[Miguel Garcia, 2014-07-17 17:24:48]

A few high level items:

  All the dependencies have landed, including the one for UMA so we should be
able to switch from the old UMA to this one.

  I moved all the operations that deal with the profile to its own class so it
can be used by tests and eventually android in addition to the message centre.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/376253005/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:2042: const PermissionRequestID
id(render_frame_host->GetProcess()->GetID(),
On 2014/07/10 11:29:38, Peter Beverloo wrote:
> Can we name this request_id? There's a lot of Ids in this code.

Done.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:2050: true, /* user gesture
always true for notifications */
Seems out of scope for the CL though, the current code as is forces user_gesture
to true with a very explicit comment

https://cs.corp.google.com/#clankium/src/chrome/browser/notifications/desktop...

Passing the user gesture over IPC is trivial and we can do that on a subsequent
CL. I have added a TODO in the mean time.


On 2014/07/10 11:29:38, Peter Beverloo wrote:
> This is not true anymore, and we'll have to pass through whether the
permission
> request was caused by a user gesture from the renderer.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:2829: base::Closure callback,
bool result) {
On 2014/07/10 11:29:38, Peter Beverloo wrote:
> Having |result| be here makes me happy :-). This allows us to eliminate yet
> another call in RenderFrameHostImpl.

Acknowledged.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/376253005/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.h:298: // TODO(beverloo) remove
once the notification callback returns
On 2014/07/10 11:29:38, Peter Beverloo wrote:
> s/beverloo/peter/. Elsewhere too.

Done.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.h:331:
base::WeakPtrFactory<ChromeContentBrowserClient> weak_factory_;
On 2014/07/10 11:29:38, Peter Beverloo wrote:
> #include "base/memory/weak_ptr.h"
> 
> (IWYU)

Done.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/permission_bubble_request_impl.cc (right):

https://codereview.chromium.org/376253005/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/permission_bubble_request_impl.cc:38: break;
On 2014/07/10 11:29:38, Peter Beverloo wrote:
> nit: in permission_context_base.cc you add *_NOTIFICATIONS at the end of the
> switch, in this file you add the defines at the beginning of the switch.
> Consistency (probably in order of the enum's value declarations) would be
nice.

Done.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/permission_bubble_request_impl.cc:56: message_id
= IDS_NOTIFICATION_PERMISSIONS;
Yes

On 2014/07/10 11:29:39, Peter Beverloo wrote:
> IDS_NOTIFICATION_PERMISSIONS has <ph name="site">,
> IDS_MIDI_SYSEX_INFOBAR_QUESTION has <ph name="url"> and
> IDS_PUSH_MESSAGES_PERMISSION_QUESTION has <ph name="URL">.
> 
> Do these mean anything? Did you confirm that the origin of the permission
> request got put in the bubble?

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_infobar_delegate.cc
(right):

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_infobar_delegate.cc:5:
#include <chrome/browser/notifications/desktop_notification_infobar_delegate.h>
On 2014/07/10 11:29:39, Peter Beverloo wrote:
> Includes of source files should use quotes, not brackets. Also, please have a
> blank line between the header file belonging to this .cc and other includes.

Yeah sorry, eclipse decides <> are better not sure why and I forgot to override
it

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_infobar_delegate.cc:40: :
PermissionInfobarDelegate(controller, id, requesting_frame),
On 2014/07/10 11:29:39, Peter Beverloo wrote:
> nit: four space indent.

Done.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_infobar_delegate.h
(right):

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_infobar_delegate.h:16: //
Returns the infobar if it was successfully added.
I don't follow this one, if I continue of the previous line I have to break the
statement in the middle since it does not fit in 80 chars

On 2014/07/10 11:29:39, Peter Beverloo wrote:
> Nit: continue on the previous line.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_infobar_delegate.h:30: //
ConfirmInfoBarDelegate:
Not sure I fully agree, the whole infobar hierarchy is really well defined and
it's really clear which subclass is responsible of what. However this is a minor
detail so I just changed it.

On 2014/07/10 11:29:39, Peter Beverloo wrote:
> s/ConfirmInfoBarDelegate:/PermissionInfobarDelegate implementation./
> 
> I see that PermissionInfobarDelegate inherits from ConfirmInfoBarDelegate,
which
> actually defines these methods, but referring to a implementation detail of
> another class will surely make this comment obsolete at some point in the
> future.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_service.cc (left):

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:256:
UMA_HISTOGRAM_COUNTS("NotificationPermissionRequest.Ignored", 1);
On 2014/07/10 11:29:39, Peter Beverloo wrote:
> We seem to lose this UMA coverage, but I've heard chatter of "the UMA
changes".
> Could you please list the patches which this one depends on?

I think we will, I decided to drop it since it only seems to be available for
infobars so the minute it switches to bubbles it would be lost anyway. I think
it'd be better to add a similar UMA to the generic class so it is tracked for
all permissions.

Happy to discuss offline. In any case I added the UMA dependency patch in the
description

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:477: void
DesktopNotificationService::ResetAllOrigins() {
On 2014/07/10 11:29:39, Peter Beverloo wrote:
> This is still used in notification_browsertest.cc.

Yeah there were two tests broken, fixed them now.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_service.h (right):

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.h:22: #include
"components/keyed_service/core/keyed_service.h"
On 2014/07/10 11:29:39, Peter Beverloo wrote:
> We don't have to include keyed_service.h directly anymore since it's not being
> used in this header file (and we get it implicitly by including
> permission_context_base.h).

Done.

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/message_center_settings_controller.cc (right):

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
chrome/browser/notifications/message_center_settings_controller.cc:314: void
MessageCenterSettingsController::SetNotifierEnabled(
Let's talk about this one offline ok? I think it deserves some design discussion
and goes beyond the scope of this refactor.



On 2014/07/10 11:29:39, Peter Beverloo wrote:
> The MessageCenterSettingsController is the settings provider for the
> MessageCenterNotificationManager, which we use as the UI Manager on the
desktop
> platforms.
> 
> On Android, however, we have the NotificationUIManagerAndroid, since we won't
be
> using the message center. This means that Android won't actually be able to
use
> the MessageCenterSettingsController, and that we thus would have to duplicate
a
> lot of the content setting code.
> 
> Would it make sense for the settings controller to live before the
abstraction,
> e.g. in call-sites of DesktopNotificationService before it delegates to the UI
> managers?

[Peter Beverloo, 2014-07-18 10:42:38]

I like this, thanks! Let's do one more iteration?

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_infobar_delegate.h
(right):

https://codereview.chromium.org/376253005/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_infobar_delegate.h:16: //
Returns the infobar if it was successfully added.
On 2014/07/17 17:24:48, Miguel Garcia wrote:
> I don't follow this one, if I continue of the previous line I have to break
the
> statement in the middle since it does not fit in 80 chars
> 
> On 2014/07/10 11:29:39, Peter Beverloo wrote:
> > Nit: continue on the previous line.
> 

Ok. I don't feel strongly about it, generally we treat comments as a single
paragraph.

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:2134: render_frame_host);
nit: four space indent

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:2136: const PermissionRequestID
id(render_process_id,
nit: s/id/request_id/ for consistency.

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:2137:
web_contents->GetRoutingID(), -1 /* bridge id */,
nit: one argument a line.

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:2147:
weak_factory_.GetWeakPtr(), callback));
nit: |callback| should be on its own line.

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.h:302: void
NotificationPermissionRequested(base::Closure callback, bool result);
const base::Closure&

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.h:302: void
NotificationPermissionRequested(base::Closure callback, bool result);
NotificationPermissionRequested doesn't modify |this|, so please make this
const.

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_bubble_request_impl.cc (right):

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_bubble_request_impl.cc:44: #endif  //
ENABLE_NOTIFICATIONS
nit: don't need the comment after the #endif if the block is <~5 lines.

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_uma_util.cc (right):

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_uma_util.cc:39: static const
char* kNotificationsUmaKey =
All of these should be |const char[] kFoo = "bar";|. Since they're defined in an
anonymous namespace, they don't have to be declared as |static| either.

That said, we rarely use constants like this to indicate the name of the metric
unless it's being used in several places, which they're not. Why don't just have
the strings in the calls themselves (lines 50, 55 and 60)?

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_profile_util.cc (right):

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_profile_util.cc:11: 
micro nit: two white lines -> one of 'em.

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_profile_util.cc:15:
profile->GetHostContentSettingsMap()->SetDefaultContentSetting(
I'm in flux. Nothing in this class uses anything from Profile other than
HostContentSettingsMap, so I'm erring towards naming this
/DesktopNotificationContentSettingsUtil/ taking HostContentSettingsMap* rather
than Profile*.

However, that'd increase noise for all the callees, given that they have the
Profile, and we'd have to call GetHostContentSettingsMap() everywhere.

Fine to leave as it is...

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_profile_util.cc:20: void
DesktopNotificationProfileUtil::ClearSetting(
static methods should be annotated with a comment that says "// static". The
comments about what the methods do are much more useful in the header file.

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_profile_util.cc:32: void
DesktopNotificationProfileUtil::ResetAllOrigins(Profile* profile) {
This method is only used for NotificationTest. Can we drop this from this class,
and instead just implement it in NotificationTest::AllowAllOrigins()?

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_profile_util.cc:79: void
DesktopNotificationProfileUtil::SetDefaultContentSetting(
Dito, we can implement this as a helper method in NotificationTest.

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_profile_util.h (right):

https://codereview.chromium.org/376253005/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_profile_util.h:12: // A series
of common operations to interact with the profile's notification
s/notification/Desktop Notification/ for now?

[Miguel Garcia, 2014-07-18 12:27:09]

Adding owners for the different directories too

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/ch...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/ch...
chrome/browser/chrome_content_browser_client.cc:2134: render_frame_host);
On 2014/07/18 10:42:38, Peter Beverloo wrote:
> nit: four space indent

Done.

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/ch...
chrome/browser/chrome_content_browser_client.cc:2136: const PermissionRequestID
id(render_process_id,
On 2014/07/18 10:42:37, Peter Beverloo wrote:
> nit: s/id/request_id/ for consistency.

Done.

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/ch...
chrome/browser/chrome_content_browser_client.cc:2137:
web_contents->GetRoutingID(), -1 /* bridge id */,
On 2014/07/18 10:42:37, Peter Beverloo wrote:
> nit: one argument a line.

Done.

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/ch...
chrome/browser/chrome_content_browser_client.cc:2147:
weak_factory_.GetWeakPtr(), callback));
On 2014/07/18 10:42:37, Peter Beverloo wrote:
> nit: |callback| should be on its own line.

Done.

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/ch...
File chrome/browser/chrome_content_browser_client.h (right):

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/ch...
chrome/browser/chrome_content_browser_client.h:302: void
NotificationPermissionRequested(base::Closure callback, bool result);
On 2014/07/18 10:42:38, Peter Beverloo wrote:
> NotificationPermissionRequested doesn't modify |this|, so please make this
> const.

Acknowledged.

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/ch...
chrome/browser/chrome_content_browser_client.h:302: void
NotificationPermissionRequested(base::Closure callback, bool result);
On 2014/07/18 10:42:38, Peter Beverloo wrote:
> NotificationPermissionRequested doesn't modify |this|, so please make this
> const.

Done.

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/co...
File chrome/browser/content_settings/permission_bubble_request_impl.cc (right):

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/co...
chrome/browser/content_settings/permission_bubble_request_impl.cc:44: #endif  //
ENABLE_NOTIFICATIONS
On 2014/07/18 10:42:38, Peter Beverloo wrote:
> nit: don't need the comment after the #endif if the block is <~5 lines.

Done.

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/co...
File chrome/browser/content_settings/permission_context_uma_util.cc (right):

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/co...
chrome/browser/content_settings/permission_context_uma_util.cc:39: static const
char* kNotificationsUmaKey =
On 2014/07/18 10:42:38, Peter Beverloo wrote:
> All of these should be |const char[] kFoo = "bar";|. Since they're defined in
an
> anonymous namespace, they don't have to be declared as |static| either.
> 
> That said, we rarely use constants like this to indicate the name of the
metric
> unless it's being used in several places, which they're not. Why don't just
have
> the strings in the calls themselves (lines 50, 55 and 60)?

Done.

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/no...
File chrome/browser/notifications/desktop_notification_profile_util.cc (right):

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/no...
chrome/browser/notifications/desktop_notification_profile_util.cc:11: 
On 2014/07/18 10:42:38, Peter Beverloo wrote:
> micro nit: two white lines -> one of 'em.

Done.

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/no...
chrome/browser/notifications/desktop_notification_profile_util.cc:15:
profile->GetHostContentSettingsMap()->SetDefaultContentSetting(
On 2014/07/18 10:42:38, Peter Beverloo wrote:
> I'm in flux. Nothing in this class uses anything from Profile other than
> HostContentSettingsMap, so I'm erring towards naming this
> /DesktopNotificationContentSettingsUtil/ taking HostContentSettingsMap* rather
> than Profile*.
> 
> However, that'd increase noise for all the callees, given that they have the
> Profile, and we'd have to call GetHostContentSettingsMap() everywhere.
> 
> Fine to leave as it is...

Acknowledged.

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/no...
chrome/browser/notifications/desktop_notification_profile_util.cc:20: void
DesktopNotificationProfileUtil::ClearSetting(
I think we usually do this when the class has both static and not static
methods. This class only allows static methods (it has a private constructor) so
I think it's clear and would add noise to add //static to every single method in
the file.

On 2014/07/18 10:42:38, Peter Beverloo wrote:
> static methods should be annotated with a comment that says "// static". The
> comments about what the methods do are much more useful in the header file.

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/no...
chrome/browser/notifications/desktop_notification_profile_util.cc:32: void
DesktopNotificationProfileUtil::ResetAllOrigins(Profile* profile) {
On 2014/07/18 10:42:38, Peter Beverloo wrote:
> This method is only used for NotificationTest. Can we drop this from this
class,
> and instead just implement it in NotificationTest::AllowAllOrigins()?

Good idea, it was only used once in the test so I just inlined it.

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/no...
chrome/browser/notifications/desktop_notification_profile_util.cc:79: void
DesktopNotificationProfileUtil::SetDefaultContentSetting(
On 2014/07/18 10:42:38, Peter Beverloo wrote:
> Dito, we can implement this as a helper method in NotificationTest.

Done.

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/no...
File chrome/browser/notifications/desktop_notification_profile_util.h (right):

https://chromiumcodereview.appspot.com/376253005/diff/40001/chrome/browser/no...
chrome/browser/notifications/desktop_notification_profile_util.h:12: // A series
of common operations to interact with the profile's notification
On 2014/07/18 10:42:38, Peter Beverloo wrote:
> s/notification/Desktop Notification/ for now?

Done.

[Miguel Garcia, 2014-07-18 12:29:24]

miket: for extenstions
mpearson: for histograms
bauerb: for content settings
miket: for the changes in notifications

[Miguel Garcia, 2014-07-18 12:34:26]

On 2014/07/18 12:29:24, Miguel Garcia wrote:
> miket: for extenstions
> mpearson: for histograms
> bauerb: for content settings
> miket: for the changes in notifications
mukai: for the changes in notifications

[Bernhard Bauer, 2014-07-18 14:10:19]

content_settings/ LGTM

[Mark P, 2014-07-18 14:14:20]

histograms.xml lgtm

[Jun Mukai, 2014-07-18 17:33:19]

notification lgtm

[Miguel Garcia, 2014-07-22 17:51:02]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-07-22 17:53:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miguelg@chromium.org/376253005/80001

[commit-bot: I haz the power, 2014-07-22 20:03:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)

[commit-bot: I haz the power, 2014-07-22 20:13:43]

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

[commit-bot: I haz the power, 2014-07-22 20:13:44]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)

[Miguel Garcia, 2014-07-23 19:25:49]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-07-23 19:28:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miguelg@chromium.org/376253005/100001

[commit-bot: I haz the power, 2014-07-24 02:55:40]

Change committed as 285130