Last Comment Bug 752228 - No showing click-to-play doorhanger icon on some pages
: No showing click-to-play doorhanger icon on some pages
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Jared Wein [:jaws] (please needinfo? me)
:
:
Mentors:
Depends on:
Blocks: 711552 click-to-play
  Show dependency treegraph
 
Reported: 2012-05-05 08:29 PDT by Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
Modified: 2012-12-11 02:28 PST (History)
7 users (show)
jaws: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for bug (1.49 KB, patch)
2012-05-08 16:13 PDT, Jared Wein [:jaws] (please needinfo? me)
felipc: review+
Details | Diff | Splinter Review

Description Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-05-05 08:29:02 PDT
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.
Comment 1 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-05-05 09:14:43 PDT
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.
Comment 2 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-05-05 10:03:27 PDT
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
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2012-05-05 10:35:15 PDT
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.
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2012-05-08 16:13:13 PDT
Created attachment 622190 [details] [diff] [review]
Patch for bug

Added a null-check for the overlay.
Comment 5 :Felipe Gomes (needinfo me!) 2012-05-08 18:42:34 PDT
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?
Comment 6 Jared Wein [:jaws] (please needinfo? me) 2012-05-08 18:58:43 PDT
(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.
Comment 7 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-05-08 19:30:13 PDT
(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.
Comment 8 :Felipe Gomes (needinfo me!) 2012-05-09 16:53:18 PDT
(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 9 :Felipe Gomes (needinfo me!) 2012-05-09 16:54:00 PDT
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
Comment 10 Jared Wein [:jaws] (please needinfo? me) 2012-05-12 15:57:37 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f77082549e0e
Comment 11 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-05-13 00:00:49 PDT
(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.
Comment 12 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-05-13 05:14:21 PDT
(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.
Comment 13 Matt Brubeck (:mbrubeck) 2012-05-13 17:41:50 PDT
https://hg.mozilla.org/mozilla-central/rev/f77082549e0e
Comment 14 Paul Silaghi, QA [:pauly] 2012-06-21 05:13:16 PDT
(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.
Comment 15 Paul Silaghi, QA [:pauly] 2012-12-11 02:28:02 PST
CTP icon is displayed and the video plays after clicking on it.
Verified fixed on Nightly 20.0a1 (2012-12-10) Win 7 x64.

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