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.
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.
Created attachment 189252 [details] Patch
I am proposing a cross-platform change. The only non-BlackBerry port that seems to be affected is Win.
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.
(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.
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.
(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()?
Will returning true for this function make it so that a PluginDocument would be created for any MIME type, including text/html?
(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 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.
(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
Comment on attachment 189252 [details] Patch I don't think that continued arguing would be productive. This patch is harmful, and cannot be landed.
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
(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.
(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?
(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.
(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.
(In reply to comment #16) > I think "shouldUsePluginDocumentForMIMEType" or simply "shouldUsePluginForMIMEType" would be a good name for this function. I like both of those!
> 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.
(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.
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.
(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?
(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.
> 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.
Created attachment 192620 [details] Patch
(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.
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
Comment on attachment 192620 [details] Patch Attachment 192620 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17041202
Created attachment 192797 [details] Patch
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.
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.
(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.
> 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.
Created attachment 194968 [details] Patch
(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 :)
Created attachment 200853 [details] Patch
(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.
Comment on attachment 200853 [details] Patch Looks good.
Comment on attachment 200853 [details] Patch Clearing flags on attachment: 200853 Committed r149830: <http://trac.webkit.org/changeset/149830>
All reviewed patches have been landed. Closing bug.