Closed Bug 812898 Opened 12 years ago Closed 12 years ago

Implement plugin preview overlay from Bug 776208

Categories

(SeaMonkey :: General, defect)

Product:
SeaMonkey
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

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

Status:
RESOLVED FIXED
Milestone:
seamonkey2.16
Tracking Flags:
Tracking Status
seamonkey2.14 --- wontfix
seamonkey2.15 --- fixed
seamonkey2.16 --- fixed
seamonkey2.17 --- fixed

People

(Reporter: philip.chee, Assigned: philip.chee)

Details

Attachments

(3 files, 1 obsolete file)

Patch v1.1 use handleEvent
25.67 KB, patch
neil
: review+
Details | Diff | Splinter Review
Part 2 Patch to get test9c working.
1.39 KB, patch
neil
: review+
Callek
: approval-comm-aurora+
Details | Diff | Splinter Review
Patch vB1.0 Combined patch for comm-beta carrying forward r=Neil
25.69 KB, patch
philip.chee
: review+
Callek
: approval-comm-beta+
Details | Diff | Splinter Review 
References:
Bug 776208 - Provide API for JavaScript extensions to create native plugins previews for specific mime type
Bug 785190 - Add canActivatePlugin to reshowClickToPlayNotification.
Attached patch Patch v1.0 Proposed fix. (obsolete) — — Splinter Review 
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 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.]
(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...]
> 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)
(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!
Attachment #683065 - Flags: review?(neil) → review+
Pushed comm-central changeset 216a373b1d1e
Target Milestone: --- → seamonkey2.16
Base Bug 776208 landed on Mozilla17. Will ask for a=comm-aurora/comm-beta once this is baked for a bit.
>>                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)
Attachment #683534 - Flags: review?(neil) → review+
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/8ae4f568bddd
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?
[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?
Attachment #683534 - Flags: approval-comm-aurora? → approval-comm-aurora+
Attachment #684644 - Flags: approval-comm-beta? → approval-comm-beta+
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
Closed: 12 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.

Attachment

General

Creator:
Created:
Updated:
Size: