[webkit-reviews] review granted: [Bug 33954] YouTube HTML5 video never starts playing on Windows : [Attachment 50138] no-tabs patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 33954: YouTube HTML5 video never starts playing on Windows
https://bugs.webkit.org/show_bug.cgi?id=33954

Attachment 50138: no-tabs patch
https://bugs.webkit.org/attachment.cgi?id=50138&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Great patch, with really good attention to detail in the code. I found a few
small anomalies, but generally it seems superb.

> +    static const char* dayStrs[] = {"Mon", "Tue", "Wed", "Thu", "Fri",
"Sat", "Sun"};
> +    static const char* monthStrs[] = {"Jan", "Feb", "Mar", "Apr", "May",
"Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"};

Should have spaces before and after the "}" and "{" charactesr to fit our usual
format.

These should be:

    static const char* const

so that the arrays are constant, not just the strings in them.

I would prefer that we not use the abbreviation "Str" in the names of these
arrays.

> +    // WebCore loaded the page with the movie url with CFNetwork but
QuickTime will 

In the comment I suggest we use "URL" instead of "url".

> +    // use WinINet to download the movie, so we need to copie any cookies
needed to

Typo: "copie" -> "copy". I think typing "cookie" all those times fried the "y"
circuit.

> +    KURL movieURL = KURL(KURL(), url);

The names here are not so great. The string is named "url" but the URL object
is named "movieURL", yet both are the same movie URL.

Is it right to just convert the string to a KURL? Should we be doing a
completeURL operation or something along those lines? What about if the URL is
not a valid one? Does the getRawCookies function handle that correctly?

> +    for (unsigned ndx = 0; ndx < documentCookies.size(); ndx++) {

The variable name "ndx" here is a bit unusual. Why not just use "int"?

Typically we use size_t for indexing into Vector. Not sure if there's a real
benefit either way, but it would be good to be consistent if possible.

My comments are all minor editorial ones, so r=me either as-is or with some of
these addressed.