[webkit-reviews] review granted: [Bug 24849] Implement HTMLMediaElement 'played' attribute : [Attachment 29552] patch v7.

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Pierre d'Herbemont
<pdherbemont at apple.com>'s request for review:
Bug 24849: Implement HTMLMediaElement 'played' attribute
https://bugs.webkit.org/show_bug.cgi?id=24849

Attachment 29552: patch v7.
https://bugs.webkit.org/attachment.cgi?id=29552&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>


> +    if (m_playing)
> +    {

Still a style violation here.

> -    // FIXME track played
> -    return TimeRanges::create();
> +    if (m_playing)
> +    {

And here.

> +	   float time = currentTime();
> +	   if (m_lastSeekTime < time)
> +	       m_playedTimeRanges->add(m_lastSeekTime, time);
> +    }
> +    return m_playedTimeRanges->copy();
>  }
>  
>  PassRefPtr<TimeRanges> HTMLMediaElement::seekable() const
> @@ -1318,9 +1333,14 @@ void HTMLMediaElement::updatePlayState()
>	   m_player->setRate(m_playbackRate);
>	   m_player->play();
>	   startPlaybackProgressTimer();
> +	   m_playing = true;
>      } else if (!shouldBePlaying && !playerPaused) {
>	   m_player->pause();
>	   m_playbackProgressTimer.stop();
> +	   m_playing = false;
> +	   float time = currentTime();
> +	   if (m_lastSeekTime < time)
> +	       m_playedTimeRanges->add(m_lastSeekTime, time);
>      }
>      
>      if (renderer())
> diff --git a/WebCore/html/HTMLMediaElement.h
b/WebCore/html/HTMLMediaElement.h
> index 43831b3..e16ec02 100644
> --- a/WebCore/html/HTMLMediaElement.h
> +++ b/WebCore/html/HTMLMediaElement.h
> @@ -218,6 +218,7 @@ protected:
>      Timer<HTMLMediaElement> m_progressEventTimer;
>      Timer<HTMLMediaElement> m_playbackProgressTimer;
>      Vector<RefPtr<Event> > m_pendingEvents;
> +    RefPtr<TimeRanges> m_playedTimeRanges;
>      
>      float m_playbackRate;
>      float m_defaultPlaybackRate;
> @@ -228,7 +229,7 @@ protected:
>      RefPtr<MediaError> m_error;
>  
>      float m_volume;
> -    float m_currentTimeDuringSeek;
> +    float m_lastSeekTime;
>      
>      unsigned m_previousProgress;
>      double m_previousProgressTime;
> @@ -248,6 +249,8 @@ protected:
>  
>      BehaviorRestrictions m_restrictions;
>  
> +    bool m_playing;
> +
>      // counter incremented while processing a callback from the media
player, so we can avoid
>      //  calling the media engine recursively
>      int m_processingMediaPlayerCallback;
> diff --git a/WebCore/html/TimeRanges.cpp b/WebCore/html/TimeRanges.cpp
> index ad81ac8..04a8fc6 100644
> --- a/WebCore/html/TimeRanges.cpp
> +++ b/WebCore/html/TimeRanges.cpp
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2007 Apple Inc.  All rights reserved.
> + * Copyright (C) 2007, 2009 Apple Inc.  All rights reserved.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions
> @@ -34,6 +34,17 @@ TimeRanges::TimeRanges(float start, float end)
>      add(start, end);
>  }
>  
> +PassRefPtr<TimeRanges> TimeRanges::copy()
> +{
> +    RefPtr<TimeRanges> newSession = TimeRanges::create();
> +    
> +    unsigned size = m_ranges.size();
> +    for (unsigned i = 0; i < size; i++)
> +	   newSession->add(m_ranges[i].m_start, m_ranges[i].m_end);
> +    
> +    return newSession.release();
> +}
> +
>  float TimeRanges::start(unsigned index, ExceptionCode& ec) const 
>  { 
>      if (index >= length()) {
> @@ -53,9 +64,46 @@ float TimeRanges::end(unsigned index, ExceptionCode& ec)
const
>  }
>  
>  void TimeRanges::add(float start, float end) 
> -{ 
> -    m_ranges.append(Range(start, end));
> -    // FIXME normalize
> +{
> +    ASSERT(start <= end);
> +    unsigned int overlappingArcIndex;
> +    Range addedRange(start, end);
> +
> +    // For each present range check if we need to:
> +    // - merge with the added range, in case we are overlapping or
contiguous
> +    // - Need to insert in place, we we are completely, not overlapping and
not contiguous
> +    // in between two ranges.
> +    //
> +    // TODO: Given that we assume that ranges are correctly ordered, this
could be optimized.
> +
> +    for (overlappingArcIndex = 0; overlappingArcIndex < m_ranges.size();
overlappingArcIndex++) {
> +	   if (addedRange.isOverlappingRange(m_ranges[overlappingArcIndex])
> +	    || addedRange.isContiguousWithRange(m_ranges[overlappingArcIndex]))
{

> +		   if (m_ranges[overlappingArcIndex -
1].isBeforeRange(addedRange)
> +		       &&
addedRange.isBeforeRange(m_ranges[overlappingArcIndex])) {

The indentation should be consistent. I prefer the second one.

r=me.