Last Comment Bug 776208 - Provide API for JavaScript extensions to create native plugins previews for specific mime type
: Provide API for JavaScript extensions to create native plugins previews for s...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Yury Delendik (:yury)
:
Mentors:
Depends on: 745030 767636 779359
Blocks: 725056 785190 839714
  Show dependency treegraph
 
Reported: 2012-07-20 20:55 PDT by Yury Delendik (:yury)
Modified: 2015-08-10 07:12 PDT (History)
16 users (show)
curtisk: sec‑review+
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Pilot solution (26.81 KB, patch)
2012-07-20 20:55 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
Pilot solution (v2) (28.22 KB, patch)
2012-07-26 08:40 PDT, Yury Delendik (:yury)
jaws: feedback+
Details | Diff | Splinter Review
Pilot solution (v3) (28.73 KB, patch)
2012-07-27 07:36 PDT, Yury Delendik (:yury)
margaret.leibovic: feedback+
Details | Diff | Splinter Review
Plugin overlay API (29.45 KB, patch)
2012-07-29 12:10 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
Plugin overlay testing (10.79 KB, patch)
2012-07-29 12:12 PDT, Yury Delendik (:yury)
jaws: feedback+
Details | Diff | Splinter Review
Plugin overlay interaction (199.52 KB, application/pdf)
2012-07-30 06:44 PDT, Yury Delendik (:yury)
no flags Details
Plugin overlay API (29.45 KB, patch)
2012-07-31 14:51 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
Plugin overlay API (29.68 KB, patch)
2012-08-03 13:35 PDT, Yury Delendik (:yury)
jaws: review-
margaret.leibovic: feedback+
Details | Diff | Splinter Review
Plugin overlay testing (17.37 KB, patch)
2012-08-06 16:52 PDT, Yury Delendik (:yury)
jaws: review+
Details | Diff | Splinter Review
Plugin overlay API (35.92 KB, patch)
2012-08-07 14:43 PDT, Yury Delendik (:yury)
ian.melven: feedback+
Details | Diff | Splinter Review
Plugin overlay interaction (211.45 KB, application/pdf)
2012-08-07 14:45 PDT, Yury Delendik (:yury)
ian.melven: feedback+
Details
Plugin overlay testing (20.62 KB, patch)
2012-08-08 17:40 PDT, Yury Delendik (:yury)
jaws: review+
Details | Diff | Splinter Review
Plugin overlay API (33.22 KB, patch)
2012-08-08 17:45 PDT, Yury Delendik (:yury)
jaas: review-
Details | Diff | Splinter Review
Plugin overlay API (33.00 KB, patch)
2012-08-16 07:15 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
Plugin overlay API (33.41 KB, patch)
2012-08-16 16:38 PDT, Yury Delendik (:yury)
john: review+
Details | Diff | Splinter Review
Plugin overlay API (34.93 KB, patch)
2012-08-18 14:44 PDT, Yury Delendik (:yury)
john: review+
jaws: review-
Details | Diff | Splinter Review
Plugin overlay interaction (89.67 KB, application/pdf)
2012-08-18 14:47 PDT, Yury Delendik (:yury)
no flags Details
Plugin overlay testing (20.59 KB, patch)
2012-08-18 14:50 PDT, Yury Delendik (:yury)
jaws: review+
Details | Diff | Splinter Review
Plugin overlay API (35.21 KB, patch)
2012-08-21 08:42 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
Plugin overlay testing (20.61 KB, patch)
2012-08-21 08:47 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
Plugin overlay API (36.29 KB, patch)
2012-08-21 13:35 PDT, Yury Delendik (:yury)
jaws: review+
margaret.leibovic: review+
Details | Diff | Splinter Review
Plugin overlay API (36.33 KB, patch)
2012-08-23 05:28 PDT, Yury Delendik (:yury)
jaas: superreview+
Details | Diff | Splinter Review
patch for checkin (36.34 KB, patch)
2012-08-23 08:27 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
patch for checkin (tests) (20.63 KB, patch)
2012-08-23 08:28 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
(not for checkin) adds canActivatePlugin check to reshowClickToPlayNotification (1.14 KB, patch)
2012-08-23 12:25 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
(not for checkin) intermitent failure test fix (4.58 KB, patch)
2012-08-24 14:27 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
patch for checkin (37.08 KB, patch)
2012-08-24 16:26 PDT, Yury Delendik (:yury)
jaws: review+
Details | Diff | Splinter Review
patch for checkin (test) (20.71 KB, patch)
2012-08-24 16:29 PDT, Yury Delendik (:yury)
jaws: review+
Details | Diff | Splinter Review

Description Yury Delendik (:yury) 2012-07-20 20:55:12 PDT
Created attachment 644585 [details] [diff] [review]
Pilot solution

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.
Comment 1 Yury Delendik (:yury) 2012-07-20 21:00:26 PDT
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?)
Comment 2 Yury Delendik (:yury) 2012-07-26 08:40:51 PDT
Created attachment 646153 [details] [diff] [review]
Pilot solution (v2)

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
Comment 3 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-07-26 10:05:46 PDT
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.
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2012-07-27 00:10:52 PDT
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;
Comment 5 Yury Delendik (:yury) 2012-07-27 07:36:03 PDT
Created attachment 646578 [details] [diff] [review]
Pilot solution (v3)

(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.
Comment 6 :Margaret Leibovic 2012-07-27 09:06:25 PDT
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.
Comment 7 Ian Melven :imelven 2012-07-27 16:55:59 PDT
David, you might be interested in this too wrt click to play ?
Comment 8 Yury Delendik (:yury) 2012-07-29 12:10:44 PDT
Created attachment 646997 [details] [diff] [review]
Plugin overlay API

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.
Comment 9 Yury Delendik (:yury) 2012-07-29 12:12:41 PDT
Created attachment 646998 [details] [diff] [review]
Plugin overlay testing

(see also https://tbpl.mozilla.org/?tree=Try&rev=235f49aba3dc)
Comment 10 Yury Delendik (:yury) 2012-07-30 06:44:50 PDT
Created attachment 647141 [details]
Plugin overlay interaction
Comment 11 Yury Delendik (:yury) 2012-07-30 11:12:07 PDT
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
Comment 12 Jared Wein [:jaws] (please needinfo? me) 2012-07-31 14:35:56 PDT
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.
Comment 13 Yury Delendik (:yury) 2012-07-31 14:51:15 PDT
Created attachment 647689 [details] [diff] [review]
Plugin overlay API

(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
Comment 14 David Keeler [:keeler] (use needinfo?) 2012-07-31 16:56:36 PDT
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).
Comment 15 Yury Delendik (:yury) 2012-08-03 13:35:26 PDT
Created attachment 648831 [details] [diff] [review]
Plugin overlay API

Per security review: IFRAME cleanup/removal when playPlugin() is invoked.
Comment 16 Jared Wein [:jaws] (please needinfo? me) 2012-08-06 13:59:38 PDT
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:');
Comment 17 :Margaret Leibovic 2012-08-06 15:59:38 PDT
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.
Comment 18 Yury Delendik (:yury) 2012-08-06 16:45:24 PDT
(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?
Comment 19 Yury Delendik (:yury) 2012-08-06 16:52:01 PDT
Created attachment 649475 [details] [diff] [review]
Plugin overlay testing

(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
Comment 20 Jared Wein [:jaws] (please needinfo? me) 2012-08-06 17:23:28 PDT
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"
Comment 21 :Margaret Leibovic 2012-08-07 07:55:07 PDT
(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 22 Jared Wein [:jaws] (please needinfo? me) 2012-08-07 12:09:58 PDT
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.
Comment 23 Yury Delendik (:yury) 2012-08-07 14:43:08 PDT
Created attachment 649818 [details] [diff] [review]
Plugin overlay API

(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
Comment 24 Yury Delendik (:yury) 2012-08-07 14:45:03 PDT
Created attachment 649819 [details]
Plugin overlay interaction

Steps 7 and 8 were changed to reflect the stopPlayPreview() use
Comment 25 Ian Melven :imelven 2012-08-08 11:58:11 PDT
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.
Comment 26 Ian Melven :imelven 2012-08-08 12:33:59 PDT
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.
Comment 27 Yury Delendik (:yury) 2012-08-08 17:40:41 PDT
Created attachment 650394 [details] [diff] [review]
Plugin overlay testing

Adds two new tests for testing bypass and fallback to click-to-play mode.
Comment 28 Yury Delendik (:yury) 2012-08-08 17:45:53 PDT
Created attachment 650395 [details] [diff] [review]
Plugin overlay API

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.
Comment 29 David Keeler [:keeler] (use needinfo?) 2012-08-09 10:27:30 PDT
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?
Comment 30 Jared Wein [:jaws] (please needinfo? me) 2012-08-09 13:51:08 PDT
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.
Comment 31 Josh Aas 2012-08-15 14:20:04 PDT
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.
Comment 32 John Schoenick [:johns] 2012-08-15 14:51:24 PDT
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"
Comment 33 John Schoenick [:johns] 2012-08-15 14:54:16 PDT
> 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.
Comment 34 Yury Delendik (:yury) 2012-08-15 15:38:05 PDT
> 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?
Comment 35 Yury Delendik (:yury) 2012-08-15 15:58:37 PDT
(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?
Comment 36 Yury Delendik (:yury) 2012-08-16 07:15:47 PDT
Created attachment 652442 [details] [diff] [review]
Plugin overlay API

(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
Comment 37 John Schoenick [:johns] 2012-08-16 11:33:23 PDT
(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.
Comment 38 Yury Delendik (:yury) 2012-08-16 16:38:41 PDT
Created attachment 652621 [details] [diff] [review]
Plugin overlay API

(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.
Comment 39 John Schoenick [:johns] 2012-08-16 19:33:45 PDT
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()
Comment 40 Yury Delendik (:yury) 2012-08-18 14:44:40 PDT
Created attachment 653105 [details] [diff] [review]
Plugin overlay API

(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.
Comment 41 Yury Delendik (:yury) 2012-08-18 14:47:22 PDT
Created attachment 653108 [details]
Plugin overlay interaction

Per comment 40, stopPlayPreview(playPlugin) is split to playPlugin() and cancelPlayPreview()
Comment 42 Yury Delendik (:yury) 2012-08-18 14:50:20 PDT
Created attachment 653109 [details] [diff] [review]
Plugin overlay testing

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
Comment 43 John Schoenick [:johns] 2012-08-20 12:39:09 PDT
Comment on attachment 653105 [details] [diff] [review]
Plugin overlay API

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

r+ on the ObjectLoadingContent changes
Comment 44 David Keeler [:keeler] (use needinfo?) 2012-08-20 13:10:17 PDT
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 45 Jared Wein [:jaws] (please needinfo? me) 2012-08-20 13:12:26 PDT
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?
Comment 46 Jared Wein [:jaws] (please needinfo? me) 2012-08-20 13:14:36 PDT
Comment on attachment 653105 [details] [diff] [review]
Plugin overlay API

Also, Margaret will need to review the mobile frontend changes.
Comment 47 Jared Wein [:jaws] (please needinfo? me) 2012-08-20 13:27:38 PDT
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.
Comment 48 Yury Delendik (:yury) 2012-08-20 18:25:08 PDT
(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.
Comment 49 Yury Delendik (:yury) 2012-08-21 06:11:38 PDT
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?
Comment 50 Yury Delendik (:yury) 2012-08-21 08:42:54 PDT
Created attachment 653765 [details] [diff] [review]
Plugin overlay API

(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.
Comment 51 Yury Delendik (:yury) 2012-08-21 08:47:24 PDT
Created attachment 653767 [details] [diff] [review]
Plugin overlay testing

(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.
Comment 52 David Keeler [:keeler] (use needinfo?) 2012-08-21 10:06:54 PDT
(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.
Comment 53 Yury Delendik (:yury) 2012-08-21 13:35:15 PDT
Created attachment 653919 [details] [diff] [review]
Plugin overlay API

(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.
Comment 54 :Margaret Leibovic 2012-08-22 15:59:03 PDT
Comment on attachment 653919 [details] [diff] [review]
Plugin overlay API

r+ on the /mobile changes.
Comment 55 Yury Delendik (:yury) 2012-08-23 05:28:03 PDT
Created attachment 654581 [details] [diff] [review]
Plugin overlay API

PRUint32 replaced by uint32_t

Per comment 31 request, asking Josh for review for the final patch.
Comment 56 Yury Delendik (:yury) 2012-08-23 08:27:43 PDT
Created attachment 654628 [details] [diff] [review]
patch for checkin
Comment 57 Yury Delendik (:yury) 2012-08-23 08:28:07 PDT
Created attachment 654630 [details] [diff] [review]
patch for checkin (tests)
Comment 59 David Keeler [:keeler] (use needinfo?) 2012-08-23 10:47:41 PDT
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.
Comment 60 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-08-23 12:10:20 PDT
Is this ready to land? Which patches exactly need to land? Is comment 59 a blocker for this landing?
Comment 61 Yury Delendik (:yury) 2012-08-23 12:25:07 PDT
Created attachment 654736 [details] [diff] [review]
(not for checkin) adds canActivatePlugin check to reshowClickToPlayNotification

(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).
Comment 62 Yury Delendik (:yury) 2012-08-23 12:58:19 PDT
(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.
Comment 64 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-08-24 10:36:16 PDT
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
Comment 66 Yury Delendik (:yury) 2012-08-24 14:27:30 PDT
Created attachment 655166 [details] [diff] [review]
(not for checkin) intermitent failure test fix

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.
Comment 67 Matt Brubeck (:mbrubeck) 2012-08-24 16:07:09 PDT
Backed out again until the text fix is ready.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f077de66e52d
Comment 68 Jared Wein [:jaws] (please needinfo? me) 2012-08-24 16:11:51 PDT
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.
Comment 69 Yury Delendik (:yury) 2012-08-24 16:26:32 PDT
Created attachment 655216 [details] [diff] [review]
patch for checkin

Merging bug 785190 (https://bugzilla.mozilla.org/attachment.cgi?id=654805) in
Comment 70 Yury Delendik (:yury) 2012-08-24 16:29:08 PDT
Created attachment 655217 [details] [diff] [review]
patch for checkin (test)

(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
Comment 71 Yury Delendik (:yury) 2012-08-25 05:34:29 PDT
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
Comment 74 neil@parkwaycc.co.uk 2012-11-18 12:27:26 PST
(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.
Comment 75 neil@parkwaycc.co.uk 2012-11-18 12:29:21 PST
(Note that the XBL content sink is "magic" as it only strips whitespace and comments from XUL nodes (and then not <label> or <description>).)

Note You need to log in before you can comment on or make changes to this bug.