[webkit-reviews] review granted: [Bug 29041] HTMLMediaElement buffered attribute should report a list of time range : [Attachment 39197] API change

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Hin-Chung Lam
<hclam at google.com>'s request for review:
Bug 29041: HTMLMediaElement buffered attribute should report a list of time
range
https://bugs.webkit.org/show_bug.cgi?id=29041

Attachment 39197: API change
https://bugs.webkit.org/attachment.cgi?id=39197&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
> +	   * html/HTMLMediaElement.cpp:
> +	   (WebCore::HTMLMediaElement::percentLoaded):
> +	   (WebCore::HTMLMediaElement::buffered):
> +	   * platform/graphics/MediaPlayer.cpp:
> +	   (WebCore::NullMediaPlayerPrivate::buffered):
> +	   (WebCore::MediaPlayer::buffered):
> +	   * platform/graphics/MediaPlayer.h:
> +	   * platform/graphics/MediaPlayerPrivate.h:
> +	   * platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:
> +	   * platform/graphics/gtk/MediaPlayerPrivateGStreamer.h:
> +	   * platform/graphics/mac/MediaPlayerPrivateQTKit.h:
> +	   * platform/graphics/mac/MediaPlayerPrivateQTKit.mm:
> +	   (WebCore::MediaPlayerPrivate::buffered):
> +	   * platform/graphics/qt/MediaPlayerPrivatePhonon.cpp:
> +	   * platform/graphics/qt/MediaPlayerPrivatePhonon.h:
> +	   * platform/graphics/win/MediaPlayerPrivateQuickTimeWin.cpp:
> +	   * platform/graphics/win/MediaPlayerPrivateQuickTimeWin.h:
> +	   * platform/graphics/wince/MediaPlayerPrivateWince.h:
> +	   * rendering/RenderThemeChromiumMac.mm:
> +	   (WebCore::RenderThemeChromiumMac::paintMediaSliderTrack):
> +	   * rendering/RenderThemeChromiumSkia.cpp:
> +	   * rendering/RenderThemeMac.mm:
> +	   (WebCore::RenderThemeMac::paintMediaSliderTrack):

  It would be nice to have a brief comment about what changed in each function.



> +    int length = m_player->buffered()->length();
> +    if (length) {
> +	   // FIXME: The end of last range doesn't represent the actual
percentage loaded.
> +	   ExceptionCode ignoredException;
> +	   float end = m_player->buffered()->end(length - 1, ignoredException);

> +	   return duration ? end / duration : 0;
> +    }
> +    return 0;


  Please put m_player->buffered() into a local, it can be expensive to
calculate the buffered ranges. 

  This code will return the wrong answer as soon as an engine returns a
TimeRanges object with more than one entry. You might as well change it now to
iterate through all of the ranges to get the total amount buffered.


> diff --git a/WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp
b/WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp

> +    RefPtr<TimeRanges> timeRanges = TimeRanges::create();
> +    if (!m_errorOccured && !m_isStreaming && maxTimeLoaded() > 0)
> +	   timeRanges->add(0, maxTimeLoaded());
> +    return timeRanges;

  You should store maxTimeLoaded() in a local, it can be expensive to
calculate.


> diff --git a/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm
b/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm

> +    RefPtr<TimeRanges> timeRanges = TimeRanges::create();
> +    if (maxTimeLoaded() > 0)
> +	   timeRanges->add(0, maxTimeLoaded());
> +    return timeRanges;

  Same thing here.


> diff --git a/WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeWin.cpp
b/WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeWin.cpp

>  #include "Timer.h"
> +#include "TimeRanges.h"

  Minor style violation here, "TimeRanges.h" sorts before "Timer.h".


> -float MediaPlayerPrivate::maxTimeBuffered() const
> +PassRefPtr<TimeRanges> MediaPlayerPrivate::buffered() const;
>  {
> +    RefPtr<TimeRanges> timeRanges = TimeRanges::create();
>      // rtsp streams are not buffered
> -    return m_isStreaming ? 0 : maxTimeLoaded();
> +    if (!m_isStreaming && maxTimeLoaded() > 0)
> +	   timeRanges->add(0, maxTimeLoaded());

  You should store maxTimeLoaded() in a local, it can be expensive to
calculate.


> @@ -1935,6 +1935,9 @@ bool
RenderThemeChromiumMac::paintMediaSliderTrack(RenderObject* o, const Render
>      float currentTime = 0;
>      float duration = 0;
>      if (MediaPlayer* player = mediaElement->player()) {
> +	   RefPtr<TimeRanges> timeRanges = player->buffered();
> +	   ExceptionCode ignoredException;
> +	   timeLoaded = timeRanges->length() ? timeRanges->end(0,
ignoredException) : 0;
>	   duration = player->duration();
>	   timeLoaded = player->maxTimeBuffered();
>	   currentTime = player->currentTime();

  I realize that this isn't new to this patch, but HTMLMediaElement has
buffered, duration, and currentTime methods so I would prefer if we used it
instead of MediaPlayer.


> @@ -1604,8 +1605,10 @@ bool
RenderThemeMac::paintMediaSliderTrack(RenderObject* o, const RenderObject::
>      float currentTime = 0;
>      float duration = 0;
>      if (MediaPlayer* player = mediaElement->player()) {
> +	   RefPtr<TimeRanges> timeRanges = player->buffered();
> +	   ExceptionCode ignoredException;
> +	   timeLoaded = timeRanges->length() ? timeRanges->end(0,
ignoredException) : 0;
>	   duration = player->duration();
> -	   timeLoaded = player->maxTimeBuffered();
>	   currentTime = player->currentTime();
>      }

  Same thing here.

  r+ assuming you address the comments.