Last Comment Bug 813963 - Port |Bug 800018 - Click-to-play is broken on various websites like cnn.com or latimes.com (2nd round)| to SeaMonkey
: Port |Bug 800018 - Click-to-play is broken on various websites like cnn.com o...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: seamonkey2.17
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-21 06:07 PST by Frank Wein [:mcsmurf]
Modified: 2015-07-12 12:04 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
fixed
fixed


Attachments
Possible patch (12.07 KB, patch)
2012-11-23 14:00 PST, neil@parkwaycc.co.uk
bugzilla: feedback-
Details | Diff | Splinter Review
Fixed patch (12.09 KB, patch)
2012-11-24 08:41 PST, neil@parkwaycc.co.uk
philip.chee: review+
bugzilla: feedback+
iann_bugzilla: feedback+
philip.chee: feedback+
philip.chee: approval‑comm‑aurora+
philip.chee: approval‑comm‑beta+
Details | Diff | Splinter Review
Patch with updated tests (14.64 KB, patch)
2012-11-25 07:04 PST, Frank Wein [:mcsmurf]
no flags Details | Diff | Splinter Review
Updated tests (6.51 KB, patch)
2012-11-29 08:52 PST, Frank Wein [:mcsmurf]
bugzilla: review+
jh: approval‑comm‑aurora+
jh: approval‑comm‑beta+
Details | Diff | Splinter Review
Cleanup (5.85 KB, patch)
2012-11-29 16:51 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Cleanup (8.25 KB, patch)
2012-12-01 12:48 PST, neil@parkwaycc.co.uk
philip.chee: review+
Details | Diff | Splinter Review

Description Frank Wein [:mcsmurf] 2012-11-21 06:07:36 PST
The changes from Bug 800018 need to be ported to SeaMonkey as currently tests inside browser_pluginnotification.js fail (this is probably also visible in the SeaMonkey UI itself, did not test). Especially those parts need to be ported:
http://hg.mozilla.org/mozilla-central/rev/c8729d9d2d97 the SeaMonkey code that's similar to browser-plugins.js lives in suite/common/bindings/notification.xml
http://hg.mozilla.org/mozilla-central/rev/cd8533b0fae7 maybe the test change needs to be ported, too. I did not check if SeaMonkey also included those tests (if not, it would be a good idea to port the whole test).
Comment 1 neil@parkwaycc.co.uk 2012-11-23 14:00:27 PST
Created attachment 684798 [details] [diff] [review]
Possible patch

As yet completely untested, just would like some eyes on this thanks.
Comment 2 Frank Wein [:mcsmurf] 2012-11-24 01:11:33 PST
Comment on attachment 684798 [details] [diff] [review]
Possible patch

In "normal" plugin mode (all plugins activated by default), plugins work. But when activating click-to-play, one cannot activate click-to-play by clicking in the placeholder UI. I used http://plugindoc.mozdev.org/testpages/pdf.html to test this. The Flash player test on http://www.redcross.ca/article.asp?id=22823&tid=001 does not work either.
Comment 3 neil@parkwaycc.co.uk 2012-11-24 08:40:17 PST
Heh, so as it happens there were two bugs:
1. We don't actually get untrusted events by default, we have to opt-in. In fact all of the comments about expecting untrusted events were bogus, as noted in bug 558517, which is the real reason that bug 465771 ever worked. Oops.
2. There was a silly typo (missing : after case label).
Comment 4 neil@parkwaycc.co.uk 2012-11-24 08:41:34 PST
Created attachment 684861 [details] [diff] [review]
Fixed patch
Comment 5 Frank Wein [:mcsmurf] 2012-11-25 06:58:42 PST
Comment on attachment 684861 [details] [diff] [review]
Fixed patch

Plugins work fine, click-to-play plugins work fine. Disabling/enabling plugins also does the right thing. "Plugin crashed" UI also works.
Regarding the automated mochitest-browser-chrome: After porting the recent test changes from browser_pluginnotification.js and browser_pluginplaypreview.js to the respective SeaMonkey tests, the automated tests pass.
Comment 6 Frank Wein [:mcsmurf] 2012-11-25 07:04:07 PST
Created attachment 684954 [details] [diff] [review]
Patch with updated tests

This is the original patch with fixed tests.
Comment 7 neil@parkwaycc.co.uk 2012-11-25 08:12:11 PST
Comment on attachment 684954 [details] [diff] [review]
Patch with updated tests

>+function handleBindingAttached(evt) {
>+  evt.target instanceof Ci.nsIObjectLoadingContent;
>+  if (evt.target.pluginFallbackType == Ci.nsIObjectLoadingContent.PLUGIN_CLICK_TO_PLAY)
>+    gClickToPlayPluginActualEvents++;
> }
This version is wrong, see below.

>+function handleBindingAttached(evt) {
>+  if (evt.target instanceof Ci.nsIObjectLoadingContent &&
>+      evt.target.pluginFallbackType == Ci.nsIObjectLoadingContent.PLUGIN_PLAY_PREVIEW)
>+    gPlayPreviewPluginActualEvents++;
> }
This version is correct. r=me on the tests fixes with that fixed.
Comment 8 Philip Chee 2012-11-26 01:18:56 PST
Comment on attachment 684861 [details] [diff] [review]
Fixed patch

Code looks ok. Did some light testing on several flash sites. No problems detected.

f=me
Comment 9 Jens Hatlak (:InvisibleSmiley) 2012-11-26 13:38:46 PST
Setting tracking flags according to base bug which landed on the Gecko 18 branch (SM 15, currently in beta).

I used http://www.adobe.com/shockwave/welcome/ (since I already have plugins for Flash and PDF active and knew about the availability of the Shockwave player) with current trunk nightlies of SM and FF on Win 7. FF showed an update link inside the CTP area and a notification bar (clicking either found the Shockwave player), SM just the CTP area without a link. With the patch (attachment 684954 [details] [diff] [review]) applied, SM's behavior matches FF's again.

Thanks for taking care of this! :-)
Comment 10 neil@parkwaycc.co.uk 2012-11-27 09:36:09 PST
Comment on attachment 684861 [details] [diff] [review]
Fixed patch

Would anyone care to upgrade their feedback into an r+?
(Including IanN if he gets there first!)
Comment 11 Frank Wein [:mcsmurf] 2012-11-29 08:52:24 PST
Created attachment 686626 [details] [diff] [review]
Updated tests
Comment 12 Frank Wein [:mcsmurf] 2012-11-29 08:53:21 PST
Comment on attachment 686626 [details] [diff] [review]
Updated tests

Pushed: https://hg.mozilla.org/comm-central/rev/b70d8f7a7b2f
Comment 13 neil@parkwaycc.co.uk 2012-11-29 13:28:15 PST
Pushed comm-central changeset 59fe5448d2ea.
Comment 14 neil@parkwaycc.co.uk 2012-11-29 16:51:27 PST
Created attachment 686869 [details] [diff] [review]
Cleanup

This is stuff that shouldn't be needed any more, but I left out originally because attachment 684861 [details] [diff] [review] needs to land on branches.
Comment 15 Philip Chee 2012-11-30 00:35:05 PST
Comment on attachment 686869 [details] [diff] [review]
Cleanup

>        <method name="pluginUnavailable">
> -        <parameter name="aEvent"/>
> +        <parameter name="aPlugin"/>
I think this is also called from "npapi-carbon-event-model-failure"

>        <method name="pluginUnavailable">
> ...
> -            // Force a style flush, so that we ensure our binding is attached.
> -            plugin.clientTop;
We don't need this any more?

>        <method name="handlePlayPreviewEvent">
> .... 
> -
> -              // Force a style flush, so that we ensure our binding is attached.
> -              pluginElement.clientTop;
We don't need this any more?
Comment 16 neil@parkwaycc.co.uk 2012-12-01 12:48:01 PST
Created attachment 687438 [details] [diff] [review]
Cleanup

Strangely enough we don't need to force the plugin binding to attach for the PluginBindingAttached handler and its dependent code. However as part of the carbon event model failure fixes I had to reintroduce the code as that event requires us to force the binding to attach.
Comment 17 Philip Chee 2012-12-02 09:03:11 PST
Comment on attachment 687438 [details] [diff] [review]
Cleanup

Looks good. r=me
Comment 18 Frank Wein [:mcsmurf] 2012-12-07 06:38:01 PST
Neil: Will you request approval for this patch for branches?
Comment 19 neil@parkwaycc.co.uk 2012-12-07 16:37:49 PST
Comment on attachment 684861 [details] [diff] [review]
Fixed patch

[Approval Request Comment]
Regression caused by (bug #): 800018
User impact if declined: can't interact with problem plugins
Testing completed (on m-c, etc.): baked on c-c for a week or so
Risk to taking this patch (and alternatives if risky): plugins only
String changes made by this patch: none
Comment 20 Frank Wein [:mcsmurf] 2012-12-12 01:56:24 PST
Neil: Will you land the patch on comm-beta and comm-aurora? :)
Comment 21 Frank Wein [:mcsmurf] 2012-12-13 06:01:28 PST
Comment on attachment 686626 [details] [diff] [review]
Updated tests

[Approval Request Comment]
Regression caused by (bug #): Bug 800018
User impact if declined: More difficult for developers to spot test failures, the original patch already got approval for -aurora and -beta
Testing completed (on m-c, etc.): Fixed tests pass on m-c
Risk to taking this patch (and alternatives if risky): none, test-only patch
String changes made by this patch: none
Comment 24 Nick Heart 2015-07-12 07:36:43 PDT
I also tried to use plugin http://plugindoc.mozdev.org/testpages/pdf.html on the site http://www.heart2heartcpr.com/mississauga/
but not get benefited. Need help from experts.
Comment 25 Frank Wein [:mcsmurf] 2015-07-12 10:07:17 PDT
Nick: Can you file a new bug on this if it's broken in a recent SeaMonkey release? This bug here is already marked as FIXED.
Comment 26 Nick Heart 2015-07-12 12:04:09 PDT
(In reply to Frank Wein [:mcsmurf] from comment #25)
> Nick: Can you file a new bug on this if it's broken in a recent SeaMonkey
> release? This bug here is already marked as FIXED.

Okay thanks a lot! I've got it!

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