[webkit-reviews] review denied: [Bug 227924] Add initial support for BroadcastChannel behind a runtime flag : [Attachment 433515] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Chris Dumez
<cdumez at apple.com>'s request for review:
Bug 227924: Add initial support for BroadcastChannel behind a runtime flag
https://bugs.webkit.org/show_bug.cgi?id=227924

Attachment 433515: Patch

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




--- Comment #7 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 433515
  --> https://bugs.webkit.org/attachment.cgi?id=433515
Patch

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

> Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:96
> +	 default: false

It might be nice to add a comment saying that it should stay off until
https://github.com/whatwg/html/issues/5803 is resolved.

> Source/WebCore/dom/BroadcastChannelRegistry.cpp:35
> +static BroadcastChannelRegistry* globalRegistry;

Is this null-initialized? I can never remember.

> Source/WebCore/dom/BroadcastChannelRegistry.cpp:94
> +	   globalRegistry = new LocalBroadcastChannelRegistry;

When would this be called?  WebKitLegacy?

> Source/WebCore/dom/DeviceOrientationAndMotionAccessController.h:32
> +#include "SecurityOriginData.h"

Aren't unified source files fun?

> Source/WebKit/NetworkProcess/NetworkBroadcastChannelRegistry.cpp:76
> +	   IPC::Connection::send(globalIdentifier.connectionIdentifier,
Messages::WebBroadcastChannelRegistry::PostMessageToRemote(globalIdentifier.cha
nnelIndentifierInProcess, message), 0);

If I'm reading this correctly, this makes a communication channel between pages
using ephemeral and non-ephemeral data stores.	There's a lot of globals that
should probably be owned by something, and we should probably add a test
verifying that pages with different data stores cannot communicate with each
other.

> Source/WebKit/NetworkProcess/NetworkBroadcastChannelRegistry.h:64
> +    using NameToChannelIdentifiersMap = HashMap<String,
Vector<GlobalBroadcastChannelIdentifier>>;

Why a Vector?  Don't we iterate this or remove from it?  Aren't they all
unique?  Do we need order?