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)
Core Graveyard
Plug-ins
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 |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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?)
Assignee | ||
Comment 2•12 years ago
|
||
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 3•12 years ago
|
||
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 4•12 years ago
|
||
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+
Assignee | ||
Comment 5•12 years ago
|
||
(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 6•12 years ago
|
||
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+
Comment 7•12 years ago
|
||
David, you might be interested in this too wrt click to play ?
Updated•12 years ago
|
Keywords: sec-review-needed
Assignee | ||
Comment 8•12 years ago
|
||
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)
Assignee | ||
Comment 9•12 years ago
|
||
(see also https://tbpl.mozilla.org/?tree=Try&rev=235f49aba3dc)
Assignee | ||
Comment 10•12 years ago
|
||
Assignee | ||
Comment 11•12 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Attachment #646997 -
Flags: review?(joshmoz)
Assignee | ||
Updated•12 years ago
|
Attachment #646998 -
Flags: review?(jaws)
Updated•12 years ago
|
Attachment #646998 -
Flags: review?(dkeeler)
Comment 12•12 years ago
|
||
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?
Assignee | ||
Comment 13•12 years ago
|
||
(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)
Updated•12 years ago
|
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)
Updated•12 years ago
|
Keywords: sec-review-needed
Assignee | ||
Comment 15•12 years ago
|
||
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)
Comment 16•12 years ago
|
||
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 17•12 years ago
|
||
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+
Assignee | ||
Comment 18•12 years ago
|
||
(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?
Assignee | ||
Comment 19•12 years ago
|
||
(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 20•12 years ago
|
||
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+
Comment 21•12 years ago
|
||
(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•12 years ago
|
||
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-
Assignee | ||
Comment 23•12 years ago
|
||
(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)
Assignee | ||
Comment 24•12 years ago
|
||
Steps 7 and 8 were changed to reflect the stopPlayPreview() use
Attachment #649819 -
Flags: feedback?(imelven)
Assignee | ||
Updated•12 years ago
|
Attachment #647141 -
Attachment is obsolete: true
Comment 25•12 years ago
|
||
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 26•12 years ago
|
||
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+
Assignee | ||
Comment 27•12 years ago
|
||
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)
Assignee | ||
Comment 28•12 years ago
|
||
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)
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?
Updated•12 years ago
|
Flags: sec-review?
Updated•12 years ago
|
Flags: sec-review? → sec-review?(mgoodwin)
Updated•12 years ago
|
Flags: sec-review?(mgoodwin) → sec-review+
Comment 30•12 years ago
|
||
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 31•12 years ago
|
||
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 32•12 years ago
|
||
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•12 years ago
|
||
> 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.
Assignee | ||
Comment 34•12 years ago
|
||
> 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?
Assignee | ||
Comment 35•12 years ago
|
||
(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?
Assignee | ||
Comment 36•12 years ago
|
||
(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)
Comment 37•12 years ago
|
||
(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.
Assignee | ||
Comment 38•12 years ago
|
||
(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 39•12 years ago
|
||
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+
Assignee | ||
Comment 40•12 years ago
|
||
(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)
Assignee | ||
Comment 41•12 years ago
|
||
Per comment 40, stopPlayPreview(playPlugin) is split to playPlugin() and cancelPlayPreview()
Attachment #649819 -
Attachment is obsolete: true
Assignee | ||
Comment 42•12 years ago
|
||
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)
Comment 43•12 years ago
|
||
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 45•12 years ago
|
||
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 46•12 years ago
|
||
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 47•12 years ago
|
||
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+
Assignee | ||
Comment 48•12 years ago
|
||
(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.
Assignee | ||
Comment 49•12 years ago
|
||
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?
Assignee | ||
Comment 50•12 years ago
|
||
(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)
Assignee | ||
Comment 51•12 years ago
|
||
(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.
Assignee | ||
Comment 53•12 years ago
|
||
(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)
Updated•12 years ago
|
Attachment #653919 -
Flags: review?(jaws) → review+
Comment 54•12 years ago
|
||
Comment on attachment 653919 [details] [diff] [review] Plugin overlay API r+ on the /mobile changes.
Attachment #653919 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 55•12 years ago
|
||
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+
Assignee | ||
Comment 56•12 years ago
|
||
Attachment #654581 -
Attachment is obsolete: true
Assignee | ||
Comment 57•12 years ago
|
||
Attachment #653767 -
Attachment is obsolete: true
Assignee | ||
Comment 58•12 years ago
|
||
Last few attempts to try server: https://tbpl.mozilla.org/?tree=Try&rev=ec04b0e830ef https://tbpl.mozilla.org/?tree=Try&rev=fa1a3e0f92c5 https://tbpl.mozilla.org/?tree=Try&rev=afb1bae8fcb0 https://tbpl.mozilla.org/?tree=Try&rev=a3ed39ba0f53 https://tbpl.mozilla.org/?tree=Try&rev=f3fb9e5bb239
Keywords: checkin-needed
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•12 years ago
|
||
Is this ready to land? Which patches exactly need to land? Is comment 59 a blocker for this landing?
Assignee | ||
Comment 61•12 years ago
|
||
(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)
Assignee | ||
Updated•12 years ago
|
Attachment #654736 -
Attachment is obsolete: true
Attachment #654736 -
Flags: review?(jaws)
Assignee | ||
Updated•12 years ago
|
Attachment #654628 -
Attachment description: Plugin overlay API → patch for checkin
Assignee | ||
Updated•12 years ago
|
Attachment #654630 -
Attachment description: Plugin overlay testing → patch for checkin (tests)
Assignee | ||
Comment 62•12 years ago
|
||
(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 63•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e80dea0213a0 https://hg.mozilla.org/integration/mozilla-inbound/rev/9e6948f52101
Target Milestone: --- → mozilla17
Comment 64•12 years ago
|
||
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 65•12 years ago
|
||
Green on Try. Sorry for the obscenely long delay. https://tbpl.mozilla.org/?tree=Try&rev=ec04b0e830ef https://hg.mozilla.org/integration/mozilla-inbound/rev/ed93b381d52a https://hg.mozilla.org/integration/mozilla-inbound/rev/4e2788b485d4
Flags: in-testsuite+
Keywords: checkin-needed
Assignee | ||
Comment 66•12 years ago
|
||
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 67•12 years ago
|
||
Backed out again until the text fix is ready. https://hg.mozilla.org/integration/mozilla-inbound/rev/f077de66e52d
Comment 68•12 years ago
|
||
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)
Assignee | ||
Comment 69•12 years ago
|
||
Merging bug 785190 (https://bugzilla.mozilla.org/attachment.cgi?id=654805) in
Attachment #654628 -
Attachment is obsolete: true
Attachment #655216 -
Flags: review?(jaws)
Assignee | ||
Comment 70•12 years ago
|
||
(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)
Updated•12 years ago
|
Attachment #655216 -
Flags: review?(jaws) → review+
Updated•12 years ago
|
Attachment #655217 -
Flags: review?(jaws) → review+
Assignee | ||
Updated•12 years ago
|
Attachment #655216 -
Attachment description: Plugin overlay API → patch for checkin
Assignee | ||
Updated•12 years ago
|
Attachment #655217 -
Attachment description: Plugin overlay testing → patch for checkin (test)
Assignee | ||
Comment 71•12 years ago
|
||
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
Comment 72•12 years ago
|
||
(In reply to Yury (:yury) from comment #71) > https://tbpl.mozilla.org/?tree=Try&rev=253c3229e056 > https://tbpl.mozilla.org/?tree=Try&rev=149d279d9aaf > https://tbpl.mozilla.org/?tree=Try&rev=baf945002b76 Looking green, even with a lot of m-oth retriggers. https://hg.mozilla.org/integration/mozilla-inbound/rev/2fee87b2367a https://hg.mozilla.org/integration/mozilla-inbound/rev/7c2aaefc3891
Keywords: checkin-needed
Comment 73•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2fee87b2367a https://hg.mozilla.org/mozilla-central/rev/7c2aaefc3891
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•12 years ago
|
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
Comment 74•12 years ago
|
||
(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•12 years ago
|
||
(Note that the XBL content sink is "magic" as it only strips whitespace and comments from XUL nodes (and then not <label> or <description>).)
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•