Refactor notification operation functionality out of PNS

Refactor notification operation functionality out of PNS

Move to its own file
- The enum with the different operations exercesied in a notification
- The ability to open notification settings

This is some prework before introducing notification operation handlers

BUG=571056
Committed: https://crrev.com/3cc06ec198342fba03eea9c2a265ca89cb39dff1
Cr-Commit-Position: refs/heads/master@{#403128}

[Miguel Garcia, 2016-06-25 21:56:00]

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

[Peter Beverloo, 2016-06-28 22:52:41]

Could you detail what the rest of the "common" behaviour is?

I suspect that it won't be as common as you suspect— the target page for the
settings button is not appropriate for non-Web Notifications, so this most
likely will have to become handler-specific behaviour.

https://codereview.chromium.org/2095223002/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/notification_operation_common.cc (right):

https://codereview.chromium.org/2095223002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_operation_common.cc:31: 
nit: --whitespace of lines 19 and 31. (Sorry, I know you're just moving the
code.)

https://codereview.chromium.org/2095223002/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/notification_operation_common.h (right):

https://codereview.chromium.org/2095223002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_operation_common.h:8: // A series of
common operations for both in page and persistent notification
Remove. (A class-level comment would be appropriate. Due to the missing
whitespace this now looks like it applies to the BrowserContext
forward-declaration.)

https://codereview.chromium.org/2095223002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_operation_common.h:13: namespace
notification_operation_common {
Please stash common behaviour like this in a class rather than in a namespace.
The former is *much* more common throughout Chromium, and it's beneficial to
maintain consistency. (Mac UI code seems to be a common exception.)

https://codereview.chromium.org/2095223002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_operation_common.h:23: void
OpenNotificationSettings(content::BrowserContext* browser_context);
This should probably take a Profile* since it's not a //content-called function.
OK to do later, but please add a TODO.

[Miguel Garcia, 2016-06-29 17:25:40]

https://chromiumcodereview.appspot.com/2095223002/diff/20001/chrome/browser/n...
File chrome/browser/notifications/notification_operation_common.cc (right):

https://chromiumcodereview.appspot.com/2095223002/diff/20001/chrome/browser/n...
chrome/browser/notifications/notification_operation_common.cc:31: 
On 2016/06/28 22:52:41, Peter Beverloo wrote:
> nit: --whitespace of lines 19 and 31. (Sorry, I know you're just moving the
> code.)

Done.

https://chromiumcodereview.appspot.com/2095223002/diff/20001/chrome/browser/n...
File chrome/browser/notifications/notification_operation_common.h (right):

https://chromiumcodereview.appspot.com/2095223002/diff/20001/chrome/browser/n...
chrome/browser/notifications/notification_operation_common.h:8: // A series of
common operations for both in page and persistent notification
On 2016/06/28 22:52:41, Peter Beverloo wrote:
> Remove. (A class-level comment would be appropriate. Due to the missing
> whitespace this now looks like it applies to the BrowserContext
> forward-declaration.)

Acknowledged.

https://chromiumcodereview.appspot.com/2095223002/diff/20001/chrome/browser/n...
chrome/browser/notifications/notification_operation_common.h:13: namespace
notification_operation_common {
On 2016/06/28 22:52:41, Peter Beverloo wrote:
> Please stash common behaviour like this in a class rather than in a namespace.
> The former is *much* more common throughout Chromium, and it's beneficial to
> maintain consistency. (Mac UI code seems to be a common exception.)

Done.

https://chromiumcodereview.appspot.com/2095223002/diff/20001/chrome/browser/n...
chrome/browser/notifications/notification_operation_common.h:23: void
OpenNotificationSettings(content::BrowserContext* browser_context);
On 2016/06/28 22:52:41, Peter Beverloo wrote:
> This should probably take a Profile* since it's not a //content-called
function.
> OK to do later, but please add a TODO.

Todo added but why exactly do we take a BrowserContext in
NotificationObjectProxy? That's the only reason why this is a browser context
and not a profile.

[Peter Beverloo, 2016-06-29 17:29:44]

Could you please answer the question I left in my previous comment?

On 2016/06/29 17:25:40, Miguel Garcia wrote:
>
https://chromiumcodereview.appspot.com/2095223002/diff/20001/chrome/browser/n...
> chrome/browser/notifications/notification_operation_common.h:23: void
> OpenNotificationSettings(content::BrowserContext* browser_context);
> On 2016/06/28 22:52:41, Peter Beverloo wrote:
> > This should probably take a Profile* since it's not a //content-called
> function.
> > OK to do later, but please add a TODO.
> 
> Todo added but why exactly do we take a BrowserContext in
> NotificationObjectProxy? That's the only reason why this is a browser context
> and not a profile.

I recall going through a period where I preferred using BrowserContext. Sorry
:-).

The only call-site of the NotificationObjectProxy has both, so it's trivial to
update at some point.

[Peter Beverloo, 2016-06-29 17:32:29]

lgtm % answering the question

https://chromiumcodereview.appspot.com/2095223002/diff/60001/chrome/browser/n...
File chrome/browser/notifications/notification_common.h (right):

https://chromiumcodereview.appspot.com/2095223002/diff/60001/chrome/browser/n...
chrome/browser/notifications/notification_common.h:19: NOTIFICATION_SETTINGS = 2
nit: since it's scoped to the NotificationCommon class, no need to prefix with
Notification again. What about {CLICK, CLOSE, SETTINGS}? You could prefix with
OPERATION_ if you so desire.

[Miguel Garcia, 2016-06-29 17:38:22]

> Could you detail what the rest of the "common" behaviour is?
Oh sorry I thought the following CLs answered it, in the short term I am
adding another enum for the type of notification. Keeping those enums in
the platform notification service would create even more circular
dependencies and I want to minimize those.

Moving forward and depending how the process of getting rid of the
delegates go I could see us adding support for Closing and Clicking as
well. If extension notifications can use them even better but I am not
counting on it.



On Wed, Jun 29, 2016 at 6:32 PM, <peter@chromium.org> wrote:

> lgtm % answering the question
>
>
>
>
https://chromiumcodereview.appspot.com/2095223002/diff/60001/chrome/browser/n...
> File chrome/browser/notifications/notification_common.h (right):
>
>
>
https://chromiumcodereview.appspot.com/2095223002/diff/60001/chrome/browser/n...
> chrome/browser/notifications/notification_common.h:19:
> NOTIFICATION_SETTINGS = 2
> nit: since it's scoped to the NotificationCommon class, no need to
> prefix with Notification again. What about {CLICK, CLOSE, SETTINGS}? You
> could prefix with OPERATION_ if you so desire.
>
> https://chromiumcodereview.appspot.com/2095223002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Miguel Garcia, 2016-06-30 08:19:55]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2016-06-30 08:19:55]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/2095223002/#ps80001
(title: "remove enum suffix")

[commit-bot: I haz the power, 2016-06-30 08:20:11]

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, 2016-06-30 09:14:23]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-30 09:14:25]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 09:15:40]

Description was changed from

==========
Refactor notification operation functionality out of PNS

Move to its own file
- The enum with the different operations exercesied in a notification
- The ability to open notification settings

This is some prework before introducing notification operation handlers

BUG=571056
==========

to

==========
Refactor notification operation functionality out of PNS

Move to its own file
- The enum with the different operations exercesied in a notification
- The ability to open notification settings

This is some prework before introducing notification operation handlers

BUG=571056

Committed: https://crrev.com/3cc06ec198342fba03eea9c2a265ca89cb39dff1
Cr-Commit-Position: refs/heads/master@{#403128}
==========

[commit-bot: I haz the power, 2016-06-30 09:15:41]

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

[Miguel Garcia, 2016-07-01 10:49:09]

https://chromiumcodereview.appspot.com/2095223002/diff/60001/chrome/browser/n...
File chrome/browser/notifications/notification_common.h (right):

https://chromiumcodereview.appspot.com/2095223002/diff/60001/chrome/browser/n...
chrome/browser/notifications/notification_common.h:19: NOTIFICATION_SETTINGS = 2
On 2016/06/29 17:32:29, Peter Beverloo wrote:
> nit: since it's scoped to the NotificationCommon class, no need to prefix with
> Notification again. What about {CLICK, CLOSE, SETTINGS}? You could prefix with
> OPERATION_ if you so desire.

The shorter the better, so I went with CLICK,CLOSE,SETTINGS.