[webkit-reviews] review granted: [Bug 30256] ~96 regression tests fail when using QuickTime 7.6 (they pass with QuickTime 7.3) : [Attachment 44248] Patch, without tabs this time.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Dec 3 09:25:30 PST 2009
Adam Roben (aroben) <aroben at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 30256: ~96 regression tests fail when using QuickTime 7.6 (they pass with
QuickTime 7.3)
https://bugs.webkit.org/show_bug.cgi?id=30256
Attachment 44248: Patch, without tabs this time.
https://bugs.webkit.org/attachment.cgi?id=44248&action=review
------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +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.
More information about the webkit-reviews
mailing list