Closed Bug 813963 Opened 7 years ago Closed 7 years ago

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

Categories

(SeaMonkey :: General, defect)

x86_64
Windows 7
defect
Not set

Tracking

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

RESOLVED FIXED
seamonkey2.17
Tracking Status
seamonkey2.15 + fixed
seamonkey2.16 --- fixed
seamonkey2.17 --- fixed

People

(Reporter: mcsmurf, Assigned: neil)

Details

Attachments

(3 files, 3 obsolete files)

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).
Attached patch Possible patch (obsolete) — Splinter Review
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)
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-
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).
Attached patch Fixed patchSplinter Review
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)
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+
Attached patch Patch with updated tests (obsolete) — Splinter Review
This is the original patch with fixed tests.
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 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! :-)
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?
Attachment #684861 - Flags: review? → review+
Attached patch Updated testsSplinter Review
Attachment #684954 - Attachment is obsolete: true
Attachment #686626 - Flags: review+
Pushed comm-central changeset 59fe5448d2ea.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Attached patch Cleanup (obsolete) — Splinter Review
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 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)
Attached patch CleanupSplinter Review
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)
Attachment #684861 - Flags: feedback?(iann_bugzilla) → feedback+
Comment on attachment 687438 [details] [diff] [review]
Cleanup

Looks good. r=me
Attachment #687438 - Flags: review?(philip.chee) → review+
Target Milestone: --- → seamonkey2.17
Neil: Will you request approval for this patch for branches?
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?
Attachment #684861 - Flags: approval-comm-beta?
Attachment #684861 - Flags: approval-comm-beta+
Attachment #684861 - Flags: approval-comm-aurora?
Attachment #684861 - Flags: approval-comm-aurora+
Neil: Will you land the patch on comm-beta and comm-aurora? :)
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+
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.
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.
(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.