WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
110308
shouldUsePluginDocument() needs to be respected when a document is created
https://bugs.webkit.org/show_bug.cgi?id=110308
Summary
shouldUsePluginDocument() needs to be respected when a document is created
Max Feil
Reported
2013-02-19 22:39:05 PST
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.
Attachments
Patch
(6.72 KB, patch)
2013-02-19 22:56 PST
,
Max Feil
no flags
Details
Formatted Diff
Diff
Patch
(12.04 KB, patch)
2013-03-11 18:42 PDT
,
Max Feil
no flags
Details
Formatted Diff
Diff
Patch
(16.17 KB, patch)
2013-03-12 13:55 PDT
,
Max Feil
no flags
Details
Formatted Diff
Diff
Patch
(18.50 KB, patch)
2013-03-25 18:15 PDT
,
Max Feil
no flags
Details
Formatted Diff
Diff
Patch
(16.28 KB, patch)
2013-05-06 16:47 PDT
,
Max Feil
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Max Feil
Comment 1
2013-02-19 22:47:43 PST
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
Comment 2
2013-02-19 22:56:08 PST
Created
attachment 189252
[details]
Patch
Max Feil
Comment 3
2013-02-19 23:03:00 PST
I am proposing a cross-platform change. The only non-BlackBerry port that seems to be affected is Win.
Alexey Proskuryakov
Comment 4
2013-02-20 10:03:35 PST
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
Comment 5
2013-02-22 12:45:34 PST
(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
Comment 6
2013-02-22 13:13:58 PST
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
Comment 7
2013-02-22 13:36:40 PST
(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
Comment 8
2013-02-22 13:48:56 PST
Will returning true for this function make it so that a PluginDocument would be created for any MIME type, including text/html?
Max Feil
Comment 9
2013-02-22 14:06:09 PST
(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
Comment 10
2013-02-22 14:24:32 PST
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
Comment 11
2013-02-25 21:51:03 PST
(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 12
2013-02-25 22:54:29 PST
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 13
2013-02-26 01:39:50 PST
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
Comment 14
2013-02-26 02:02:50 PST
(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
Comment 15
2013-02-26 07:39:47 PST
(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
Comment 16
2013-02-26 08:28:14 PST
(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
Comment 17
2013-02-26 13:51:38 PST
(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
Comment 18
2013-02-26 14:04:28 PST
(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
Comment 19
2013-02-26 19:34:24 PST
> 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
Comment 20
2013-02-27 00:19:42 PST
(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
Comment 21
2013-02-27 00:49:22 PST
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
Comment 22
2013-02-27 07:20:15 PST
(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
Comment 23
2013-02-27 08:41:23 PST
(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
Comment 24
2013-02-27 11:10:33 PST
> 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
Comment 25
2013-03-11 18:42:34 PDT
Created
attachment 192620
[details]
Patch
Max Feil
Comment 26
2013-03-11 18:45:37 PDT
(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 27
2013-03-12 00:37:03 PDT
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 28
2013-03-12 02:53:39 PDT
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
Comment 29
2013-03-12 13:55:49 PDT
Created
attachment 192797
[details]
Patch
Max Feil
Comment 30
2013-03-18 16:20:12 PDT
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
Comment 31
2013-03-18 16:52:31 PDT
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
Comment 32
2013-03-18 17:03:01 PDT
(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
Comment 33
2013-03-18 17:15:08 PDT
> 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
Comment 34
2013-03-25 18:15:35 PDT
Created
attachment 194968
[details]
Patch
Max Feil
Comment 35
2013-03-25 18:18:59 PDT
(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
Comment 36
2013-05-06 16:47:33 PDT
Created
attachment 200853
[details]
Patch
Max Feil
Comment 37
2013-05-06 16:48:52 PDT
(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 38
2013-05-09 11:36:29 PDT
Comment on
attachment 200853
[details]
Patch Looks good.
WebKit Commit Bot
Comment 39
2013-05-09 11:48:19 PDT
Comment on
attachment 200853
[details]
Patch Clearing flags on attachment: 200853 Committed
r149830
: <
http://trac.webkit.org/changeset/149830
>
WebKit Commit Bot
Comment 40
2013-05-09 11:48:24 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug