Bug 234534 – Make Notification identifiers be a UUID string instead of a uint64_t

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Website

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 447676 [details]
Does it build and test v1

[Brady Eidson]

Created attachment 447677 [details]
Does it build and test v2

[Brady Eidson]

Created attachment 447681 [details]
Does it build and test v3

[Brady Eidson]

Created attachment 447682 [details]
Does it build and test v3.1

[Alex Christensen]

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.

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 447726 [details]
PFR

[Brady Eidson]

Filed https://bugs.webkit.org/show_bug.cgi?id=234571 for making UUID be a 128bit integer instead of a String.

[Alex Christensen]

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.

[Brady Eidson]

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

[Brady Eidson]

Created attachment 447753 [details]
PFL v1

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/86792419>

[Ryan Haddad]

*** Bug 234437 has been marked as a duplicate of this bug. ***

[Ryan Haddad]

*** Bug 234436 has been marked as a duplicate of this bug. ***