Bug 118330 – The callback for WKPageGetPlugInInformation needs info about whether or not there are any non-playing instances of the plug-in on the page

• Component: WebKit2
• Status: RESOLVED FIXED

[Jessie Berlin]

<rdar://problem/14320278>

[Jessie Berlin]

Created attachment 205954 [details]
Patch

[Dean Jackson]

Comment on attachment 205954 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:1000
> +        HTMLPlugInImageElement* plugInImageElement = toHTMLPlugInImageElement(node);
> +        mimeTypes.add(plugInImageElement->loadedMimeType());

Nit: merge into one line.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:1022
> +        RefPtr<NodeList> plugIns = frame->document()->getElementsByTagName(WebCore::HTMLNames::embedTag.localName());

What about <object>?

[Dean Jackson]

Comment on attachment 205954 [details]
Patch

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

>> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:1022
>> +        RefPtr<NodeList> plugIns = frame->document()->getElementsByTagName(WebCore::HTMLNames::embedTag.localName());
> 
> What about <object>?

Duh! right below :)

[Andy Estes]

Comment on attachment 205954 [details]
Patch

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

> Source/WebKit2/Shared/Plugins/Netscape/PluginInformation.cpp:106
> +    return ASCIILiteral("PluginInformationPageContainsNonPlayingInstanceOfPlugIn");

Plugin or PlugIn?

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:1024
> +        RefPtr<NodeList> plugIns = frame->document()->getElementsByTagName(WebCore::HTMLNames::embedTag.localName());
> +        if (plugIns)
> +            addPlugInMimeTypesFromNodeListForNonPlayingPlugIns(plugIns, nonPlayingPlugInMimeTypes);

Why keep a RefPtr here instead of just passing the PassRefPtr you got from getElementsByTagName() into addPlugInMimeTypesFromNodeListForNonPlayingPlugIns()? You seem to be adding some refcount churn that isn't necessary.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:1028
> +        plugIns = frame->document()->getElementsByTagName(WebCore::HTMLNames::objectTag.localName());
> +        if (plugIns)
> +            addPlugInMimeTypesFromNodeListForNonPlayingPlugIns(plugIns, nonPlayingPlugInMimeTypes);

Ditto.

Also, should you be looking for <applet> elements?

[Anders Carlsson]

Comment on attachment 205954 [details]
Patch

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

>> Source/WebKit2/Shared/Plugins/Netscape/PluginInformation.cpp:106
>> +    return ASCIILiteral("PluginInformationPageContainsNonPlayingInstanceOfPlugIn");
> 
> Plugin or PlugIn?

PlugIn!

> Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:503
> +    for (const String& plugInMimeType : nonPlayingPlugInInstanceMimeTypes) {

No space before the colon.

>> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:1028
>> +            addPlugInMimeTypesFromNodeListForNonPlayingPlugIns(plugIns, nonPlayingPlugInMimeTypes);
> 
> Ditto.
> 
> Also, should you be looking for <applet> elements?

Yes, should look for <applet>s.

[Jessie Berlin]

(In reply to comment #5)
> (From update of attachment 205954 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205954&action=review
> 
> >> Source/WebKit2/Shared/Plugins/Netscape/PluginInformation.cpp:106
> >> +    return ASCIILiteral("PluginInformationPageContainsNonPlayingInstanceOfPlugIn");
> > 
> > Plugin or PlugIn?
> 
> PlugIn!

Fixed!

> 
> > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:503
> > +    for (const String& plugInMimeType : nonPlayingPlugInInstanceMimeTypes) {
> 
> No space before the colon.

Fixed.

> 
> >> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:1028
> >> +            addPlugInMimeTypesFromNodeListForNonPlayingPlugIns(plugIns, nonPlayingPlugInMimeTypes);
> > 
> > Ditto.

I was mimicking HTMLPlugInImageElement::restartSimilarPlugIns a little too closely.

Fixed, and added an early return in addPlugInMimeTypesFromNodeListForNonPlayingPlugIns if !plugIns. 

> > 
> > Also, should you be looking for <applet> elements?
> 
> Yes, should look for <applet>s.

No, we do not snapshot Java.

Added an assertion in addPlugInMimeTypesFromNodeListForNonPlayingPlugIns:

ASSERT(!plugInElement->hasTagName(WebCore::HTMLNames::appletTag));

[Jessie Berlin]

Comment on attachment 205954 [details]
Patch

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