Bug 234534 - Make Notification identifiers be a UUID string instead of a uint64_t
Summary: Make Notification identifiers be a UUID string instead of a uint64_t
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Website (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
: 234436 234437 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-12-20 15:54 PST by Brady Eidson
Modified: 2021-12-22 14:04 PST (History)
8 users (show)

See Also:


Attachments
Does it build and test v1 (69.96 KB, patch)
2021-12-20 20:08 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Does it build and test v2 (70.14 KB, patch)
2021-12-20 20:17 PST, Brady Eidson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Does it build and test v3 (70.13 KB, patch)
2021-12-20 20:41 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Does it build and test v3.1 (69.99 KB, patch)
2021-12-20 20:43 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
PFR (80.23 KB, patch)
2021-12-21 09:35 PST, Brady Eidson
achristensen: review+
Details | Formatted Diff | Diff
PFL v1 (80.34 KB, patch)
2021-12-21 14:53 PST, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2021-12-20 15:54:32 PST
Make Notification identifiers be a UUID string instead of a uint64_t

This will let the identifier assigned at creation time in each WebProcess be global across the entire process ecosystem, simplifying a lot of code.
Comment 1 Brady Eidson 2021-12-20 20:08:47 PST
Created attachment 447676 [details]
Does it build and test v1
Comment 2 Brady Eidson 2021-12-20 20:17:12 PST
Created attachment 447677 [details]
Does it build and test v2
Comment 3 Brady Eidson 2021-12-20 20:41:42 PST
Created attachment 447681 [details]
Does it build and test v3
Comment 4 Brady Eidson 2021-12-20 20:43:09 PST
Created attachment 447682 [details]
Does it build and test v3.1
Comment 5 Alex Christensen 2021-12-20 22:13:16 PST
Comment on attachment 447682 [details]
Does it build and test v3.1

I wonder if it would be worth making a better storage for WTF::UUID that stores a std::pair<uint64_t, uint64_t> and generates the string on demand.
Comment 6 Brady Eidson 2021-12-21 08:33:42 PST
(In reply to Alex Christensen from comment #5)
> Comment on attachment 447682 [details]
> Does it build and test v3.1
> 
> I wonder if it would be worth making a better storage for WTF::UUID that
> stores a std::pair<uint64_t, uint64_t> and generates the string on demand.

Now that we're identifying more than one class via UUID, I think it's very worth having that discussion.

I had the same thought before I went to WTF/UUID - If we had a nice and easy to use 128bit int type I could've rolled my own with <WebPageProxyIdentifier + per-WebContent-generated-uint64_t>

And that system would probably be generally applicable to many/most UUID users.
Comment 7 Brady Eidson 2021-12-21 09:35:09 PST
Created attachment 447726 [details]
PFR
Comment 8 Brady Eidson 2021-12-21 11:30:21 PST
Filed https://bugs.webkit.org/show_bug.cgi?id=234571 for making UUID be a 128bit integer instead of a String.
Comment 9 Alex Christensen 2021-12-21 14:31:28 PST
Comment on attachment 447726 [details]
PFR

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

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:95
> +    m_notifications.set(notification->coreNotificationID(), notification.copyRef());

I'm not sure if copyRef is needed these days.

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:140
>      for (auto it = m_notifications.begin(), end = m_notifications.end(); it != end; ++it) {

Can this be a C++11-style for loop?

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:181
> +    ASSERT(notification);

We have lots of early returns elsewhere in this function.  Maybe an early return with an assert not reached to be more robust?

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.h:91
> +    HashMap<uint64_t, String> m_globalNotificationMap;

Can this be a typed identifier?

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.h:92
> +    HashMap<String, RefPtr<WebNotification>> m_notifications;

I think the value can be a Ref.
Comment 10 Brady Eidson 2021-12-21 14:42:09 PST
Most feedback addressed without additional comment.

As for this one:

(In reply to Alex Christensen from comment #9)
> 
> > Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.h:91
> > +    HashMap<uint64_t, String> m_globalNotificationMap;
> 
> Can this be a typed identifier?

This will become a new UUID type in the next patch
Comment 11 Brady Eidson 2021-12-21 14:53:09 PST
Created attachment 447753 [details]
PFL v1
Comment 12 EWS 2021-12-21 18:32:59 PST
Committed r287339 (245481@main): <https://commits.webkit.org/245481@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447753 [details].
Comment 13 Radar WebKit Bug Importer 2021-12-21 18:34:18 PST
<rdar://problem/86792419>
Comment 14 Ryan Haddad 2021-12-22 14:04:39 PST
*** Bug 234437 has been marked as a duplicate of this bug. ***
Comment 15 Ryan Haddad 2021-12-22 14:04:54 PST
*** Bug 234436 has been marked as a duplicate of this bug. ***