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

[bugzilla-daemon at webkit.org]

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





------- Comment #6 from pdherbemont at apple.com  2009-04-14 18:26 PDT -------
(In reply to comment #5)
> (From update of attachment 29471 [review])
> 
> >      if (m_readyState == HAVE_NOTHING || !m_player) {
> > +        printf("HTMLMediaElement::seek INVALID_STATE_ERR\n");
> 
> You have an extraneous printf here

Removed as off v4.

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

Here, we do fully inclusive range. All the case should be handled. Most of them
are handled in the test case. Because we are doing inclusive range, corner case
are less 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.

Ok. I'll switch to semi exclusive range (from [---] to [---[ )

> 
> > +        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?

Yes. Attempt a want to be clear Proof:
              [---0---]
 [----1----]  [----2----]
    [--------3-------]
       [------A-----]
------------------------->
Range A regarding the four possible overlapping ranges cases (range 0, 1, 2,
3):

Range.end is in A <=> Range overlaps with A, as would Range 1 or Range 0.
Range.start is in A <=> Range overlaps with A, as would Range 2 or Range 0.
a.end is in Range <=> Range overlaps with A, as would Range 3.

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

oki.

> r- to fix the isPointInRange thing.

I will then.


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