731897 - Maple: make js set the java viewport synchronously

Status: RESOLVED DUPLICATE of bug 732564

(Firefox for Android Graveyard :: General, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

This will avoid the need to tie viewport modification with painting, which is bad for performance and complicated.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: The rough idea

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: 2nd draft

This version also takes care to avoid the resize race.

Comment 3

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

Comment on attachment 601949 [details] [diff] [review]
2nd draft

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

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +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.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: Remove redraw

I don't believe we need this now that we get paints from gecko directly. I'm not sure though.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Attachment: 3rd draft

This version mostly works. However, it breaks link clicking.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Attachment: 4th draft

Cleans things a bit to make it easier to understand what the patch is doing.

Comment 7

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

(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 8

[(no longer active)]

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/GeckoLayerClient.java
@@ +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.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Attachment: 5th draft

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.

Comment 10

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

Bug 732564 incorporates these patches. This bug could probably be duped to that one.

Comment 12

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

Comment on attachment 601976 [details] [diff] [review]
Remove redraw

Clearing my review queue. This patch is now part of bug 732564.