[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 #29471|                            |review-
               Flag|                            |
  Attachment #29471|1                           |0
        is obsolete|                            |




------- Comment #5 from simon.fraser at apple.com  2009-04-14 18:13 PDT -------
(From update of attachment 29471)

>      if (m_readyState == HAVE_NOTHING || !m_player) {
> +        printf("HTMLMediaElement::seek INVALID_STATE_ERR\n");

You have an extraneous printf here

> diff --git a/WebCore/html/TimeRanges.cpp b/WebCore/html/TimeRanges.cpp
> index ad81ac8..2d69229 100644

> +    unsigned int overlappingArcIndex;
> +    Range addedRange(start, end);
> +
> +    for (overlappingArcIndex = 0; overlappingArcIndex < m_ranges.size(); overlappingArcIndex++) {
> +        if (addedRange.isOverlappingRange(m_ranges[overlappingArcIndex])) {
> +            addedRange = addedRange.unionWithOverlappingRange(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]))
> +                    break;
> +            } else {
> +                if (m_ranges[overlappingArcIndex - 1].isBeforeRange(addedRange)
> +                    && addedRange.isBeforeRange(m_ranges[overlappingArcIndex])) {
> +                    break;
> +                }
> +            }
> +        }
> +    }
> +
> +    // Now that we are sure we don't overlap with any arc, just add it.
> +    m_ranges.insert(overlappingArcIndex, addedRange);

How confident are you that this handles all possible range configurations?
Range math is tricky.

> +        inline bool isPointInRange(float point)
> +        {
> +            return m_start <= point && point <= m_end;
> +        }

I think this should be 
  return m_start <= point && point < m_end;

so you'll also have to test for contiguous ranges in your amalgamation code.

> +        inline bool isOverlappingRange(Range range)
> +        {
> +            return isPointInRange(range.m_start) || isPointInRange(range.m_end) || range.isPointInRange(m_end);
> +        }

Are you sure this handles both types of entirely nested ranges?

> +        inline Range unionWithOverlappingRange(Range range)
> +        {
> +            Range ret;
> +
> +            ret.m_start = fminf(m_start, range.m_start);
> +            ret.m_end = fmaxf(m_end, range.m_end);

WebCore style is to use std::min/std::max.

r- to fix the isPointInRange thing.


-- 
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.