[webkit-reviews] review granted: [Bug 48775] Both the WebProcessConnection and PluginProcessConnection should have NPRemoteObjectMaps : [Attachment 72545] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Anders Carlsson
<andersca at apple.com>'s request for review:
Bug 48775: Both the WebProcessConnection and PluginProcessConnection should
have NPRemoteObjectMaps
https://bugs.webkit.org/show_bug.cgi?id=48775

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=72545&action=review

> WebKit2/Shared/Plugins/NPRemoteObjectMap.cpp:57
> +uint64_t NPRemoteObjectMap::registerNPObject(NPObject* npObject)
> +{
> +    uint64_t npObjectID = generateNPObjectID();
> +
> +    OwnPtr<NPObjectMessageReceiver> npObjectMessageReceiver =
NPObjectMessageReceiver::create(npObject);
> +    m_registeredNPObjects.set(npObjectID,
npObjectMessageReceiver.leakPtr());

I'm not sure the local variable really helps here.

It doesn't seem like this function will do the right thing if the same npObject
is passed in twice.

> WebKit2/Shared/Plugins/NPRemoteObjectMap.h:33
> +#include <wtf/HashMap.h>
>  #include <wtf/Noncopyable.h>
> +#include <WebCore/npruntime.h>

ASCII order please!

> WebKit2/Shared/Plugins/NPRemoteObjectMap.h:42
> +class NPObjectProxy;
> +class NPObjectMessageReceiver;

Here too!

> WebKit2/Shared/Plugins/NPRemoteObjectMap.h:53
> +    // Creates an NPObjectProxy wrapper for the remote object with the given
remote object ID.
> +    NPObjectProxy* getOrCreateNPObjectProxy(uint64_t remoteObjectID);
> +
> +    uint64_t registerNPObject(NPObject*);

No comment here? This function seems at least as mysterious as
getOrCreateNPObjectProxy.