[webkit-reviews] review denied: [Bug 65838] Implement temporal dimension portion of Media Fragments URI specification for video/audio : [Attachment 120974] Updated patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Eric Carlson <eric.carlson at apple.com>'s
request for review:
Bug 65838: Implement temporal dimension portion of Media Fragments URI
specification for video/audio
https://bugs.webkit.org/show_bug.cgi?id=65838

Attachment 120974: Updated patch
https://bugs.webkit.org/attachment.cgi?id=120974&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=120974&action=review


> Source/WebCore/html/HTMLMediaElement.cpp:3693
> +    OwnPtr<MediaFragmentURIParser> fragmentParser =
MediaFragmentURIParser::create(m_currentSrc);

Can we just allocate the parser on the stack instead of allocating it?

> Source/WebCore/html/HTMLMediaElement.cpp:3697
> +    float dur = duration();

Why is this a float and everything else a double?

> Source/WebCore/html/HTMLMediaElement.cpp:3704
> +    m_fragmentStartTime = (start > 0) ? start :
MediaFragmentURIParser::invalidTimeValue();
> +    if (m_fragmentStartTime > dur)
> +	   m_fragmentStartTime = dur;
> +    
> +    m_fragmentEndTime = (end > 0 && end > start) ? end :
MediaFragmentURIParser::invalidTimeValue();
> +    if (m_fragmentEndTime > dur)
> +	   m_fragmentEndTime = dur;

It seems like this code is making the assumption about what
MediaFragmentURIParser::invalidTimeValue() is.	It seems like you could
restructure it so the following if-statements only happen if you are not
assigning MediaFragmentURIParser::invalidTimeValue().

> Source/WebCore/html/MediaFragmentURIParser.cpp:50
> +static void skipWhiteSpace(const String& input, unsigned& position)
> +{
> +    while (position < input.length() && WTF::isASCIISpace(input[position]))
> +	   ++position;

I find for functions like this, it is good to either quote the spec, or at
least link to it, to explain, for instance, why isASCIISpace is the right
whitespace function.  I also believe Micheal has been trying to get people off
of iterating Strings directly, and instead using the characters8() or
characters16() functions, but I will ask him to verify that.

> Source/WebCore/html/MediaFragmentURIParser.cpp:58
> +    StringBuilder digits;
> +    while (position < input.length() && isASCIIDigit(input[position]))
> +	   digits.append(input[position++]);
> +    return digits.toString();

Same comment as above.

> Source/WebCore/html/MediaFragmentURIParser.cpp:70
> +    StringBuilder digits;
> +    if (input[position] != '.')
> +	   return String();
> +
> +    digits.append(input[position++]);
> +    while (position < input.length() && isASCIIDigit(input[position]))
> +	   digits.append(input[position++]);
> +    return digits.toString();

Same comment as above.

> Source/WebCore/html/MediaFragmentURIParser.cpp:192
> +    ASSERT(m_timeFormat == None);
> +
> +    if (m_fragments.isEmpty())
> +	   parseFragments();
> +
> +    double start;
> +    double end;
> +
> +    for (unsigned i = 0; i < m_fragments.size(); ++i) {
> +	   pair<String, String>& fragment = m_fragments[i];
> +
> +	   //
http://www.w3.org/2008/WebVideo/Fragments/WD-media-fragments-spec/#naming-time
> +	   // Temporal clipping is denoted by the name t, and specified as an
interval with a begin 
> +	   // time and an end time
> +	   if (fragment.first != "t")
> +	       continue;
> +
> +	   if (parseNPTFragment(fragment.second, start, end)) {
> +	       m_startTime = start;
> +	       m_endTime = end;
> +	       m_timeFormat = NormalPlayTime;
> +
> +	       // Although we have a valid fragment, don't return yet because
when a fragment 
> +	       // dimensions occurs multiple times, only the last occurrence of
that dimension
> +	       // is used:
> +	       //
http://www.w3.org/2008/WebVideo/Fragments/WD-media-fragments-spec/#error-uri-ge
neral
> +	       // Multiple occurrences of the same dimension: only the last
valid occurrence of a dimension
> +	       // (e.g., t=10 in #t=2&t=10) is interpreted, all previous
occurrences (valid or invalid) 
> +	       // SHOULD be ignored by the UA. 
> +	   }
> +    }

Could you kill the fragment vector once you have finished parsing?

> Source/WebCore/html/MediaFragmentURIParser.cpp:209
> +    if (timeString.substring(0, nptIdentiferLength) == "npt:")
> +	   offset += nptIdentiferLength;

You could probably do this more efficiently since substring allocates a String.
 I would recommend just looking at the characters.

> Source/WebCore/html/MediaFragmentURIParser.cpp:331
> +}
> +
> +
> +}
> +
> +#endif

Got some extra whitespace here.

> Source/WebCore/html/MediaFragmentURIParser.h:35
> +#include <wtf/text/StringBuilder.h>
> +#include <wtf/text/StringHash.h>

You don't seem to use these here.

> Source/WebCore/html/MediaFragmentURIParser.h:43
> +    virtual ~MediaFragmentURIParser() { }

You don't have any virtual functions or reason to subclass this, so you could
probably remove this destructor and mark the class FINAL.