Bug 776208 implemented the base API, however: 1) Click-to-play logic can not be reused (e.g. sites whitelist); 2) Only single extension (e.g. shumway) can use this API. The click-to-play shall be taken in account: the extension will be used only as static (or dynamic) overlay. Also, there were requests from pdf.js team to use this API to fix bug 738967

https://tbpl.mozilla.org/?tree=Try&rev=1db23749ae68 Extends registerPlayPreviewMimeType methods signature to accept two optional parameters: ignoreCTP and redirectURL. The ignoreCTP forces the PlayPreview to follow click-to-play rules (such as whitelist). The redirectURL allows to substitute |"data:application/x-moz-playpreview;," + mimeType| iframe URL to custom one.

Differences between ignoreCTP modes: # PlayPreview in "ignore CTP" mode If loading plugin was not activated or play preview was canceled, replace the standard CTP interface with the IFRAME that will play Shumway. The Shumway may cancel the play preview and display native plugin, or fallback to standard CTP UI. # PlayPreview in a regular mode Same as above but additional rules are enforced to make PlayPreview work: - the native plugin must present and permitted; - the native plugin is not disabled; - the native plugin(s) is not whitelisted for the site.

Comment on attachment 712197 [details] [diff] [review] Extends "PlayPreview" API (+tests) (also looking for reviewer)

Comment on attachment 712197 [details] [diff] [review] Extends "PlayPreview" API (+tests) Review of attachment 712197 [details] [diff] [review]: ----------------------------------------------------------------- Jared can probably review the front-end changes, and Josh or John would be good for the back-end changes. As a general comment, however, I'm concerned about the interactions between play preview and click-to-play. I would like to see more tests involving instances of both on the same page. Also, it was my understanding that play preview would be used to give a preview of what each plugin instance would look like if a user decided to click-to-play it. If that's the case, then I don't really see why these are two different things. Shouldn't the click-to-play/whitelisting/showing the popup/etc. be handled by click-to-play code, while the play preview just grabs the first frame and shoves it in the overlay? ::: browser/base/content/test/Makefile.in @@ +188,5 @@ > browser_pluginnotification.js \ > browser_plugins_added_dynamically.js \ > browser_CTPScriptPlugin.js \ > browser_pluginplaypreview.js \ > + browser_pluginplaypreview2.js \ Is browser_pluginplaypreview.js particularly large? In other words, is there a compelling reason to have the plugin play preview tests in two separate files? ::: browser/base/content/test/browser_pluginplaypreview2.js @@ +16,5 @@ > +function registerPlayPreview(mimeType, targetUrl) { > + var ph = Cc["@mozilla.org/plugin/host;1"].getService(Ci.nsIPluginHost); > + ph.registerPlayPreviewMimeType(mimeType, false, targetUrl); > + > + return (gPlayPreviewRegistration = { This function returns a value, yet it seems that each caller ignores it. @@ +72,5 @@ > + // This just allows the events to fire before we then go on to test the states > + > + // iframe might triggers load event as well, making sure we skip some to let > + // all iframes on the page be loaded as well > + if (gNextTestSkip) { I imagine this will be fragile. Is there another event this test can listen for so it knows when to continue? ::: dom/plugins/base/nsIPluginHost.idl @@ +96,5 @@ > > void unregisterPlayPreviewMimeType(in AUTF8String mimeType); > > + nsIPluginPlayPreviewInfo getPlayPreviewInfo(in AUTF8String mimeType); > + Whitespace. ::: dom/plugins/base/nsPluginHost.cpp @@ +1670,5 @@ > nsPluginHost::UnregisterPlayPreviewMimeType(const nsACString& mimeType) > { > nsAutoCString mimeTypeToRemove(mimeType); > for (uint32_t i = mPlayPreviewMimeTypes.Length(); i > 0;) { > + nsRefPtr<nsPluginPlayPreviewInfo> pp = mPlayPreviewMimeTypes[--i]; The "--i" in this line should probably be in the for statement.

(In reply to David Keeler (:keeler) from comment #7) > Comment on attachment 712197 [details] [diff] [review] > Extends "PlayPreview" API (+tests) > > Review of attachment 712197 [details] [diff] [review]: > ----------------------------------------------------------------- > > Also, it was my understanding that play preview would be used to give a > preview of what each plugin instance would look like if a user decided to > click-to-play it. If that's the case, then I don't really see why these are > two different things. Shouldn't the click-to-play/whitelisting/showing the > popup/etc. be handled by click-to-play code, while the play preview just > grabs the first frame and shoves it in the overlay? This API is mostly designed to show how the flash movie will look like, instead of having static picture of the click-to-play. The first frame in some cases is generated using ActionScript code and depends on the external resources, so something more than one frame has to played to see the preview. Currently "PlayPreview" API is designed to show the movie preview regardless of click-to-play settings (e.g. sites whitelisted by user) and can be shown even if plugin is not installed. > ::: dom/plugins/base/nsPluginHost.cpp > @@ +1670,5 @@ > > nsPluginHost::UnregisterPlayPreviewMimeType(const nsACString& mimeType) > > { > > nsAutoCString mimeTypeToRemove(mimeType); > > for (uint32_t i = mPlayPreviewMimeTypes.Length(); i > 0;) { > > + nsRefPtr<nsPluginPlayPreviewInfo> pp = mPlayPreviewMimeTypes[--i]; > > The "--i" in this line should probably be in the for statement. In this case the i variable is uint32_t, so |for (uint32_t i = mPlayPreviewMimeTypes.Length() - 1; i >= 0; --i)| will not work

(In reply to David Keeler (:keeler) from comment #7) > Comment on attachment 712197 [details] [diff] [review] > Extends "PlayPreview" API (+tests) > > Review of attachment 712197 [details] [diff] [review]: > ----------------------------------------------------------------- > As a general comment, however, I'm concerned about the interactions between > play preview and click-to-play. I would like to see more tests involving > instances of both on the same page. browser_pluginplaypreview.js and browser_pluginplaypreview2.js test PlayPreview functionality, which includes falling back to click-to-play logic and if the click-to-play setting affects the PlayPreview. I can see if it's possible to test whitelisting of the plugins for particular web site. Anything else has to be tested? > > ::: browser/base/content/test/Makefile.in > @@ +188,5 @@ > > browser_pluginnotification.js \ > > browser_plugins_added_dynamically.js \ > > browser_CTPScriptPlugin.js \ > > browser_pluginplaypreview.js \ > > + browser_pluginplaypreview2.js \ > > Is browser_pluginplaypreview.js particularly large? In other words, is there > a compelling reason to have the plugin play preview tests in two separate > files? The browser_pluginplaypreview.js is not really that large, but following the logic in those two files is not a simple task, so I decided to separate into two files with ignoreCTP=true (browser_pluginplaypreview.js) and =false (browser_pluginplaypreview2.js). I can merge those two files if necessary.

(In reply to David Keeler (:keeler) from comment #7) > Comment on attachment 712197 [details] [diff] [review] > Extends "PlayPreview" API (+tests) > > Review of attachment 712197 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: dom/plugins/base/nsIPluginHost.idl > @@ +96,5 @@ > > > > void unregisterPlayPreviewMimeType(in AUTF8String mimeType); > > > > + nsIPluginPlayPreviewInfo getPlayPreviewInfo(in AUTF8String mimeType); > > + > > Whitespace. > > ::: dom/plugins/base/nsPluginHost.cpp > @@ +1670,5 @@ > > nsPluginHost::UnregisterPlayPreviewMimeType(const nsACString& mimeType) > > { > > nsAutoCString mimeTypeToRemove(mimeType); > > for (uint32_t i = mPlayPreviewMimeTypes.Length(); i > 0;) { > > + nsRefPtr<nsPluginPlayPreviewInfo> pp = mPlayPreviewMimeTypes[--i]; > > The "--i" in this line should probably be in the for statement. Replaced by "i - 1" and "--i" in for statement for better code reading

Comment on attachment 712706 [details] [diff] [review] Extends "PlayPreview" API v2 Delegating review to John Schoenick.

Comment on attachment 712706 [details] [diff] [review] Extends "PlayPreview" API v2 Review of attachment 712706 [details] [diff] [review]: ----------------------------------------------------------------- r- because I don't like the spaghetti ShouldPreview/ShouldPlay/ignoreCTP logic, otherwise this looks good ::: content/base/src/nsObjectLoadingContent.cpp @@ +2686,5 @@ > + // using play preview, if one is specified, instead of standard CTP UI > + aReason = eFallbackPlayPreview; > + return false; > + } > + The logic here is getting pretty convoluted -- We call ShouldPreview, then ShouldPlay, but ShouldPlay might return eFallbackPlayPreview. I think this would be simpler if we merged ShouldPreview into ShouldPlay, then have ShouldPlay use GetPluginPlayPreviewInfo to get rid of the weirdness here. ::: dom/plugins/base/nsIPluginHost.idl @@ +19,5 @@ > + readonly attribute boolean ignoreCTP; > + readonly attribute AUTF8String redirectURL; > +}; > + > +[scriptable, uuid(3ac8fe33-c38c-4623-b2f0-0e8a2824b9c5)] This needs to be rev'd ::: dom/plugins/base/nsPluginHost.h @@ +80,5 @@ > nsIURI *aURL, > nsPluginInstanceOwner *aOwner); > nsresult IsPluginEnabledForType(const char* aMimeType); > nsresult IsPluginEnabledForExtension(const char* aExtension, const char* &aMimeType); > + bool IsPluginPlayPreviewForType(const char *aMimeType, bool ignoreCTP); Isn't this redundant now that we have getPluginPlayPreviewInfo? ::: dom/plugins/base/nsPluginPlayPreviewInfo.cpp @@ +8,5 @@ > +using namespace mozilla; > + > +nsPluginPlayPreviewInfo::nsPluginPlayPreviewInfo(const char* aMimeType, > + bool aIgnoreCTP, > + const char* aRedirectURL) Indent should be aligned with other params

(In reply to John Schoenick [:johns] from comment #12) > Comment on attachment 712706 [details] [diff] [review] > Extends "PlayPreview" API v2 > > Review of attachment 712706 [details] [diff] [review]: > ----------------------------------------------------------------- > > The logic here is getting pretty convoluted -- We call ShouldPreview, then > ShouldPlay, but ShouldPlay might return eFallbackPlayPreview. > > I think this would be simpler if we merged ShouldPreview into ShouldPlay, > then have ShouldPlay use GetPluginPlayPreviewInfo to get rid of the > weirdness here. The play-preview logic is moved into ShouldPlay method. > ::: dom/plugins/base/nsIPluginHost.idl > @@ +19,5 @@ > > + readonly attribute boolean ignoreCTP; > > + readonly attribute AUTF8String redirectURL; > > +}; > > + > > +[scriptable, uuid(3ac8fe33-c38c-4623-b2f0-0e8a2824b9c5)] > > This needs to be rev'd Fixed > ::: dom/plugins/base/nsPluginHost.h > @@ +80,5 @@ > > nsIURI *aURL, > > nsPluginInstanceOwner *aOwner); > > nsresult IsPluginEnabledForType(const char* aMimeType); > > nsresult IsPluginEnabledForExtension(const char* aExtension, const char* &aMimeType); > > + bool IsPluginPlayPreviewForType(const char *aMimeType, bool ignoreCTP); > > Isn't this redundant now that we have getPluginPlayPreviewInfo? IsPluginPlayPreviewForType removed > ::: dom/plugins/base/nsPluginPlayPreviewInfo.cpp > @@ +8,5 @@ > > +using namespace mozilla; > > + > > +nsPluginPlayPreviewInfo::nsPluginPlayPreviewInfo(const char* aMimeType, > > + bool aIgnoreCTP, > > + const char* aRedirectURL) > > Indent should be aligned with other params Fixed