Bug 33954 - YouTube HTML5 video never starts playing on Windows
Summary: YouTube HTML5 video never starts playing on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Eric Carlson
URL: http://www.youtube.com/watch?v=pd7ILa...
Keywords: InRadar, PlatformOnly
Depends on:
Blocks:
 
Reported: 2010-01-21 08:17 PST by Adam Roben (:aroben)
Modified: 2010-03-08 09:37 PST (History)
4 users (show)

See Also:


Attachments
Proposed patch (5.12 KB, patch)
2010-02-05 12:39 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Updated patch (5.90 KB, patch)
2010-03-05 14:55 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
no-tabs patch (5.90 KB, patch)
2010-03-05 16:15 PST, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2010-01-21 08:17:14 PST
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.
Comment 1 Adam Roben (:aroben) 2010-01-21 08:17:45 PST
<rdar://problem/7565100>
Comment 2 Eric Carlson 2010-02-05 12:39:49 PST
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.
Comment 3 WebKit Review Bot 2010-02-05 12:47:28 PST
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.
Comment 4 Eric Carlson 2010-02-05 13:49:20 PST
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.
Comment 5 Darin Adler 2010-02-05 13:53:11 PST
(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.
Comment 6 Darin Adler 2010-02-05 13:53:31 PST
Another option that is clearer than using find is to use the String::startsWith function.
Comment 7 Darin Adler 2010-02-05 13:54:08 PST
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.
Comment 8 Darin Adler 2010-02-05 14:01:34 PST
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.
Comment 9 Eric Seidel (no email) 2010-02-08 11:21:19 PST
Attachment 48250 [details] was posted by a committer and has review+, assigning to Eric Carlson for commit.
Comment 10 Eric Carlson 2010-03-05 14:26:46 PST
Comment on attachment 48250 [details]
Proposed patch

Preparing a new patch, this doesn't do everything is should.
Comment 11 Eric Carlson 2010-03-05 14:55:57 PST
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.
Comment 12 WebKit Review Bot 2010-03-05 16:10:56 PST
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.
Comment 13 Eric Carlson 2010-03-05 16:15:35 PST
Created attachment 50138 [details]
no-tabs patch

Oh the horrors!
Comment 14 Darin Adler 2010-03-07 09:54:14 PST
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.
Comment 15 Eric Carlson 2010-03-08 09:37:15 PST
http://trac.webkit.org/changeset/55668