Bug 33954 – YouTube HTML5 video never starts playing on Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Adam Roben (:aroben)]

To reproduce:

1. Go to http://www.youtube.com/html5 and join the HTML5 beta
2. Go to http://www.youtube.com/watch?v=pd7ILa_mhgA&feature=related

The video never starts playing. The spinner just spins forever.

The video does start playing on Mac.

[Adam Roben (:aroben)]

<rdar://problem/7565100>

[Eric Carlson]

Created attachment 48250 [details]
Proposed patch

No test added because http://bugs.webkit.org/show_bug.cgi?id=33954 should cover it, but still fails. That failure happens because the test tries to load an mp4 file directly into an iframe, which still doesn't work on Windows. 

I would like to get these changes in and fix the test later because they have been sitting in my tree for a week now.

[WebKit Review Bot]

Attachment 48250 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeWin.cpp:208:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

The "offending" line is testing the first character of a string:

    if (cookieURL.find('.') == 0)
        cookieURL.remove(0);

I think the "correct" style would make the purpose of the test less clear.

[Darin Adler]

(In reply to comment #4)
> The "offending" line is testing the first character of a string:
> 
>     if (cookieURL.find('.') == 0)
>         cookieURL.remove(0);
> 
> I think the "correct" style would make the purpose of the test less clear.

That's an unnecessarily-inefficient idiom for checking the first character. Instead you should write:

    if (cookieURL[0] == '.')
        cookieURL.remove(0);

The reason that works is that indexing with WebCore::String does a range check and returns a 0 character if you index past the end.

This code is quite fast and won't scan the whole string.

[Darin Adler]

Another option that is clearer than using find is to use the String::startsWith function.

[Darin Adler]

And if we wanted, we could overload String::startsWith to take a UChar and so make a clear way to write the s[0] == c code too.

[Darin Adler]

Comment on attachment 48250 [details]
Proposed patch

>  
> +
> +typedef DWORD (WINAPI* InternetSetCookieExWFunction)(LPCWSTR lpszUrl, LPCWSTR lpszCookieName, LPCWSTR lpszCookieData, DWORD dwFlags, DWORD_PTR dwReserved);

Would be better to put this typedef at the top of the file instead of just before it's going to be used. But you won't even need it if you use the SoftLinking.h header as I suggest below.

> +static InternetSetCookieExWFunction findInternetSetCookieExWFunction()
> +{
> +    HMODULE dll = LoadLibraryA("Wininet.dll");
> +    if (dll == INVALID_HANDLE_VALUE)
> +        return 0;
> +    return reinterpret_cast<InternetSetCookieExWFunction>(GetProcAddress(dll, "InternetSetCookieExW"));
> +}
> +
> +static DWORD internetSetCookieEx(LPCWSTR lpszUrl, LPCWSTR lpszCookieName, LPCWSTR lpszCookieData, DWORD dwFlags, DWORD_PTR dwReserved)
> +{
> +    static InternetSetCookieExWFunction function = findInternetSetCookieExWFunction();
> +    if (!function)
> +        return FALSE;
> +    return function(lpszUrl, lpszCookieName, lpszCookieData, dwFlags, dwReserved);
> +}

The header SoftLinking.h is supposed to make it easier to do this type of thing correctly. You just use the SOFT_LINK_LIBRARY and SOFT_LINK macros. There are examples in files like ScrollbarThemeWin.h.

The code you wrote looks fine, but it's better to not reinvent the wheel if possible.

> +void MediaPlayerPrivate::setupCookiesForQuickTime(const String& url)

The verb is "set up" so I normally capitalize the "U".

> +    if (frame->page() && !frame->page()->cookieEnabled())
> +        return;

If frame->page() is zero do we really want to do this cookie work at all?

> +        Cookie cookie = documentCookies[ndx];

I think it's more efficient to use const Cookie& here instead of copying the cookie.

> +        String cookieString = cookie.name;
> +        if (!cookie.value.isEmpty())
> +            cookieString += "=" + cookie.value + ";";

Sadly, although easy to write, this is an inefficient way to build a string with WebCore::String, which will require a lot of memory allocation. It would be better to use either StringBuilder or Vector<UChar>.

> +    String dateStringFromTime(CFAbsoluteTime time);

I think you left this in by accident.

I would have suggested removing the argument name "time", if we were keeping this.

I’m going to say review+ but I think this could use a bit of refinement.

[Eric Seidel (no email)]

Attachment 48250 [details] was posted by a committer and has review+, assigning to Eric Carlson for commit.

[Eric Carlson]

Comment on attachment 48250 [details]
Proposed patch

Preparing a new patch, this doesn't do everything is should.

[Eric Carlson]

Created attachment 50128 [details]
Updated patch

This version copies all cookie parameters, not just the value, so WinINet knows if it can be cached, etc. Also use StringBuilder to create cookie and url.

[WebKit Review Bot]

Attachment 50128 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeWin.cpp:177:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 50138 [details]
no-tabs patch

Oh the horrors!

[Darin Adler]

Comment on attachment 50138 [details]
no-tabs patch

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.

[Eric Carlson]

http://trac.webkit.org/changeset/55668