[Webkit-unassigned] [Bug 33954] YouTube HTML5 video never starts playing on Windows

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=33954


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #48250|review?                     |review+
               Flag|                            |




--- Comment #8 from Darin Adler <darin at apple.com>  2010-02-05 14:01:34 PST ---
(From update of attachment 48250)
>  
> +
> +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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.