701830 - Rescaling at new resolution after zoom does not occur

Status: VERIFIED FIXED

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

Description

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

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 2

[Patrick Walton (:pcwalton)]

This isn't just text, it's any content. Working on this now.

Comment 3

[Patrick Walton (:pcwalton)]

Attachment: WIP patch.

WIP patch attached.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

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]]

FYI: This is currently being considered a blocker for releasing the Native UI nightly to Test Drivers.

Comment 6

[Patrick Walton (:pcwalton)]

Patches for this are in bug 703141.

Comment 7

[Patrick Walton (:pcwalton)]

Fixed with the landing of bug 703141.

Comment 8

[Andreea Pod]

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)