Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskRunner::postCancellableTask (2)

third_party/WebKit/Source/core/html/HTMLMediaElement.cpp

Index: third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
index a1f0ce6a74d593703d34135af4a59c9ecdc1652e..8d80ea3c40f3894547b277449d008386fb03d548 100644
--- a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
@@ -82,7 +82,6 @@
 #include "platform/audio/AudioSourceProviderClient.h"
 #include "platform/graphics/GraphicsLayer.h"
 #include "platform/mediastream/MediaStreamDescriptor.h"
-#include "platform/scheduler/CancellableTaskFactory.h"
 #include "platform/weborigin/SecurityOrigin.h"
 #include "public/platform/Platform.h"
 #include "public/platform/WebAudioSourceProvider.h"
@@ -452,12 +451,6 @@ HTMLMediaElement::HTMLMediaElement(const QualifiedName& tagName,
       m_audioTracks(this, AudioTrackList::create(*this)),
       m_videoTracks(this, VideoTrackList::create(*this)),
       m_textTracks(this, nullptr),
-      m_playPromiseResolveTask(CancellableTaskFactory::create(
-          this,
-          &HTMLMediaElement::resolveScheduledPlayPromises)),
-      m_playPromiseRejectTask(CancellableTaskFactory::create(
-          this,
-          &HTMLMediaElement::rejectScheduledPlayPromises)),
       m_audioSourceNode(nullptr),
       m_autoplayHelperClient(AutoplayHelperClientImpl::create(this)),
       m_autoplayHelper(
@@ -792,14 +785,14 @@ void HTMLMediaElement::invokeLoadAlgorithm() {
   //
   // TODO(mlamouri): don't run the callback synchronously if we are not allowed
   // to run scripts. It can happen in some edge cases. https://crbug.com/660382
-  if (m_playPromiseResolveTask->isPending() &&
+  if (m_playPromiseResolveTaskHandle.isActive() &&
       !ScriptForbiddenScope::isScriptForbidden()) {
-    m_playPromiseResolveTask->cancel();
+    m_playPromiseResolveTaskHandle.cancel();
     resolveScheduledPlayPromises();
   }
-  if (m_playPromiseRejectTask->isPending() &&
+  if (m_playPromiseRejectTaskHandle.isActive() &&
       !ScriptForbiddenScope::isScriptForbidden()) {
-    m_playPromiseRejectTask->cancel();
+    m_playPromiseRejectTaskHandle.cancel();
     rejectScheduledPlayPromises();
   }
 
@@ -3969,18 +3962,24 @@ void HTMLMediaElement::scheduleResolvePlayPromises() {
   // latter approach is preferred because it might be the less observable
   // change.
   DCHECK(m_playPromiseResolveList.isEmpty() ||
-         m_playPromiseResolveTask->isPending());
+         m_playPromiseResolveTaskHandle.isActive());
   if (m_playPromiseResolvers.isEmpty())
     return;
 
   m_playPromiseResolveList.appendVector(m_playPromiseResolvers);
   m_playPromiseResolvers.clear();
 
-  if (m_playPromiseResolveTask->isPending())
+  if (m_playPromiseResolveTaskHandle.isActive())
     return;
 
-  TaskRunnerHelper::get(TaskType::MediaElementEvent, &document())
-      ->postTask(BLINK_FROM_HERE, m_playPromiseResolveTask->cancelAndCreate());
+  // wrapWeakPersistent(this) is safe because a posted task is canceled when the
+  // task handle is destroyed on the dtor of this HTMLMediaElement.
+  m_playPromiseResolveTaskHandle =
+      TaskRunnerHelper::get(TaskType::MediaElementEvent, &document())
+          ->postCancellableTask(
+              BLINK_FROM_HERE,
+              WTF::bind(&HTMLMediaElement::resolveScheduledPlayPromises,
+                        wrapWeakPersistent(this)));

[haraken, 2016/11/17 06:35:19]

I'm not 100% sure, but this should probably be wrapPersistent?

The spec requires to fire the event, which means that the event should keep
alive the media element.

[nhiroki, 2016/11/17 08:10:15]

Before this CL, CancellableTaskFactory posts a task in the same manner with this
postCancellableTask+wrapWeakPersistent, so replacing this with wrapPersistent
may change behavior. We might want to have inputs from experts.

+mlamouri@, what do you think about this? I chose you because you recently
changed this class. Please forward if you know more appropriate reviewer.

 }
 
 void HTMLMediaElement::scheduleRejectPlayPromises(ExceptionCode code) {
@@ -3991,21 +3990,28 @@ void HTMLMediaElement::scheduleRejectPlayPromises(ExceptionCode code) {
   // latter approach is preferred because it might be the less observable
   // change.
   DCHECK(m_playPromiseRejectList.isEmpty() ||
-         m_playPromiseRejectTask->isPending());
+         m_playPromiseRejectTaskHandle.isActive());
   if (m_playPromiseResolvers.isEmpty())
     return;
 
   m_playPromiseRejectList.appendVector(m_playPromiseResolvers);
   m_playPromiseResolvers.clear();
 
-  if (m_playPromiseRejectTask->isPending())
+  if (m_playPromiseRejectTaskHandle.isActive())
     return;
 
   // TODO(mlamouri): because cancellable tasks can't take parameters, the
   // error code needs to be saved.

[tzik, 2016/11/17 06:20:15]

Can we put m_playPromiseErrorCode into the closure?
# That's not need to do in this CL, but could you leave a TODO comment here?

[nhiroki, 2016/11/17 08:10:16]

Done.

   m_playPromiseErrorCode = code;
-  TaskRunnerHelper::get(TaskType::MediaElementEvent, &document())
-      ->postTask(BLINK_FROM_HERE, m_playPromiseRejectTask->cancelAndCreate());
+
+  // wrapWeakPersistent(this) is safe because a posted task is canceled when the
+  // task handle is destroyed on the dtor of this HTMLMediaElement.

[tzik, 2016/11/17 06:20:15]

"wrapWeakPersistent(this) is safe because..." sounds weird to me, since
WeakPersistent also cancels the task when |this| has gone. So, unlike
unretained, this is safe even without the TaskHandle destruction.
Could you update the comment or just remove it?

[nhiroki, 2016/11/17 08:10:15]

Good point. Removed the comment entirely.

+  m_playPromiseRejectTaskHandle =
+      TaskRunnerHelper::get(TaskType::MediaElementEvent, &document())
+          ->postCancellableTask(
+              BLINK_FROM_HERE,
+              WTF::bind(&HTMLMediaElement::rejectScheduledPlayPromises,
+                        wrapWeakPersistent(this)));

[haraken, 2016/11/17 06:35:19]

Ditto.

 }
 
 void HTMLMediaElement::scheduleNotifyPlaying() {