[Webkit-unassigned] [Bug 24849] Implement HTMLMediaElement 'played' attribute

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=24849


simon.fraser at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #29539|review?(simon.fraser at apple.c|review+
               Flag|om)                         |




------- Comment #9 from simon.fraser at apple.com  2009-04-16 11:31 PDT -------
(From update of attachment 29539)
> 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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.