920927 - Fix plugin overlay handling

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Georg Fritzsche [:gfritzsche]]

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.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix pluginCrashed handler & CSS

* 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)

Comment 2

[Georg Fritzsche [:gfritzsche]]

https://tbpl.mozilla.org/?tree=Try&rev=1bb1e50bd013

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Looks like bug 790483 will be uplifted to 26 as well, is the plan to just fix this in the uplifted patch there?

Comment 4

[Georg Fritzsche [:gfritzsche]]

I was planning on requesting uplift on both, so they can be tracked properly.

Comment 5

[[e-mail]]

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.

Comment 7

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ebf2657b295

Comment 8

[Carsten Book [:Tomcat]]

had to back this out since we had failed Android Reftest like https://tbpl.mozilla.org/php/getParsedLog.php?id=28552587&tree=Mozilla-Inbound

Comment 9

[Georg Fritzsche [:gfritzsche]]

Attachment: unexpected pass screenshot

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

Comment 10

[Till Schneidereit [:till]]

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.

Comment 11

[Till Schneidereit [:till]]

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.

Comment 12

[Georg Fritzsche [:gfritzsche]]

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.

Comment 13

[Georg Fritzsche [:gfritzsche]]

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.

Comment 14

[Georg Fritzsche [:gfritzsche]]

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.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix plugin overlay handling on mobile

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.

Comment 16

[Georg Fritzsche [:gfritzsche]]

Attachment: Skip reftest that's broken on Android

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.

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 18

[:Margaret Leibovic]

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.

Comment 19

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix plugin overlay handling on mobile

(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.

Comment 20

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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?

Comment 21

[Georg Fritzsche [:gfritzsche]]

(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?

Comment 22

[: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.

Comment 23

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7759d30a7ae5
https://hg.mozilla.org/integration/mozilla-inbound/rev/99c716714856
https://hg.mozilla.org/integration/mozilla-inbound/rev/f585d3d5d879

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7759d30a7ae5
https://hg.mozilla.org/mozilla-central/rev/99c716714856
https://hg.mozilla.org/mozilla-central/rev/f585d3d5d879

Comment 25

[Georg Fritzsche [:gfritzsche]]

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.

Comment 26

[Georg Fritzsche [:gfritzsche]]

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.

Comment 27

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 815442 [details] [diff] [review]
Skip reftest that's broken on Android

[Approval Request Comment]
Test-fixup for the above.

Comment 28

[Georg Fritzsche [:gfritzsche]]

In bug 926956, the click-to-play overlay behavior was already verified for Android.

Comment 29

[Georg Fritzsche [:gfritzsche]]

Paul, can you verify the plugin click-to-play, missing and crashed overlays behaving correctly today?

Comment 31

[Paul Silaghi, QA [:pauly]]

(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.

Comment 32

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a62b94423cac
https://hg.mozilla.org/releases/mozilla-aurora/rev/8d79bd551eb3
https://hg.mozilla.org/releases/mozilla-aurora/rev/eb6bfb3ab0d3

Comment 34

[Paul Silaghi, QA [:pauly]]

(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)