On Thu Aug 25 22:59:28 2022 +0000, Ben Cottrell wrote:
> I wish you had cut to the point. I'm closing this merge request for now,
> and reconsidering whether I should contribute again, because this has
> been more difficult than it should have been.
What do you mean? A complex piece of software like this has rigid standards in order to prevent potential issues from arising, whether immediately or in the future.
I too made a few mistakes in my merge requests, but I have always taken the time to fix them and make things look nice. This is what improving is about.
Also, consider that generally the more reviewers the better:
- @julliard noticed that you changed the WinAPI definitions and explained that you cannot do that.
- @jhol spotted the icon issue.
- I tested your changes and reported back the results.
Imagine if this merge request was accepted as it was right at the beginning: guaranteed breakage.
If you deem your contribution(s) important to the project, please put on hold the MR until you have time to fix it.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/672#note_6968
On Thu Aug 25 18:51:50 2022 +0000, jhol wrote:
> It looks like you just rotated the up arrow 90-degrees, and it isn't
> aligned with the pixel grid making it look blurry.
> The original icon was derrived from
> `tango-icon-theme-0.8.90/scalable/actions/go-XXX.svg`, but with the
> layout adjusted to reduce the size to 24x24 while preserving the
> 1-pixel. The end result should match
> `tango-icon-theme-0.8.90/22x22/actions/go-XXX.svg` with a 1-pixel
> transparent border all around.
I wish you had cut to the point. I'm closing this merge request for now, and reconsidering whether I should contribute again, because this has been more difficult than it should have been.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/672#note_6965
This should implement all of sessionStorage and most of the missing localStorage (except for the space quota for the latter). Some other common parts are still missing and pending (using props to directly access items in the underlying storage, and StorageEvents, which will come later).
On native, sessionStorage seems to be per-thread, and based on a specific origin, so it's implemented that way using a rbtree for origins in the thread local storage. The diff below (applied after all of the patches) should show that, which works on native as expected, but it's not in the actual commits because it crashes wine-gecko due to known multi-threading issues ([bug 37906](https://bugs.winehq.org/show_bug.cgi?id=37906)).
```diff
diff --git a/dlls/mshtml/tests/misc.c b/dlls/mshtml/tests/misc.c
index c2c8370..e358e9c 100644
--- a/dlls/mshtml/tests/misc.c
+++ b/dlls/mshtml/tests/misc.c
@@ -193,6 +193,32 @@ static HRESULT get_sessionstorage(IHTMLDocument2 *doc, IHTMLStorage **storage)
return hres;
}
+static DWORD WINAPI test_HTMLStorage_thread(void *data)
+{
+ IHTMLStorage *storage;
+ IHTMLDocument2 *doc;
+ BSTR key = data;
+ HRESULT hres;
+ VARIANT var;
+
+ CoInitialize(NULL);
+
+ doc = create_doc_from_url(L"http://www.codeweavers.com/");
+ hres = get_sessionstorage(doc, &storage);
+ ok(hres == S_OK, "got %08lx\n", hres);
+
+ V_VT(&var) = 0xdead;
+ hres = IHTMLStorage_getItem(storage, key, &var);
+ ok(hres == S_OK, "getItem failed: %08lx\n", hres);
+ ok(V_VT(&var) == VT_NULL, "got %d\n", V_VT(&var));
+
+ IHTMLStorage_Release(storage);
+ IHTMLDocument2_Release(doc);
+
+ CoUninitialize();
+ return 0;
+}
+
static void test_HTMLStorage(void)
{
IHTMLDocument2 *doc, *doc2;
@@ -200,7 +226,9 @@ static void test_HTMLStorage(void)
LONG space, length, lval;
VARIANT var;
BSTR key, value;
+ HANDLE thread;
HRESULT hres;
+ DWORD ret;
doc = create_doc_from_url(L"http://www.codeweavers.com/");
doc2 = create_doc_from_url(L"http://www.codeweavers.com/");
@@ -607,6 +635,12 @@ static void test_HTMLStorage(void)
ok(hres == S_OK, "get_remainingSpace failed %08lx\n", hres);
ok(lval == space, "remainingSpace = %ld\n", lval);
+ /* Different thread */
+ thread = CreateThread(NULL, 0, test_HTMLStorage_thread, key, 0, NULL);
+ ret = WaitForSingleObject(thread, INFINITE);
+ ok(ret == WAIT_OBJECT_0, "WaitForSingleObject returned %08lx\n", ret);
+ CloseHandle(thread);
+
hres = IHTMLStorage_clear(storage);
ok(hres == S_OK, "clear failed %08lx\n", hres);
```
There's another rbtree for the actual storage on a given origin, which contains key/value pairs, with keys stored inline because they do not change.
--
v2: mshtml: Implement remainingSpace prop for sessionStorage.
mshtml: Implement length prop for Storage.
mshtml: Implement key() for localStorage.
mshtml: Implement key() for sessionStorage.
mshtml: Implement clear() for Storage.
mshtml: Implement removeItem() for sessionStorage.
mshtml: Implement getItem() for sessionStorage.
mshtml: Implement setItem() for sessionStorage.
https://gitlab.winehq.org/wine/wine/-/merge_requests/704