707439 - Flash plugin wrong location on zoom & pan

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[David Ascher (:davida)]

STRs:

open nytimes.com
notice the flash video player usually in top 3rd of page
pinch-zoom to roughly get the video right size
touch-pan to move it to the center of the screen

expected: pan keeps flash plugin in its location relative to the CSS text
actual: flash plugin is offset, indicating what feels like a sign error in layout code.

Comment 3

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Also occurs with : http://hybridmind.com/tutorials/simple-as3-viewport-tutorial/

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix Flash plugin positioning

Comment 5

[Chris Lord [:cwiiis]]

Comment on attachment 589368 [details] [diff] [review]
Fix Flash plugin positioning

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

I think the below needs to be addressed before r+.

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +1683,5 @@
>  
> +  nsAutoString metadata;
> +  nsCOMPtr<nsIAndroidDrawMetadataProvider> metadataProvider =
> +      AndroidBridge::Bridge()->GetDrawMetadataProvider();
> +  metadataProvider->GetDrawMetadata(metadata);

Will this break xul-fennec? We only have the metadata provider with Android native-fennec, but it looks like this code builds depends just on MOZ_WIDGET_ANDROID, which would be defined when doing an Android build of xul-fennec?

@@ +1692,4 @@
>    jclass cls = env->FindClass("org/mozilla/gecko/GeckoAppShell");
>    jmethodID method = env->GetStaticMethodID(cls,
>                                              "addPluginView",
> +                                            "(Landroid/view/View;IIIILjava/lang/String;)V");

And same here, I guess the addPluginView method in xul-fennec doesn't have this extra parameter?

::: layout/generic/nsObjectFrame.cpp
@@ +1726,5 @@
> +
> +    frameGfxRect.MoveTo(0, 0);
> +    matrix2d.NudgeToIntegers();
> +
> +    mInstanceOwner->Paint(ctx, matrix2d.Transform(frameGfxRect), dirtyGfxRect);

I'm not entirely sure what's going on here, this definitely needs a comment. I get the general idea, but what's with the MoveTo call?

::: mobile/android/base/GeckoApp.java
@@ -1304,2 @@
>  
> -                ViewportMetrics geckoViewport = mSoftwareLayerClient.getGeckoViewportMetrics();

I guess getGeckoViewportMetrics is unused now, it should be removed.

@@ +1335,3 @@
>                      try {
>                          mPluginContainer.updateViewLayout(view, lp);
> +                        //mPluginContainer.invalidate();

This still meant to be here?

@@ +1381,5 @@
>                  view.setVisibility(View.VISIBLE);
>              }
>  
>              mPluginContainer.updateViewLayout(view, lp);
> +            //mPluginContainer.invalidate();

And this?

::: mobile/android/base/GeckoAppShell.java
@@ +1346,2 @@
>      {
>          Log.i(LOGTAG, "addPluginView:" + view + " @ x:" + x + " y:" + y + " w:" + w + " h:" + h ) ;

Should probably modify this log to show the metadata too?

::: mobile/android/base/ui/PanZoomController.java
@@ +601,5 @@
>               * animation by setting the state to PanZoomState.NOTHING. Handle this case and bail
>               * out.
>               */
>              if (mState != PanZoomState.FLING) {
> +                GeckoApp.mAppContext.showPluginViews();

Are the plugin views hidden during flinging?

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Chris Lord [:cwiiis] from comment #5)
> Comment on attachment 589368 [details] [diff] [review]
> Fix Flash plugin positioning
> 
> Review of attachment 589368 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the below needs to be addressed before r+.
> 
> ::: dom/plugins/base/nsPluginInstanceOwner.cpp
> @@ +1683,5 @@
> >  
> > +  nsAutoString metadata;
> > +  nsCOMPtr<nsIAndroidDrawMetadataProvider> metadataProvider =
> > +      AndroidBridge::Bridge()->GetDrawMetadataProvider();
> > +  metadataProvider->GetDrawMetadata(metadata);
> 
> Will this break xul-fennec? We only have the metadata provider with Android
> native-fennec, but it looks like this code builds depends just on
> MOZ_WIDGET_ANDROID, which would be defined when doing an Android build of
> xul-fennec?

Ugh, indeed.

> 
> @@ +1692,4 @@
> >    jclass cls = env->FindClass("org/mozilla/gecko/GeckoAppShell");
> >    jmethodID method = env->GetStaticMethodID(cls,
> >                                              "addPluginView",
> > +                                            "(Landroid/view/View;IIIILjava/lang/String;)V");
> 
> And same here, I guess the addPluginView method in xul-fennec doesn't have
> this extra parameter?

Yeah. I'll #ifdef it.

> 
> ::: layout/generic/nsObjectFrame.cpp
> @@ +1726,5 @@
> > +
> > +    frameGfxRect.MoveTo(0, 0);
> > +    matrix2d.NudgeToIntegers();
> > +
> > +    mInstanceOwner->Paint(ctx, matrix2d.Transform(frameGfxRect), dirtyGfxRect);
> 
> I'm not entirely sure what's going on here, this definitely needs a comment.
> I get the general idea, but what's with the MoveTo call?

The matrix includes the translation of the frame (frameGfxRect) already, so we need to transform it relative to 0,0. I can add a comment.

> 
> ::: mobile/android/base/GeckoApp.java
> @@ -1304,2 @@
> >  
> > -                ViewportMetrics geckoViewport = mSoftwareLayerClient.getGeckoViewportMetrics();
> 
> I guess getGeckoViewportMetrics is unused now, it should be removed.

I dunno, I think it could come in useful later?

> 
> @@ +1335,3 @@
> >                      try {
> >                          mPluginContainer.updateViewLayout(view, lp);
> > +                        //mPluginContainer.invalidate();
> 
> This still meant to be here?

Nope. Removed.

> 
> @@ +1381,5 @@
> >                  view.setVisibility(View.VISIBLE);
> >              }
> >  
> >              mPluginContainer.updateViewLayout(view, lp);
> > +            //mPluginContainer.invalidate();
> 
> And this?

Nope, same.

> 
> ::: mobile/android/base/GeckoAppShell.java
> @@ +1346,2 @@
> >      {
> >          Log.i(LOGTAG, "addPluginView:" + view + " @ x:" + x + " y:" + y + " w:" + w + " h:" + h ) ;
> 
> Should probably modify this log to show the metadata too?

Done.

> 
> ::: mobile/android/base/ui/PanZoomController.java
> @@ +601,5 @@
> >               * animation by setting the state to PanZoomState.NOTHING. Handle this case and bail
> >               * out.
> >               */
> >              if (mState != PanZoomState.FLING) {
> > +                GeckoApp.mAppContext.showPluginViews();
> 
> Are the plugin views hidden during flinging?

I think something else was calling fling() at the end (scale?), but I'm not really seeing where that was now. I am reworking all of that in my next patch, though. Removing this seems to leave things working, so I've done so.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix Flash plugin positioning

Comment 8

[Chris Lord [:cwiiis]]

Comment on attachment 589497 [details] [diff] [review]
Fix Flash plugin positioning

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

I'd still get rid of GeckoSoftwareLayerClient.getGeckoViewportMetrics() - It's leaking information we really oughtn't to out of GeckoSoftwareLayerClient (and I felt bad adding it in the first place :)). Not a deal-breaker though, r+ with a comment added to GetTransformToAncestor.

::: layout/base/nsLayoutUtils.h
@@ +524,5 @@
>                                               const nsRect& aRect,
>                                               nsIFrame* aAncestor);
>  
> +
> +  static gfx3DMatrix GetTransformToAncestor(nsIFrame *aFrame, nsIFrame *aAncestor);

Needs a documentation comment.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/cfd3838b4dc2

Comment 10

[Timothy Nikkel (:tnikkel)]

Comment on attachment 589497 [details] [diff] [review]
Fix Flash plugin positioning

>+    mInstanceOwner->Paint(ctx, matrix2d.Transform(frameGfxRect), dirtyGfxRect);

Do we not want to transform the dirty rect too?

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/cfd3838b4dc2

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 589497 [details] [diff] [review]
Fix Flash plugin positioning

[Approval Request Comment]
This is a low-risk patch that greatly improves the Flash experience on Android. Without this patch, Flash instances are frequently in the wrong place on the page.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8f8496ba9bbf

Comment 14

[Timothy Nikkel (:tnikkel)]

Looks like comment 10 was overlooked.

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Timothy Nikkel (:tn) from comment #10)
> Comment on attachment 589497 [details] [diff] [review]
> Fix Flash plugin positioning
> 
> >+    mInstanceOwner->Paint(ctx, matrix2d.Transform(frameGfxRect), dirtyGfxRect);
> 
> Do we not want to transform the dirty rect too?

We don't use that information so I didn't bother. Due to the way plugins are drawn on Android, we'll never use it.

Comment 17

[Cristian Nicolae (:xti)]

I am still able to reproduce this issue on the latest Nightly build. Reopening bug

--
Firefox 13.0a1 (2012-03-02)
Device: Samsung Galaxy S2
OS: Android 2.3.4

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Working here with fixes from bug 728369.