Last Comment Bug 812898 - Implement plugin preview overlay from Bug 776208
: Implement plugin preview overlay from Bug 776208
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.16
Assigned To: Philip Chee
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-18 10:02 PST by Philip Chee
Modified: 2012-12-01 01:48 PST (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
fixed
fixed
fixed


Attachments
Patch v1.0 Proposed fix. (24.63 KB, patch)
2012-11-18 10:12 PST, Philip Chee
no flags Details | Diff | Splinter Review
Patch v1.1 use handleEvent (25.67 KB, patch)
2012-11-19 04:40 PST, Philip Chee
neil: review+
Details | Diff | Splinter Review
Part 2 Patch to get test9c working. (1.39 KB, patch)
2012-11-20 04:57 PST, Philip Chee
neil: review+
bugspam.Callek: approval‑comm‑aurora+
Details | Diff | Splinter Review
Patch vB1.0 Combined patch for comm-beta carrying forward r=Neil (25.69 KB, patch)
2012-11-23 02:32 PST, Philip Chee
philip.chee: review+
bugspam.Callek: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Philip Chee 2012-11-18 10:02:29 PST
References:
Bug 776208 - Provide API for JavaScript extensions to create native plugins previews for specific mime type
Bug 785190 - Add canActivatePlugin to reshowClickToPlayNotification.
Comment 1 Philip Chee 2012-11-18 10:12:24 PST
Created attachment 682902 [details] [diff] [review]
Patch v1.0 Proposed fix.

EST_PATH=suite/browser/test/browser_pluginplaypreview.js  pymake -C ../objdir-sm/ mochitest-browser-chrome

INFO TEST-START | Shutdown
Browser Chrome Test Summary
        Passed: 23
        Failed: 0
        Todo: 0

TEST_PATH=suite/browser/test/browser_pluginnotification.js pymake -C ../objdir-sm/ mochitest-browser-chrome

INFO TEST-START | Shutdown
Browser Chrome Test Summary
        Passed: 161
        Failed: 3
        Todo: 1

mochitest-browser-chrome failed:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_pluginnotification.js | leaked until shutdown [nsGlobalWindow #56 chrome://mochitests/content/browser/suite/browser/test/plugin_hidden_to_visible.html]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_pluginnotification.js | leaked until shutdown [nsGlobalWindow #55 chrome://mochitests/content/browser/suite/browser/test/plugin_test.html]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_pluginnotification.js | leaked until shutdown [nsGlobalWindow #52 http://127.0.0.1:8888/browser/suite/browser/test/plugin_test.html]
c:\t1\hg\comm-central\suite\build.mk:80:0: command 'errors=`grep "TEST-UNEXPECTED-" mochitest-browser-chrome.log` ; if test "$errors" ; then echo "mochitest-browser-chrome failed:"; echo "$errors";  exit 1; fi' failed, return code 1
c:\t1\hg\objdir-sm\Makefile:52:0: command 'c:/DEV/mozilla-build/python/python.exe c:/t1/hg/comm-central/mozilla/build/pymake/pymake/../make.py -C mozilla mochitest-browser-chrome' failed, return code 2

>        <method name="activateSinglePlugin">
>          <parameter name="aPlugin"/>
>          <body>
>            <![CDATA[
>              var objLoadingContent = aPlugin.QueryInterface(Components.interfaces.nsIObjectLoadingContent);
> -            if (!objLoadingContent.activated)
> +            if (this.canActivatePlugin(objLoadingContent))
>                objLoadingContent.playPlugin();
>          
>              var haveUnplayedPlugins = this.contentWindowUtils.plugins.some(function(plugin) {
>                var hupObjLoadingContent = plugin.QueryInterface(Components.interfaces.nsIObjectLoadingContent);
>                return plugin != aPlugin && !hupObjLoadingContent.activated;
In Firefox the above line was changed to use this.canActivatePlugin() but when I do it here test9c() errors out.
Comment 2 neil@parkwaycc.co.uk 2012-11-18 11:38:01 PST
Comment on attachment 682902 [details] [diff] [review]
Patch v1.0 Proposed fix.

>+              var iframe = previewContent.getElementsByClassName("previewPluginContentFrame")[0];
[I'm surprised there isn't an easier test.]

>+            var playPreviewUri = "data:application/x-moz-playpreview;," + pluginInfo.mimetype;
[Odd, why the ; ?]

>+            previewContent.addEventListener("MozPlayPlugin", function playPluginHandler(aEvent) {
>+              if (!aEvent.isTrusted)
>+                return;
>+
>+              previewContent.removeEventListener("MozPlayPlugin", playPluginHandler, true);
Unfortunately you bound the event listener before adding it, so you can't remove it like this.
However, there is an ingenious way of dealing with this: add the notificationbox itself as the event listener, and put this code in a method named handleEvent. (We used to do this for missing plugins before bug 667201 switched to link click callbacks, and we forgot to remove the nsIDOMEventListener from the implementation...) Obviously the pluginElement and previewContent variables won't be available, but fortunately previewContent will be event.currentTarget, and pluginElement is probably previewContent.ownerDocument.getBindingParent(previewContent).

>           var pluginNeedsActivation = this.contentWindowUtils.plugins.some(function(aPlugin) {
>             var objLoadingContent = aPlugin.QueryInterface(Components.interfaces.nsIObjectLoadingContent);
>-            return !objLoadingContent.activated;
>-          });
>+            return this.canActivatePlugin(objLoadingContent);
>+          }.bind(this));
[I'm tempted to suggest writing the function out here, as you use nsIObjectLoadingContent anyway.]
Comment 3 neil@parkwaycc.co.uk 2012-11-18 13:08:06 PST
(In reply to comment #2)
> (From update of attachment 682902 [details] [diff] [review])
> >+              var iframe = previewContent.getElementsByClassName("previewPluginContentFrame")[0];
> [I'm surprised there isn't an easier test.]
I found that it got changed in bug 776208 due to a review nit. Sigh.

> >+            var playPreviewUri = "data:application/x-moz-playpreview;," + pluginInfo.mimetype;
> [Odd, why the ; ?]
[I guess I should have asked too, but too late to change now I guess...]
Comment 4 Philip Chee 2012-11-19 04:40:49 PST
Created attachment 683065 [details] [diff] [review]
Patch v1.1 use handleEvent

> However, there is an ingenious way of dealing with this: add the notificationbox 
> itself as the event listener, and put this code in a method named handleEvent. 
> (We used to do this for missing plugins before bug 667201 switched to link click 
> callbacks, and we forgot to remove the nsIDOMEventListener from the 
> implementation...) Obviously the pluginElement and previewContent variables 
> won't be available, but fortunately previewContent will be event.currentTarget, 
> and pluginElement is probably
> previewContent.ownerDocument.getBindingParent(previewContent).
Done. I think.

>            var pluginNeedsActivation = this.contentWindowUtils.plugins.some(function(aPlugin) {
>              var objLoadingContent = aPlugin.QueryInterface(Components.interfaces.nsIObjectLoadingContent);
> -            return !objLoadingContent.activated;
> -          });
> +            return this.canActivatePlugin(objLoadingContent);
> +          }, this);
I had a look at the docs for Array.some() on DevMo and realized that instead of .bind(this) I could pass "this" directly to .some()

No change in test results.
Comment 5 neil@parkwaycc.co.uk 2012-11-19 04:42:41 PST
(In reply to Philip Chee from comment #4)
> I had a look at the docs for Array.some() on DevMo and realized that instead
> of .bind(this) I could pass "this" directly to .some()
Good catch!
Comment 6 Philip Chee 2012-11-19 08:09:23 PST
Pushed comm-central changeset 216a373b1d1e
Comment 7 Philip Chee 2012-11-19 08:10:51 PST
Base Bug 776208 landed on Mozilla17. Will ask for a=comm-aurora/comm-beta once this is baked for a bit.
Comment 8 Philip Chee 2012-11-20 04:57:20 PST
Created attachment 683534 [details] [diff] [review]
Part 2 Patch to get test9c working.

>>                return plugin != aPlugin && !hupObjLoadingContent.activated;
> In Firefox the above line was changed to use this.canActivatePlugin() but when
>  I do it here test9c() errors out.
I just realized that I need to pass the right "this" to .some() and then the test9c passes.
Comment 9 Philip Chee 2012-11-20 05:17:46 PST
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/8ae4f568bddd
Comment 10 Philip Chee 2012-11-23 02:10:49 PST
Comment on attachment 683534 [details] [diff] [review]
Part 2 Patch to get test9c working.

[Approval Request Comment]
Regression caused by (bug #): Bug 812898
User impact if declined: Part 1 made it to the merge. Part 2 fixes the remining issue.
Testing completed (on m-c, etc.): tested on c-c (and is a port of a m-c bug)
Risk to taking this patch (and alternatives if risky): none.
String changes made by this patch: None.
Comment 11 Philip Chee 2012-11-23 02:32:02 PST
Created attachment 684644 [details] [diff] [review]
Patch vB1.0 Combined patch for comm-beta carrying forward r=Neil

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Core patch Bug 776208 landed on Mozilla17
User impact if declined: CTP Preview won't work.
Testing completed (on m-c, etc.): tested on c-c.
Risk to taking this patch (and alternatives if risky): low.
String or UUID changes made by this patch: none.
Comment 12 Alex Keybl [:akeybl] 2012-11-26 17:08:24 PST
Comment on attachment 684644 [details] [diff] [review]
Patch vB1.0 Combined patch for comm-beta carrying forward r=Neil

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
String changes made by this patch:
Comment 13 Philip Chee 2012-12-01 01:48:44 PST
Pushed to Part 2 to comm-aurora:
http://hg.mozilla.org/releases/comm-aurora/rev/0b55b98f4774
Pushed combined patch to comm-beta:
http://hg.mozilla.org/releases/comm-beta/rev/3363831600df

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