Closed Bug 776208 Opened 12 years ago Closed 12 years ago

Provide API for JavaScript extensions to create native plugins previews for specific mime type

Categories

(Core Graveyard :: Plug-ins, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla17

People

(Reporter: yury, Assigned: yury)

References

Details

Attachments

(3 files, 25 obsolete files)

89.67 KB, application/pdf
Details
37.08 KB, patch
jaws
: review+
Details | Diff | Splinter Review
20.71 KB, patch
jaws
: review+
Details | Diff | Splinter Review
Attached patch Pilot solution (obsolete) — Splinter Review
The proposed solution is to add the additional NS_EVENT_STATE_TYPE_PLAY_PREVIEW state to the nsIObjectLoadingContent, this state CSS styles, and the iframe-element to the pluginProblem.xml. Also, it's necessary to modify the browser.js files for the android or desktop platform.

The JavaScript extension must register the mime type with the nsIPluginHost it wishes to provide the preview functionality for:

   let Ph = Cc["@mozilla.org/plugin/host;1"].getService(Ci.nsIPluginHost);
   Ph.registerPlayPreviewMimeType(TARGET_MIME_TYPE);

and implement the stream converter for the 'application/x-moz-playpreview' type. The overlay's iframe-element will be initialized with the URL in the following format:

data:application/x-moz-playpreview;,<target-mime-type>,<base-URI>,<HTML-markup-for-OBJECT-element>

The converter shall "intercept" the content with the 'application/m-moz-playpreview' mime type and url with registered/target mime type, and provide the alternative HTML/JS content to provide the preview experience that corresponds to the target plugin.
TODO:
  - overlays don't get bound to invisible plugins (ignore those plugins?)
  - add mechanism to invoke the native plug-in (message from the extension or overlay UI?)
Attached patch Pilot solution (v2) (obsolete) — Splinter Review
Changes:
  - simplifies play preview URL to: data:application/x-moz-playpreview;,<target-mime-type>
  - skips invisible plugins
  - adds mechanism to invoke the native plug-ins: the overlay waits for MozPlayPlugin event
Attachment #644585 - Attachment is obsolete: true
Attachment #646153 - Flags: feedback?(joshmoz)
Attachment #646153 - Flags: feedback?(benjamin)
Comment on attachment 646153 [details] [diff] [review]
Pilot solution (v2)

I'm going on vacation, and I think jaws can give better feedback on this than I can.
Attachment #646153 - Flags: feedback?(benjamin) → feedback?(jaws)
Comment on attachment 646153 [details] [diff] [review]
Pilot solution (v2)

Review of attachment 646153 [details] [diff] [review]:
-----------------------------------------------------------------

Margaret, can you take a look at the mobile changes here?

::: browser/base/content/browser-plugins.js
@@ +303,5 @@
> +    let pluginInfo = getPluginInfo(aPlugin);
> +    let playPreviewUri = "data:application/x-moz-playpreview;," + pluginInfo.mimetype;
> +    previewContent.src = playPreviewUri;
> +
> +    previewContent.addEventListener("MozPlayPlugin", function(aEvent) {

Where does the MozPlayPlugin event get dispatched?

@@ +306,5 @@
> +
> +    previewContent.addEventListener("MozPlayPlugin", function(aEvent) {
> +      if (aEvent.isTrusted)
> +        gPluginHandler.activateSinglePlugin(aEvent.target.ownerDocument.defaultView.top, aPlugin);
> +    }, true, true);

This last argument should probably be removed. aWantsUntrusted=true will allow the event to be triggered by untrusted content, but the listener here checks to make sure that the event is trusted. Is this intended?

::: content/base/src/nsObjectLoadingContent.h
@@ +40,5 @@
>    ePluginCrashed,
>    ePluginClickToPlay,  // The plugin is disabled until the user clicks on it
>    ePluginVulnerableUpdatable, // The plugin is vulnerable (update available)
> +  ePluginVulnerableNoUpdate,  // The plugin is vulnerable (no update available)
> +  ePluginPlayPreview,  // The plugin is disabled until the user clicks on it

Can you add something to this comment to explain how ePluginPlayPreview is different from ePluginClickToPlay?

::: toolkit/mozapps/plugins/content/pluginProblemContent.css
@@ +56,5 @@
> +:-moz-handler-playpreview .previewPluginContent {
> +  display: block;
> +  width: inherit;
> +  height: inherit;
> +  border: none 0px;

border: none;
Attachment #646153 - Flags: feedback?(margaret.leibovic)
Attachment #646153 - Flags: feedback?(jaws)
Attachment #646153 - Flags: feedback+
Attached patch Pilot solution (v3) (obsolete) — Splinter Review
(In reply to Jared Wein [:jaws] from comment #4)
> Comment on attachment 646153 [details] [diff] [review]
> Pilot solution (v2)

> Where does the MozPlayPlugin event get dispatched?

The comment was added to the browser.js files. The extension chrome code might do the following to fallback to the native plugin:

    var obj = htmlOverlayWindow.frameElement;
    var doc = obj.ownerDocument;
    var e = doc.createEvent("CustomEvent");
    e.initCustomEvent("MozPlayPlugin", true, true, null);
    obj.dispatchEvent(e);


> > +    previewContent.addEventListener("MozPlayPlugin", function(aEvent) {
> > +      if (aEvent.isTrusted)
> > +        gPluginHandler.activateSinglePlugin(aEvent.target.ownerDocument.defaultView.top, aPlugin);
> > +    }, true, true);
> 
> This last argument should probably be removed. aWantsUntrusted=true will
> allow the event to be triggered by untrusted content, but the listener here
> checks to make sure that the event is trusted. Is this intended?

Thanks. The fourth argument was removed.

> > +  ePluginPlayPreview,  // The plugin is disabled until the user clicks on it
> 
> Can you add something to this comment to explain how ePluginPlayPreview is
> different from ePluginClickToPlay?

The comment was changed.

> border: none;

Fixed.
Attachment #646153 - Attachment is obsolete: true
Attachment #646153 - Flags: feedback?(margaret.leibovic)
Attachment #646153 - Flags: feedback?(joshmoz)
Attachment #646578 - Flags: feedback?(margaret.leibovic)
Attachment #646578 - Flags: feedback?(joshmoz)
Comment on attachment 646578 [details] [diff] [review]
Pilot solution (v3)

Review of attachment 646578 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good overall! I just have some minor comments:

::: mobile/android/chrome/content/browser.js
@@ +112,5 @@
>    return aURI;
>  }
>  
> +function getPluginMimeType(pluginElement)
> +{

Nit: We usually put the opening brace on the same line as the function declaration.

@@ +127,5 @@
> +  }
> +
> +  return tagMimetype;
> +}
> +

This function should be moved into the PluginHelper object, lower down in this file.

@@ +2822,5 @@
>          break;
>        }
>  
> +      case "PluginPlayPreview": {
> +        let plugin = aEvent.target;

We found we need to call plugin.clientTop as a hack to force a style flush, which makes sure the overlay is attached. See: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2765

@@ +2827,5 @@
> +        let doc = plugin.ownerDocument;
> +        let previewContent = doc.getAnonymousElementByAttribute(plugin, "class", "previewPluginContent");
> +        if (!previewContent)
> +          break;
> +        let mimeType = getPluginMimeType(plugin);

If you move getPluginMimeType into PluginHelper, this will need to be updated.
Attachment #646578 - Flags: feedback?(margaret.leibovic) → feedback+
David, you might be interested in this too wrt click to play ?
Attached patch Plugin overlay API (obsolete) — Splinter Review
Performs lazy initialization of the iframe -- the HTML frame is not created and src attribute is set until play preview functionality is requested.

(In reply to Margaret Leibovic [:margaret] from comment #6)
> Comment on attachment 646578 [details] [diff] [review]
> Pilot solution (v3)

> > +function getPluginMimeType(pluginElement)
> > +{
> 
> Nit: We usually put the opening brace on the same line as the function
> declaration.

> This function should be moved into the PluginHelper object, lower down in
> this file.

Moved to the PluginHelper.

> We found we need to call plugin.clientTop as a hack to force a style flush,

Fixed.
Attachment #646578 - Attachment is obsolete: true
Attachment #646578 - Flags: feedback?(joshmoz)
Attached file Plugin overlay interaction (obsolete) —
Security review Q&As:

> 1. Who is/are the point of contact(s) for this review? 

  Yury Delendik
  Jet Villegas
  Ian Melven

> 2. Please provide a short description of the feature / application (e.g. problem solved, use cases, etc.): 

  To provide the API for the JavaScript extension to create the preview of a plug-in for specific mime type. Typical use case is to allow creation of the firefox extensions to provide the secure and interactive preview for or fully replace the native plugins.

> 3. Please provide links to additional information (e.g. feature  page, wiki) if available and not yet included in feature description: 

  A play preview extension and Firefox plugin core interaction https://bugzilla.mozilla.org/attachment.cgi?id=647141

> 4. Does this request block another bug? If so, please indicate the bug number 

No bug filed, but this is a key component of the Shumway implementation of a web-native SWF runtime, specifically the browser integration with Firefox.

> 5. This review will be scheduled amongst other requested reviews. What is the urgency or needed completion date of this review? 

  Mid August 2012

> 6. To help prioritize this work request, does this project support  a goal specifically listed on this quarter's goal list?  If so, which  goal? 

The goal is to respond to several Mobile and Desktop requirements regarding Firefox dependence on plugins, including the Flash Player. See this page for more info:

http://mozilla.github.com/shumway/

> 7. Please answer the following few questions: (Note: If you are asked to describe anything, 1-2 sentences shall suffice.) 
>    * Does this feature or code change affect Firefox, Thunderbird or any product or service the Mozilla ships to end users? 

      Adds this feature to the Firefox for Android and Desktop: extensions might (temporary) replace the native plugin(s) such as Flash,  Adobe Reader, etc.

>    * Are there any portions of the project that interact with 3rd party services? 

      No

>    * Will your application/service collect user data? If so, please describe 

      No
> 8. If you feel something is missing here or you would like to  provide other kind of feedback, feel free to do so here (no limits on  size): 

  N/A

> 9. Desired Date of review (if known from https://mail.mozilla.com/home/ckoenig@mozilla.com/Security%20Review.html) and whom to invite. 

  Mid August 2012
Attachment #646997 - Flags: review?(joshmoz)
Attachment #646998 - Flags: review?(jaws)
Attachment #646998 - Flags: review?(dkeeler)
Comment on attachment 646997 [details] [diff] [review]
Plugin overlay API

Review of attachment 646997 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/browser.js
@@ +2829,5 @@
> +
> +          PluginHelper.playPlugin(plugin);
> +
> +          // Force a style flush, so that we ensure our binding is attached.
> +          plugin.clientTop;

These two lines should be moved to the top of the PluginPlayPreview function, before doc.getAnonymousElementByAttribute is called. See http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2759 for an example.
Attachment #646997 - Flags: review?
Attached patch Plugin overlay API (obsolete) — Splinter Review
(In reply to Jared Wein [:jaws] from comment #12)
> Comment on attachment 646997 [details] [diff] [review]
> Plugin overlay API
> 
> Review of attachment 646997 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +2829,5 @@
> > +
> > +          PluginHelper.playPlugin(plugin);
> > +
> > +          // Force a style flush, so that we ensure our binding is attached.
> > +          plugin.clientTop;
> 
> These two lines should be moved to the top of the PluginPlayPreview
> function, before doc.getAnonymousElementByAttribute is called. See
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#2759 for an example.

The lines moved
Attachment #646997 - Attachment is obsolete: true
Attachment #646997 - Flags: review?(joshmoz)
Attachment #646997 - Flags: review?
Attachment #647689 - Flags: review?(joshmoz)
Attachment #647689 - Flags: review?(jaws)
Attachment #647689 - Flags: review?(margaret.leibovic)
Comment on attachment 646998 [details] [diff] [review]
Plugin overlay testing

Review of attachment 646998 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good overall - just a few things it would be nice to change.

::: browser/base/content/test/browser_pluginplaypreview.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var rootDir = getRootDirectory(gTestPath);
> +const gTestRoot = rootDir;
> +const gHttpTestRoot = rootDir.replace("chrome://mochitests/content/", "http://127.0.0.1:8888/");

We shouldn't be navigating to chrome:// pages anymore when we're dealing with plugins unless we have to (which I think is only when we call script in those pages, which we can probably re-work so we're not doing anymore). So, go ahead and use gHttpTestRoot instead of gTestRoot everywhere. Ideally, I think determining gHttpTestRoot would be factored out into head.js.

@@ +10,5 @@
> +var gNextTest = null;
> +var gPlayPreviewPluginActualEvents = 0;
> +var gPlayPreviewPluginExpectedEvents = 1;
> +
> +function get_test_plugin() {

I think get_test_plugin() should be factored into head.js.

@@ +98,5 @@
> +
> +    // nsIStreamListener::onDataAvailable
> +    onDataAvailable: function(aRequest, aContext, aInputStream, aOffset, aCount) {
> +      // Do nothing since all the data loading is handled by the viewer.
> +      log('SANITY CHECK: onDataAvailable SHOULD NOT BE CALLED!');

Maybe do ok(false, "onDataAvailable should not be called"); here (so that it actually fails if it gets called).
Attachment #646998 - Flags: review?(dkeeler)
Attached patch Plugin overlay API (obsolete) — Splinter Review
Per security review: IFRAME cleanup/removal when playPlugin() is invoked.
Attachment #647689 - Attachment is obsolete: true
Attachment #647689 - Flags: review?(margaret.leibovic)
Attachment #647689 - Flags: review?(joshmoz)
Attachment #647689 - Flags: review?(jaws)
Attachment #648831 - Flags: review?(margaret.leibovic)
Attachment #648831 - Flags: review?(joshmoz)
Attachment #648831 - Flags: review?(jaws)
Depends on: 780311
No longer depends on: 780311
Comment on attachment 646998 [details] [diff] [review]
Plugin overlay testing

Review of attachment 646998 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Please address David's feedback and request review again :)

::: browser/base/content/test/browser_pluginplaypreview.js
@@ +154,5 @@
> +  gTestBrowser = gBrowser.selectedBrowser;
> +  gTestBrowser.addEventListener("load", pageLoad, true);
> +  gTestBrowser.addEventListener("PluginPlayPreview", handlePluginPlayPreview, true);
> +
> +  registerPlayPreview('application/x-test', 'about:robots');

registerPlayPreview('application/x-test', 'about:');
Attachment #646998 - Flags: review?(jaws) → feedback+
Comment on attachment 648831 [details] [diff] [review]
Plugin overlay API

Review of attachment 648831 [details] [diff] [review]:
-----------------------------------------------------------------

I just looked at the mobile changes, and the things you added look good, but I have a few concerns about things we may be missing. Will this completely replace "PluginClickToPlay" events with "PluginPlayPreview" events? If so, we'll also need logic in here to handle showing a doorhanger for plugins that don't have visible overlays. We also have logic in our "PluginClickToPlay" handler that checks to see if the user has already chosen to activate plugins for a page, and if so, we automatically activate any new plugin objects that get added to the page (see tab.clickToPlayPluginsActivated).

::: mobile/android/chrome/content/browser.js
@@ +2823,5 @@
> +        previewContent.addEventListener("MozPlayPlugin", function(e) {
> +          if (!e.isTrusted)
> +            return;
> +
> +          PluginHelper.playPlugin(plugin);

For mobile, we decided to activate all plugins on the page when the user clicks on one overlay, mainly because we don't have a way to put an indicator in the URL bar to let users choose to activate invisible plugins (like that little blue lego icon on desktop). I think that until we have a solution to that problem, we should keep the behavior in here consistent with that in the "PluginClickToPlay" handler:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2822

@@ +2826,5 @@
> +
> +          PluginHelper.playPlugin(plugin);
> +
> +          // cleaning up: removes overlay iframe from the DOM 
> +          previewContent.removeChild(iframe);

If we replace the playPlugin() call above with playAllPlugins() we'd have to do something different to make sure we're cleaning up the iframes for all the overlays.
Attachment #648831 - Flags: review?(margaret.leibovic) → feedback+
(In reply to Margaret Leibovic [:margaret] from comment #17)
> Comment on attachment 648831 [details] [diff] [review]
> Plugin overlay API
> 
> Review of attachment 648831 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I just looked at the mobile changes, and the things you added look good, but
> I have a few concerns about things we may be missing. Will this completely
> replace "PluginClickToPlay" events with "PluginPlayPreview" events?

"PluginPlayPreview" will replace "PluginClickToPlay" behavior for certain plugin types.

> For mobile, we decided to activate all plugins on the page when the user
> clicks on one overlay, mainly because we don't have a way to put an
> indicator in the URL bar to let users choose to activate invisible plugins
> (like that little blue lego icon on desktop).

If the page contains only invisible plugins, there is no way to activate the plugins, right?
Attached patch Plugin overlay testing (obsolete) — Splinter Review
(In reply to David Keeler from comment #14)
> Comment on attachment 646998 [details] [diff] [review]
> Plugin overlay testing
> 
> Review of attachment 646998 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > +const gHttpTestRoot = rootDir.replace("chrome://mochitests/content/", "http://127.0.0.1:8888/");
> 
> We shouldn't be navigating to chrome:// pages anymore when we're dealing
> with plugins unless we have to (which I think is only when we call script in
> those pages, which we can probably re-work so we're not doing anymore). So,
> go ahead and use gHttpTestRoot instead of gTestRoot everywhere. Ideally, I
> think determining gHttpTestRoot would be factored out into head.js.

A little bit confused here, but I removed the gHttpTestRoot line from the file 

> > +function get_test_plugin() {
> 
> I think get_test_plugin() should be factored into head.js.

Moved to head.js

> > +      // Do nothing since all the data loading is handled by the viewer.
> > +      log('SANITY CHECK: onDataAvailable SHOULD NOT BE CALLED!');
> 
> Maybe do ok(false, "onDataAvailable should not be called"); here (so that it
> actually fails if it gets called).

Changed

(In reply to Jared Wein [:jaws] from comment #16)
> Comment on attachment 646998 [details] [diff] [review]
> Plugin overlay testing
> 
> Review of attachment 646998 [details] [diff] [review]:
> -----------------------------------------------------------------
> > +  registerPlayPreview('application/x-test', 'about:robots');
> 
> registerPlayPreview('application/x-test', 'about:');

Changed
Attachment #646998 - Attachment is obsolete: true
Attachment #649475 - Flags: review?(jaws)
Comment on attachment 649475 [details] [diff] [review]
Plugin overlay testing

Review of attachment 649475 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/browser_pluginplaypreview.js
@@ +215,5 @@
> +  var objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
> +  ok(objLoadingContent.activated, "Test 1b, Plugin should be activated");
> +
> +  is(gPlayPreviewPluginActualEvents, gPlayPreviewPluginExpectedEvents,
> +     "There should be a exactly one PluginPlayPreview event");

"There should be exactly one PluginPlayPreview event"
Attachment #649475 - Flags: review?(jaws) → review+
(In reply to Yury (:yury) from comment #18)
> (In reply to Margaret Leibovic [:margaret] from comment #17)
> > Comment on attachment 648831 [details] [diff] [review]
> > Plugin overlay API
> > 
> > Review of attachment 648831 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I just looked at the mobile changes, and the things you added look good, but
> > I have a few concerns about things we may be missing. Will this completely
> > replace "PluginClickToPlay" events with "PluginPlayPreview" events?
> 
> "PluginPlayPreview" will replace "PluginClickToPlay" behavior for certain
> plugin types.

Okay, so if it replaces "PluginClickToPlay" for flash, that would effectively replace it entirely on mobile, so we'd want to port over the same behavior. I wonder if we can even just use the same code to handle both events, but then add the extra iframe functionality you need for the "PluginPlayPreview" case.

> > For mobile, we decided to activate all plugins on the page when the user
> > clicks on one overlay, mainly because we don't have a way to put an
> > indicator in the URL bar to let users choose to activate invisible plugins
> > (like that little blue lego icon on desktop).
> 
> If the page contains only invisible plugins, there is no way to activate the
> plugins, right?

If the page contains only invisible plugins, we show a doorhanger notification asking the user if we should activate them. The logic around this is a little messy though because we need to keep track of whether or not a visible plugin has been shown:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2791
Comment on attachment 648831 [details] [diff] [review]
Plugin overlay API

Review of attachment 648831 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-plugins.js
@@ +302,5 @@
> +      return; // the XBL binding is not attached (element is display:none)
> +    let iframe = previewContent.lastChild;
> +    if (!iframe || iframe.localName != "iframe") {
> +      // lazy initialization of the iframe
> +      iframe = doc.createElementNS("http://www.w3.org/1999/xhtml", "html:iframe");

iframe = doc.createElementNS("http://www.w3.org/1999/xhtml", "iframe");

@@ +318,5 @@
> +
> +      gPluginHandler.activateSinglePlugin(aEvent.target.ownerDocument.defaultView.top, aPlugin);
> +
> +      // cleaning up: removes overlay iframe from the DOM 
> +      previewContent.removeChild(iframe);

Can you get to the 'iframe' element without pulling the iframe variable in through the closure? Is event.target == iframe?

::: toolkit/mozapps/plugins/content/pluginProblemContent.css
@@ +54,5 @@
> +.previewPluginContent {
> +  display: none;
> +}
> +
> +.previewPluginContent iframe {

Please use child selectors (>) here.
Attachment #648831 - Flags: review?(jaws) → review-
Attached patch Plugin overlay API (obsolete) — Splinter Review
(In reply to Margaret Leibovic [:margaret] from comment #17)
> Comment on attachment 648831 [details] [diff] [review]
> Plugin overlay API
> 
> Review of attachment 648831 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I just looked at the mobile changes, and the things you added look good, but
> I have a few concerns about things we may be missing. Will this completely
> replace "PluginClickToPlay" events with "PluginPlayPreview" events? If so,
> we'll also need logic in here to handle showing a doorhanger for plugins
> that don't have visible overlays. We also have logic in our
> "PluginClickToPlay" handler that checks to see if the user has already
> chosen to activate plugins for a page, and if so, we automatically activate
> any new plugin objects that get added to the page (see
> tab.clickToPlayPluginsActivated).

I would like to keep preview mode for the plugins of the target mime time. The invisible plugins will be subject to the click-to-play logic. Also, the extension may choose the fallback to the default plugin behavior (e.g. for really small plugin displays) or bypass the click-to-play stage. If the playAllPlugins() is called, the preview mode will be preserved.

New function for the nsIObjectLoadingContent is introduced: stopPlayPreview(playPlugin). Calling it the playPlugin==true is equal to the call of the playPlugin() method -- bypasses the click-to-play.

(In reply to Jared Wein [:jaws] from comment #22)
> Comment on attachment 648831 [details] [diff] [review]
> Plugin overlay API
> 
> Review of attachment 648831 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > +      iframe = doc.createElementNS("http://www.w3.org/1999/xhtml", "html:iframe");
> 
> iframe = doc.createElementNS("http://www.w3.org/1999/xhtml", "iframe");

Changed

> Can you get to the 'iframe' element without pulling the iframe variable in
> through the closure? Is event.target == iframe?

The iframe is removed from the closure.

> > +.previewPluginContent iframe {
> 
> Please use child selectors (>) here.

Changed
Attachment #648831 - Attachment is obsolete: true
Attachment #648831 - Flags: review?(joshmoz)
Attachment #649818 - Flags: review?(margaret.leibovic)
Attachment #649818 - Flags: review?(joshmoz)
Attachment #649818 - Flags: review?(jaws)
Attachment #649818 - Flags: feedback?(imelven)
Attached file Plugin overlay interaction (obsolete) —
Steps 7 and 8 were changed to reflect the stopPlayPreview() use
Attachment #649819 - Flags: feedback?(imelven)
Attachment #647141 - Attachment is obsolete: true
Comment on attachment 649819 [details]
Plugin overlay interaction

Sounds good to me, the interaction with click to play is probably the most interesting bit at this point.
Attachment #649819 - Flags: feedback?(imelven) → feedback+
Comment on attachment 649818 [details] [diff] [review]
Plugin overlay API

I like that there are checks that the events are trusted and that StopPlayPreview checks the caller is chrome :) 

It seems like, if anything fails, we fall back to whatever the current click to play behavior is - if the user chooses to active the preview and load the plugin we skip any click to play that we might have done. That makes sense to me.
Attachment #649818 - Flags: feedback?(imelven) → feedback+
Attached patch Plugin overlay testing (obsolete) — Splinter Review
Adds two new tests for testing bypass and fallback to click-to-play mode.
Attachment #649475 - Attachment is obsolete: true
Attachment #650394 - Flags: review?(jaws)
Attached patch Plugin overlay API (obsolete) — Splinter Review
Merges the patch with current mozilla-central (mostly due to bug 745030 - Refactor nsObjectLoadingContent). Also, adds the playPreview read-only property to the nsIObjectLoadingContent to indicate play preview mode of the plugin.
Attachment #649818 - Attachment is obsolete: true
Attachment #649818 - Flags: review?(margaret.leibovic)
Attachment #649818 - Flags: review?(joshmoz)
Attachment #649818 - Flags: review?(jaws)
Attachment #650395 - Flags: review?(margaret.leibovic)
Attachment #650395 - Flags: review?(joshmoz)
Attachment #650395 - Flags: review?(jaws)
Depends on: 745030
I wonder if, rather than adding a property specific to playPreview, something like the patch I put in bug 767636 would serve an equivalent purpose and yet be applicable in more cases?
Flags: sec-review? → sec-review?(mgoodwin)
Flags: sec-review?(mgoodwin) → sec-review+
Comment on attachment 650394 [details] [diff] [review]
Plugin overlay testing

Review of attachment 650394 [details] [diff] [review]:
-----------------------------------------------------------------

I don't like the name "playPreview" but the rest of this looks fine.
Attachment #650394 - Flags: review?(jaws) → review+
Attachment #650395 - Flags: review?(jschoenick)
Comment on attachment 650395 [details] [diff] [review]
Plugin overlay API

Review of attachment 650395 [details] [diff] [review]:
-----------------------------------------------------------------

Giving this r- largely for the fact that I think some of the new variables on nsObjectLoadingContent aren't necessary. John will explain more. I'd like to review a final patch.

The approach generally looks good though, thanks for doing this!

::: browser/base/content/browser-plugins.js
@@ +318,5 @@
> +    let pluginInfo = getPluginInfo(aPlugin);
> +    let playPreviewUri = "data:application/x-moz-playpreview;," + pluginInfo.mimetype;
> +    iframe.src = playPreviewUri;
> +
> +    // MozPlayPlugin event can be dispatch from the extension chrome code

"dispatched"

::: content/base/public/nsIObjectLoadingContent.idl
@@ +104,5 @@
> +  /**
> +   * This attribute will return true if the plugin has been opened in play
> +   * preview fallback mode.
> +   */
> +  readonly attribute boolean playPreview;

Add these to the end of the idl method list.

::: content/base/src/nsObjectLoadingContent.cpp
@@ +2458,5 @@
> +NS_IMETHODIMP
> +nsObjectLoadingContent::StopPlayPreview(bool playPlugin)
> +{
> +  if (!nsContentUtils::IsCallerChrome())
> +    return NS_OK;

NS_ERROR_NOT_AVAILABLE?

::: content/base/src/nsObjectLoadingContent.h
@@ +447,5 @@
>      // activated by PlayPlugin(). (see ShouldPlay())
>      bool                        mActivated : 1;
>  
> +    // Used to keep track of whether or not a plugin is blocked by play-preview.
> +    bool                        mPlayPreviewDisabled : 1;

This makes me think that play preview won't/shouldn't be done when it is true. It's really being used to say that play preview has happened and is done now. This is confusing, particularly since preview isn't something that will always happen. Maybe rename this to something like "mPlayPreviewCompleted"?

@@ +451,5 @@
> +    bool                        mPlayPreviewDisabled : 1;
> +
> +    // Used to keep track of whether or not a plugin need a click-to-play check.
> +    // This flags is used by play-preview.
> +    bool                        mClickToPlayDisabled : 1;

Same issue with this variable name.

I also think one or both of these variables might not be necessary. I'll let John Schoenick give his analysis on that.

::: dom/plugins/base/nsIPluginHost.idl
@@ +33,5 @@
> +   * @param mimeType - specified mime type
> +   */
> +  void registerPlayPreviewMimeType(in AUTF8String mimeType);
> +
> +  void unregisterPlayPreviewMimeType(in AUTF8String mimeType);

Add these new members at the end.
Attachment #650395 - Flags: review?(joshmoz) → review-
Comment on attachment 650395 [details] [diff] [review]
Plugin overlay API

Review of attachment 650395 [details] [diff] [review]:
-----------------------------------------------------------------

Looking at just the ObjectLoadingContent stuff here -- r- for the same reasons josh mentioned, mainly the questionably-necessary member variables. If don't need to support stopping the preview but then going to the click-to-play dialog, then we can probably away with no new variables and just using playPlugin() for both purposes. Other than that this approach looks good.

::: content/base/public/nsIObjectLoadingContent.idl
@@ +98,5 @@
> +  /**
> +   * This method will disable the play-preview plugin state. If the playPlugin
> +   * parameter is set to true, the click-to-play feature will be ignored.
> +   */
> +  void stopPlayPreview(in boolean playPlugin);

Do we ever want to stop the preview, but then display click-to-play? It seems like this could be much simpler if mActivated was used to bypass both CTP and play preview, but I don't know if we have a specific use case in mind.

@@ +104,5 @@
> +  /**
> +   * This attribute will return true if the plugin has been opened in play
> +   * preview fallback mode.
> +   */
> +  readonly attribute boolean playPreview;

This should probably check the fallback type, rather than ShouldPreview -- also, dkeeler's patch in bug 767636 will make this unnecessary

::: content/base/src/nsObjectLoadingContent.cpp
@@ +1613,5 @@
>        }
>      }
>    }
>  
> +  if (ShouldPreview()) {

This should be |mType == eType_Plugin && ShouldPreview()|, unless we want to display previews even when a real plugin wasn't found (which would require some other checks)

@@ +2464,5 @@
> +  if (mPlayPreviewDisabled || mActivated)
> +    return NS_OK;
> +
> +  mPlayPreviewDisabled = true;
> +  mClickToPlayDisabled = playPlugin;

mClickToPlayDisabled seems superfluous here, we should just set mActivated in the manner of PlayPlugin()

Also, mPlayPreviewDisabled may be superfluous - unless we specifically want to be able to stop the preview but then sit on the click-to-play screen, mActivated could be used for both. If we do want that ability, i would prefer this be something like mPlayPreviewCompleted like josh suggested

::: content/base/src/nsObjectLoadingContent.h
@@ +287,5 @@
>       */
>      bool ShouldPlay(FallbackType &aReason);
>  
>      /**
> +     * If this object is allowed to preview plugin content.

Not very clear, maybe "If the object should display preview content for the current mContentType"
> If don't need to support stopping the preview but then going to the
> click-to-play dialog, then we can probably away with no new variables and
> just using playPlugin() for both purposes.

Err, english seems to have failed me here, let me try again:

If we don't need to support stopping the preview and then going to the click-to-play dialog, we can probably get away with no new variables by using mActivated/playPlugin() for both purposes.
> Do we ever want to stop the preview, but then display click-to-play? It
> seems like this could be much simpler if mActivated was used to bypass both
> CTP and play preview, but I don't know if we have a specific use case in
> mind.
> 

John, the fallback to CTP is important for the case when plugin is invisible, really small, or play preview cannot handle it. This fallback was created to address comment 17. Could you help me to address this comment if you disagree with how the current solution is implemented?
(In reply to John Schoenick [:johns] from comment #32)
> Comment on attachment 650395 [details] [diff] [review]
> Plugin overlay API
> 
> Review of attachment 650395 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: content/base/src/nsObjectLoadingContent.cpp
> @@ +1613,5 @@
> >        }
> >      }
> >    }
> >  
> > +  if (ShouldPreview()) {
> 
> This should be |mType == eType_Plugin && ShouldPreview()|, unless we want to
> display previews even when a real plugin wasn't found (which would require
> some other checks)

In case if we want to have a preview even when a read plugin is not found, which additional checks would it be required to add?
Attached patch Plugin overlay API (obsolete) — Splinter Review
(In reply to Josh Aas (Mozilla Corporation) from comment #31)
> Review of attachment 650395 [details] [diff] [review]:

> ::: browser/base/content/browser-plugins.js
> > +    // MozPlayPlugin event can be dispatch from the extension chrome code
> 
> "dispatched"

Fixed (also for mobile//plugins.js)

> 
> ::: content/base/public/nsIObjectLoadingContent.idl
> @@ +104,5 @@
> > +  readonly attribute boolean playPreview;

Moved

> ::: content/base/src/nsObjectLoadingContent.cpp
> @@ +2458,5 @@
> > +nsObjectLoadingContent::StopPlayPreview(bool playPlugin)
> > +{
> > +  if (!nsContentUtils::IsCallerChrome())
> > +    return NS_OK;
> 
> NS_ERROR_NOT_AVAILABLE?

Changed to NS_ERROR_NOT_AVAILABLE. Originally the IsCallerChrome() check was taken from the PlayPlugin() method.

> ::: content/base/src/nsObjectLoadingContent.h
> @@ +447,5 @@
> > +    // Used to keep track of whether or not a plugin is blocked by play-preview.
> > +    bool                        mPlayPreviewDisabled : 1;
> 
>  Maybe rename this to something like "mPlayPreviewCompleted"?

Agree. Changed.

> @@ +451,5 @@
> > +    bool                        mPlayPreviewDisabled : 1;
> > +
> > +    // Used to keep track of whether or not a plugin need a click-to-play check.
> > +    // This flags is used by play-preview.
> > +    bool                        mClickToPlayDisabled : 1;

Changed to mClickToPlayCompleted.

> I also think one or both of these variables might not be necessary. I'll let
> John Schoenick give his analysis on that.

Blocked by comment 34.

> ::: dom/plugins/base/nsIPluginHost.idl
> @@ +33,5 @@
> > +   * @param mimeType - specified mime type
> > +   */
> > +  void registerPlayPreviewMimeType(in AUTF8String mimeType);
> > +
> > +  void unregisterPlayPreviewMimeType(in AUTF8String mimeType);
> 
> Add these new members at the end.

Moved

(In reply to John Schoenick [:johns] from comment #32)
> Review of attachment 650395 [details] [diff] [review]:
> ::: content/base/public/nsIObjectLoadingContent.idl
> @@ +98,5 @@
> > +  /**
> > +   * This method will disable the play-preview plugin state. If the playPlugin
> > +   * parameter is set to true, the click-to-play feature will be ignored.
> > +   */
> > +  void stopPlayPreview(in boolean playPlugin);
> 
> Do we ever want to stop the preview, but then display click-to-play? It
> seems like this could be much simpler if mActivated was used to bypass both
> CTP and play preview, but I don't know if we have a specific use case in
> mind.

Blocked by comment 34

> @@ +104,5 @@
> > +  /**
> > +   * This attribute will return true if the plugin has been opened in play
> > +   * preview fallback mode.
> > +   */
> > +  readonly attribute boolean playPreview;
> 
> This should probably check the fallback type, rather than ShouldPreview --
> also, dkeeler's patch in bug 767636 will make this unnecessary

The pluginFallbackType was not implemented when this bug was opened. I will consider to use it after checkin of bug 767636

> ::: content/base/src/nsObjectLoadingContent.cpp
> @@ +1613,5 @@
> > +  if (ShouldPreview()) {
> 
> This should be |mType == eType_Plugin && ShouldPreview()|, unless we want to
> display previews even when a real plugin wasn't found (which would require
> some other checks)

Blocked by comment 35

> 
> @@ +2464,5 @@
> > +  if (mPlayPreviewDisabled || mActivated)
> > +    return NS_OK;
> > +
> > +  mPlayPreviewDisabled = true;
> > +  mClickToPlayDisabled = playPlugin;
> 
> mClickToPlayDisabled seems superfluous here, we should just set mActivated
> in the manner of PlayPlugin()
> 
> Also, mPlayPreviewDisabled may be superfluous - unless we specifically want
> to be able to stop the preview but then sit on the click-to-play screen,
> mActivated could be used for both. If we do want that ability, i would
> prefer this be something like mPlayPreviewCompleted like josh suggested

Blocked by comment 34 and comment 35

> 
> ::: content/base/src/nsObjectLoadingContent.h
> @@ +287,5 @@
> >       */
> >      bool ShouldPlay(FallbackType &aReason);
> >  
> >      /**
> > +     * If this object is allowed to preview plugin content.
> 
> Not very clear, maybe "If the object should display preview content for the
> current mContentType"

Fixed
Attachment #650395 - Attachment is obsolete: true
Attachment #650395 - Flags: review?(margaret.leibovic)
Attachment #650395 - Flags: review?(jschoenick)
Attachment #650395 - Flags: review?(jaws)
(In reply to Yury (:yury) from comment #34)
> John, the fallback to CTP is important for the case when plugin is
> invisible, really small, or play preview cannot handle it. This fallback was
> created to address comment 17. Could you help me to address this comment if
> you disagree with how the current solution is implemented?

In that case, I'm okay with having a mPlayPreviewCompleted bool here, but I'm not convinced the "mClickToPlayCompleted" variable is necessary - mActivated exists for this reason, so stopPlayPreview could just set activated if necessary to bypass CTP, unless I'm missing something.

Also, ShouldPreview might need to check mActivated - e.g. if the frontend activates the plugin (playPlugin()) it shouldn't also need to call stopPlayPreview() (or maybe these two calls could just be merged? it seems stopPlayPreview basically obsoletes playPlugin)

(In reply to Yury (:yury) from comment #35)
> In case if we want to have a preview even when a read plugin is not found,
> which additional checks would it be required to add?

Well, at this point mType could be Image, Document, Null, or Plugin.

It may be Null, for instance, if the plugin has bogus parameters and wouldn't be loaded (even if there *is* a plugin for mContentType, see UpdateObjectParameters and the stateInvalid bool), so enabling the preview at that point might not be desirable. It could also be type Document or Image, at which point the behavior is less clear - these play previews are registered with nsPluginHost, so it doesn't seem like giving a preview to something we're about to handle as a subdocument or image is correct.

One option would be to have UpdateObjectParameters null out mContentType if stateInvalid is true, then have this check be |(mType == eType_Null || mType == eType_Plugin) && ShouldPreview()| -- that way invalid plugins wont be checked for previews (they wont have a content type), and items resolved as Image/Document will bypass this check.
Attached patch Plugin overlay API (obsolete) — Splinter Review
(In reply to John Schoenick [:johns] from comment #37)
> (In reply to Yury (:yury) from comment #34)
> > John, the fallback to CTP is important for the case when plugin is
> > invisible, really small, or play preview cannot handle it. This fallback was
> > created to address comment 17. Could you help me to address this comment if
> > you disagree with how the current solution is implemented?
> 
> In that case, I'm okay with having a mPlayPreviewCompleted bool here, but
> I'm not convinced the "mClickToPlayCompleted" variable is necessary -
> mActivated exists for this reason, so stopPlayPreview could just set
> activated if necessary to bypass CTP, unless I'm missing something.
> 
> Also, ShouldPreview might need to check mActivated - e.g. if the frontend
> activates the plugin (playPlugin()) it shouldn't also need to call
> stopPlayPreview() (or maybe these two calls could just be merged? it seems
> stopPlayPreview basically obsoletes playPlugin)
> 

Agree. Removed the mClickToPlayCompleted and it's usage -- the mActivated is set in the stopPlayPreview() instead.

> (In reply to Yury (:yury) from comment #35)
> > In case if we want to have a preview even when a read plugin is not found,
> > which additional checks would it be required to add?
> 
> Well, at this point mType could be Image, Document, Null, or Plugin.
> 
> It may be Null, for instance, if the plugin has bogus parameters and
> wouldn't be loaded (even if there *is* a plugin for mContentType, see
> UpdateObjectParameters and the stateInvalid bool), so enabling the preview
> at that point might not be desirable. It could also be type Document or
> Image, at which point the behavior is less clear - these play previews are
> registered with nsPluginHost, so it doesn't seem like giving a preview to
> something we're about to handle as a subdocument or image is correct.
> 
> One option would be to have UpdateObjectParameters null out mContentType if
> stateInvalid is true, then have this check be |(mType == eType_Null || mType
> == eType_Plugin) && ShouldPreview()| -- that way invalid plugins wont be
> checked for previews (they wont have a content type), and items resolved as
> Image/Document will bypass this check.

The code changed to reflect that. Thank you.
Attachment #652442 - Attachment is obsolete: true
Attachment #652621 - Flags: review?(jschoenick)
Comment on attachment 652621 [details] [diff] [review]
Plugin overlay API

Review of attachment 652621 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on the ObjectLoadingContent parts with minor nits below

::: content/base/public/nsIObjectLoadingContent.idl
@@ +109,5 @@
> +  /**
> +   * This method will disable the play-preview plugin state. If the playPlugin
> +   * parameter is set to true, the click-to-play feature will be ignored.
> +   */
> +  void stopPlayPreview(in boolean playPlugin);

The parameter here could possibly be removed now - playPlugin() will set mActivated and thus bypass CTP and previews, so this function could just be used for the cases where we *just* want to stop the preview.

::: content/base/src/nsObjectLoadingContent.cpp
@@ +1422,5 @@
>    //
>  
>    if (stateInvalid) {
>      newType = eType_Null;
> +    newMime.AssignLiteral("");

This could just be newMime.Truncate()
Attachment #652621 - Flags: review?(jschoenick) → review+
Attached patch Plugin overlay API (obsolete) — Splinter Review
(In reply to John Schoenick [:johns] from comment #32)
> Comment on attachment 650395 [details] [diff] [review]
> Plugin overlay API
> 
> Review of attachment 650395 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/public/nsIObjectLoadingContent.idl
> @@ +104,5 @@
> > +  /**
> > +   * This attribute will return true if the plugin has been opened in play
> > +   * preview fallback mode.
> > +   */
> > +  readonly attribute boolean playPreview;
> 
> This should probably check the fallback type, rather than ShouldPreview --
> also, dkeeler's patch in bug 767636 will make this unnecessary

The playPreview was removed -- the tests and XBL overlay checks the pluginFallbackType property to ensure that plugin might be in play preview state. The activated property getter logic had to be modified to add ShouldPreview() call since the pluginFallbackType does not indicate if preview was stopped and actual plugin is displayed. 


(In reply to John Schoenick [:johns] from comment #39)
> Comment on attachment 652621 [details] [diff] [review]
> Plugin overlay API
> 
> Review of attachment 652621 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ on the ObjectLoadingContent parts with minor nits below
> 
> ::: content/base/public/nsIObjectLoadingContent.idl
> @@ +109,5 @@
> > +  /**
> > +   * This method will disable the play-preview plugin state. If the playPlugin
> > +   * parameter is set to true, the click-to-play feature will be ignored.
> > +   */
> > +  void stopPlayPreview(in boolean playPlugin);
> 
> The parameter here could possibly be removed now - playPlugin() will set
> mActivated and thus bypass CTP and previews, so this function could just be
> used for the cases where we *just* want to stop the preview.

The stopPlayPreview() method was renamed to cancelPlayPreview() and the parameter was removed. 

> 
> ::: content/base/src/nsObjectLoadingContent.cpp
> @@ +1422,5 @@
> >    //
> >  
> >    if (stateInvalid) {
> >      newType = eType_Null;
> > +    newMime.AssignLiteral("");
> 
> This could just be newMime.Truncate()

Fixed.
Attachment #652621 - Attachment is obsolete: true
Attachment #653105 - Flags: review?(jschoenick)
Attachment #653105 - Flags: review?(jaws)
Per comment 40, stopPlayPreview(playPlugin) is split to playPlugin() and cancelPlayPreview()
Attachment #649819 - Attachment is obsolete: true
Attached patch Plugin overlay testing (obsolete) — Splinter Review
The playPreview property logic and replaced by the activated and pluginFallbackType checks. Also, updated to current m-c

https://tbpl.mozilla.org/?tree=Try&rev=2fc2b57d0410
Attachment #650394 - Attachment is obsolete: true
Attachment #653109 - Flags: review?(jaws)
Depends on: 767636
Comment on attachment 653105 [details] [diff] [review]
Plugin overlay API

Review of attachment 653105 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on the ObjectLoadingContent changes
Attachment #653105 - Flags: review?(jschoenick) → review+
Comment on attachment 653105 [details] [diff] [review]
Plugin overlay API

Review of attachment 653105 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-plugins.js
@@ +187,5 @@
>  
>    activateSinglePlugin: function PH_activateSinglePlugin(aContentWindow, aPlugin) {
>      let objLoadingContent = aPlugin.QueryInterface(Ci.nsIObjectLoadingContent);
> +    if (!objLoadingContent.activated &&
> +        objLoadingContent.pluginFallbackType !== Ci.nsIObjectLoadingContent.PLUGIN_PLAY_PREVIEW)

Unless I'm misunderstanding something, activateSinglePlugin is only called from the click handler that gets added in the PluginClickToPlay handler. Why is this added check necessary? Can something cause "PluginClickToPlay" to be fired while having its pluginFallbackType be PLUGIN_PLAY_PREVIEW?

::: dom/plugins/base/nsPluginHost.cpp
@@ +1836,5 @@
> +NS_IMETHODIMP
> +nsPluginHost::UnregisterPlayPreviewMimeType(const nsACString& mimeType)
> +{
> +  nsCAutoString mimeTypeToRemove(mimeType);
> +  for (PRUint32 i = mPlayPreviewMimeTypes.Length(); i > 0;) {

If it's important that this loop be backwards (i.e. starting at the end of the list and going to the front), maybe a comment explaining why would be nice.
Comment on attachment 653105 [details] [diff] [review]
Plugin overlay API

Review of attachment 653105 [details] [diff] [review]:
-----------------------------------------------------------------

r- on the browser frontend changes. Shouldn't be much more to change, but the iframe class addition will touch a lot of parts of this patch.

::: browser/base/content/browser-plugins.js
@@ +313,5 @@
> +      // the XBL binding is not attached (element is display:none), fallback to click-to-play logic
> +      gPluginHandler.stopPlayPreview(aPlugin, false);
> +      return;
> +    }
> +    let iframe = previewContent.lastChild;

This is too fragile. I'd rather see a class added to the iframe so you can use previewContent.getElementsByClassName("clsname")[0].

@@ +328,5 @@
> +    iframe.src = playPreviewUri;
> +
> +    // MozPlayPlugin event can be dispatched from the extension chrome
> +    // code to replace the preview content with the native plugin
> +    previewContent.addEventListener("MozPlayPlugin", function(aEvent) {

This event listener should be removed when this function is called.

@@ +332,5 @@
> +    previewContent.addEventListener("MozPlayPlugin", function(aEvent) {
> +      if (!aEvent.isTrusted)
> +        return;
> +
> +      let playPlugin = typeof aEvent.detail === "boolean" ? aEvent.detail : true;

Should this default to false?
Attachment #653105 - Flags: review?(jaws) → review-
Comment on attachment 653105 [details] [diff] [review]
Plugin overlay API

Also, Margaret will need to review the mobile frontend changes.
Attachment #653105 - Flags: review?(margaret.leibovic)
Comment on attachment 653109 [details] [diff] [review]
Plugin overlay testing

Review of attachment 653109 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with changes addressed.

::: browser/base/content/test/browser_pluginplaypreview.js
@@ +136,5 @@
> +  waitForExplicitFinish();
> +  registerCleanupFunction(function() {
> +    if (gPlayPreviewRegistration)
> +      gPlayPreviewRegistration.unregister();
> +    Services.prefs.setBoolPref("plugins.click_to_play", false);

Services.prefs.clearUserPref("plugins.click_to_play");

@@ +197,5 @@
> +
> +  var overlay = doc.getAnonymousElementByAttribute(plugin, "class", "previewPluginContent");
> +  ok(overlay, "Test 1a, the overlay div is expected");
> +
> +  var iframe = overlay.lastChild;

Please update this test with the requested iframe class from the implementation patch.

@@ +304,5 @@
> +  var objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
> +  ok(objLoadingContent.activated, "Test 5b, Plugin should be activated");
> +
> +  Services.prefs.setBoolPref("plugins.click_to_play", false);
> +  unregisterPlayPreview();

These two lines shouldn't be needed as the function passed to registerCleanupFunction will handle this.

::: browser/base/content/test/head.js
@@ +87,5 @@
>    }, 100);
>    var moveOn = function() { clearInterval(interval); nextTest(); };
>  }
> + 
> +function getTestPlugin() {

nit: trailing whitespace.
Attachment #653109 - Flags: review?(jaws) → review+
(In reply to David Keeler from comment #44)
> Comment on attachment 653105 [details] [diff] [review]
> Plugin overlay API
> 
> Review of attachment 653105 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-plugins.js
> @@ +187,5 @@
> >  
> >    activateSinglePlugin: function PH_activateSinglePlugin(aContentWindow, aPlugin) {
> >      let objLoadingContent = aPlugin.QueryInterface(Ci.nsIObjectLoadingContent);
> > +    if (!objLoadingContent.activated &&
> > +        objLoadingContent.pluginFallbackType !== Ci.nsIObjectLoadingContent.PLUGIN_PLAY_PREVIEW)
> 
> Unless I'm misunderstanding something, activateSinglePlugin is only called
> from the click handler that gets added in the PluginClickToPlay handler. Why
> is this added check necessary? Can something cause "PluginClickToPlay" to be
> fired while having its pluginFallbackType be PLUGIN_PLAY_PREVIEW?

The play preview is not complete substitution of the click-to-play. It can an intermediate stage before, instead of it or both. As explained in comment 23, the playAllPlugins() will not be touching plugins in the play preview mode, this check is just for that.

> 
> ::: dom/plugins/base/nsPluginHost.cpp
> @@ +1836,5 @@
> > +  for (PRUint32 i = mPlayPreviewMimeTypes.Length(); i > 0;) {
> 
> If it's important that this loop be backwards (i.e. starting at the end of
> the list and going to the front), maybe a comment explaining why would be
> nice.

There is nothing special: that would make sense if array will contain multiple duplicates to save amount of moved elements. I don't anticipate lots of duplicate entries, so it might be good either way in this case.
Forgot for mention, during try testing, before I added the |plugin.clientTop;| hack just after the iframe is inserted, there was very intermittent failure similar to:

> Log
>
> Rev4 MacOSX Snow Leopard 10.6 try opt test mochitest-other on 2012-08-16 16:18:25 PDT for push 6f307f0e3a8e
>
> Summary
>
> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginplaypreview.js | Test 1a, the overlay about: content is expected
> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginplaypreview.js | an unexpected uncaught JS exception reported through window.onerror - TypeError: Components.classes is undefined at jar:file:///Users/cltbld/talos-slave/test/build/FirefoxNightly.app/Contents/MacOS/omni.ja!/chrome/toolkit/content/global/about.xhtml:38
> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginplaypreview.js | Found an unexpected tab at the end of test run: jar:file:///Users/cltbld/talos-slave/test/build/FirefoxNightly.app/Contents/MacOS/omni.ja!/chrome/toolkit/content/global/about.xhtml
> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginplaypreview.js | Found an unexpected tab at the end of test run: chrome://mochitests/content/browser/browser/base/content/test/plugin_test.html

It's hard to replicate it locally: just after the m-c build is done, only first time, and running the browser_pluginplaypreview.js mochitest. When I was able able to replicate (on Windows), the console contains the following warning and the extra tab is appearing during the test:

> WARNING: Subdocument container has no frame: file c:/mozilla-central/layout/base/nsDocumentViewer.cpp, line 2401

Looks like the clientTop-hack solved the issue locally and it is not appearing on try servers anymore. However I would like to keep track of it.

Jared, is there somebody who can verify that this is a right way to solve the issue and it will not become an intermittent failure on the m-c?
Attached patch Plugin overlay API (obsolete) — Splinter Review
(In reply to Jared Wein [:jaws] from comment #45)
> Comment on attachment 653105 [details] [diff] [review]
> Plugin overlay API
> 
> Review of attachment 653105 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-plugins.js
> @@ +313,5 @@
> > +    let iframe = previewContent.lastChild;
> 
> This is too fragile. I'd rather see a class added to the iframe so you can
> use previewContent.getElementsByClassName("clsname")[0].

Changed to use a class attribute.

> 
> @@ +328,5 @@
> > +    iframe.src = playPreviewUri;
> > +
> > +    // MozPlayPlugin event can be dispatched from the extension chrome
> > +    // code to replace the preview content with the native plugin
> > +    previewContent.addEventListener("MozPlayPlugin", function(aEvent) {
> 
> This event listener should be removed when this function is called.

Fixed.

> 
> @@ +332,5 @@
> > +    previewContent.addEventListener("MozPlayPlugin", function(aEvent) {
> > +      if (!aEvent.isTrusted)
> > +        return;
> > +
> > +      let playPlugin = typeof aEvent.detail === "boolean" ? aEvent.detail : true;
> 
> Should this default to false?

The meaning of the aEvent.detail is reversed, so default is false now.
Attachment #653105 - Attachment is obsolete: true
Attachment #653105 - Flags: review?(margaret.leibovic)
Attached patch Plugin overlay testing (obsolete) — Splinter Review
(In reply to Jared Wein [:jaws] from comment #47)
> Comment on attachment 653109 [details] [diff] [review]
> Plugin overlay testing
> 
> Review of attachment 653109 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/test/browser_pluginplaypreview.js
> @@ +136,5 @@
> > +  waitForExplicitFinish();
> > +  registerCleanupFunction(function() {
> > +    if (gPlayPreviewRegistration)
> > +      gPlayPreviewRegistration.unregister();
> > +    Services.prefs.setBoolPref("plugins.click_to_play", false);
> 
> Services.prefs.clearUserPref("plugins.click_to_play");

Fixed.

> @@ +197,5 @@
> > +
> > +  var overlay = doc.getAnonymousElementByAttribute(plugin, "class", "previewPluginContent");
> > +  ok(overlay, "Test 1a, the overlay div is expected");
> > +
> > +  var iframe = overlay.lastChild;
> 
> Please update this test with the requested iframe class from the
> implementation patch.

Updated to use class attribute instead of lastChild.

> @@ +304,5 @@
> > +  Services.prefs.setBoolPref("plugins.click_to_play", false);
> > +  unregisterPlayPreview();
> 
> These two lines shouldn't be needed as the function passed to
> registerCleanupFunction will handle this.

Removed.

> ::: browser/base/content/test/head.js
> @@ +87,5 @@
> >    }, 100);
> >    var moveOn = function() { clearInterval(interval); nextTest(); };
> >  }
> > + 
> > +function getTestPlugin() {
> 
> nit: trailing whitespace.

Fixed.
Attachment #653109 - Attachment is obsolete: true
(In reply to Yury (:yury) from comment #48)
> The play preview is not complete substitution of the click-to-play. It can
> an intermediate stage before, instead of it or both. As explained in comment
> 23, the playAllPlugins() will not be touching plugins in the play preview
> mode, this check is just for that.

I see how this works for the android browser.js, but unfortunately things are a bit different with the desktop browser-plugins.js. Bascially, activatePlugins doesn't call activateSinglePlugin, so we don't need that check there. However, most (if not all) times when the code loops over cwu.plugins, we'll need to add a check of that sort.
Attached patch Plugin overlay API (obsolete) — Splinter Review
(In reply to David Keeler from comment #52)
> (In reply to Yury (:yury) from comment #48)
> > The play preview is not complete substitution of the click-to-play. It can
> > an intermediate stage before, instead of it or both. As explained in comment
> > 23, the playAllPlugins() will not be touching plugins in the play preview
> > mode, this check is just for that.
> 
> I see how this works for the android browser.js, but unfortunately things
> are a bit different with the desktop browser-plugins.js. Bascially,
> activatePlugins doesn't call activateSinglePlugin, so we don't need that
> check there. However, most (if not all) times when the code loops over
> cwu.plugins, we'll need to add a check of that sort.

Good catch. I added the canActivatePlugin function in browser-plugins.js, which is used in activatePlugins and activateSinglePlugin functions.
Attachment #653765 - Attachment is obsolete: true
Attachment #653919 - Flags: review?(margaret.leibovic)
Attachment #653919 - Flags: review?(jaws)
Attachment #653919 - Flags: review?(jaws) → review+
Comment on attachment 653919 [details] [diff] [review]
Plugin overlay API

r+ on the /mobile changes.
Attachment #653919 - Flags: review?(margaret.leibovic) → review+
Attached patch Plugin overlay API (obsolete) — Splinter Review
PRUint32 replaced by uint32_t

Per comment 31 request, asking Josh for review for the final patch.
Attachment #653919 - Attachment is obsolete: true
Attachment #654581 - Flags: superreview?(joshmoz)
Attachment #654581 - Flags: superreview?(joshmoz) → superreview+
Attached patch patch for checkin (obsolete) — Splinter Review
Attachment #654581 - Attachment is obsolete: true
Attached patch patch for checkin (tests) (obsolete) — Splinter Review
Attachment #653767 - Attachment is obsolete: true
Comment on attachment 654628 [details] [diff] [review]
patch for checkin

Review of attachment 654628 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-plugins.js
@@ +198,5 @@
>      let cwu = aContentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>                              .getInterface(Ci.nsIDOMWindowUtils);
>      let haveUnplayedPlugins = cwu.plugins.some(function(plugin) {
>        let objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
> +      return (plugin != aPlugin && gPluginHandler.canActivatePlugin(objLoadingContent));

Looks like we still need a similar check in reshowClickToPlayNotification and _removeClickToPlayOverlays.
Is this ready to land? Which patches exactly need to land? Is comment 59 a blocker for this landing?
(In reply to David Keeler from comment #59)
> Comment on attachment 654628 [details] [diff] [review]
> Plugin overlay API
> 
> Review of attachment 654628 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-plugins.js
> @@ +198,5 @@
> >      let cwu = aContentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> >                              .getInterface(Ci.nsIDOMWindowUtils);
> >      let haveUnplayedPlugins = cwu.plugins.some(function(plugin) {
> >        let objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
> > +      return (plugin != aPlugin && gPluginHandler.canActivatePlugin(objLoadingContent));
> 
> Looks like we still need a similar check in reshowClickToPlayNotification

Yeah, that might case to show an icon on desktop when preview exists and click-to-play is on. 

> and _removeClickToPlayOverlays.

Not really necessary -- the preview does not use mainBox (it's hidden in this mode).
Attachment #654736 - Flags: review?(jaws)
Attachment #654736 - Attachment is obsolete: true
Attachment #654736 - Flags: review?(jaws)
Blocks: 785190
Attachment #654628 - Attachment description: Plugin overlay API → patch for checkin
Attachment #654630 - Attachment description: Plugin overlay testing → patch for checkin (tests)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #60)
> Is this ready to land? Which patches exactly need to land? Is comment 59 a
> blocker for this landing?

Bug 785190 opened to follow up on comment 59 and comment 61.
Backed out of inbound for common failures:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginplaypreview.js | Test 1a, the overlay about: content is expected
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginplaypreview.js | an unexpected uncaught JS exception reported through window.onerror - TypeError: Components.classes is undefined at jar:file:///c:/talos-slave/test/build/firefox/omni.ja!/chrome/toolkit/content/global/about.xhtml:38
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginplaypreview.js | an unexpected uncaught JS exception reported through window.onerror - TypeError: Components.classes is undefined at jar:file:///c:/talos-slave/test/build/firefox/omni.ja!/chrome/toolkit/content/global/about.xhtml:38
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginplaypreview.js | Found an unexpected tab at the end of test run: jar:file:///c:/talos-slave/test/build/firefox/omni.ja!/chrome/toolkit/content/global/about.xhtml
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginplaypreview.js | Found an unexpected tab at the end of test run: chrome://mochitests/content/browser/browser/base/content/test/plugin_test.html

<philor> bsmedberg: https://tbpl.mozilla.org/php/getParsedLog.php?id=14672431&tree=Mozilla-Inbound, https://tbpl.mozilla.org/php/getParsedLog.php?id=14671196&tree=Mozilla-Inbound, https://tbpl.mozilla.org/php/getParsedLog.php?id=14671428&tree=Mozilla-Inbound
<philor> https://tbpl.mozilla.org/php/getParsedLog.php?id=14673285&tree=Mozilla-Inbound
I think it's possible that iframe's onload event triggered way later than documents and the test1a() starts earlier than needed -- fixing that by counting the onload events.
Attachment #655166 - Flags: review?(jaws)
Comment on attachment 655166 [details] [diff] [review]
(not for checkin) intermitent failure test fix

Review of attachment 655166 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure if this will fix the issue or not, but if you decide to go with this then I think we can make the following change.

::: browser/base/content/test/browser_pluginplaypreview.js
@@ +176,2 @@
>    var nextTest = gNextTest;
>    gNextTest = null;

I think we can remove this null assignment if we make this change.
Attachment #655166 - Flags: review?(jaws)
Merging bug 785190 (https://bugzilla.mozilla.org/attachment.cgi?id=654805) in
Attachment #654628 - Attachment is obsolete: true
Attachment #655216 - Flags: review?(jaws)
(In reply to Jared Wein [:jaws] from comment #68)
> Comment on attachment 655166 [details] [diff] [review]
> (not for checkin) intermitent failure test fix
> 
> Review of attachment 655166 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure if this will fix the issue or not, but if you decide to go with
> this then I think we can make the following change.
> 
> ::: browser/base/content/test/browser_pluginplaypreview.js
> @@ +176,2 @@
> >    var nextTest = gNextTest;
> >    gNextTest = null;
> 
> I think we can remove this null assignment if we make this change.

Removed.

The intermittent failure test fix merged with the play preview tests.

https://tbpl.mozilla.org/?tree=Try&rev=149d279d9aaf
Attachment #654630 - Attachment is obsolete: true
Attachment #655166 - Attachment is obsolete: true
Attachment #655217 - Flags: review?(jaws)
Attachment #655216 - Flags: review?(jaws) → review+
Attachment #655217 - Flags: review?(jaws) → review+
Attachment #655216 - Attachment description: Plugin overlay API → patch for checkin
Attachment #655217 - Attachment description: Plugin overlay testing → patch for checkin (test)
Tests were modified to take in account two onload event (from window and overlay iframe). Try attempts with the patches:

https://tbpl.mozilla.org/?tree=Try&rev=253c3229e056
https://tbpl.mozilla.org/?tree=Try&rev=149d279d9aaf
https://tbpl.mozilla.org/?tree=Try&rev=baf945002b76
Status: NEW → ASSIGNED
Keywords: checkin-needed
Summary: Provide API for the JavaScript extension to create the "preview" of a plug-in for specific mime type → Provide API for JavaScript extensions to create native plugins previews for specific mime type
(In reply to Jared Wein from comment #45)
> > +    let iframe = previewContent.lastChild;
> This is too fragile. I'd rather see a class added to the iframe so you can
> use previewContent.getElementsByClassName("clsname")[0].
Better still, remove the comment from inside the <div> so you can tell if it is empty. Failing that, you could use firstElementChild which would avoid having to distinguish it from the comment.
(Note that the XBL content sink is "magic" as it only strips whitespace and comments from XUL nodes (and then not <label> or <description>).)
Blocks: 839714
See Also: → 1192831
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.