Remove userCancelledLoad and refactor the code at the call sites

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 3bae974c27f776b7d77c0d3f2de88d5fd937df28..eea696b9fdc1426c8c2c5a52a749efaef3757720 100644
--- a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
@@ -384,7 +384,7 @@ HTMLMediaElement::~HTMLMediaElement()
     removeElementFromDocumentMap(this, &document());
 
     // Destroying the player may cause a resource load to be canceled,
-    // which could result in userCancelledLoad() being called back.
+    // which could result in cancelledLoad() being called back.

[philipj_slow, 2015/11/27 14:01:28]

This comment seems inaccurate, destroying the player doesn't cause
cancelledLoad() to be called, at least not "called back." The bug is 403, but I
assume that if userCancelledLoad() was called, it was via stop().

It doesn't really make sense to set something in the destructor for the benefit
of a later call on that stale object, but perhaps it fixed a crash that was
otherwise too hard to track down.

I think that if cancelledLoad() is simplified a bit more to not use
m_isFinalizing at all, this could be removed.

[Srirama, 2015/11/30 12:43:34]

Done.

     // Setting m_isFinalizing ensures that such a call will not cause
     // us to dispatch an abort event, which would result in a crash.
     // See http://crbug.com/233654 for more details.
@@ -477,7 +477,7 @@ void HTMLMediaElement::didMoveToNewDocument(Document& oldDocument)
     // previous document. A proper fix would provide a mechanism to allow this
     // object to refresh the MediaPlayer's LocalFrame and FrameLoader references on
     // document changes so that playback can be resumed properly.
-    userCancelledLoad();
+    cancelledLoad();
 
     // Decrement the load event delay count on oldDocument now that m_webMediaPlayer has been destroyed
     // and there is no risk of dispatching a load event from within the destructor.
@@ -2975,16 +2975,13 @@ void HTMLMediaElement::stopPeriodicTimers()
     m_playbackProgressTimer.stop();
 }
 
-void HTMLMediaElement::userCancelledLoad()
+// TODO(srirama.m): Refactor this along with clearMediaPlayer and resetMediaPlayerAndMediaSource.
+void HTMLMediaElement::cancelledLoad()

[philipj_slow, 2015/11/27 14:01:27]

OK, so this is called from didMoveToNewDocument() and stop(). I don't think both
call sites actually need the same things.

In didMoveToDocument(), the key thing is that the WebMediaPlayer must be
destroyed, but otherwise it would be nice if the element were still usable. How
about replacing this with the equivalent of "invoke the resource load
algorithm", i.e. the same two lines that are in parseAttribute for srcAttr? It
still wouldn't be per spec, but could at least be explained in terms of the
spec.

In stop(), I think it's enough to just clear the player and reset enough state
such that hasPendingActivity() will return false. In other words, clear
m_shouldDelayLoadEvent and m_networkState. I think the rest is already taken
care of.

It should in other words be possible to remove this method entirely.

[Srirama, 2015/11/30 12:43:34]

Done, But the changes for didMoveToNewDocument() are creating problem for
gc-pending-event-inactive-document.html test, it is timing out.

 {
-    WTF_LOG(Media, "HTMLMediaElement::userCancelledLoad(%p)", this);
+    WTF_LOG(Media, "HTMLMediaElement::cancelledLoad(%p)", this);
 
-    // If the media data fetching process is aborted by the user:
-
-    // 1 - The user agent should cancel the fetching process.
     clearMediaPlayer(-1);
     // Reset m_readyState and m_readyStateMaximum since m_webMediaPlayer is gone.
-    ReadyState readyState = m_readyState;
     m_readyState = HAVE_NOTHING;
     m_readyStateMaximum = HAVE_NOTHING;
 
@@ -2992,27 +2989,11 @@ void HTMLMediaElement::userCancelledLoad()
     if (m_networkState == NETWORK_EMPTY || m_completelyLoaded || m_isFinalizing)
         return;
 
-    // 2 - Set the error attribute to a new MediaError object whose code attribute is set to MEDIA_ERR_ABORTED.
-    m_error = MediaError::create(MediaError::MEDIA_ERR_ABORTED);
-
-    // 3 - Queue a task to fire a simple event named error at the media element.
-    scheduleEvent(EventTypeNames::abort);
+    setNetworkState(NETWORK_EMPTY);
+    scheduleEvent(EventTypeNames::emptied);
 
-    // 4 - If the media element's readyState attribute has a value equal to HAVE_NOTHING, set the
-    // element's networkState attribute to the NETWORK_EMPTY value and queue a task to fire a
-    // simple event named emptied at the element. Otherwise, set the element's networkState
-    // attribute to the NETWORK_IDLE value.
-    if (readyState == HAVE_NOTHING) {
-        setNetworkState(NETWORK_EMPTY);
-        scheduleEvent(EventTypeNames::emptied);
-    } else {
-        setNetworkState(NETWORK_IDLE);
-    }
-
-    // 5 - Set the element's delaying-the-load-event flag to false. This stops delaying the load event.
     setShouldDelayLoadEvent(false);
 
-    // 6 - Abort the overall resource selection algorithm.
     m_currentSourceNode = nullptr;
 
     invalidateCachedTime();
@@ -3067,11 +3048,11 @@ void HTMLMediaElement::stop()
 
     recordMetricsIfPausing();
 
-    // Close the async event queue so that no events are enqueued by userCancelledLoad.
+    // Close the async event queue so that no events are enqueued by cancelledLoad.

[philipj_slow, 2015/11/27 14:01:28]

If cancelledLoad() is removed, in principle I think you could remove the
->close() call, but I'd keep it and "so that no further events can be enqueued."

[Srirama, 2015/11/30 12:43:34]

Acknowledged.

     cancelPendingEventsAndCallbacks();
     m_asyncEventQueue->close();
 
-    userCancelledLoad();
+    cancelledLoad();
 
     // Stop the playback without generating events
     m_playing = false;