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

RESOLVED FIXED in mozilla21

Status

()

Core
Plug-ins
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: yury, Assigned: yury)

Tracking

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

5 years ago
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
(Assignee)

Updated

5 years ago
Depends on: 776208
(Assignee)

Updated

5 years ago
Assignee: nobody → ydelendik
Assignee: ydelendik → nobody
Assignee: nobody → ydelendik
(Assignee)

Comment 2

5 years ago
Created attachment 712085 [details] [diff] [review]
WIP1a
Attachment #712046 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
Created attachment 712179 [details] [diff] [review]
Extends "PlayPreview" API
Attachment #712085 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
Created attachment 712197 [details] [diff] [review]
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.
Attachment #712179 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

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

(also looking for reviewer)
Attachment #712197 - Flags: feedback?(dkeeler)
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.
Attachment #712197 - Flags: feedback?(dkeeler) → feedback+
(Assignee)

Comment 8

5 years ago
(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
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
Created attachment 712706 [details] [diff] [review]
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
(Assignee)

Updated

5 years ago
Attachment #712197 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #712706 - Flags: review?(jwein)
Attachment #712706 - Flags: review?(joshmoz)

Comment 11

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

Delegating review to John Schoenick.
Attachment #712706 - Flags: review?(joshmoz) → review?(jschoenick)
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
Attachment #712706 - Flags: review?(jschoenick) → review-
(Assignee)

Comment 13

5 years ago
Created attachment 713172 [details] [diff] [review]
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
Attachment #712706 - Attachment is obsolete: true
Attachment #712706 - Flags: review?(jwein)
Attachment #713172 - Flags: review?(jwein)
Attachment #713172 - Flags: review?(jschoenick)
Attachment #713172 - Flags: review?(jschoenick) → review+
Attachment #713172 - Flags: review?(jAwS) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/45b37a31ac9c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(Assignee)

Updated

5 years ago
Blocks: 738967
You need to log in before you can comment on or make changes to this bug.