Bug 110308 – shouldUsePluginDocument() needs to be respected when a document is created

• Component: Plug-ins
• Status: RESOLVED FIXED

[Max Feil]

The client needs to be able to specify certain mime types that should be treated as a plugin no matter what. This will allow the "missing plugin" icon to be displayed in those cases where the plugin dll cannot be loaded.

For example, Adobe Flashplayer swf files have a mime type of "application/x-shockwave-flash". Without an explicit call to shouldUsePluginDocument() in DOMImplementation::createHTMLDocument() we end up with web pages displaying swf binary as gibberish HTML (BlackBerry) or downloading the .swf file immediately (Chrome). This fix is specifically targeted at plugin mime types referenced as the src property of iframes.

[Max Feil]

Note that this patch does not impact any non-BlackBerry ports except Windows. For all other ports, shouldUsePluginDocument() always returns false. For Windows the layout tests should provide adequate coverage.

[Max Feil]

Created attachment 189252 [details]
Patch

[Max Feil]

I am proposing a cross-platform change. The only non-BlackBerry port that seems to be affected is Win.

[Alexey Proskuryakov]

It's not very clear what the shouldUsePluginDocument() function is supposed to do, given that it's not implemented on most platforms. It probably needs a better name.

[Max Feil]

(In reply to comment #4)
> It's not very clear what the shouldUsePluginDocument() function is supposed to do, given that it's not implemented on most platforms. It probably needs a better name.

I think the name is quite descriptive. It is already used as the basis for deciding whether PluginDocument::create() should be called at the beginning of  DocumentWriter::createDocument(). I am simply proposing a matching check in the only other place PluginDocument::create() is called (see my proposed patch).

To me this patch makes things consistent, and is low risk.

[Alexey Proskuryakov]

So looking at shouldUsePluginDocument() implementation in WebKit2, I see that it always returns false. If the name was accurate and descriptive, that would mean that WebKit2 never uses PluginDocument, which is obviously incorrect.

[Max Feil]

(In reply to comment #6)
> So looking at shouldUsePluginDocument() implementation in WebKit2, I see that it always returns false. If the name was accurate and descriptive, that would mean that WebKit2 never uses PluginDocument, which is obviously incorrect.

That's getting a bit picky, but I see your point. How about shouldForcePluginDocument()?

[Alexey Proskuryakov]

Will returning true for this function make it so that a PluginDocument would be created for any MIME type, including text/html?

[Max Feil]

(In reply to comment #8)
> Will returning true for this function make it so that a PluginDocument would be created for any MIME type, including text/html?

No, so my suggestion is not good. I really find the current name fine. To get an exactly appropriate name we'd end up with "shouldForcePluginDocumentWithExceptions()" or "shouldTendToUsePluginDocument()" or something equally horrible :)

[Alexey Proskuryakov]

I was not nitpicking, the current name really makes no sense to me.

If there is no good name for the concept, then it just shouldn't exist, and adding more code paths that depend on on it is not advisable.

[Max Feil]

(In reply to comment #10)
> If there is no good name for the concept, then it just shouldn't exist, and adding more code paths that depend on on it is not advisable.

I disagree. We can't make all function names contain the full details of what the function does. For that information a developer can look in the code.

The shouldUsePluginDocument() function simply allows a mime type to be interpreted as being associated with a plugin document without the plugin already being available. There are of course exceptions. A port can't turn text/html into a plugin mime type.

So I would re-word what you said above as "if there is no good way to describe a concept, then its implementation should not be proliferated". But "name of concept" does not equate to "name of function".

Can somebody else chime in here? I really think that an idea for a useful patch has been side tracked by 8 straight comments about a function name that was already implemented and reviewed long ago. If not everyone is happy with that name, then I'll be happy to create a separate bug and cc the original author and reviewers who chose that name.

I'm feeling like Arthur Two Sheds Jackson: http://www.youtube.com/watch?v=HLjS3gzHetA

[Alexey Proskuryakov]

Comment on attachment 189252 [details]
Patch

I don't think that continued arguing would be productive. This patch is harmful, and cannot be landed.

[Carlos Garcia Campos]

Comment on attachment 189252 [details]
Patch

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

I don't know the original purpose of the existing shouldUsePluginDocument(), but maybe instead of trying to come up with a better name, we could add a new method for this particular use case, because it might not be the same. Something like shouldAlwaysCreatePluginDocumentForMIMEType() which means that even if there isn't a plugin in the database to handle the MIME type we still want to create a PluginDocument for this particular MIME type.

> Source/WebCore/dom/DOMImplementation.cpp:420
> -    if (type != "text/plain" && pluginData && pluginData->supportsMimeType(type)) 
> +    if (type != "text/plain" && ((pluginData && pluginData->supportsMimeType(type)) || (frame && frame->loader()->client()->shouldUsePluginDocument(type))))

pluginData is NULL when plugins are disabled (or there's not frame nor page) so I think we should still honor that and only create the PluginDocument if plugins are allowed, what about something like this?

if (type != "text/plain" && pluginData && (pluginData->supportsMimeType(type) || frame->loader()->client()->shouldAlwaysCreatePluginDocumentForMIMEType(type)))

We don't need to check frame because pluginData can only be != NULL if frame is also != NULL

[Max Feil]

(In reply to comment #13)
> pluginData is NULL when plugins are disabled (or there's not frame nor page) so I think we should still honor that and only create the PluginDocument if plugins are allowed, what about something like this?

Thanks for the comment Carlos. The only problem with your suggestion is that when pluginData is null, a non-plugin action will be performed on the src of the iframe, i.e. execution will continue down below the check in question and HTMLDocument::create() will be called. In the case of an Adobe Flashplayer .swf file, the BlackBerry port will render gibberish text for the entire binary (the very problem I am trying to solve). For other ports, if current behaviour holds, the user will be prompted to download the .swf (Safari) or it will just be downloaded automatically (Chrome).

The intent of allowing a client to associate a mime type with a plugin document is to ensure that the missing plugin icon gets rendered, and I think this is also the correct behaviour when pluginData is null.

[Joe Mason]

(In reply to comment #10)
> I was not nitpicking, the current name really makes no sense to me.
> 
> If there is no good name for the concept, then it just shouldn't exist, and adding more code paths that depend on on it is not advisable.

What is the correct way to determine that a page should show the missing plugin icon for a mime type rather than displaying or downloading the content?

[Joe Mason]

(In reply to comment #9)
> (In reply to comment #8)
> > Will returning true for this function make it so that a PluginDocument would be created for any MIME type, including text/html?
> 
> No, so my suggestion is not good. I really find the current name fine. To get an exactly appropriate name we'd end up with "shouldForcePluginDocumentWithExceptions()" or "shouldTendToUsePluginDocument()" or something equally horrible :)

I think "shouldUsePluginDocumentForMIMEType" or simply "shouldUsePluginForMIMEType" would be a good name for this function.

[Max Feil]

(In reply to comment #15)
> What is the correct way to determine that a page should show the missing plugin icon for a mime type rather than displaying or downloading the content?

In the BlackBerry port, we don't use the missing plugin icon. We do (sort of) use the missing plugin text. See the function unavailablePluginReplacementText().

This text gets rendered when HTMLEmbedElement.cpp initiates its subframe load. Plugins are typically placed inside <embed> or <object> elements. This same code path is executed when a PluginDocument is created, which is why my patch works.

When an iframe loads, the code is not assuming that it will contain a plugin (unlike <embed> and <object>). That is why the client needs the ability to tell it to create a plugin document (similar to what is already done for top level frames in DocumentWriter::createDocument()).

Historically, iframes have been an area where code fixes have lagged behind. Often a feature or bug fix is implemented, only to later require a special case for iframes. I feel this bug is no different.

[Max Feil]

(In reply to comment #16)
> I think "shouldUsePluginDocumentForMIMEType" or simply "shouldUsePluginForMIMEType" would be a good name for this function.

I like both of those!

[Alexey Proskuryakov]

> I think "shouldUsePluginDocumentForMIMEType" or simply "shouldUsePluginForMIMEType" would be a good name for this function.

How are these different from "shouldUsePluginDocument"? I think that all the same comments that I made about it still apply.

[Carlos Garcia Campos]

(In reply to comment #19)
> > I think "shouldUsePluginDocumentForMIMEType" or simply "shouldUsePluginForMIMEType" would be a good name for this function.
> 
> How are these different from "shouldUsePluginDocument"? I think that all the same comments that I made about it still apply.

I think shouldAlwaysCreatePluginDocumentForMIMEType() or shouldAlwaysUsePluginDocumentForMIMEType() would address your concerns, since it would make clear that even if they return false, that doesn't mean PluginDocument could not be used.

[Alexey Proskuryakov]

I assume that the proposed name is for an additional client call, as existing usage of shouldUsePluginDocument() doesn't appear to match it.

shouldAlwaysUsePluginDocument() appears correct and descriptive, but please don't add the "ForMIMEType" part. It doesn't add meaning, as a function call always returns a result for its arguments.

However I'm concerned that this adds a new way to do something that is already doable. If I understood the problem statement correctly, Safari doesn't have this issue. How does Mac port implement the desired behavior?

What I tested was opening <data:text/html,%3Ciframe%20src=%22http://www.adobe.com/swf/software/flash/about/flashAbout_info_small.swf%22%3E%3C/iframe%3E> without Flash installed.

[Joe Mason]

(In reply to comment #21)
> However I'm concerned that this adds a new way to do something that is already doable. If I understood the problem statement correctly, Safari doesn't have this issue. How does Mac port implement the desired behavior?
> 
> What I tested was opening <data:text/html,%3Ciframe%20src=%22http://www.adobe.com/swf/software/flash/about/flashAbout_info_small.swf%22%3E%3C/iframe%3E> without Flash installed.

I don't have a Mac, but both Chrome and Safari for Windows do NOT implement the desired behaviour. When I go to the above link without Flash installed, they download the .swf file. 

Our target behaviour for Blackberry is to show the "missing plugin" icon in the iframe. Can you confirm whether Safari for Mac does this?

[Max Feil]

(In reply to comment #21)
> I assume that the proposed name is for an additional client call, as existing usage of shouldUsePluginDocument() doesn't appear to match it.

I would prefer not to add an existing client call, since what we need appears to have exactly the same usage as shouldUsePluginDocument(). This call already partially implements what is needed. My patch simply completes the implementation. Also see my comment #5.

And as I mentioned in comment #17, one reason that I believe that the existing implementation is not complete is that the iframe case is often forgotten at first.

[Alexey Proskuryakov]

> Our target behaviour for Blackberry is to show the "missing plugin" icon in the iframe. Can you confirm whether Safari for Mac does this?

Safari shows a blank iframe. Depending on how it's implemented, it might be possible to show "missing plug-in" too.

[Max Feil]

Created attachment 192620 [details]
Patch

[Max Feil]

(In reply to comment #25)
> Created an attachment (id=192620) [details]
> Patch

Updated my patch to include the name change from comment #12. I updated the name everywhere except WebKit2.

[Build Bot]

Comment on attachment 192620 [details]
Patch

Attachment 192620 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17038160

[Build Bot]

Comment on attachment 192620 [details]
Patch

Attachment 192620 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17041202

[Max Feil]

Created attachment 192797 [details]
Patch

[Max Feil]

Ping. I would like to find out if the only thing preventing an r+ of this patch is lack of a layout test, i.e. all else is ok now. If so, I will go ahead and write a test.

[Alexey Proskuryakov]

I do not see a satisfactory resolution for the naming concern.

I'm also still unsure if any new client calls are needed - while Safari currently chooses to just show blank for missing plug-in, I think that it used to display an icon before.

[Max Feil]

(In reply to comment #31)
> I do not see a satisfactory resolution for the naming concern.

Hi Alexey. You yourself approved the new name. See your comment #12: "shouldAlwaysUsePluginDocument() appears correct and descriptive, but please don't add the "ForMIMEType" part."

> I'm also still unsure if any new client calls are needed - while Safari currently chooses to just show blank for missing plug-in, I think that it used to display an icon before.

No new client calls are needed. Any new client calls are not within the scope of my patch. That is the good thing about it. Any client can choose to implement shouldAlwaysUsePluginDocument() whenever they need this functionality.

My patch really just makes a correction to a mechanism that has been in place for quite a while, and already has been working for the case of navigating directly to a file associated with a plugin (for example a .swf file). My patch simply gets this working for the oft-forgotten iframe case as well.

[Alexey Proskuryakov]

> See your comment #12: "shouldAlwaysUsePluginDocument() appears correct and descriptive, but please don't add the "ForMIMEType" part."

Indeed. Sorry, this was some time ago.

I don't have any objections against this patch.

[Max Feil]

Created attachment 194968 [details]
Patch

[Max Feil]

(In reply to comment #34)
> Created an attachment (id=194968) [details]
> Patch

This patch adds a layout test. Otherwise it is unchanged from the previous patch. I'm hoping for a quick review process :)

[Max Feil]

Created attachment 200853 [details]
Patch

[Max Feil]

(In reply to comment #36)
> Created an attachment (id=200853) [details]
> Patch

Same as previous patch, but resolved conflicts from during the 6 weeks this patch has been up for review.

[Rob Buis]

Comment on attachment 200853 [details]
Patch

Looks good.

[WebKit Commit Bot]

Comment on attachment 200853 [details]
Patch

Clearing flags on attachment: 200853

Committed r149830: <http://trac.webkit.org/changeset/149830>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.