Bug 46209

Summary: WebKit2 should look for WebKit2WebProcess.exe next to WebKit.dll
Product: WebKit Reporter: Brian Weinstein <bweinstein>
Component: WebKit2Assignee: Brian Weinstein <bweinstein>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aroben, eric, sfalken, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows 7   
Attachments:
Description Flags
[PATCH] Fix aroben: review+, bweinstein: commit-queue-

Description Brian Weinstein 2010-09-21 12:49:32 PDT 
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
Comment 1 Brian Weinstein 2010-09-21 12:56:06 PDT 
Created attachment 68277 [details]
[PATCH] Fix
Comment 2 Steve Falkenburg 2010-09-21 13:04:30 PDT 
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,
Comment 3 Adam Roben (:aroben) 2010-09-21 13:07:35 PDT 
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.
Comment 4 Brian Weinstein 2010-09-21 13:10:16 PDT 
(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.
Comment 5 Brian Weinstein 2010-09-21 13:11:16 PDT 
(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.
Comment 6 Brian Weinstein 2010-09-21 13:20:22 PDT 
Landed in r67979.