Eliminate MediaPlayer abstraction(seek, duration, poster APIs)

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 1961 matching lines @@
         if (m_lastSeekTime < now)
             addPlayedRange(m_lastSeekTime, now);
     }
     m_lastSeekTime = time;
     m_sentEndEvent = false;
 
     // 8 - Queue a task to fire a simple event named seeking at the element.
     scheduleEvent(EventTypeNames::seeking);
 
     // 9 - Set the current playback position to the given new playback position
-    m_player->seek(time);
+    webMediaPlayer()->seek(time);

[philipj_slow, 2014/07/13 22:12:49]

I missed in this in the previous review, but the m_player check in this function
doesn't guarantee that webMediaPlayer() is not null, or at least not obviously
so. What makes it safe?

[Srirama, 2014/07/14 07:50:54]

The check at the top if (m_readyState == HAVE_NOTHING) return; ensures that
webmediaplayer is not null(this is how we arrived at the bug 382721 and solved
before landing these patches).
On 2014/07/13 22:12:49, philipj wrote:

[philipj_slow, 2014/07/14 10:54:57]

OK. So since m_readyState >= HAVE_METADATA now implies that webMediaPlayer() is
not null, the check at the top is confusing. The !m_player part of the
expression never matters and can be removed.

[Srirama, 2014/07/14 12:18:21]

Done.

 
     // 10-14 are handled, if necessary, when the engine signals a readystate change or otherwise
     // satisfies seek completion and signals a time change.
 }
 
 void HTMLMediaElement::finishSeek()
 {
     WTF_LOG(Media, "HTMLMediaElement::finishSeek");
 
     // 4.8.10.9 Seeking completion
@@ skipping 19 matching lines @@
     return webMediaPlayer() && webMediaPlayer()->hasAudio();
 }
 
 bool HTMLMediaElement::seeking() const
 {
     return m_seeking;
 }
 
 void HTMLMediaElement::refreshCachedTime() const
 {
-    m_cachedTime = m_player->currentTime();
+    if (!webMediaPlayer())
+        return;
+
+    m_cachedTime = webMediaPlayer()->currentTime();
     m_cachedTimeWallClockUpdateTime = WTF::currentTime();
 }
 
 void HTMLMediaElement::invalidateCachedTime()
 {
     WTF_LOG(Media, "HTMLMediaElement::invalidateCachedTime");
 
     // Don't try to cache movie time when playback first starts as the time reported by the engine
     // sometimes fluctuates for a short amount of time, so the cached time will be off if we take it
     // too early.
     static const double minimumTimePlayingBeforeCacheSnapshot = 0.5;
 
     m_minimumWallClockTimeToCacheMediaTime = WTF::currentTime() + minimumTimePlayingBeforeCacheSnapshot;
     m_cachedTime = MediaPlayer::invalidTime();
 }
 
 // playback state
 double HTMLMediaElement::currentTime() const
 {
 #if LOG_CACHED_TIME_WARNINGS
     static const double minCachedDeltaForWarning = 0.01;
 #endif
 
-    if (!m_player)
+    if (!m_player || !webMediaPlayer())

[philipj_slow, 2014/07/13 22:12:48]

!m_player implies !webMediaPlayer(), so just the latter check here is enough.
m_player is made unused in this function anyway.

[Srirama, 2014/07/14 07:50:54]

Done.

         return 0;
 
     if (m_seeking) {
         WTF_LOG(Media, "HTMLMediaElement::currentTime - seeking, returning %f", m_lastSeekTime);
         return m_lastSeekTime;
     }
 
     if (m_cachedTime != MediaPlayer::invalidTime() && m_paused) {
 #if LOG_CACHED_TIME_WARNINGS
-        double delta = m_cachedTime - m_player->currentTime();
+        double delta = m_cachedTime - webMediaPlayer()->currentTime();
         if (delta > minCachedDeltaForWarning)
             WTF_LOG(Media, "HTMLMediaElement::currentTime - WARNING, cached time is %f seconds off of media time when paused", delta);
 #endif
         return m_cachedTime;
     }
 
     refreshCachedTime();
 
     return m_cachedTime;
 }
@@ skipping 18 matching lines @@
     // case because MediaSource and SourceBuffer do not notify the element
     // directly upon duration changes caused by endOfStream, remove, or append
     // operations; rather the notification is triggered by the WebMediaPlayer
     // implementation observing that the underlying engine has updated duration
     // and notifying the element to consult its MediaSource for current
     // duration. See http://crbug.com/266644
 
     if (m_mediaSource)
         return m_mediaSource->duration();
 
-    return m_player->duration();
+    return webMediaPlayer()->duration();

[philipj_slow, 2014/07/13 22:12:48]

Change the !m_player check at the top to match?

[Srirama, 2014/07/14 07:50:54]

Removed !m_player check but !webmediaplayer() check is not requried as the other
check (m_readyState < HAVE_METADATA) makes sure that webmediaplayer is not null.
On 2014/07/13 22:12:48, philipj wrote:

[philipj_slow, 2014/07/14 10:54:57]

The check at the top is equivalent to the one in seek, so maybe simplify it in
the same way? Both of these can be done in a separate CL if you think they'd
pollute this one, though.

[philipj_slow, 2014/07/14 11:06:56]

My bad, you already fixed this one.

 }
 
 bool HTMLMediaElement::paused() const
 {
     return m_paused;
 }
 
 double HTMLMediaElement::defaultPlaybackRate() const
 {
     return m_defaultPlaybackRate;
@@ skipping 913 matching lines @@
 
 void HTMLMediaElement::mediaPlayerTimeChanged()
 {
     WTF_LOG(Media, "HTMLMediaElement::mediaPlayerTimeChanged");
 
     updateActiveTextTrackCues(currentTime());
 
     invalidateCachedTime();
 
     // 4.8.10.9 steps 12-14. Needed if no ReadyState change is associated with the seek.
-    if (m_seeking && m_readyState >= HAVE_CURRENT_DATA && !m_player->seeking())
+    if (m_seeking && m_readyState >= HAVE_CURRENT_DATA && !webMediaPlayer()->seeking())
         finishSeek();
 
     // Always call scheduleTimeupdateEvent when the media engine reports a time discontinuity,
     // it will only queue a 'timeupdate' event if we haven't already posted one at the current
     // movie time.
     scheduleTimeupdateEvent(false);
 
     double now = currentTime();
     double dur = duration();
 
@@ skipping 929 matching lines @@
 
 #if ENABLE(WEB_AUDIO)
 void HTMLMediaElement::clearWeakMembers(Visitor* visitor)
 {
     if (!visitor->isAlive(m_audioSourceNode) && audioSourceProvider())
         audioSourceProvider()->setClient(0);
 }
 #endif
 
 }