The default bug view has changed. See this FAQ.

Port |Bug 800018 - Click-to-play is broken on various websites like cnn.com or latimes.com (2nd round)| to SeaMonkey

RESOLVED FIXED in seamonkey2.17

Status

SeaMonkey
General
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: mcsmurf, Assigned: neil@parkwaycc.co.uk)

Tracking

Trunk
seamonkey2.17
x86_64
Windows 7

SeaMonkey Tracking Flags

(seamonkey2.15+ fixed, seamonkey2.16 fixed, seamonkey2.17 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

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

Comment 1

4 years ago
Created attachment 684798 [details] [diff] [review]
Possible patch

As yet completely untested, just would like some eyes on this thanks.
Attachment #684798 - Flags: feedback?(philip.chee)
Attachment #684798 - Flags: feedback?(iann_bugzilla)
Attachment #684798 - Flags: feedback?(bugzilla)
(Reporter)

Comment 2

4 years ago
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.
Attachment #684798 - Flags: feedback?(bugzilla) → feedback-
(Assignee)

Comment 3

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

Comment 4

4 years ago
Created attachment 684861 [details] [diff] [review]
Fixed patch
Attachment #684798 - Attachment is obsolete: true
Attachment #684798 - Flags: feedback?(philip.chee)
Attachment #684798 - Flags: feedback?(iann_bugzilla)
Attachment #684861 - Flags: feedback?(philip.chee)
Attachment #684861 - Flags: feedback?(iann_bugzilla)
Attachment #684861 - Flags: feedback?(bugzilla)
(Reporter)

Comment 5

4 years ago
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.
Attachment #684861 - Flags: feedback?(bugzilla) → feedback+
(Reporter)

Comment 6

4 years ago
Created attachment 684954 [details] [diff] [review]
Patch with updated tests

This is the original patch with fixed tests.
(Assignee)

Comment 7

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

4 years ago
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
Attachment #684861 - Flags: feedback?(philip.chee) → feedback+
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! :-)
status-seamonkey2.15: --- → affected
status-seamonkey2.16: --- → affected
status-seamonkey2.17: --- → affected
tracking-seamonkey2.15: --- → +
(Assignee)

Comment 10

4 years ago
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!)
Attachment #684861 - Flags: review?

Updated

4 years ago
Attachment #684861 - Flags: review? → review+
(Reporter)

Comment 11

4 years ago
Created attachment 686626 [details] [diff] [review]
Updated tests
Attachment #684954 - Attachment is obsolete: true
Attachment #686626 - Flags: review+
(Reporter)

Comment 12

4 years ago
Comment on attachment 686626 [details] [diff] [review]
Updated tests

Pushed: https://hg.mozilla.org/comm-central/rev/b70d8f7a7b2f
(Assignee)

Comment 13

4 years ago
Pushed comm-central changeset 59fe5448d2ea.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

4 years ago
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.
Assignee: nobody → neil
Attachment #686869 - Flags: review?(philip.chee)

Comment 15

4 years ago
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?
Attachment #686869 - Flags: review?(philip.chee)
(Assignee)

Comment 16

4 years ago
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.
Attachment #686869 - Attachment is obsolete: true
Attachment #687438 - Flags: review?(philip.chee)

Updated

4 years ago
Attachment #684861 - Flags: feedback?(iann_bugzilla) → feedback+

Comment 17

4 years ago
Comment on attachment 687438 [details] [diff] [review]
Cleanup

Looks good. r=me
Attachment #687438 - Flags: review?(philip.chee) → review+
(Reporter)

Updated

4 years ago
Target Milestone: --- → seamonkey2.17
(Reporter)

Comment 18

4 years ago
Neil: Will you request approval for this patch for branches?
(Assignee)

Comment 19

4 years ago
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
Attachment #684861 - Flags: approval-comm-beta?
Attachment #684861 - Flags: approval-comm-aurora?

Updated

4 years ago
Attachment #684861 - Flags: approval-comm-beta?
Attachment #684861 - Flags: approval-comm-beta+
Attachment #684861 - Flags: approval-comm-aurora?
Attachment #684861 - Flags: approval-comm-aurora+
(Reporter)

Comment 20

4 years ago
Neil: Will you land the patch on comm-beta and comm-aurora? :)
(Reporter)

Comment 21

4 years ago
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
Attachment #686626 - Flags: approval-comm-beta?
Attachment #686626 - Flags: approval-comm-aurora?
Attachment #686626 - Flags: approval-comm-beta?
Attachment #686626 - Flags: approval-comm-beta+
Attachment #686626 - Flags: approval-comm-aurora?
Attachment #686626 - Flags: approval-comm-aurora+
(Reporter)

Comment 22

4 years ago
Attachment 684861 [details] [diff] pushed to comm-aurora: http://hg.mozilla.org/releases/comm-aurora/rev/76227e306cb4
Attachment 686626 [details] [diff] pushed to comm-aurora: http://hg.mozilla.org/releases/comm-aurora/rev/5f2853b46959
status-seamonkey2.16: affected → fixed
status-seamonkey2.17: affected → fixed
(Reporter)

Comment 23

4 years ago
Attachment 684861 [details] [diff] pushed to comm-beta (by Callek): http://hg.mozilla.org/releases/comm-beta/rev/b550afb021fe
Attachment 686626 [details] [diff] pushed to comm-beta (by Callek): http://hg.mozilla.org/releases/comm-beta/rev/c1eb13cdcbc6
status-seamonkey2.15: affected → fixed

Comment 24

2 years ago
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.
(Reporter)

Comment 25

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

2 years ago
(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!
You need to log in before you can comment on or make changes to this bug.