[PATCH v5 0/1] MR6123: shdocvw: Add IEParseDisplayNameWithBCW semi-stub.
-- v5: shdocvw: Add IEParseDisplayNameWithBCW semi-stub. https://gitlab.winehq.org/wine/wine/-/merge_requests/6123
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/shdocvw/Makefile.in | 2 +- dlls/shdocvw/shdocvw_main.c | 56 +++++++++++++++++++++-- dlls/shdocvw/tests/shdocvw.c | 81 ++++++++++++++++++++++++++++++++++ dlls/shell32/tests/shlfolder.c | 8 ++-- 4 files changed, 139 insertions(+), 8 deletions(-) diff --git a/dlls/shdocvw/Makefile.in b/dlls/shdocvw/Makefile.in index d38782eaf22..bea902a7357 100644 --- a/dlls/shdocvw/Makefile.in +++ b/dlls/shdocvw/Makefile.in @@ -1,7 +1,7 @@ MODULE = shdocvw.dll IMPORTLIB = shdocvw IMPORTS = uuid shlwapi advapi32 -DELAYIMPORTS = version ole32 oleaut32 ieframe +DELAYIMPORTS = version ole32 oleaut32 ieframe shell32 SOURCES = \ shdocvw.rc \ diff --git a/dlls/shdocvw/shdocvw_main.c b/dlls/shdocvw/shdocvw_main.c index 6556223e1d7..af3de6243b4 100644 --- a/dlls/shdocvw/shdocvw_main.c +++ b/dlls/shdocvw/shdocvw_main.c @@ -431,11 +431,61 @@ DWORD WINAPI ParseURLFromOutsideSourceA(LPCSTR url, LPSTR out, LPDWORD plen, LPD /****************************************************************** * IEParseDisplayNameWithBCW (SHDOCVW.218) */ -HRESULT WINAPI IEParseDisplayNameWithBCW(DWORD codepage, LPCWSTR lpszDisplayName, LPBC pbc, LPITEMIDLIST *ppidl) +HRESULT WINAPI IEParseDisplayNameWithBCW(DWORD codepage, LPCWSTR name, IBindCtx *pbc, LPITEMIDLIST *ppidl) { +#include <pshpack1.h> + struct ie_pidl_data + { + BYTE type; /* 0x61 - PT_IESPECIAL1 */ + BYTE dummy1; /* 0x80 */ + DWORD dummy2; /* 0 */ + WCHAR name[1]; /* terminated by double \0 */ + } *ie_data; +#include <poppack.h> + HRESULT hr; + LPITEMIDLIST parent, child, next, ret; + DWORD size, len; + PARSEDURLW res; + /* Guessing at parameter 3 based on IShellFolder's ParseDisplayName */ - FIXME("stub: 0x%lx %s %p %p\n",codepage,debugstr_w(lpszDisplayName),pbc,ppidl); - return E_FAIL; + FIXME("%lu %s %p %p: semi-stub\n", codepage, debugstr_w(name), pbc, ppidl); + + /* Make sure that it's correct URL */ + res.cbSize = sizeof(res); + hr = ParseURLW(name, &res); + if (hr != S_OK) return E_FAIL; + + hr = SHGetFolderLocation(0, CSIDL_INTERNET, 0, 0, &parent); + if (hr != S_OK) return hr; + + len = wcslen(name) + 1; + size = FIELD_OFFSET(struct ie_pidl_data, name[len + 2]); + + child = SHAlloc(size + sizeof(*next)); + if (!child) return E_OUTOFMEMORY; + + child->mkid.cb = size; + ie_data = (struct ie_pidl_data *)child->mkid.abID; + ie_data->type = 0x61; + ie_data->dummy1 = 0x80; + ie_data->dummy2 = 0; + wcscpy(ie_data->name, name); + ie_data->name[len] = 0; + ie_data->name[len + 1] = 0; + + next = (LPITEMIDLIST)((char *)child + child->mkid.cb); + next->mkid.cb = 0; + next->mkid.abID[0] = 0; + + ret = ILCombine(parent, child); + + SHFree(parent); + SHFree(child); + + if (!ret) return E_OUTOFMEMORY; + + *ppidl = ret; + return S_OK; } /****************************************************************** diff --git a/dlls/shdocvw/tests/shdocvw.c b/dlls/shdocvw/tests/shdocvw.c index fcfd4a644db..da917cfe0db 100644 --- a/dlls/shdocvw/tests/shdocvw.c +++ b/dlls/shdocvw/tests/shdocvw.c @@ -18,6 +18,7 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA */ +#define COBJMACROS #include <stdarg.h> @@ -26,6 +27,8 @@ #include "winreg.h" #include "wininet.h" #include "winnls.h" +#include "shtypes.h" +#include "shlobj.h" #include "wine/test.h" @@ -35,6 +38,7 @@ static HMODULE hshdocvw; static HRESULT (WINAPI *pURLSubRegQueryA)(LPCSTR, LPCSTR, DWORD, LPVOID, DWORD, DWORD); static DWORD (WINAPI *pParseURLFromOutsideSourceA)(LPCSTR, LPSTR, LPDWORD, LPDWORD); static DWORD (WINAPI *pParseURLFromOutsideSourceW)(LPCWSTR, LPWSTR, LPDWORD, LPDWORD); +static HRESULT (WINAPI *pIEParseDisplayNameWithBCW)(DWORD, LPCWSTR, LPBC, LPITEMIDLIST *); static const char appdata[] = "AppData"; static const char common_appdata[] = "Common AppData"; @@ -76,6 +80,7 @@ static void init_functions(void) pURLSubRegQueryA = (void *) GetProcAddress(hshdocvw, (LPSTR) 151); pParseURLFromOutsideSourceA = (void *) GetProcAddress(hshdocvw, (LPSTR) 169); pParseURLFromOutsideSourceW = (void *) GetProcAddress(hshdocvw, (LPSTR) 170); + pIEParseDisplayNameWithBCW = (void *) GetProcAddress(hshdocvw, (LPSTR) 218); } /* ################ */ @@ -351,11 +356,87 @@ static void test_ParseURLFromOutsideSourceW(void) /* ################ */ +static void test_IE_pidl(LPITEMIDLIST pidl, const WCHAR *name) +{ +#include <pshpack1.h> + struct ie_pidl_data + { + BYTE type; /* 0x61 - PT_IESPECIAL1 */ + BYTE dummy1; /* 0x80 */ + DWORD dummy2; /* 0 */ + WCHAR name[1]; /* terminated by double \0 */ + } *data; +#include <poppack.h> + + while (pidl->mkid.cb) + { + int len; + + if (pidl->mkid.abID[0] == 0x1f) + { + GUID *guid = (GUID *)&pidl->mkid.abID[2]; + ok(IsEqualGUID(guid, &CLSID_Internet), "got %s\n", wine_dbgstr_guid(guid)); + ok(pidl->mkid.abID[1] == 0, "got %#x\n", pidl->mkid.abID[1]); + len = sizeof(GUID) + 4; + ok(pidl->mkid.cb == len, "expected %d, got %d\n", len, pidl->mkid.cb); + } + else if (pidl->mkid.abID[0] == 0x61) + { + data = (struct ie_pidl_data *)pidl->mkid.abID; + ok(data->dummy1 == 0x80, "got %#x\n", data->dummy1); + ok(data->dummy2 == 0, "got %#lx\n", data->dummy2); + ok(!wcscmp(data->name, name), "got %s\n", wine_dbgstr_w(data->name)); + len = wcslen(data->name) + 1; + ok(data->name[len] == 0, "got %d\n", data->name[len]); + ok(data->name[len + 1] == 0, "got %d\n", data->name[len + 1]); + len = FIELD_OFFSET(struct ie_pidl_data, name) + (len + 2) * sizeof(WCHAR); + ok(pidl->mkid.cb == len, "expected %d, got %d\n", len, pidl->mkid.cb); + } + + pidl = (LPITEMIDLIST)((char *)pidl + pidl->mkid.cb); + } +} + +static void test_IEParseDisplayNameWithBCW(void) +{ + HRESULT hr; + IBindCtx *ctx; + LPITEMIDLIST pidl; + + if (!pIEParseDisplayNameWithBCW) + { + skip("IEParseDisplayNameWithBCW not found\n"); + return; + } + + hr = CreateBindCtx(0, &ctx); + ok(hr == S_OK, "got %#lx\n", hr); + + hr = pIEParseDisplayNameWithBCW(0, L"custom://url", ctx, &pidl); + ok(hr == S_OK, "got %#lx\n", hr); + test_IE_pidl(pidl, L"custom://url"); + SHFree(pidl); + + hr = pIEParseDisplayNameWithBCW(0, L"readme.txt", ctx, &pidl); + ok(hr == E_FAIL, "got %#lx\n", hr); + + hr = pIEParseDisplayNameWithBCW(0, L"ftp://url.org/readme.txt", ctx, &pidl); + ok(hr == S_OK, "got %#lx\n", hr); + test_IE_pidl(pidl, L"ftp://url.org/readme.txt"); + SHFree(pidl); + + if (0) /* hangs */ + hr = pIEParseDisplayNameWithBCW(0, L"file://readme.txt", ctx, &pidl); + + IBindCtx_Release(ctx); +} + START_TEST(shdocvw) { init_functions(); test_URLSubRegQueryA(); test_ParseURLFromOutsideSourceA(); test_ParseURLFromOutsideSourceW(); + test_IEParseDisplayNameWithBCW(); FreeLibrary(hshdocvw); } diff --git a/dlls/shell32/tests/shlfolder.c b/dlls/shell32/tests/shlfolder.c index f1cae84257a..21152f98484 100644 --- a/dlls/shell32/tests/shlfolder.c +++ b/dlls/shell32/tests/shlfolder.c @@ -213,10 +213,10 @@ static struct {{'t','e','s','t','\\',0}, 0x80070002}, {{'s','u','b','\\','d','i','r',0}, 0x80070002}, {{'s','u','b','/','d','i','r',0}, E_INVALIDARG, 1}, - {{'h','t','t','p',':',0}, S_OK, 1}, - {{'h','t','t','p',':','t','e','s','t',0}, S_OK, 1}, - {{'h','t','t','p',':','\\','t','e','s','t',0}, S_OK, 1}, - {{'x','x',':',0}, S_OK, 1}, + {{'h','t','t','p',':',0}, S_OK}, + {{'h','t','t','p',':','t','e','s','t',0}, S_OK}, + {{'h','t','t','p',':','\\','t','e','s','t',0}, S_OK}, + {{'x','x',':',0}, S_OK}, }; static void test_ParseDisplayName(void) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6123
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=147391 Your paranoid android. === debian11b (64 bit WoW report) === user32: input.c:4305: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 00000000006F00F0, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032
On Sun Jul 28 15:52:56 2024 +0000, Dmitry Timoshkov wrote:
changed this line in [version 5 of the diff](/wine/wine/-/merge_requests/6123/diffs?diff_id=123977&start_sha=a2b23f8d3ffc9271c24863db0f666652ff4ceb43#2da6a60bd85e580d0caed81089d54aa91c2f6997_462_462) I have updated the patch, is there anything else that could be improved?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6123#note_77300
On Sun Jul 28 15:16:23 2024 +0000, Dmitry Timoshkov wrote:
It looks like `IEParseDisplayNameWithBCW() was removed in later versions, so no, we can't use an importlib.` Maybe it's not the best idea to use it in shell32 then? We could just implement it there instead.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6123#note_77360
On Tue Jul 30 14:54:04 2024 +0000, Jacek Caban wrote:
Maybe it's not the best idea to use it in shell32 then? We could just implement it there instead. We already have it, this implementation works and the application that I have had here lo longer fails. Moving the implementation could be a separate effort.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6123#note_77361
On Tue Jul 30 15:00:06 2024 +0000, Dmitry Timoshkov wrote:
We already have it, this implementation works and the application that I have had here lo longer fails. Moving the implementation could be a separate effort. If your tests are skipped on all Windows version we care about, they are not very useful. It would be more interesting to know what shell32 does, that's what you're aiming to fix anyway. There is no guarantee that it should do the same thing, esp. if the reference is some ancient IE.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6123#note_77684
On Fri Aug 2 19:30:35 2024 +0000, Jacek Caban wrote:
If your tests are skipped on all Windows version we care about, they are not very useful. It would be more interesting to know what shell32 does, that's what you're aiming to fix anyway. There is no guarantee that it should do the same thing, esp. if the reference is some ancient IE. This patch aims to implement IEParseDisplayNameWithBCW() which is an existing dependency that shell32 calls. The tests pass on XP where this export exists, and the implementation in this MR was created on the observed results of these tests . I.e. this MR implements an existing API that currently being called by shell32, and doesn't introduce anything new. Investigating what shell32 does is far beyond this MR, and putting burden on me to do that is not a fair move IMO.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6123#note_77686
On Fri Aug 2 19:45:50 2024 +0000, Dmitry Timoshkov wrote:
This patch aims to implement IEParseDisplayNameWithBCW() which is an existing dependency that shell32 calls. The tests pass on XP where this export exists, and the implementation in this MR was created on the observed results of these tests . I.e. this MR implements an existing API that currently being called by shell32, and doesn't introduce anything new. Investigating what shell32 does is far beyond this MR, and putting burden on me to do that is not a fair move IMO. That's not what you previously stated:
Wine's dlls/shell32 uses IEParseDisplayNameWithBCW() to parse desktop pidls and the application that I was working on failed because of it.
Anyway, I'm not an expert on shell32, it was blocked on me because of an old assumption that shdocvw is related to WebBrowser control. I gave you my opinion, but the MR doesn't need my approval and I removed myself as a reviewer to not block it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6123#note_77687
On Fri Aug 2 20:06:55 2024 +0000, Jacek Caban wrote:
That's not what you previously stated:
Wine's dlls/shell32 uses IEParseDisplayNameWithBCW() to parse desktop pidls and the application that I was working on failed because of it. Anyway, I'm not an expert on shell32, it was blocked on me because of an old assumption that shdocvw is related to WebBrowser control. I gave you my opinion, but the MR doesn't need my approval and I removed myself as a reviewer to not block it. Could you please elaborate on what I previously stated?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6123#note_77688
On Fri Aug 2 20:18:27 2024 +0000, Dmitry Timoshkov wrote:
Could you please elaborate on what I previously stated? The caller is just IShellFolder::ParseDisplayName() on the desktop folder, which wraps this function pretty much directly. It seems very easy to move the tests to shell32 and convert them to call IShellFolder::ParseDisplayName() instead of IEParseDisplayNameWithBC() [which would allow the tests to actually work on any of our testbot machines], and it seems very easy to move the implementation to shell32 and stop calling into shdocvw [which would apparently match the architecture of modern Windows]. Is there any reason not to do this?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6123#note_77725
On Sat Aug 3 20:23:16 2024 +0000, Elizabeth Figura wrote:
The caller is just IShellFolder::ParseDisplayName() on the desktop folder, which wraps this function pretty much directly. It seems very easy to move the tests to shell32 and convert them to call IShellFolder::ParseDisplayName() instead of IEParseDisplayNameWithBC() [which would allow the tests to actually work on any of our testbot machines], and it seems very easy to move the implementation to shell32 and stop calling into shdocvw [which would apparently match the architecture of modern Windows]. Is there any reason not to do this? By working on this project and submitting patches we generously give away the most valuable thing that we have - our time. It's pretty much disappointing that the project maintainers take that for granted. This particular patch is old, already took several rounds of suggestions and changes before the assigned for the review person even looked at it. After that the reviewer asked for a code change, and once it was done asked for a complete rewrite of the completely different part of the code from scratch, and after that dropped the ball. Isn't that a little bit not fair? And that's just for this MR with a single patch, there are plenty of other MRs without any attempt for a review or a comment.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6123#note_77728
participants (5)
-
Dmitry Timoshkov -
Dmitry Timoshkov (＠dmitry)
-
Elizabeth Figura (＠zfigura)
-
Jacek Caban (＠jacek)
-
Marvin