Closed Bug 728614 Opened 12 years ago Closed 12 years ago

Fix the zoom level on rotation on Maple

Categories

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

x86
Android
defect

Tracking

(firefox14 verified, firefox15 verified, blocking-fennec1.0 beta+)

VERIFIED FIXED
Firefox 14
Tracking Status
firefox14 --- verified
firefox15 --- verified
blocking-fennec1.0 --- beta+

People

(Reporter: jpr, Assigned: ehsan.akhgari)

References

Details

(Whiteboard: MAPLE mwc-demo)

Attachments

(9 files, 6 obsolete files)

7.63 KB, patch
dougt
: review+
Details | Diff | Splinter Review
71.41 KB, patch
Details | Diff | Splinter Review
1.56 KB, patch
dougt
: review+
Details | Diff | Splinter Review
12.63 KB, patch
dougt
: review+
Details | Diff | Splinter Review
12.13 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
2.43 KB, patch
dougt
: review+
Details | Diff | Splinter Review
3.37 KB, patch
kats
: review+
Details | Diff | Splinter Review
3.96 KB, patch
kats
: review+
Details | Diff | Splinter Review
1.71 KB, patch
kats
: review-
Details | Diff | Splinter Review
Refactor viewport implementation to remove js parts of the implementation, this will clean up the code to make it easier to fix related issues.
Attachment #598585 - Flags: review?(doug.turner)
Attached patch patch (obsolete) — Splinter Review
Attachment #598585 - Attachment is obsolete: true
Attachment #598586 - Flags: review?(doug.turner)
Attachment #598585 - Flags: review?(doug.turner)
Blocks: 728249
Comment on attachment 598586 [details] [diff] [review]
patch

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

::: mobile/android/base/GeckoApp.java
@@ +1305,5 @@
> +
> +        GeckoAppShell.sendEventToGecko(
> +            GeckoEvent.createMetaViewportQueryEvent(tab.getId(),
> +                new GeckoEvent.Callback() {
> +                    public void callback(GeckoEvent ev, String data) {

This callback does nothing.  I don't see the point.

::: mobile/android/base/GeckoEvent.java
@@ +390,4 @@
>          return event;
>      }
>  
> +    public static GeckoEvent createMetaViewportQueryEvent(int tabId, Callback callback) {

semicolon isn't spaced correctly.

::: widget/android/AndroidJavaWrappers.cpp
@@ -616,4 @@
>              break;
>          }
>  
> -        case SCREENSHOT: {

This was removed, but:

private static final int SCREENSHOT = 25;

was not removed.

@@ +673,5 @@
> +AndroidGeckoEvent::DoCallback(const nsAString& data) {
> +    JNIEnv* env = AndroidBridge::GetJNIEnv();
> +    if (!env)
> +        return;
> +    jstring jData = env->NewString(nsPromiseFlatString(data).get(), data.Length());

You want to have a jni frame here:

AndroidBridge::AutoLocalJNIFrame(env, 1);

::: widget/android/nsAppShell.cpp
@@ +461,5 @@
> +        if (!domWindow)
> +            break;
> +        nsCOMPtr<nsIDOMDocument> domDocument;
> +        domWindow->GetDocument(getter_AddRefs(domDocument));
> +        nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDocument);

test for null |doc|
Attachment #598586 - Flags: review?(doug.turner) → review-
Attached patch patchSplinter Review
Attachment #598586 - Attachment is obsolete: true
Attachment #598597 - Flags: review?(doug.turner)
Attachment #598597 - Flags: review?(doug.turner) → review+
Blocks: 727993
Blocks: 726467
Blocks: 728486
Attached patch WIP (obsolete) — Splinter Review
This is my work in progress patch to move viewport handling to the Java UI side, and eliminate it from the browser.js side.  This patch is still early in its infancy, and should be treated with care.  Specifically, it should not be expected to work correctly, and much less so to link successfully!  I'd be curious to see high-level comments about it, but please don't spend a lot of time reviewing the details as they will probably change.
Attached patch WIP 2Splinter Review
Still completely untested.
Attachment #598612 - Attachment is obsolete: true
Just to add a comment to reflect what I've said on IRC;

I think this is a sound and good idea, but I think we should base this patch on top of a clean-up of what's already there first. I think some of the bugs we're seeing could be worked out without taking drastic measures and there's an awful lot of unused code left over from ripping out the compositor.

A summary of things that need cleaning up:

- viewportExcess in browser.js is unused - this needs to be removed and scrollToFocusedInput possibly rewritten

- updateTransform in browser.js is unused - this can just be removed

- Viewport offset needs to be removed. This is represented by ViewportMetrics.mViewportOffset in Java, and by offsetX and offsetY in the viewport array in browser.js. This is being ignored when setting the displayport in browser.js, and is causing touch events to have incorrect positions.

- The viewport array in browser.js can be removed. All of its values except zoom are now unused or stored elsewhere anyway. This can be replaced with just a zoom variable, I think

I think once this has been done, the code will become a bit more readable again and we can reassess from there.
Attachment #598947 - Flags: review?(chrislord.net)
Attachment #598947 - Flags: review?(chrislord.net) → review+
Attachment #598953 - Flags: review?(doug.turner)
Attachment #598953 - Flags: review?(chrislord.net)
Attachment #598953 - Flags: review?(doug.turner) → review+
So one thing to note is that our current implementation of ScrollToFocusedInput is broken.  You can try this by typing test in the address bar, and focusing the text box at the very end of the google search results page.  The content gets scrolled to somewhere in the middle of the page, which does not include the search box.
In fact I measured the values we measure in scrollToFocusedInput for the excessViewport object.  I see values similar to this: {x:0, y:0.1500244140625}.

So given the fact that the current viewportExcess handling no longer works correctly, and even if it did the values would not be large enough to actually scroll the focused input into view, I will rip this code out until somebody fixes this for realz.
(In reply to Ehsan Akhgari [:ehsan] from comment #14)
> In fact I measured the values we measure in scrollToFocusedInput for the
> excessViewport object.  I see values similar to this: {x:0,
> y:0.1500244140625}.
> 
> So given the fact that the current viewportExcess handling no longer works
> correctly, and even if it did the values would not be large enough to
> actually scroll the focused input into view, I will rip this code out until
> somebody fixes this for realz.

Please file a bug too
(In reply to Mark Finkle (:mfinkle) from comment #15)
> (In reply to Ehsan Akhgari [:ehsan] from comment #14)
> > In fact I measured the values we measure in scrollToFocusedInput for the
> > excessViewport object.  I see values similar to this: {x:0,
> > y:0.1500244140625}.
> > 
> > So given the fact that the current viewportExcess handling no longer works
> > correctly, and even if it did the values would not be large enough to
> > actually scroll the focused input into view, I will rip this code out until
> > somebody fixes this for realz.
> 
> Please file a bug too

Removed viewportExcess: https://hg.mozilla.org/projects/maple/rev/ab2c10f183e4 and filed bug 728978.
Attachment #599230 - Flags: review?(chrislord.net)
Comment on attachment 598953 [details] [diff] [review]
Part 2: Remove viewport offsets

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

r+ with the comment addressed.

::: mobile/android/base/gfx/ViewportMetrics.java
@@ +131,5 @@
>      }
>  
>      public PointF getDisplayportOrigin() {
> +        return new PointF(mViewportRect.left,
> +                          mViewportRect.top);

This isn't right - Looking at browser.js, the viewport is always in the centre of the displayport now. I'm not sure if this matters anymore, but if it doesn't, then this function (and probably getOptimumViewportOffset) should be removed entirely.
Comment on attachment 599230 [details] [diff] [review]
Part 4: Remove the viewport struct from browser.js

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

Looks good to me.
Attachment #599230 - Flags: review?(chrislord.net) → review+
(In reply to Chris Lord [:cwiiis] from comment #18)
> Comment on attachment 598953 [details] [diff] [review]
> Part 2: Remove viewport offsets
> 
> Review of attachment 598953 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the comment addressed.
> 
> ::: mobile/android/base/gfx/ViewportMetrics.java
> @@ +131,5 @@
> >      }
> >  
> >      public PointF getDisplayportOrigin() {
> > +        return new PointF(mViewportRect.left,
> > +                          mViewportRect.top);
> 
> This isn't right - Looking at browser.js, the viewport is always in the
> centre of the displayport now. I'm not sure if this matters anymore, but if
> it doesn't, then this function (and probably getOptimumViewportOffset)
> should be removed entirely.

Filed bug 729183.
(In reply to Chris Lord [:cwiiis] from comment #19)
> Comment on attachment 599230 [details] [diff] [review]
> Part 4: Remove the viewport struct from browser.js
> 
> Review of attachment 599230 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.

https://hg.mozilla.org/projects/maple/rev/75fdc3a367da
No longer blocks: 728249
Whiteboard: mwc-demo → MAPLE mwc-demo
this patch only add refs the wrapped objects in AndroidGeckoEvents. Other subclasses of WrappedJavaObject already do their own add ref'ing so the previous patch was interfering with it.
Attachment #599318 - Attachment is obsolete: true
Attachment #599467 - Flags: review?(doug.turner)
Attachment #599318 - Flags: review?(doug.turner)
Comment on attachment 599467 [details] [diff] [review]
patch to add ref wrapped java objects in events (and stub out xul)

lets see a follow up to remove the Dispose method on the AndroidLayerRendererFrame class.
Attachment #599467 - Flags: review?(doug.turner) → review+
Blocks: 725091
I backed out parts of Brad's patch as I think I won't need them here.  If I do, I will reland it:

https://hg.mozilla.org/projects/maple/rev/580381e2805c
Summary: Refactor viewport implementation to remove js parts of the implementation → Fix the zoom level on rotation on Maple
Attached patch Part 6: fix the zoom level (obsolete) — Splinter Review
This patch fixes the real issue.  The abortAnimation call doesn't need to do anything if nothing is happening, and setting the viewport size multiple times just confuses the browser.

There are still painting issues with this patch, and I will work on fixing them next.
Attachment #599639 - Flags: review?
Attachment #599639 - Flags: review?(chrislord.net)
Attachment #599639 - Flags: review?(bugmail.mozilla)
Attachment #599639 - Flags: review?
Comment on attachment 599639 [details] [diff] [review]
Part 6: fix the zoom level

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

r+ from me, but I'm dubious about the change below - hopefully kats will have more insight.

::: mobile/android/base/ui/PanZoomController.java
@@ -230,5 @@
>          case NOTHING:
>              // Don't do animations here; they're distracting and can cause flashes on page
>              // transitions.
> -            mController.setViewportMetrics(getValidViewportMetrics());
> -            mController.notifyLayerClientOfGeometryChange();

I'm a little wary of this, but I don't know/can't remember enough to know whether removing this is dangerous or not.
Attachment #599639 - Flags: review?(chrislord.net) → review+
(In reply to Chris Lord [:cwiiis] from comment #29)
> Comment on attachment 599639 [details] [diff] [review]
> Part 6: fix the zoom level
> 
> Review of attachment 599639 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ from me, but I'm dubious about the change below - hopefully kats will
> have more insight.
> 
> ::: mobile/android/base/ui/PanZoomController.java
> @@ -230,5 @@
> >          case NOTHING:
> >              // Don't do animations here; they're distracting and can cause flashes on page
> >              // transitions.
> > -            mController.setViewportMetrics(getValidViewportMetrics());
> > -            mController.notifyLayerClientOfGeometryChange();
> 
> I'm a little wary of this, but I don't know/can't remember enough to know
> whether removing this is dangerous or not.

Just to note btw, the reason I'm wary of this is that all the other cases fall into the NOTHING case - I'd be less wary if this code were moved into the case above and a break added.
Comment on attachment 599639 [details] [diff] [review]
Part 6: fix the zoom level

>+++ b/mobile/android/base/ui/PanZoomController.java
>@@ -225,18 +225,16 @@ public class PanZoomController
>             // fall through
>         case ANIMATED_ZOOM:
>             // the zoom that's in progress likely makes no sense any more (such as if
>             // the screen orientation changed) so abort it
>             // fall through
>         case NOTHING:
>             // Don't do animations here; they're distracting and can cause flashes on page
>             // transitions.
>-            mController.setViewportMetrics(getValidViewportMetrics());
>-            mController.notifyLayerClientOfGeometryChange();
>             break;

A couple of things: one is that this *might* be ok for the NOTHING case, but the other cases above also fall through to this one, and I'm pretty sure it's not good to remove this for the other cases. So you'd have to move this code up so that it still gets run for the non-NOTHING cases of the switch.

The other is that if you remove this code and rotate while at the bottom-right corner of a page, does that leave the page in an overscroll position? This code was mostly intended to ensure that after unexpected viewport size changes (such as on a rotation) we don't end up in overscroll and leave it that way.
Attachment #599639 - Flags: review?(bugmail.mozilla) → review-
Attachment #599639 - Attachment is obsolete: true
Attachment #599671 - Flags: review?(bugmail.mozilla)
Comment on attachment 599671 [details] [diff] [review]
Part 6: fix the zoom level

> 
>-        PointF newFocus = new PointF(size.width / 2.0f, size.height / 2.0f);
>+        // For rotations, we want the focus point to be at the top left.
>+        boolean rotation = (size.width > oldWidth && size.height < oldHeight) ||
>+                           (size.width < oldWidth && size.height > oldHeight);

Are there any phones with screen resolutions where this might fail? The screen would have to be near-square, like the Motorola Flipout.

Otherwise seems ok based on the testing we did offline.
Attachment #599671 - Flags: review?(bugmail.mozilla) → review+
This seems to match the behavior of the stock browser.  Basically, painting anything with the wrong viewport when the rotation happens makes us look bad.
Attachment #599730 - Flags: review?(bugmail.mozilla)
Attachment #599731 - Flags: review?(bugmail.mozilla)
Comment on attachment 599730 [details] [diff] [review]
Part 7: pause painting when rotation happens

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

Update commit message to say "viewport size" instead of "orientation"

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +134,5 @@
> +        // If the viewport has changed but we still don't have the latest viewport
> +        // from Gecko, ignore the viewport passed to us from Gecko since that is going
> +        // to be wrong.
> +        if (!mFirstPaint && mIgnorePaintsPendingViewportSizeChange && !mUpdateViewportOnEndDraw) {
> +            return null;

The condition here doesn't need the "&& !mUpdateViewportOnEndDraw" clause, I think. It only matters if a second viewportSizeChanged gets called before the first one is completely finished processing though.
Attachment #599730 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 599731 [details] [diff] [review]
Part 8: Remove updateLayerAfterDraw

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

I just landed two patches I had pending from before I left last week; one of them does this, along with a bunch of other cleanup.
Attachment #599731 - Flags: review?(bugmail.mozilla) → review-
... and this to address the review comment: https://hg.mozilla.org/projects/maple/rev/6b4f7dee6fe8
Priority: -- → P1
Attachment #599731 - Attachment is obsolete: true
Attachment #599779 - Flags: review?(bugmail.mozilla)
Attachment #598953 - Flags: review?(chrislord.net)
Comment on attachment 599779 [details] [diff] [review]
Part 8: Simplify getViewTransform

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

I'm ok with taking out the useless getOrigin() call, but I'm not sure about moving that stuff outside the synchronized block. The ViewportMetrics object that you get from the layer controller can be concurrently modified by other threads (e.g. the java UI thread) and so the synchronization seems like a good idea to avoid getting inconsistent values out of it.
Attachment #599779 - Flags: review?(bugmail.mozilla) → review-
Everything that needed to be done here should be finished.  The rest is bug 725091, which is visible on Galaxy Nexus, but not other phones that I have tried.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
blocking-fennec1.0: --- → beta+
No longer blocks: 728486
Target Milestone: --- → Firefox 14
Closing bug as verified fixed on:

Firefox 15.0a1 (2012-05-29)
Firefox 14.0a2 (2012-05-29)

Device: Galaxy Nexus
OS: Android 4.0.2
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: