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=

Review-Url: https://codereview.chromium.org/2338483004
Cr-Commit-Position: refs/heads/master@{#419183}

third_party/WebKit/Source/modules/notifications/Notification.cpp

@@ -100,29 +100,29 @@ Notification* Notification::create(ExecutionContext* context, const String& titl
     if (exceptionState.hadException())
         return nullptr;
 
-    Notification* notification = new Notification(context, data);
+    Notification* notification = new Notification(context, Type::NonPersistent, data);
     notification->schedulePrepareShow();
-    notification->suspendIfNeeded();
 
+    notification->suspendIfNeeded();
     return notification;
 }
 
 Notification* Notification::create(ExecutionContext* context, const String& notificationId, const WebNotificationData& data, bool showing)
 {
-    Notification* notification = new Notification(context, data);
+    Notification* notification = new Notification(context, Type::Persistent, data);
+    notification->setState(showing ? State::Showing : State::Closed);
     notification->setNotificationId(notificationId);
-    notification->setState(showing ? NotificationStateShowing : NotificationStateClosed);
-    notification->suspendIfNeeded();
 
+    notification->suspendIfNeeded();
     return notification;
 }
 
-Notification::Notification(ExecutionContext* context, const WebNotificationData& data)
+Notification::Notification(ExecutionContext* context, Type type, const WebNotificationData& data)
     : ActiveScriptWrappable(this)
     , ActiveDOMObject(context)
+    , m_type(type)
+    , m_state(State::Loading)
     , m_data(data)
-    , m_state(NotificationStateIdle)
-    , m_prepareShowMethodRunner(AsyncMethodRunner<Notification>::create(this, &Notification::prepareShow))
 {
     DCHECK(notificationManager());
 }
@@ -133,15 +133,16 @@ Notification::~Notification()
 
 void Notification::schedulePrepareShow()
 {
-    DCHECK_EQ(m_state, NotificationStateIdle);
-    DCHECK(!m_prepareShowMethodRunner->isActive());
+    DCHECK_EQ(m_state, State::Loading);
+    DCHECK(!m_prepareShowMethodRunner);
 
+    m_prepareShowMethodRunner = AsyncMethodRunner<Notification>::create(this, &Notification::prepareShow);
     m_prepareShowMethodRunner->runAsync();
 }
 
 void Notification::prepareShow()
 {
-    DCHECK_EQ(m_state, NotificationStateIdle);
+    DCHECK_EQ(m_state, State::Loading);
     if (NotificationManager::from(getExecutionContext())->permissionStatus() != mojom::blink::PermissionStatus::GRANTED) {
         dispatchErrorEvent();
         return;
@@ -161,28 +162,30 @@ void Notification::didLoadResources(NotificationResourcesLoader* loader)
     notificationManager()->show(WebSecurityOrigin(origin), m_data, loader->getResources(), this);
     m_loader.clear();
 
-    m_state = NotificationStateShowing;
+    m_state = State::Showing;
 }
 
 void Notification::close()
 {
-    if (m_state != NotificationStateShowing)
+    if (m_state != State::Showing)
         return;
 
-    if (m_notificationId.isNull()) {
-        // Fire the close event asynchronously.
+    // Schedule the "close" event to be fired for non-persistent notifications.
+    // Persistent notifications won't get such events for programmatic closes.
+    if (m_type == Type::NonPersistent) {
         getExecutionContext()->postTask(BLINK_FROM_HERE, createSameThreadTask(&Notification::dispatchCloseEvent, wrapPersistent(this)));
+        m_state = State::Closing;
 
-        m_state = NotificationStateClosing;
         notificationManager()->close(this);
-    } else {
-        m_state = NotificationStateClosed;
+        return;
+    }
 
-        SecurityOrigin* origin = getExecutionContext()->getSecurityOrigin();
-        DCHECK(origin);
+    m_state = State::Closed;
 
-        notificationManager()->closePersistent(WebSecurityOrigin(origin), m_data.tag, m_notificationId);
-    }
+    SecurityOrigin* origin = getExecutionContext()->getSecurityOrigin();
+    DCHECK(origin);
+
+    notificationManager()->closePersistent(WebSecurityOrigin(origin), m_data.tag, m_notificationId);
 }
 
 void Notification::dispatchShowEvent()
@@ -206,10 +209,10 @@ void Notification::dispatchCloseEvent()
 {
     // The notification will be showing when the user initiated the close, or it will be
     // closing if the developer initiated the close.
-    if (m_state != NotificationStateShowing && m_state != NotificationStateClosing)
+    if (m_state != State::Showing && m_state != State::Closing)
         return;
 
-    m_state = NotificationStateClosed;
+    m_state = State::Closed;
     dispatchEvent(Event::create(EventTypeNames::close));
 }
 
@@ -376,9 +379,10 @@ void Notification::stop()
 {
     notificationManager()->notifyDelegateDestroyed(this);
 
-    m_state = NotificationStateClosed;
+    m_state = State::Closed;
 
-    m_prepareShowMethodRunner->stop();
+    if (m_prepareShowMethodRunner)
+        m_prepareShowMethodRunner->stop();
 
     if (m_loader)
         m_loader->stop();
@@ -386,7 +390,12 @@ void Notification::stop()
 
 bool Notification::hasPendingActivity() const
 {
-    return m_state == NotificationStateShowing || m_prepareShowMethodRunner->isActive() || m_loader;
+    // Non-persistent notification can receive events until they've been closed.
+    // Persistent notifications should be subject to regular garbage collection.
+    if (m_type == Type::NonPersistent)
+        return m_state != State::Closed;
+
+    return false;
 }
 
 DEFINE_TRACE(Notification)

third_party/WebKit/Source/modules/notifications/Notification.h

@@ -52,8 +52,6 @@
 namespace blink {
 
 class ExecutionContext;
-class NotificationAction;
-class NotificationManager;
 class NotificationOptions;
 class NotificationPermissionCallback;
 class NotificationResourcesLoader;
@@ -63,12 +61,13 @@ class MODULES_EXPORT Notification final : public EventTargetWithInlineData, publ
     USING_GARBAGE_COLLECTED_MIXIN(Notification);
     DEFINE_WRAPPERTYPEINFO();
 public:
-    // Used for JavaScript instantiations of the Notification object. Will automatically schedule for
-    // the notification to be displayed to the user when the developer-provided data is valid.
+    // Used for JavaScript instantiations of non-persistent notifications. Will
+    // automatically schedule for the notification to be displayed to the user
+    // when the developer-provided data is valid.
     static Notification* create(ExecutionContext*, const String& title, const NotificationOptions&, ExceptionState&);
 
-    // Used for embedder-created Notification objects. If |showing| is true, will initialize the
-    // Notification's state as showing, or as closed otherwise.
+    // Used for embedder-created persistent notifications. Initializes the state
+    // of the notification as either Showing or Closed based on |showing|.
     static Notification* create(ExecutionContext*, const String& notificationId, const WebNotificationData&, bool showing);
 
     ~Notification() override;
@@ -125,42 +124,50 @@ protected:
     DispatchEventResult dispatchEventInternal(Event*) final;
 
 private:
-    Notification(ExecutionContext*, const WebNotificationData&);
+    // The type of notification this instance represents. Non-persistent
+    // notifications will have events delivered to their instance, whereas
+    // persistent notification will be using a Service Worker.
+    enum class Type {
+        NonPersistent,
+        Persistent
+    };
+
+    // The current phase of the notification in its lifecycle.
+    enum class State {
+        Loading,
+        Showing,
+        Closing,
+        Closed
+    };
+
+    Notification(ExecutionContext*, Type, const WebNotificationData&);
+
+    // Sets the state of the notification in its lifecycle.
+    void setState(State state) { m_state = state; }
+
+    // Sets the notification ID to |notificationId|. This should be done once
+    // the notification has shown for non-persistent notifications, and at
+    // object initialisation time for persistent notifications.
+    void setNotificationId(const String& notificationId) { m_notificationId = notificationId; }
 
     // Schedules an asynchronous call to |prepareShow|, allowing the constructor
     // to return so that events can be fired on the notification object.
     void schedulePrepareShow();
 
-    // Checks permission and loads any necessary resources (this may be async)
-    // before showing the notification.
+    // Verifies that permission has been granted, then asynchronously starts
+    // loading the resources associated with this notification.
     void prepareShow();
 
-    // Shows the notification, using the resources loaded by the
-    // NotificationResourcesLoader.
+    // Shows the notification through the embedder using the loaded resources.
     void didLoadResources(NotificationResourcesLoader*);
 
-    void setNotificationId(const String& notificationId) { m_notificationId = notificationId; }
+    Type m_type;
+    State m_state;
 
     WebNotificationData m_data;
 
-    // Notifications can either be bound to the page, which means they're identified by
-    // their delegate, or persistent, which means they're identified by a persistent Id
-    // given to us by the embedder. This influences how we close the notification.
     String m_notificationId;
 
-    enum NotificationState {
-        NotificationStateIdle,
-        NotificationStateShowing,
-        NotificationStateClosing,
-        NotificationStateClosed
-    };
-
-    // Only to be used by the Notification::create() method when notifications were created
-    // by the embedder rather than by Blink.
-    void setState(NotificationState state) { m_state = state; }
-
-    NotificationState m_state;
-
     Member<AsyncMethodRunner<Notification>> m_prepareShowMethodRunner;
 
     Member<NotificationResourcesLoader> m_loader;