Bug 46209 – WebKit2 should look for WebKit2WebProcess.exe next to WebKit.dll

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brian Weinstein]

WebKit2WebProcess should live with WebKit.dll, which means we should pass a full path to CreateProcess, instead of just assuming that WebKit.dll (and WebKitWebProcess.exe) are in the same folder as Safari.exe

[Brian Weinstein]

Created attachment 68277 [details]
[PATCH] Fix

[Steve Falkenburg]

Comment on attachment 68277 [details]
[PATCH] Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=68277&action=review

> WebKit2/UIProcess/Launcher/win/ProcessLauncherWin.cpp:57
> +    HMODULE webkitModule = GetModuleHandle(webkitDLLName);

We've been using :: to namespace quality Win32 calls lately.

> WebKit2/UIProcess/Launcher/win/ProcessLauncherWin.cpp:63
> +    if (!GetModuleFileName(webkitModule, pathStr, MAX_PATH) || GetLastError() != ERROR_SUCCESS)

You shouldn't need to check GetLastError and the result of the function. Just checking the result for 0 should be enough to check for failure.
Same comment about the ::.

> WebKit2/UIProcess/Launcher/win/ProcessLauncherWin.cpp:66
> +    PathRemoveFileSpec(pathStr);

Why not error check this one since you did with the others?
Same comment about ::

> WebKit2/UIProcess/Launcher/win/ProcessLauncherWin.cpp:67
> +    if (!PathAppend(pathStr, webProcessName))

And again,

[Adam Roben (:aroben)]

Comment on attachment 68277 [details]
[PATCH] Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=68277&action=review

> WebKit2/UIProcess/Launcher/win/ProcessLauncherWin.cpp:35
> +const LPCWSTR webkitDLLName = L"WebKit_debug.dll";

Should be webKitDLLName (with a capital "K").

>> WebKit2/UIProcess/Launcher/win/ProcessLauncherWin.cpp:57
>> +    HMODULE webkitModule = GetModuleHandle(webkitDLLName);
> 
> We've been using :: to namespace quality Win32 calls lately.

We've also been using the W versions of the APIs, so: ::GetModuleHandleW(webKitDLLName);

>> WebKit2/UIProcess/Launcher/win/ProcessLauncherWin.cpp:63
>> +    if (!GetModuleFileName(webkitModule, pathStr, MAX_PATH) || GetLastError() != ERROR_SUCCESS)
> 
> You shouldn't need to check GetLastError and the result of the function. Just checking the result for 0 should be enough to check for failure.
> Same comment about the ::.

_countof(pathStr) would be better than repeating MAX_PATH.

[Brian Weinstein]

(In reply to comment #2)
> (From update of attachment 68277 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68277&action=review
> 
> > WebKit2/UIProcess/Launcher/win/ProcessLauncherWin.cpp:57
> > +    HMODULE webkitModule = GetModuleHandle(webkitDLLName);
> 
> We've been using :: to namespace quality Win32 calls lately.

Fixed.

> 
> > WebKit2/UIProcess/Launcher/win/ProcessLauncherWin.cpp:63
> > +    if (!GetModuleFileName(webkitModule, pathStr, MAX_PATH) || GetLastError() != ERROR_SUCCESS)
> 
> You shouldn't need to check GetLastError and the result of the function. Just checking the result for 0 should be enough to check for failure.

Removed the GetLastError call.

> Same comment about the ::.

Fixed.

> 
> > WebKit2/UIProcess/Launcher/win/ProcessLauncherWin.cpp:66
> > +    PathRemoveFileSpec(pathStr);
> 
> Why not error check this one since you did with the others?

This didn't seem to have as clearly defined a failure case and success case as the others (the function returns nonzero if something was removed, or zero otherwise), so I omitted the check for now.


> Same comment about ::

Fixed.

> 
> > WebKit2/UIProcess/Launcher/win/ProcessLauncherWin.cpp:67
> > +    if (!PathAppend(pathStr, webProcessName))
> 
> And again,

Fixed.

[Brian Weinstein]

(In reply to comment #3)
> (From update of attachment 68277 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68277&action=review
> 
> > WebKit2/UIProcess/Launcher/win/ProcessLauncherWin.cpp:35
> > +const LPCWSTR webkitDLLName = L"WebKit_debug.dll";
> 
> Should be webKitDLLName (with a capital "K").

Yes, it should. Fixed.

> 
> >> WebKit2/UIProcess/Launcher/win/ProcessLauncherWin.cpp:57
> >> +    HMODULE webkitModule = GetModuleHandle(webkitDLLName);
> > 
> > We've been using :: to namespace quality Win32 calls lately.
> 
> We've also been using the W versions of the APIs, so: ::GetModuleHandleW(webKitDLLName);

Fixed.

> 
> >> WebKit2/UIProcess/Launcher/win/ProcessLauncherWin.cpp:63
> >> +    if (!GetModuleFileName(webkitModule, pathStr, MAX_PATH) || GetLastError() != ERROR_SUCCESS)
> > 
> > You shouldn't need to check GetLastError and the result of the function. Just checking the result for 0 should be enough to check for failure.
> > Same comment about the ::.
> 
> _countof(pathStr) would be better than repeating MAX_PATH.

Fixed.

[Brian Weinstein]

Landed in r67979.

[WebKit Review Bot]

http://trac.webkit.org/changeset/67979 might have broken Qt Windows 32-bit Debug
The following changes are on the blame list:
http://trac.webkit.org/changeset/67973
http://trac.webkit.org/changeset/67974
http://trac.webkit.org/changeset/67975
http://trac.webkit.org/changeset/67976
http://trac.webkit.org/changeset/67977
http://trac.webkit.org/changeset/67978
http://trac.webkit.org/changeset/67979