Closed Bug 701830 Opened 13 years ago Closed 13 years ago

Rescaling at new resolution after zoom does not occur

Categories

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

ARM
Android
defect

Tracking

(firefox11 fixed, fennec11+)

VERIFIED FIXED
Tracking Status
firefox11 --- fixed
fennec 11+ ---

People

(Reporter: nhirata, Assigned: pcwalton)

References

Details

(Whiteboard: [testday-20111111])

Attachments

(1 obsolete file)

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.
Whiteboard: [testday-20111111]
Assignee: nobody → pwalton
Summary: Reformat text on zoom does not occur → Re-rendering of text at new resolution after zoom does not occur
This isn't just text, it's any content. Working on this now.
Summary: Re-rendering of text at new resolution after zoom does not occur → Rescaling at new resolution after zoom does not occur
Attached patch WIP patch. (obsolete) (deleted) — Splinter Review
WIP patch attached.
Attachment #573993 - Flags: feedback?(kgupta)
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.
Attachment #573993 - Flags: feedback?(kgupta) → feedback+
Priority: -- → P1
FYI: This is currently being considered a blocker for releasing the Native UI nightly to Test Drivers.
Depends on: 703141
Attachment #573993 - Attachment is obsolete: true
Patches for this are in bug 703141.
Fixed with the landing of bug 703141.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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)
Status: RESOLVED → VERIFIED
tracking-fennec: --- → 11+
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: