839714 - Extend plugins "PlayPreview" API to take CTP logic into account and handle more than one mimeType

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Yury Delendik (:yury)]

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

Comment 1

[Yury Delendik (:yury)]

Attachment: WIP1

Comment 2

[Yury Delendik (:yury)]

Attachment: WIP1a

Comment 3

[Yury Delendik (:yury)]

Attachment: Extends "PlayPreview" API

Comment 4

[Yury Delendik (:yury)]

Attachment: Extends "PlayPreview" API (+tests)

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.

Comment 5

[Yury Delendik (:yury)]

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 6

[Yury Delendik (:yury)]

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

(also looking for reviewer)

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 8

[Yury Delendik (:yury)]

(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

Comment 9

[Yury Delendik (:yury)]

(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.

Comment 10

[Yury Delendik (:yury)]

Attachment: Extends "PlayPreview" API v2

(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 11

[Josh Aas]

Comment on attachment 712706 [details] [diff] [review]
Extends "PlayPreview" API v2

Delegating review to John Schoenick.

Comment 12

[John Schoenick [:johns]]

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

Comment 13

[Yury Delendik (:yury)]

Attachment: Extends "PlayPreview" API v3

(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

Comment 14

[Yury Delendik (:yury)]

Attachment: patch for checkin

https://tbpl.mozilla.org/?tree=Try&rev=129439b76d60

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/45b37a31ac9c

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/45b37a31ac9c