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

[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 29539: patch v6.
https://bugs.webkit.org/attachment.cgi?id=29539&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> diff --git a/LayoutTests/media/video-played-expected.txt
b/LayoutTests/media/video-played-expected.txt
> new file mode 100644
> index 0000000..ff13698
> --- /dev/null
> +++ b/LayoutTests/media/video-played-expected.txt
> @@ -0,0 +1,125 @@
> +FAIL: Timed out waiting for notifyDone to be called

FAIL indeed! Expected results should not have FAIL in them.

> -2009-04-15  Pierre d'Herbemont  <pdherbemont at apple.com>
> +2009-04-13  Pierre d'Herbemont  <pdherbemont at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Add support the the media element 'played' attribute.

"for the"

I'd like to see a little more verbage saying what adding this support entailed.


>  PassRefPtr<TimeRanges> HTMLMediaElement::played() const
>  {
> -    // FIXME track played
> -    return TimeRanges::create();
> +    if (m_playing)
> +	   m_playedTimeRanges->add(m_lastSeekTime, currentTime());
> +    return m_playedTimeRanges;
>  }

I think this needs to return a copy of the ranges, so that the pointer you hand
back
is not 'live'.

> +{
> +    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, we
could optimize the insert place.
> +
> +    for (overlappingArcIndex = 0; overlappingArcIndex < m_ranges.size();
overlappingArcIndex++) {
> +	   if (addedRange.isOverlappingRange(m_ranges[overlappingArcIndex])
> +	    || addedRange.isContiguousWithRange(m_ranges[overlappingArcIndex]))


Normal style is to put the operators on the end of the previous line.

> +	   {

Brace should be on previous line.

> +	       // We need to merge the addedRange and that range.
> +	       addedRange =
addedRange.unionWithOverlappingOrContiguousRange(m_ranges[overlappingArcIndex])
;
> +	       m_ranges.remove(overlappingArcIndex);
> +	       overlappingArcIndex--;
> +	   } else {
> +	       // Check the case for which there is no more to do
> +	       if (!overlappingArcIndex) {
> +		   if (addedRange.isBeforeRange(m_ranges[0]))
> +		   {

Brace style wrong here.

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

Put && on previous line.

> diff --git a/WebCore/html/TimeRanges.h b/WebCore/html/TimeRanges.h

> +    // We consider all the Ranges to be semi-interval [------[

"to be a semi-interval", or "to be the semi-interval".

> +	   inline bool isOverlappingRange(Range range)

The argument should be a const Range&

> +	   inline bool isContiguousWithRange(Range range)

The argument should be a const Range&

> +	   inline Range unionWithOverlappingOrContiguousRange(Range range)

The argument should be a const Range&

> +	   inline bool isBeforeRange(Range range)

The argument should be a const Range&

r=me with those fixes.