Merge GeckoLayerClient into other classes

ASSIGNED
Assigned to

Status

()

Firefox for Android
GeckoView
ASSIGNED
9 days ago
3 days ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Assignee)

Description

9 days ago
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.
(Assignee)

Updated

9 days ago
No longer blocks: 1415994
Depends on: 1415994
(Assignee)

Updated

3 days ago
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

3 days ago
mozreview-review
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 7

3 days ago
mozreview-review
Comment on attachment 8929173 [details]
Bug 1416310 - 2. Use resize 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 8

3 days ago
mozreview-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 9

3 days ago
mozreview-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+
(Assignee)

Comment 10

3 days ago
> > -        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.
You need to log in before you can comment on or make changes to this bug.