Need the ability to get the information for a plug-in with a particular process id that may be running on a page

[Jessie Berlin]

<rdar://problem/13332450>

[Jessie Berlin]

Created attachment 203465 [details]
Patch

[Early Warning System Bot]

Comment on attachment 203465 [details]
Patch

Attachment 203465 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/763088

[EFL EWS Bot]

Comment on attachment 203465 [details]
Patch

Attachment 203465 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/746209

[Jessie Berlin]

Created attachment 203466 [details]
Patch (with build fixes)

[Anders Carlsson]

Comment on attachment 203465 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:496
> +    if (!pluginProcessProxy) {
> +        callback->invalidate();
> +        return;
> +    }

I think this can be an assertion.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:354
> +    void containsPlugInsWithProcessToken(uint64_t plugInProcessToken, uint64_t callbackID);

Maybe call this containsPluginViewsWithPluginProcessToken to match the other two member functions.

[Jessie Berlin]

(In reply to comment #5)
> (From update of attachment 203465 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203465&action=review
> 
> > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:496
> > +    if (!pluginProcessProxy) {
> > +        callback->invalidate();
> > +        return;
> > +    }
> 
> I think this can be an assertion.

Done (removed the call to invalidate and the early return and replaced it with an assertion).

> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.h:354
> > +    void containsPlugInsWithProcessToken(uint64_t plugInProcessToken, uint64_t callbackID);
> 
> Maybe call this containsPluginViewsWithPluginProcessToken to match the other two member functions.

Done.

For those watching at home. Anders r+’d the second version of the patch with these same considerations.

[Jessie Berlin]

Comment on attachment 203466 [details]
Patch (with build fixes)

Committed in http://trac.webkit.org/changeset/151043

[Darin Adler]

Comment on attachment 203466 [details]
Patch (with build fixes)

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

I know Anders already reviewed it, but I did too. r=me

> Source/WebKit2/UIProcess/API/C/mac/WKPagePrivateMac.h:39
> +typedef void (^WKPageGetInformationForPlugInWithProcessIDBlock)(WKDictionaryRef, WKErrorRef);
> +WK_EXPORT void WKPageGetInformationForPlugInWithProcessID(WKPageRef, pid_t plugInProcessID, WKPageGetInformationForPlugInWithProcessIDBlock);

I think these names are too wordy. How about PlugInInformation instead of InformationForPlugIn and omitting WithProcessID. Do we need other GetPlugInInformation functions that don’t take a process ID?

> Source/WebKit2/UIProcess/Plugins/PluginProcessManager.h:67
> +    PluginProcessProxy* plugInProcessWithProcessID(pid_t plugInProcessID) const;
> +    PluginProcessProxy* plugInProcessWithToken(uint64_t plugInProcessToken) const;

The naming here seems sort of “Cocoa method style”. I would probably have called these findPlugInProcessByID and findPlugInProcessByToken.

> Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:475
> +    m_plugInInformationCallbacks.set(callbackID, callback.get());

Should be callback.release() rather than callback.get().

> Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.h:194
> +    virtual uint64_t plugInProcessToken() const OVERRIDE { return 0; }

Why protected instead of private?

[Jessie Berlin]

(In reply to comment #8)
> (From update of attachment 203466 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203466&action=review
> 
> I know Anders already reviewed it, but I did too. r=me
> 
> > Source/WebKit2/UIProcess/API/C/mac/WKPagePrivateMac.h:39
> > +typedef void (^WKPageGetInformationForPlugInWithProcessIDBlock)(WKDictionaryRef, WKErrorRef);
> > +WK_EXPORT void WKPageGetInformationForPlugInWithProcessID(WKPageRef, pid_t plugInProcessID, WKPageGetInformationForPlugInWithProcessIDBlock);
> 
> I think these names are too wordy. How about PlugInInformation instead of InformationForPlugIn and omitting WithProcessID. Do we need other GetPlugInInformation functions that don’t take a process ID?

I modeled this a bit off of WKContextGetInfoForInstalledPlugIns in WKContextPrivateMac.h, which does not take a process ID.

However, I am fine with PlugInInformation given that the type of the argument is clearly pid_t.

> 
> > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.h:67
> > +    PluginProcessProxy* plugInProcessWithProcessID(pid_t plugInProcessID) const;
> > +    PluginProcessProxy* plugInProcessWithToken(uint64_t plugInProcessToken) const;
> 
> The naming here seems sort of “Cocoa method style”. I would probably have called these findPlugInProcessByID and findPlugInProcessByToken.

Will do.

> 
> > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:475
> > +    m_plugInInformationCallbacks.set(callbackID, callback.get());
> 
> Should be callback.release() rather than callback.get().

Will fix.

> 
> > Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.h:194
> > +    virtual uint64_t plugInProcessToken() const OVERRIDE { return 0; }
> 
> Why protected instead of private?

I will make it private.

I already committed the patch. rs=you on these changes?

Thanks!

[Jessie Berlin]

Committed the follow-up patch with a rubber-stamp from Anders in http://trac.webkit.org/changeset/151054