Closed Bug 920927 Opened 6 years ago Closed 6 years ago

Fix plugin overlay handling

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla27
Tracking Status
firefox26 + verified
firefox27 + verified

People

(Reporter: gfritzsche, Assigned: gfritzsche)

References

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

Bug 790483 broke the visibility handling for the "plugin crashed" overlay as the overlay now defaults to "visibility:hidden" and gPluginHandler.pluginCrashed() was not updated accordingly.
* fix the pluginCrashed handler
* fix a comment regarding that handler
* also move the visibility styling to the correct CSS class (.mainBox styles the element we actually retrieve in browser-plugins)
Attachment #810437 - Flags: review?(jaws)
Looks like bug 790483 will be uplifted to 26 as well, is the plan to just fix this in the uplifted patch there?
I was planning on requesting uplift on both, so they can be tracked properly.
Attachment #810437 - Flags: review?(jaws) → review?(neil)
Comment on attachment 810437 [details] [diff] [review]
Fix pluginCrashed handler & CSS

Seems reasonable from code inspection only.
Will do a full review later if you still need it.
Attachment #810437 - Flags: feedback+
Attachment #810437 - Flags: review?(neil) → review+
Duplicate of this bug: 921152
had to back this out since we had failed Android Reftest like https://tbpl.mozilla.org/php/getParsedLog.php?id=28552587&tree=Mozilla-Inbound
In reftests the plugins are not click-to-play, so we should see a plugin drawing here - but in the screenshot it seems the element is hidden. On desktop the plugin is drawing fine for the test-cases and i'm not sure yet what the difference on Android is for this scenario:
http://dxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/580160-1.html
http://dxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/580160-1-ref.html
Is there something we can do in Shumway to make it show even without this being landed? If not, we have a problem on our hands.
Ok, I've added a temporary workaround that makes the EMBED/OBJECT node always visible. We'll have to remove that once this bug is fixed, but at least we can land now.
So, i completely missed that we only make the overlay visible in the desktop front-end, not on mobile.
That might be it - and i'm surprised that i only triggered one test-failure.
The mobile front-end change was probably missing anyway, but didn't fix the R2:
https://tbpl.mozilla.org/?tree=Try&rev=1ed0c802c981

I'm working on getting a non-failing mobile build so i can take an actual look.
I've at least got a mobile build working now and confirmed that the plugin-missing overlay is not visible and that this seems to be the issue with the R2 failure.
I'm not sure yet though why my PluginHelper.js changes are not fixing it.
Margaret, hg history suggests you as a reviewer?

Per bug 790483 switched the plugin binding contents to being invisible by default and added resize handlers for switching the visibility.
This patch addresses the missing handling for this mobile.

Open question: We miss a test for verifying the overlays visibility, are there other overlay tests i could integrate this in? If not where should this go?
We can't use reftests as the "PluginBindingAttached", which triggers the visibility arrives too late AFAICT.
Attachment #815439 - Flags: review?(margaret.leibovic)
Skip the broken reftest on Android instead of expecting it to fail.

We don't have the test-plugin available on Android and the reftest tests plugin drawing invalidation.
This "fails-if" broke as the plugin overlay is now initially invisible and only made visible after the "PluginBindingAttached" event. A proper test for overlays would have to wait for that, but this isn't testing overlays so i just disabled it.
Attachment #815442 - Flags: review?(jmaher)
Comment on attachment 815442 [details] [diff] [review]
Skip reftest that's broken on Android

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

::: layout/reftests/bugs/reftest.list
@@ +1561,5 @@
>  == 579349-1.html 579349-1-ref.html
>  == 579655-1.html 579655-1-ref.html
>  skip-if(B2G) fails-if(Android) == 579808-1.html 579808-1-ref.html
>  skip-if(B2G) fails-if(Android) random-if(layersGPUAccelerated) == 579985-1.html 579985-1-ref.html # bug 623452 for WinXP; this bug was only for a regression in BasicLayers anyway
> +skip-if(B2G) skip-if(Android) == 580160-1.html 580160-1-ref.html

Please put this bug number in here as a comment at the end of this line.
Attachment #815442 - Flags: review?(jmaher) → review+
Comment on attachment 815439 [details] [diff] [review]
Fix plugin overlay handling on mobile

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

This seems reasonable to me. I wonder if we should decide to show the doorhanger if the overlay becomes hidden?

I'd like David Keeler to also take a look at this, since he's worked on this more recently than me.
Attachment #815439 - Flags: review?(margaret.leibovic)
Attachment #815439 - Flags: review+
Attachment #815439 - Flags: feedback?(dkeeler)
(In reply to :Margaret Leibovic from comment #18)
> This seems reasonable to me. I wonder if we should decide to show the
> doorhanger if the overlay becomes hidden?

Right, that sounds sensible - i added this as i don't see a big downside to this.
Attachment #815439 - Attachment is obsolete: true
Attachment #815439 - Flags: feedback?(dkeeler)
Attachment #816645 - Flags: feedback?(dkeeler)
Comment on attachment 816645 [details] [diff] [review]
Fix plugin overlay handling on mobile

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

LGTM. My only concern is pulling variables into the callback enclosures, but since that already happens in the PluginPlayPreview handler, I guess it's not a big deal?
Attachment #816645 - Flags: feedback?(dkeeler) → feedback+
(In reply to David Keeler (:keeler) from comment #20)
> LGTM. My only concern is pulling variables into the callback enclosures, but
> since that already happens in the PluginPlayPreview handler, I guess it's
> not a big deal?

Margaret, do you have concerns there or do you think this patch is good to go for now?
Flags: needinfo?(margaret.leibovic)
(In reply to Georg Fritzsche [:gfritzsche] from comment #21)
> (In reply to David Keeler (:keeler) from comment #20)
> > LGTM. My only concern is pulling variables into the callback enclosures, but
> > since that already happens in the PluginPlayPreview handler, I guess it's
> > not a big deal?
> 
> Margaret, do you have concerns there or do you think this patch is good to
> go for now?

I think this should be fine, since the overlay and the tab for the plugin shouldn't be changing.
Flags: needinfo?(margaret.leibovic)
Keywords: verifyme
Comment on attachment 810437 [details] [diff] [review]
Fix pluginCrashed handler & CSS

[Approval Request Comment]
Bug caused by (feature/regressing bug #): click-to-play redesign & bug 790483
User impact if declined: breakage from bug 790483, required for this.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Low, worst-case is affecting visibility of plugin click-to-play overlay.
String or IDL/UUID changes made by this patch: None.

Note: this is required for bug 790483.
Attachment #810437 - Flags: approval-mozilla-aurora?
Comment on attachment 816645 [details] [diff] [review]
Fix plugin overlay handling on mobile

[Approval Request Comment]
Bug caused by (feature/regressing bug #): click-to-play redesign & bug 790483
User impact if declined: mobile breakage from bug 790483, required for this.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Low, worst-case is affecting visibility of plugin click-to-play overlay.
String or IDL/UUID changes made by this patch: None.

Note: this is required for bug 790483.
Attachment #816645 - Flags: approval-mozilla-aurora?
Comment on attachment 815442 [details] [diff] [review]
Skip reftest that's broken on Android

[Approval Request Comment]
Test-fixup for the above.
Attachment #815442 - Flags: approval-mozilla-aurora?
In bug 926956, the click-to-play overlay behavior was already verified for Android.
Paul, can you verify the plugin click-to-play, missing and crashed overlays behaving correctly today?
Flags: needinfo?(paul.silaghi)
tracking-fennec: --- → ?
Duplicate of this bug: 926956
(In reply to Georg Fritzsche [:gfritzsche] from comment #29)
> Paul, can you verify the plugin click-to-play, missing and crashed overlays
> behaving correctly today?
Missing and crashed overlays are properly displayed.
Verified fixed FF 27.0a1 (2013-10-18) Win 7, Ubuntu 12.04, Mac OS X 10.8.4.
The only thing that's not working is the overlay's X button, but that's not a regression from bug 790483.
Flags: needinfo?(paul.silaghi)
(In reply to Paul Silaghi, QA [:pauly] from comment #31)
> The only thing that's not working is the overlay's X button, but that's not
> a regression from bug 790483.

Right, that's bug 921730.
Attachment #810437 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #815442 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #816645 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-fennec: ? → ---
(In reply to Paul Silaghi, QA [:pauly] from comment #31)
> Missing and crashed overlays are properly displayed.
> Verified fixed FF 27.0a1 (2013-10-18) Win 7, Ubuntu 12.04, Mac OS X 10.8.4.
Verified fixed 26.0a2 (2013-10-20)
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.