[webkit-reviews] review granted: [Bug 98241] [WebKit2] Add the ability to send a message to all processes in a WebPageGroup : [Attachment 166827] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andy Estes <aestes at apple.com>'s
request for review:
Bug 98241: [WebKit2] Add the ability to send a message to all processes in a
WebPageGroup
https://bugs.webkit.org/show_bug.cgi?id=98241

Attachment 166827: Patch
https://bugs.webkit.org/attachment.cgi?id=166827&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=166827&action=review


I’m surprised that it’s useful to have a function that might or might not send
messages to processes and does not indicate whether it succeeded or failed.
Depending on how you intend to use this, it might make sense to me, though. In
this patch it’s just dead code so I don’t know why it’s OK.

> Source/WebKit2/UIProcess/WebPageGroup.h:67
> +    template<typename U> void sendToAllProcessesInGroup(const U& message,
uint64_t destinationID);

A private member function? So this is to be used inside the WebPageGroup class?


> Source/WebKit2/UIProcess/WebPageGroup.h:74
> +template<typename U> inline void
WebPageGroup::sendToAllProcessesInGroup(const U& message, uint64_t
destinationID)

Why chose the name U for the message type? I can imagine using names such as
MessageType or T, but U seems curious.

Why inline?

> Source/WebKit2/UIProcess/WebPageGroup.h:76
> +    HashSet<WebProcessProxy*> webProcessProxiesSeen;

As a local variable this can use a shorter name. I’d call it proxiesSeen or
processesSeen.

> Source/WebKit2/UIProcess/WebPageGroup.h:81
> +	   HashSet<WebProcessProxy*>::AddResult result =
webProcessProxiesSeen.add(webProcessProxy);
> +	   if (!result.isNewEntry)

No need for the local variable here.