[webkit-reviews] review denied: [Bug 24849] Implement HTMLMediaElement 'played' attribute : [Attachment 29471] patch v3.

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied 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 29471: patch v3.
https://bugs.webkit.org/attachment.cgi?id=29471&action=review

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

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