Remove userCancelledLoad and refactor the code at the call sites

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
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 366 matching lines @@
     if (m_textTracks)
         m_textTracks->clearOwner();
     m_audioTracks->shutdown();
     m_videoTracks->shutdown();
 
     closeMediaSource();
 
     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.
     m_isFinalizing = true;
 
     // Destroying the player may cause a resource load to be canceled,
     // which could result in Document::dispatchWindowLoadEvent() being
     // called via ResourceFetch::didLoadResource() then
     // FrameLoader::checkCompleted(). To prevent load event dispatching during
     // object destruction, we use Document::incrementLoadEventDelayCount().
@@ skipping 72 matching lines @@
     }
 
     removeElementFromDocumentMap(this, &oldDocument);
     addElementToDocumentMap(this, &document());
 
     // FIXME: This is a temporary fix to prevent this object from causing the
     // MediaPlayer to dereference LocalFrame and FrameLoader pointers from the
     // 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.
     oldDocument.decrementLoadEventDelayCount();
 
     ActiveDOMObject::didMoveToNewExecutionContext(&document());
     HTMLElement::didMoveToNewDocument(oldDocument);
 }
 
 bool HTMLMediaElement::supportsFocus() const
@@ skipping 2477 matching lines @@
     if (layoutObject())
         layoutObject()->updateFromElement();
 }
 
 void HTMLMediaElement::stopPeriodicTimers()
 {
     m_progressEventTimer.stop();
     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;
 
     // TODO(srirama.m): Investigate if this condition can be dropped entirely without any issues.
     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);
+    setNetworkState(NETWORK_EMPTY);
+    scheduleEvent(EventTypeNames::emptied);
 
-    // 3 - Queue a task to fire a simple event named error at the media element.
-    scheduleEvent(EventTypeNames::abort);
-
-    // 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();
     cueTimeline().updateActiveCues(0);
 }
 
 void HTMLMediaElement::clearMediaPlayerAndAudioSourceProviderClientWithoutLocking()
 {
 #if ENABLE(WEB_AUDIO)
     audioSourceProvider().setClient(nullptr);
@@ skipping 34 matching lines @@
     if (layoutObject())
         layoutObject()->setShouldDoFullPaintInvalidation();
 }
 
 void HTMLMediaElement::stop()
 {
     WTF_LOG(Media, "HTMLMediaElement::stop(%p)", this);
 
     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;
     m_paused = true;
     m_seeking = false;
 
     if (layoutObject())
         layoutObject()->updateFromElement();
 
     stopPeriodicTimers();
@@ skipping 597 matching lines @@
     visitor->trace(m_client);
 }
 
 DEFINE_TRACE(HTMLMediaElement::AudioSourceProviderImpl)
 {
     visitor->trace(m_client);
 }
 #endif
 
 }