46209 2010-09-21 12:49:32 -0700 WebKit2 should look for WebKit2WebProcess.exe next to WebKit.dll 2010-09-21 13:53:23 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) PC Windows 7 RESOLVED FIXED P2 Normal --- 1 bweinstein bweinstein abarth aroben eric sfalken webkit.review.bot oldest_to_newest 282490 0 bweinstein 2010-09-21 12:49:32 -0700 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 282491 1 68277 bweinstein 2010-09-21 12:56:06 -0700 Created attachment 68277 [PATCH] Fix 282496 2 68277 sfalken 2010-09-21 13:04:30 -0700 Comment on attachment 68277 [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, 282499 3 68277 aroben 2010-09-21 13:07:35 -0700 Comment on attachment 68277 [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. 282506 4 bweinstein 2010-09-21 13:10:16 -0700 (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. 282507 5 bweinstein 2010-09-21 13:11:16 -0700 (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. 282515 6 bweinstein 2010-09-21 13:20:22 -0700 Landed in r67979. 282542 7 webkit.review.bot 2010-09-21 13:53:23 -0700 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 68277 2010-09-21 12:56:06 -0700 2010-09-21 13:07:35 -0700 [PATCH] Fix web_process_webkit_dll.patch text/plain 2687 bweinstein SW5kZXg6IFdlYktpdDIvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYktpdDIvQ2hhbmdlTG9n CShyZXZpc2lvbiA2Nzk3NykKKysrIFdlYktpdDIvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTYgQEAKKzIwMTAtMDktMjEgIEJyaWFuIFdlaW5zdGVpbiAgPGJ3ZWluc3RlaW5A YXBwbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIFdlYktpdDIgc2hvdWxkIGxvb2sgZm9yIFdlYktpdDJXZWJQcm9jZXNzLmV4ZSBuZXh0IHRv IFdlYktpdC5kbGwKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTQ2MjA5CisgICAgICAgIAorICAgICAgICBGaW5kIFdlYktpdDJXZWJQcm9jZXNzLmV4ZSBi eSBnZXR0aW5nIHRoZSBmdWxsIHBhdGggdG8gV2ViS2l0LmRsbCwgYW5kIHRoZW4KKyAgICAgICAg cmVtb3ZpbmcgdGhlIGxhc3QgcGF0aCBjb21wb25lbnQgYW5kIHJlcGxhY2luZyBpdCB3aXRoIFdl YktpdDJXZWJQcm9jZXNzLmV4ZS4KKworICAgICAgICAqIFVJUHJvY2Vzcy9MYXVuY2hlci93aW4v UHJvY2Vzc0xhdW5jaGVyV2luLmNwcDoKKyAgICAgICAgKFdlYktpdDo6UHJvY2Vzc0xhdW5jaGVy OjpsYXVuY2hQcm9jZXNzKToKKwogMjAxMC0wOS0yMSAgQW5kZXJzIENhcmxzc29uICA8YW5kZXJz Y2FAYXBwbGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEFkYW0gUm9iZW4uCkluZGV4OiBX ZWJLaXQyL1VJUHJvY2Vzcy9MYXVuY2hlci93aW4vUHJvY2Vzc0xhdW5jaGVyV2luLmNwcAo9PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09Ci0tLSBXZWJLaXQyL1VJUHJvY2Vzcy9MYXVuY2hlci93aW4vUHJvY2Vzc0xhdW5jaGVy V2luLmNwcAkocmV2aXNpb24gNjc4ODgpCisrKyBXZWJLaXQyL1VJUHJvY2Vzcy9MYXVuY2hlci93 aW4vUHJvY2Vzc0xhdW5jaGVyV2luLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMjcsMTIgKzI3LDE1 IEBACiAKICNpbmNsdWRlICJDb25uZWN0aW9uLmgiCiAjaW5jbHVkZSAiUnVuTG9vcC5oIgorI2lu Y2x1ZGUgPHNobHdhcGkuaD4KICNpbmNsdWRlIDx3dGYvdGV4dC9XVEZTdHJpbmcuaD4KIAogI2lm ICFkZWZpbmVkKE5ERUJVRykgJiYgKCFkZWZpbmVkKERFQlVHX0lOVEVSTkFMKSB8fCBkZWZpbmVk KERFQlVHX0FMTCkpCiBjb25zdCBMUENXU1RSIHdlYlByb2Nlc3NOYW1lID0gTCJXZWJLaXQyV2Vi UHJvY2Vzc19kZWJ1Zy5leGUiOworY29uc3QgTFBDV1NUUiB3ZWJraXRETExOYW1lID0gTCJXZWJL aXRfZGVidWcuZGxsIjsKICNlbHNlCiBjb25zdCBMUENXU1RSIHdlYlByb2Nlc3NOYW1lID0gTCJX ZWJLaXQyV2ViUHJvY2Vzcy5leGUiOworY29uc3QgTFBDV1NUUiB3ZWJraXRETExOYW1lID0gTCJX ZWJLaXQuZGxsIjsKICNlbmRpZgogCiBuYW1lc3BhY2UgV2ViS2l0IHsKQEAgLTQ4LDEyICs1MSwy NSBAQCB2b2lkIFByb2Nlc3NMYXVuY2hlcjo6bGF1bmNoUHJvY2VzcygpCiAKICAgICAvLyBFbnN1 cmUgdGhhdCB0aGUgY2hpbGQgcHJvY2VzcyBpbmhlcml0cyB0aGUgY2xpZW50IGlkZW50aWZpZXIu CiAgICAgOjpTZXRIYW5kbGVJbmZvcm1hdGlvbihjbGllbnRJZGVudGlmaWVyLCBIQU5ETEVfRkxB R19JTkhFUklULCBIQU5ETEVfRkxBR19JTkhFUklUKTsKLSAgICAgICAgCi0gICAgVmVjdG9yPFVD aGFyPiBjb21tYW5kTGluZVZlY3RvcjsKIAotICAgIC8vIEZJWE1FOiBXZSB3b3VsZCBsaWtlIHRv IHBhc3MgYSBmdWxsIHBhdGggdG8gdGhlIC5leGUgaGVyZS4KKyAgICAvLyBUbyBnZXQgdGhlIGZ1 bGwgZmlsZSBwYXRoIHRvIFdlYktpdDJXZWJQcm9jZXNzLmV4ZSwgd2UgZmlsZCB0aGUgbG9jYXRp b24gb2YgV2ViS2l0LmRsbCwKKyAgICAvLyByZW1vdmUgdGhlIGxhc3QgcGF0aCBjb21wb25lbnQs IGFuZCB0aGVuIGFwcGVuZCBXZWJLaXQyV2ViUHJvY2VzcyhfZGVidWcpLmV4ZS4KKyAgICBITU9E VUxFIHdlYmtpdE1vZHVsZSA9IEdldE1vZHVsZUhhbmRsZSh3ZWJraXRETExOYW1lKTsKKyAgICBB U1NFUlQod2Via2l0TW9kdWxlKTsKKyAgICBpZiAoIXdlYmtpdE1vZHVsZSkKKyAgICAgICAgcmV0 dXJuOworCisgICAgV0NIQVIgcGF0aFN0cltNQVhfUEFUSF07CisgICAgaWYgKCFHZXRNb2R1bGVG aWxlTmFtZSh3ZWJraXRNb2R1bGUsIHBhdGhTdHIsIE1BWF9QQVRIKSB8fCBHZXRMYXN0RXJyb3Io KSAhPSBFUlJPUl9TVUNDRVNTKQorICAgICAgICByZXR1cm47CisKKyAgICBQYXRoUmVtb3ZlRmls ZVNwZWMocGF0aFN0cik7CisgICAgaWYgKCFQYXRoQXBwZW5kKHBhdGhTdHIsIHdlYlByb2Nlc3NO YW1lKSkKKyAgICAgICAgcmV0dXJuOworCisgICAgU3RyaW5nIGNvbW1hbmRMaW5lKHBhdGhTdHIp OwogCi0gICAgU3RyaW5nIGNvbW1hbmRMaW5lKHdlYlByb2Nlc3NOYW1lKTsKKyAgICBWZWN0b3I8 VUNoYXI+IGNvbW1hbmRMaW5lVmVjdG9yOwogICAgIGFwcGVuZChjb21tYW5kTGluZVZlY3Rvciwg Y29tbWFuZExpbmUpOwogICAgIGFwcGVuZChjb21tYW5kTGluZVZlY3RvciwgIiAtbW9kZSB3ZWJw cm9jZXNzIik7CiAgICAgYXBwZW5kKGNvbW1hbmRMaW5lVmVjdG9yLCAiIC1jbGllbnRJZGVudGlm aWVyICIpOwo=