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

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

 /*
  * Copyright (C) 2007, 2008, 2009, 2010, 2011, 2012, 2013 Apple Inc. All rights
  * reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 64 matching lines @@
 #include "platform/ContentType.h"
 #include "platform/Histogram.h"
 #include "platform/LayoutTestSupport.h"
 #include "platform/MIMETypeFromURL.h"
 #include "platform/RuntimeEnabledFeatures.h"
 #include "platform/UserGestureIndicator.h"
 #include "platform/audio/AudioBus.h"
 #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"
 #include "public/platform/WebContentDecryptionModule.h"
 #include "public/platform/WebInbandTextTrack.h"
 #include "public/platform/WebMediaPlayerSource.h"
 #include "public/platform/WebMediaStream.h"
 #include "public/platform/modules/remoteplayback/WebRemotePlaybackAvailability.h"
 #include "public/platform/modules/remoteplayback/WebRemotePlaybackClient.h"
 #include "public/platform/modules/remoteplayback/WebRemotePlaybackState.h"
@@ skipping 349 matching lines @@
       m_ignorePreloadNone(false),
       m_textTracksVisible(false),
       m_shouldPerformAutomaticTrackSelection(true),
       m_tracksAreReady(true),
       m_processingPreferenceChange(false),
       m_playingRemotely(false),
       m_inOverlayFullscreenVideo(false),
       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(
           AutoplayExperimentHelper::create(m_autoplayHelperClient.get())),
       m_autoplayUmaHelper(AutoplayUmaHelper::create(this)),
       m_remotePlaybackClient(nullptr),
       m_autoplayVisibilityObserver(nullptr) {
   ThreadState::current()->registerPreFinalizer(this);
 
   BLINK_MEDIA_LOG << "HTMLMediaElement(" << (void*)this << ")";
@@ skipping 314 matching lines @@
   // play promises or project pending play prmoises algorithms, immediately
   // resolve or reject those promises in the order the corresponding tasks
   // were queued.
   //
   // TODO(mlamouri): the promises are first resolved then rejected but the
   // order between resolved/rejected promises isn't respected. This could be
   // improved when the same task is used for both cases.
   //
   // 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();
   }
 
   // 4 - Remove each task in pending tasks from its task queue.
   cancelPendingEventsAndCallbacks();
 
   // 5 - If the media element's networkState is set to NETWORK_LOADING or
   // NETWORK_IDLE, queue a task to fire a simple event named abort at the media
   // element.
   if (m_networkState == kNetworkLoading || m_networkState == kNetworkIdle)
@@ skipping 3149 matching lines @@
 }
 
 void HTMLMediaElement::scheduleResolvePlayPromises() {
   // TODO(mlamouri): per spec, we should create a new task but we can't create
   // a new cancellable task without cancelling the previous one. There are two
   // approaches then: cancel the previous task and create a new one with the
   // appended promise list or append the new promise to the current list. The
   // 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) {
   // TODO(mlamouri): per spec, we should create a new task but we can't create
   // a new cancellable task without cancelling the previous one. There are two
   // approaches then: cancel the previous task and create a new one with the
   // appended promise list or append the new promise to the current list. The
   // 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() {
   scheduleEvent(EventTypeNames::playing);
   scheduleResolvePlayPromises();
 }
 
 void HTMLMediaElement::resolveScheduledPlayPromises() {
   for (auto& resolver : m_playPromiseResolveList)
     resolver->resolve();
@@ skipping 168 matching lines @@
 
 IntRect HTMLMediaElement::AutoplayHelperClientImpl::absoluteBoundingBoxRect()
     const {
   IntRect result;
   if (LayoutObject* object = m_element->layoutObject())
     result = object->absoluteBoundingBoxRect();
   return result;
 }
 
 }  // namespace blink