Closed Bug 752228 Opened 12 years ago Closed 12 years ago

No showing click-to-play doorhanger icon on some pages

Categories

(Core Graveyard :: Plug-ins, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla15

People

(Reporter: tetsuharu, Assigned: jaws)

References

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1
Build ID: 20120505030510

Steps to reproduce:

[Environment]
http://hg.mozilla.org/mozilla-central/rev/0a48e6561534

1. enable click to play
2. visit this page: http://www.slideshare.net/haricot/how-browser-engines-work/
3. Flip a slide to 60.


Actual results:

The page loads an embedded YouTube video with flash plugin.
But browser does not showing click-to-play doorhanger icon.
Blocks: 711552
OS: Windows 7 → All
Hardware: x86_64 → All
Happen the following error When occurring this bug:
Timestamp: 2012/05/06 1:07:45
Error: TypeError: overlay is null
Source File: chrome://browser/content/browser.js
Line: 10711

And I found a strange thing. This bug does not occur when I set breakpoints to the above point with Venkman & stop with breakpoint. This bug may be related to the timing of structuring DOM.
Stack trace from Venkman:
#0: function PH_handleClickToPlayEvent(aPlugin=function:) in <chrome://browser/content/browser.js> line 10710
#1: function anonymous() in <chrome://browser/content/browser.js> line 10608
There are cases where the overlay can be null (such as if the element is display:none when the element is added to the DOM). Mobile checks for this and we're not, so we need to add a null check in PH_handlieClickToPlayEvent.

Thanks saneyuki_s for finding this.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attached patch Patch for bugSplinter Review
Added a null-check for the overlay.
Attachment #622190 - Flags: review?(felipc)
This will not fix the problem that clicking the overlay won't work after it's made visible, right? did mobile come up with any solution for that?
(In reply to Felipe Gomes (:felipe) from comment #5)
> This will not fix the problem that clicking the overlay won't work after
> it's made visible, right? did mobile come up with any solution for that?

AFAIK, they didn't come up with a solution for this. See bug 741130 for the mobile version of this case.
(In reply to Felipe Gomes (:felipe) from comment #5)
> This will not fix the problem that clicking the overlay won't work after
> it's made visible, right? did mobile come up with any solution for that?

If we fix this bug by the approach of attachment 622190 [details] [diff] [review], we may need to add the context menu to play & permit plugins & permit on Click-to-play placeholder.
(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #7)
> If we fix this bug by the approach of attachment 622190 [details] [diff] [review]
> [review], we may need to add the context menu to play & permit plugins &
> permit on Click-to-play placeholder.

Why do we need that, Ohzeki? you mean because of bug 741130? We are planning on dealing with that on a separate bug
Comment on attachment 622190 [details] [diff] [review]
Patch for bug

Review of attachment 622190 [details] [diff] [review]:
-----------------------------------------------------------------

So this bug is just about not throwing an exception here that prevents the function to continue and the doorhanger from being shown. We should investigate on another bug how to fix bug 741130, probably by having the click handler directly on the XBL as we talked in person

::: browser/base/content/browser.js
@@ +7277,1 @@
>  

please add braces around the `if (overlay)` because this spans more than 1 line
Attachment #622190 - Flags: review?(felipc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f77082549e0e
Flags: in-testsuite-
Target Milestone: --- → mozilla15
(In reply to Felipe Gomes (:felipe) from comment #8)
> Why do we need that, Ohzeki? you mean because of bug 741130? We are planning
> on dealing with that on a separate bug

The patch of this bug cannot add the event listener to a placeholder when |overlay| is undefined. In such case, user cannot only play the plugin which is in such case. (This related to bug 742753.)
I'll talk about this at bug 741130 or another new bug.
(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #2)
> Stack trace from Venkman:
> #0: function PH_handleClickToPlayEvent(aPlugin=function:) in
> <chrome://browser/content/browser.js> line 10710
> #1: function anonymous() in <chrome://browser/content/browser.js> line 10608

I account for confirmation.

I confirm with Venkman that aPlugin seems like undefined.
I'm sorry if I could not come across to you via this stack trace.
https://hg.mozilla.org/mozilla-central/rev/f77082549e0e
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #0)
> User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0
> Firefox/15.0a1
> Build ID: 20120505030510
> 
> Steps to reproduce:
> 
> [Environment]
> http://hg.mozilla.org/mozilla-central/rev/0a48e6561534
> 
> 1. enable click to play
> 2. visit this page:
> http://www.slideshare.net/haricot/how-browser-engines-work/
> 3. Flip a slide to 60.
> 
> 
> Actual results:
> 
> The page loads an embedded YouTube video with flash plugin.
> But browser does not showing click-to-play doorhanger icon.

Now the CtP icon is displayed, but the video won't play after clicking on it. I understand that bug 741130 will fix this ? Tested on Nightly 20-06-2012.
CTP icon is displayed and the video plays after clicking on it.
Verified fixed on Nightly 20.0a1 (2012-12-10) Win 7 x64.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: