Last Comment Bug 707439 - Flash plugin wrong location on zoom & pan
: Flash plugin wrong location on zoom & pan
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P1 critical (vote)
: Firefox 12
Assigned To: James Willcox (:snorp) (jwillcox@mozilla.com)
:
: Sebastian Kaspari (:sebastian)
Mentors:
: 707706 707765 719000 (view as bug list)
Depends on: 728369
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-03 08:15 PST by David Ascher (:davida)
Modified: 2012-03-14 14:25 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
affected
11+


Attachments
Fix Flash plugin positioning (19.29 KB, patch)
2012-01-17 17:27 PST, James Willcox (:snorp) (jwillcox@mozilla.com)
chrislord.net: review-
Details | Diff | Splinter Review
Fix Flash plugin positioning (19.12 KB, patch)
2012-01-18 07:20 PST, James Willcox (:snorp) (jwillcox@mozilla.com)
chrislord.net: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description David Ascher (:davida) 2011-12-03 08:15:41 PST
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 1 Aaron Train [:aaronmt] 2011-12-05 10:05:54 PST
*** Bug 707706 has been marked as a duplicate of this bug. ***
Comment 2 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-05 12:29:48 PST
*** Bug 707765 has been marked as a duplicate of this bug. ***
Comment 3 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-01-05 23:18:03 PST
Also occurs with : http://hybridmind.com/tutorials/simple-as3-viewport-tutorial/
Comment 4 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-17 17:27:18 PST
Created attachment 589368 [details] [diff] [review]
Fix Flash plugin positioning
Comment 5 Chris Lord [:cwiiis] 2012-01-18 03:47:03 PST
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) (jwillcox@mozilla.com) 2012-01-18 07:03:54 PST
(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) (jwillcox@mozilla.com) 2012-01-18 07:20:10 PST
Created attachment 589497 [details] [diff] [review]
Fix Flash plugin positioning
Comment 8 Chris Lord [:cwiiis] 2012-01-18 07:31:04 PST
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) (jwillcox@mozilla.com) 2012-01-18 18:01:17 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/cfd3838b4dc2
Comment 10 Timothy Nikkel (:tnikkel) 2012-01-18 23:19:06 PST
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] 2012-01-19 02:59:15 PST
https://hg.mozilla.org/mozilla-central/rev/cfd3838b4dc2
Comment 12 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-20 09:19:04 PST
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?) 2012-01-20 13:48:34 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/8f8496ba9bbf
Comment 14 Timothy Nikkel (:tnikkel) 2012-01-20 15:01:15 PST
Looks like comment 10 was overlooked.
Comment 15 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-23 07:12:45 PST
(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 16 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-23 14:29:42 PST
*** Bug 719000 has been marked as a duplicate of this bug. ***
Comment 17 Cristian Nicolae (:xti) 2012-03-02 09:09:48 PST
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) (jwillcox@mozilla.com) 2012-03-14 14:25:24 PDT
Working here with fixes from bug 728369.

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