Closed Bug 731897 Opened 13 years ago Closed 13 years ago

Maple: make js set the java viewport synchronously


(Firefox for Android Graveyard :: General, defect)

Not set


(Not tracked)



(Reporter: jrmuizel, Unassigned)




(2 files, 4 obsolete files)

This will avoid the need to tie viewport modification with painting, which is bad for performance and complicated.
Blocks: 731603
Attached patch The rough idea (obsolete) — Splinter Review
Attached patch 2nd draft (obsolete) — Splinter Review
This version also takes care to avoid the resize race.
Attachment #601845 - Attachment is obsolete: true
Comment on attachment 601949 [details] [diff] [review]
2nd draft

Review of attachment 601949 [details] [diff] [review]:

::: mobile/android/base/gfx/
@@ +161,5 @@
>      /** This function is invoked by Gecko via JNI; be careful when modifying signature. */
>      public void endDrawing() {
> +        //updateViewport(!mUpdateViewportOnEndDraw);
> +        mUpdateViewportOnEndDraw = false;

You can take out the mUpdateViewportOnEndDraw flag as well.

@@ +233,5 @@
> +            mGeckoViewport = viewport;
> +            mGeckoViewport.setSize(viewportSize);
> +
> +            RectF position = mGeckoViewport.getViewport();
> +            mRootLayer.setPositionAndResolution(RectUtils.round(position), mGeckoViewport.getZoomFactor());

I'm not sure setting the position on the root layer at this point will cause touch events to get mapped incorrectly temporarily. The convertViewPointToLayerPoint code in LayerController may need to be changed as well.

@@ +330,5 @@
>              mIgnorePaintsPendingViewportSizeChange = false;
>              // Redraw everything.
> +            //Rect rect = new Rect(0, 0, mBufferSize.width, mBufferSize.height);
> +            //GeckoAppShell.sendEventToGecko(GeckoEvent.createDrawEvent(rect));

I think removing this createDrawEvent part should be pulled out into a separate patch first. We should verify that Gecko handles invalidating stuff internally at all the points that this could happen, otherwise we'll end up with things not repainting correctly.

@@ +372,5 @@
>      void geometryChanged() {
>          /* Let Gecko know if the screensize has changed */
>          sendResizeEventIfNecessary(false);
> +        //if (mLayerController.getRedrawHint())
> +        //    adjustViewport();

This part should not be commented out as it will break panning and probably other things. If we leave it in it will be slightly inefficient (double event) in the case of rotation but fine otherwise.
Attached patch Remove redrawSplinter Review
I don't believe we need this now that we get paints from gecko directly. I'm not sure though.
Attachment #601976 - Flags: review?(ehsan)
Attachment #601976 - Flags: review?(bugmail.mozilla)
Attached patch 3rd draft (obsolete) — Splinter Review
This version mostly works. However, it breaks link clicking.
Attachment #601949 - Attachment is obsolete: true
Attached patch 4th draft (obsolete) — Splinter Review
Cleans things a bit to make it easier to understand what the patch is doing.
Attachment #601980 - Attachment is obsolete: true
Attachment #601976 - Flags: review?(ehsan) → review+
(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> Created attachment 601976 [details] [diff] [review]
> Remove redraw
> I don't believe we need this now that we get paints from gecko directly. I'm
> not sure though.

So the draw is sent from browser.js in the following scenarios:
- document-shown event is received
- a fragment jump
- after scrolling to an input field that received focus

The last two also change the scroll position in Gecko and should do invalidation/draw properly. The first one will need verification because it is used in conjunction with one of our paint suppression techniques - specifically during page load to avoid drawing the old page at coordinates that map to the new page, and viewport changes because of <meta viewport> tags processed during page load. (IIRC).
Comment on attachment 601981 [details] [diff] [review]
4th draft

Review of attachment 601981 [details] [diff] [review]:

This is not totally crazy, but it needs a lot more work!

::: mobile/android/base/gfx/
@@ +126,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) {
> +        //    return false;

This code is meant to stop painting when the size changes on the Java side and resume it when we get the new viewport from js.  Removing this should at least take some precaution against unnecessary paints during rotation.
Attached patch 5th draftSplinter Review
This adds back paint suppression. We can kill it later if we don't need it anymore.

I can't seem to reproduce the clicking problems anymore. However, I do see some other problems:
- We seem to be at the wrong zoom when we first load a page quite often
- We flash checkerboard in-between page loading.
Attachment #601981 - Attachment is obsolete: true
Depends on: 732091
Depends on: 732089
Blocks: 729653
Bug 732564 incorporates these patches. This bug could probably be duped to that one.
Closed: 13 years ago
Resolution: --- → DUPLICATE
Comment on attachment 601976 [details] [diff] [review]
Remove redraw

Clearing my review queue. This patch is now part of bug 732564.
Attachment #601976 - Flags: review?(bugmail.mozilla)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.