Closed Bug 1416310 Opened 7 years ago Closed 7 years ago

Merge GeckoLayerClient into other classes

Categories

(GeckoView :: General, enhancement)

Product:
GeckoView
Component:
General
Platform:
All
Android
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(5 files)

Bug 1416310 - 1. Remove getMatrixForLayerRectToViewRect;
59 bytes, text/x-review-board-request
rbarker
: review+
Details
Bug 1416310 - 2. Use per-GeckoView event to handle scroll-to-focused-input;
59 bytes, text/x-review-board-request
rbarker
: review+
Details
Bug 1416310 - 3. Merge GeckoLayerClient into other classes;
59 bytes, text/x-review-board-request
rbarker
: review+
Details
Bug 1416310 - 4. Don't use GeckoLayerClient in native code;
59 bytes, text/x-review-board-request
rbarker
: review+
Details
Bug 1416310 - 5. Remove GeckoLayerClient.java and update generated bindings;
59 bytes, text/x-review-board-request
jchen
: review+
Details 
With bug 1415994, we have a LayerSession class that has some functionality taken from GeckoLayerClient. What remains in GeckoLayerClient consist of,

1) PanZoomController and DynamicToolbarAnimator instances, which should be moved to the session class.

2) Some viewport metrics code, which I think we can either remove or move to the session class.

3) The "ScrollTo:FocusedInput" event that tells Gecko to scroll to any focused input after a size change. I think this should be done all in C++ or in JS.

4) Mouse/touch event synthesis code, which I think should be moved to NativePanZoomController.
No longer blocks: 1415994
Depends on: 1415994
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Comment on attachment 8929172 [details]
Bug 1416310 - 1. Remove getMatrixForLayerRectToViewRect;

https://reviewboard.mozilla.org/r/200464/#review205650

Looks good to me, but could you verify the the origin issue?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoInputConnection.java:372
(Diff revision 1)
>  
> -        Matrix matrix = view.getMatrixForLayerRectToViewRect();
> -        if (matrix == null) {
> -            if (DEBUG) {
> -                Log.d(LOGTAG, "Cannot get Matrix to convert from Gecko coords to layer view coords");
> -            }
> +        // First aRects element is the widget bounds in device units.
> +        final float zoom = view.getZoomFactor();
> +        final Matrix matrix = new Matrix();
> +        matrix.postScale(zoom, zoom);
> +        matrix.postTranslate(aRects[0].left, aRects[0].top);

The previous version negated the origin to translate it. I'm guessing the origin is always 0,0 right now so it doesn't matter?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/GeckoLayerClient.java
(Diff revision 1)
> -        float geckoZoom = geckoViewport.zoomFactor;
> -
> -        Matrix matrix = new Matrix();
> -        matrix.postTranslate(geckoOrigin.x / geckoZoom, geckoOrigin.y / geckoZoom);
> -        matrix.postScale(zoom, zoom);
> -        matrix.postTranslate(-origin.x, -origin.y);

Here is where we were negating the origin for the translate.
Attachment #8929172 - Flags: review?(rbarker) → review+
Comment on attachment 8929173 [details]
Bug 1416310 - 2. Use per-GeckoView event to handle scroll-to-focused-input;

https://reviewboard.mozilla.org/r/200466/#review205658

I remember adding that timer in browser.js because sometimes the input field wouldn't be scrolled into view. Maybe the bug that was causing the resize to get lost has been fixed?
Attachment #8929173 - Flags: review?(rbarker) → review+
Comment on attachment 8929174 [details]
Bug 1416310 - 3. Merge GeckoLayerClient into other classes;

https://reviewboard.mozilla.org/r/200468/#review205662

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:324
(Diff revision 1)
>          mCompositor = session.mCompositor;
>          mCompositor.layerView = this;
>  
> +        // Reset the content-document-is-displayed flag.
> +        mCompositor.updateRootFrameMetrics(/* scroolX */ 0, /* scrollY */ 0,
> +                                           /* zoom */ 1.0f);

I'm not sure I understand this. Is it just to reset the flag in the compositor? I would rather have a function that resets the flag as that would be clearer. I assume the values passed in just get over-written once the compositor starts calling the function it self?
Attachment #8929174 - Flags: review?(rbarker) → review+
Comment on attachment 8929175 [details]
Bug 1416310 - 4. Don't use GeckoLayerClient in native code;

https://reviewboard.mozilla.org/r/200470/#review205664
Attachment #8929175 - Flags: review?(rbarker) → review+
> > -        Matrix matrix = new Matrix();
> > -        matrix.postTranslate(geckoOrigin.x / geckoZoom, geckoOrigin.y / geckoZoom);
> > -        matrix.postScale(zoom, zoom);
> > -        matrix.postTranslate(-origin.x, -origin.y);
> 
> Here is where we were negating the origin for the translate.

So these two lines,

> matrix.postTranslate(geckoOrigin.x / geckoZoom, geckoOrigin.y / geckoZoom);
> matrix.postTranslate(-origin.x, -origin.y);

Actually become a no-op when you do the scale in-between, so the result is only a scale by `zoom`.

> I remember adding that timer in browser.js because sometimes the input field
> wouldn't be scrolled into view. Maybe the bug that was causing the resize to
> get lost has been fixed?

Let me test it a bit more.

> > +        // Reset the content-document-is-displayed flag.
> > +        mCompositor.updateRootFrameMetrics(/* scroolX */ 0, /* scrollY */ 0,
> > +                                           /* zoom */ 1.0f);
> 
> I'm not sure I understand this. Is it just to reset the flag in the
> compositor? I would rather have a function that resets the flag as that
> would be clearer. I assume the values passed in just get over-written once
> the compositor starts calling the function it self?

Ok. Yeah it's just to reset the flag.
Comment on attachment 8929173 [details]
Bug 1416310 - 2. Use per-GeckoView event to handle scroll-to-focused-input;

Ok I found some edge cases with the original approach, so this new patch is more similar to what we had before. It does fix several issues and adds standalone GeckoView support over the existing code.
Attachment #8929173 - Flags: review+ → review?(rbarker)
Comment on attachment 8929173 [details]
Bug 1416310 - 2. Use per-GeckoView event to handle scroll-to-focused-input;

https://reviewboard.mozilla.org/r/200466/#review206548
Attachment #8929173 - Flags: review?(rbarker) → review+
Comment on attachment 8929176 [details]
Bug 1416310 - 5. Remove GeckoLayerClient.java and update generated bindings;

https://reviewboard.mozilla.org/r/200472/#review206658
Attachment #8929176 - Flags: review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8942f0a3c773
1. Remove getMatrixForLayerRectToViewRect; r=rbarker
https://hg.mozilla.org/integration/autoland/rev/946c1489acdd
2. Use per-GeckoView event to handle scroll-to-focused-input; r=rbarker
https://hg.mozilla.org/integration/autoland/rev/c4136f1d28d4
3. Merge GeckoLayerClient into other classes; r=rbarker
https://hg.mozilla.org/integration/autoland/rev/dd54872af9e7
4. Don't use GeckoLayerClient in native code; r=rbarker
https://hg.mozilla.org/integration/autoland/rev/bc5cc9edd496
5. Remove GeckoLayerClient.java and update generated bindings; r=jchen
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 59 → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: