Implement plugin preview overlay from Bug 776208

RESOLVED FIXED in seamonkey2.16

Status

SeaMonkey
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

Trunk
seamonkey2.16

SeaMonkey Tracking Flags

(seamonkey2.14 wontfix, seamonkey2.15 fixed, seamonkey2.16 fixed, seamonkey2.17 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
References:
Bug 776208 - Provide API for JavaScript extensions to create native plugins previews for specific mime type
Bug 785190 - Add canActivatePlugin to reshowClickToPlayNotification.
(Assignee)

Comment 1

5 years ago
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.
Attachment #682902 - Flags: review?(neil)

Comment 2

5 years ago
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

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

Comment 4

5 years ago
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.
Attachment #682902 - Attachment is obsolete: true
Attachment #682902 - Flags: review?(neil)
Attachment #683065 - Flags: review?(neil)

Comment 5

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

Updated

5 years ago
Attachment #683065 - Flags: review?(neil) → review+
(Assignee)

Comment 6

5 years ago
Pushed comm-central changeset 216a373b1d1e
Target Milestone: --- → seamonkey2.16
(Assignee)

Comment 7

5 years ago
Base Bug 776208 landed on Mozilla17. Will ask for a=comm-aurora/comm-beta once this is baked for a bit.
(Assignee)

Comment 8

5 years ago
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.
Attachment #683534 - Flags: review?(neil)

Updated

5 years ago
Attachment #683534 - Flags: review?(neil) → review+
(Assignee)

Comment 9

5 years ago
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/8ae4f568bddd
(Assignee)

Comment 10

5 years ago
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.
Attachment #683534 - Flags: approval-comm-aurora?
(Assignee)

Comment 11

5 years ago
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.
Attachment #684644 - Flags: review+
Attachment #684644 - Flags: approval-mozilla-beta?
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:
Attachment #684644 - Flags: approval-mozilla-beta? → approval-comm-beta?

Updated

5 years ago
Attachment #683534 - Flags: approval-comm-aurora? → approval-comm-aurora+

Updated

5 years ago
Attachment #684644 - Flags: approval-comm-beta? → approval-comm-beta+
(Assignee)

Comment 13

5 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-seamonkey2.14: --- → wontfix
status-seamonkey2.15: --- → fixed
status-seamonkey2.16: --- → fixed
status-seamonkey2.17: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.