Clean up the Notification class in advance of merging the Close methods

Clean up the Notification class in advance of merging the Close methods

  - Moves from class to enum class for notification state and type.
  - Notification instances now have clear knowledge of their type.
  - Don't instantiate an AsyncMethodRunner for non-persistent ones.
  - Simplify the lifetime of Notification instances: non-persistent
    notifications now follow their state machine, whereas persistent
    notifications now follow regular GC rules.

This will make it somewhat easier to merge the close{Persistent,}()
methods on the WebNotificationManager.

BUG=

Committed: https://crrev.com/c1e8be7b2f4b9d22ff8c596868ab9ed4e77dd956
Cr-Commit-Position: refs/heads/master@{#419183}

[Peter Beverloo, 2016-09-13 13:59:34]

peter@chromium.org changed reviewers:
+ harkness@chromium.org, johnme@chromium.org

[Peter Beverloo, 2016-09-13 13:59:34]

+harkness for review
+johnme for secondary review and OWNERS

[harkness, 2016-09-14 16:23:46]

LGTM % comment

https://codereview.chromium.org/2338483004/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/2338483004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/Notification.cpp:182:
SecurityOrigin* origin = getExecutionContext()->getSecurityOrigin();
It looks like you only use the origin in the else, so I think if you move it
there, you can merge the 175 and 185 if statements. 

If you think you need to check that the origin is secure in both cases, you
could move the check to before line 173 and then merge the if statements as
well.

[Peter Beverloo, 2016-09-14 16:51:07]

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

[Peter Beverloo, 2016-09-14 16:51:19]

Thank you! John?

https://codereview.chromium.org/2338483004/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/2338483004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/Notification.cpp:182:
SecurityOrigin* origin = getExecutionContext()->getSecurityOrigin();
On 2016/09/14 16:23:46, harkness wrote:
> It looks like you only use the origin in the else, so I think if you move it
> there, you can merge the 175 and 185 if statements. 
> 
> If you think you need to check that the origin is secure in both cases, you
> could move the check to before line 173 and then merge the if statements as
> well.

Oops, this is a left over from the more ambitious clean-up I started doing :)
Done.

[commit-bot: I haz the power, 2016-09-14 16:51:23]

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, 2016-09-14 18:14:15]

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

[commit-bot: I haz the power, 2016-09-14 18:14:16]

Dry run: This issue passed the CQ dry run.

[johnme, 2016-09-16 14:14:35]

Nice cleanup. lgtm with nits

https://codereview.chromium.org/2338483004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/2338483004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.cpp:182: if (m_type
== NotificationType::NonPersistent) {
Nit: I'm not sure splitting this into two if-elses with identical conditions
improved readability?

https://codereview.chromium.org/2338483004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/2338483004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.h:130: enum class
NotificationType {
Nit: s/Notification::NotificationType/Notification::Type/

https://codereview.chromium.org/2338483004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.h:136: enum class
NotificationState {
Nit: s/Notification::NotificationState/Notification::State/

https://codereview.chromium.org/2338483004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.h:145: // Sets the
state of the notification in its lifetime.
Nit: s/lifetime/lifecycle/ ?

[Peter Beverloo, 2016-09-16 14:45:37]

https://codereview.chromium.org/2338483004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/2338483004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.cpp:182: if (m_type
== NotificationType::NonPersistent) {
On 2016/09/16 14:14:35, johnme wrote:
> Nit: I'm not sure splitting this into two if-elses with identical conditions
> improved readability?

Done.

https://codereview.chromium.org/2338483004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/2338483004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.h:130: enum class
NotificationType {
On 2016/09/16 14:14:35, johnme wrote:
> Nit: s/Notification::NotificationType/Notification::Type/

Done.

https://codereview.chromium.org/2338483004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.h:136: enum class
NotificationState {
On 2016/09/16 14:14:35, johnme wrote:
> Nit: s/Notification::NotificationState/Notification::State/

Done.

https://codereview.chromium.org/2338483004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.h:145: // Sets the
state of the notification in its lifetime.
On 2016/09/16 14:14:35, johnme wrote:
> Nit: s/lifetime/lifecycle/ ?

Done.

[Peter Beverloo, 2016-09-16 14:45:50]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2016-09-16 14:45:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
harkness@chromium.org, johnme@chromium.org
Link to the patchset: https://codereview.chromium.org/2338483004/#ps40001
(title: "Clean up the Notification class in advance of merging the Close
methods")

[commit-bot: I haz the power, 2016-09-16 14:46:10]

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-09-16 16:21:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-16 16:23:13]

Description was changed from

==========
Clean up the Notification class in advance of merging the Close methods

  - Moves from class to enum class for notification state and type.
  - Notification instances now have clear knowledge of their type.
  - Don't instantiate an AsyncMethodRunner for non-persistent ones.
  - Simplify the lifetime of Notification instances: non-persistent
    notifications now follow their state machine, whereas persistent
    notifications now follow regular GC rules.

This will make it somewhat easier to merge the close{Persistent,}()
methods on the WebNotificationManager.

BUG=
==========

to

==========
Clean up the Notification class in advance of merging the Close methods

  - Moves from class to enum class for notification state and type.
  - Notification instances now have clear knowledge of their type.
  - Don't instantiate an AsyncMethodRunner for non-persistent ones.
  - Simplify the lifetime of Notification instances: non-persistent
    notifications now follow their state machine, whereas persistent
    notifications now follow regular GC rules.

This will make it somewhat easier to merge the close{Persistent,}()
methods on the WebNotificationManager.

BUG=

Committed: https://crrev.com/c1e8be7b2f4b9d22ff8c596868ab9ed4e77dd956
Cr-Commit-Position: refs/heads/master@{#419183}
==========

[commit-bot: I haz the power, 2016-09-16 16:23:14]

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