Last Comment Bug 701830 - Rescaling at new resolution after zoom does not occur
: Rescaling at new resolution after zoom does not occur
Status: VERIFIED FIXED
[testday-20111111]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P1 normal (vote)
: ---
Assigned To: Patrick Walton (:pcwalton)
:
: Sebastian Kaspari (:sebastian)
Mentors:
: 701933 (view as bug list)
Depends on: native_droid_zooming 703141
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-11 12:55 PST by Naoki Hirata :nhirata (please use needinfo instead of cc)
Modified: 2012-01-09 14:57 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
WIP patch. (25.21 KB, patch)
2011-11-11 20:08 PST, Patrick Walton (:pcwalton)
bugmail: feedback+
Details | Diff | Splinter Review

Description Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-11-11 12:55:57 PST
1. go to the preferences
2. select reformat text on zoom
3. go to a website with some text and zoom

Expected: no pixelation on the text
Actual: text does not get reformatted on zoom

Note:
1. known issue I believe, didn't see a bug on it.
Comment 1 Matt Brubeck (:mbrubeck) 2011-11-11 16:24:03 PST
*** Bug 701933 has been marked as a duplicate of this bug. ***
Comment 2 Patrick Walton (:pcwalton) 2011-11-11 16:24:54 PST
This isn't just text, it's any content. Working on this now.
Comment 3 Patrick Walton (:pcwalton) 2011-11-11 20:08:31 PST
Created attachment 573993 [details] [diff] [review]
WIP patch.

WIP patch attached.
Comment 4 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-14 08:04:04 PST
Comment on attachment 573993 [details] [diff] [review]
WIP patch.

>@@ -945,16 +946,18 @@ abstract public class GeckoApp
>         tab.updateTitle(title);
>         loadFavicon(tab);
> 
>         mMainHandler.post(new Runnable() {
>             public void run() {
>                 if (Tabs.getInstance().isSelectedTab(tab))
>                     mBrowserToolbar.setTitle(tab.getDisplayTitle());
>                 onTabsChanged();
>+
>+                mSoftwareLayerClient.geckoLoadedNewContent();
>             }
>         });
>     }

I'm not sure this is the best place for this call. It's ok for now, but we might need something a little earlier, when gecko first starts loading the new content. This will only get triggered after the page is loaded.

>+    private PanZoomPosition mGeckoPanZoomPosition;
>+    /* The pan/zoom position that Gecko is currently displaying. If null, we don't know. */
>+    private PanZoomPosition mPendingPanZoomPosition;
>+    /*
>+     * The pan/zoom position that we're telling Gecko to display. If this is non-null, Gecko is
>+     * currently busy panning or zooming to this location.
>+     */
>+    private PanZoomPosition mQueuedPanZoomPosition;
>+    /*
>+     * The pan/zoom position we're waiting to send to Gecko. It will be sent once
>+     * Gecko finishes panning to mPendingPanZoomPosition.
>+     */
>+    private boolean mDrawPending;

Could you place the comments for the fields above the field, rather than below? This is inconsistent and much more confusing :/

>     public void endDrawing(int x, int y, int width, int height) {
>+        if (mGeckoPanZoomPosition == null) {
>+            /*
>+             * Ignore the draw. We don't know where Gecko is currently panned or zoomed to, so we
>+             * can't safely draw.
>+             */
>+            mDrawPending = true;
>+            return;
>+        }
>+
>+        Log.e("Fennec", "### Drawing at " + mGeckoPanZoomPosition.toJSON());
>+
>         LayerController controller = getLayerController();
>-        //controller.unzoom();  /* FIXME */
>+        float zoomFactor = controller.getZoomFactor();
>+        controller.unzoom();
>         controller.notifyViewOfGeometryChange();
> 
>-        mViewportController.setVisibleRect(mGeckoVisibleRect);
>-
>-        if (mGeckoVisibleRect != null) {
>-            RectF layerRect = mViewportController.untransformVisibleRect(mGeckoVisibleRect,
>-                                                                         getPageSize());
>-            mTileLayer.origin = new PointF(layerRect.left, layerRect.top);
>-        }
>+        //mViewportMetrics.setPosition(mGeckoPanZoomPosition);
>+        mTileLayer.origin = mGeckoPanZoomPosition.untransformPoint();
> 
>         repaint(new Rect(x, y, x + width, y + height));

Don't really need a separate method for repaint(); can be inlined.

>+
>+        mDrawPending = false;
>+
>+        mPendingPanZoomPosition = null;
>+        if (mQueuedPanZoomPosition != null) {
>+            //PanZoomPosition position = mQueuedPanZoomPosition.scale(1.0f / zoomFactor);
>+            mQueuedPanZoomPosition = null;
>+            //sendPanZoomPositionToGecko(position);
>+        } else if (mDrawPending) {
>+            mDrawPending = false;
>+            GeckoAppShell.scheduleRedraw();
>+        }
>     }

I assume the commented-out lines here will be uncommented eventually.
Also, how can mDrawPending be true here? It was set to false just a few lines above.

>     /**
>      * Returns true if this controller is fine with performing a redraw operation or false if it
>      * would prefer that the action didn't take place.
>      */
>     public boolean getRedrawHint() {
>-        return aboutToCheckerboard();
>+        return aboutToCheckerboard() ||
>+            (getZoomFactor() != 1.0f && mPanZoomController.getRedrawHint());
>     }

I don't understand why zoom factor is checked here.

>+    public boolean getRedrawHint() {
>+        switch (mState) {
>+            case NOTHING:
>+            case TOUCHING:
>+            case PANNING_HOLD:
>+                return true;
>+            case FLING:
>+            case PANNING:
>+            case PINCHING:
>+                return false;
>+        }
>+
>+        throw new RuntimeException("Unknown state!");
>+    }
>+

I'd prefer a log+graceful handling here rather than an exception.

>+
>+    /** Returns a new zoom position, with the offset and zoom scaled by the given factor. */
>+    public PanZoomPosition scale(float factor) {
>+      return new PanZoomPosition(new PointF(x * factor, y * factor), zoom * factor);
>+    }

indent should be 4 spaces.

Also something I noticed was that in a bunch of places you do == comparisons on floats. I'm not sure that's a good idea, I'd prefer to see Math.abs(x-y) < epsilon to be robust against floating-point rounding errors.
Comment 5 Alex Keybl [:akeybl] 2011-11-22 12:16:25 PST
FYI: This is currently being considered a blocker for releasing the Native UI nightly to Test Drivers.
Comment 6 Patrick Walton (:pcwalton) 2011-11-22 12:18:33 PST
Patches for this are in bug 703141.
Comment 7 Patrick Walton (:pcwalton) 2011-11-23 11:19:25 PST
Fixed with the landing of bug 703141.
Comment 8 Andreea Pod 2011-11-25 07:08:29 PST
Verified fixed on: Mozilla /5.0 (Android;Linux armv7l;rv:11.0a1) Gecko/20111124 Firefox/11.0a1 Fennec/11.0a1
Device: LG Optimus 2X (Android 2.2)

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