Bug 30256 - ~96 regression tests fail when using QuickTime 7.6 (they pass with QuickTime 7.3)
Summary: ~96 regression tests fail when using QuickTime 7.6 (they pass with QuickTime ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar, PlatformOnly
Depends on:
Blocks:
 
Reported: 2009-10-09 11:20 PDT by Brian Weinstein
Modified: 2009-12-03 14:22 PST (History)
3 users (show)

See Also:


Attachments
Proposed patch (2.96 KB, patch)
2009-12-03 09:01 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch, without tabs this time. (2.97 KB, patch)
2009-12-03 09:08 PST, Eric Carlson
aroben: review+
Details | Formatted Diff | Diff
Simplified patch. (3.02 KB, patch)
2009-12-03 13:41 PST, Eric Carlson
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Weinstein 2009-10-09 11:20:37 PDT
When the Windows buildbots were updated to Quicktime 7.6, ~100 media tests started failing. These were media tests and other tests that depended on media attributes (some of the fast/js tests were failing because they couldn't find an Audio object, for example).

When researching, I noticed the Quicktime SDK is only at version 7.3, which could have something to do with it.
Comment 1 Brian Weinstein 2009-10-09 11:23:28 PDT
<rdar://problem/7290966>
Comment 2 Eric Carlson 2009-12-03 09:01:45 PST
Created attachment 44247 [details]
Proposed patch
Comment 3 WebKit Review Bot 2009-12-03 09:03:47 PST
Attachment 44247 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp:216:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1
Comment 4 Eric Carlson 2009-12-03 09:08:58 PST
Created attachment 44248 [details]
Patch, without tabs this time.
Comment 5 WebKit Review Bot 2009-12-03 09:14:16 PST
style-queue ran check-webkit-style on attachment 44248 [details] without any errors.
Comment 6 Adam Roben (:aroben) 2009-12-03 09:25:31 PST
Comment on attachment 44248 [details]
Patch, without tabs this time.

> +static void addQTDirToPATH()
> +{
> +    static LPCWSTR pathEnvironmentVariable = L"PATH";
> +    static LPCWSTR quickTimeKeyName = L"Software\\Apple Computer, Inc.\\QuickTime";
> +    static LPCWSTR quickTimeSysDir = L"QTSysDir";
> +    static bool initialized;
> +
> +    if (initialized)
> +        return;
> +    initialized = true;
> +
> +    // Get the QuickTime dll directory from the registry. The key can be in either HKLM or HKCU.
> +    HKEY hKey;
> +    if (RegOpenKeyEx(HKEY_LOCAL_MACHINE, quickTimeKeyName, 0, KEY_READ, &hKey)) {
> +        if (RegOpenKeyEx(HKEY_CURRENT_USER, quickTimeKeyName, 0, KEY_READ, &hKey))
> +            return;
> +    }

I think (throughout this function) it would be better to compare with ERROR_SUCCESS.

> +    DWORD type;
> +    DWORD bufferSize;
> +    LONG err = RegQueryValueEx(hKey, quickTimeSysDir, 0, &type, 0, &bufferSize);
> +    if (err || !bufferSize || type != REG_SZ) {
> +        RegCloseKey(hKey);
> +        return;
> +    }
> +
> +    Vector<TCHAR> qtDir(bufferSize);
> +    err = RegQueryValueEx(hKey, quickTimeSysDir, 0, 0, (LPBYTE)qtDir.data(), &bufferSize);
> +    RegCloseKey(hKey);
> +    if (err || !bufferSize)
> +        return;

Can we use SHGetValue instead? That would be a lot simpler.

> +
> +    // Read the current PATH
> +    DWORD pathSize = GetEnvironmentVariable(pathEnvironmentVariable, 0, 0);
> +    Vector<TCHAR> oldPath(pathSize);
> +    if (!GetEnvironmentVariable(pathEnvironmentVariable, oldPath.data(), oldPath.size()))
> +        return;
> +
> +    // And add the QuickTime dll.
> +    wstring newPath;
> +    newPath.append(qtDir.data(), qtDir.size() - 1);
> +    newPath.append(L";");
> +    newPath.append(oldPath.data(), oldPath.size());
> +    SetEnvironmentVariable(pathEnvironmentVariable, newPath.c_str());
> +}

There's a strange mix of TCHAR and WCHAR here. I'd just always use WCHAR and call the *W versions of the APIs (e.g., SetEnvironmentVariableW).

> @@ -263,6 +309,8 @@ static void initialize()
>          for (int i = 0; i < ARRAYSIZE(fontsToInstall); ++i)
>              textRenderer->registerPrivateFont(wstring(resourcesPath + fontsToInstall[i]).c_str());
>  
> +    addQTDirToPATH();

I think it's worth adding a comment explaining why this is necessary.

r=me, but you should consider these comments.
Comment 7 Eric Carlson 2009-12-03 13:41:47 PST
Created attachment 44267 [details]
Simplified patch.
Comment 8 WebKit Review Bot 2009-12-03 13:42:29 PST
style-queue ran check-webkit-style on attachment 44267 [details] without any errors.
Comment 9 Adam Roben (:aroben) 2009-12-03 13:47:23 PST
Comment on attachment 44267 [details]
Simplified patch.

> +    // Read the current PATH.
> +    DWORD pathSize = GetEnvironmentVariableW(pathEnvironmentVariable, 0, 0);
> +    Vector<TCHAR> oldPath(pathSize);
> +    if (!GetEnvironmentVariable(pathEnvironmentVariable, oldPath.data(), oldPath.size()))
> +        return;

That should be Vector<WCHAR>.

r=me