[webkit-reviews] review granted: [Bug 234534] Make Notification identifiers be a UUID string instead of a uint64_t : [Attachment 447726] PFR

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted Brady Eidson
<beidson at apple.com>'s request for review:
Bug 234534: Make Notification identifiers be a UUID string instead of a
uint64_t
https://bugs.webkit.org/show_bug.cgi?id=234534

Attachment 447726: PFR

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




--- Comment #9 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 447726
  --> https://bugs.webkit.org/attachment.cgi?id=447726
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.