Closed
Bug 1416310
Opened 7 years ago
Closed 7 years ago
Merge GeckoLayerClient into other classes
Categories
(GeckoView :: General, enhancement)
Tracking
(firefox59 fixed)
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: jchen, Assigned: jchen)
References
Details
Attachments
(5 files)
59 bytes,
text/x-review-board-request
|
rbarker
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
rbarker
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
rbarker
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
rbarker
:
review+
|
Details |
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.
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years 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•7 years 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•7 years ago
|
||
mozreview-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 8•7 years 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•7 years 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•7 years 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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 years ago
|
||
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 17•7 years ago
|
||
mozreview-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/#review206548
Attachment #8929173 -
Flags: review?(rbarker) → review+
Assignee | ||
Comment 18•7 years ago
|
||
mozreview-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+
Comment 19•7 years ago
|
||
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
Comment 20•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8942f0a3c773 https://hg.mozilla.org/mozilla-central/rev/946c1489acdd https://hg.mozilla.org/mozilla-central/rev/c4136f1d28d4 https://hg.mozilla.org/mozilla-central/rev/dd54872af9e7 https://hg.mozilla.org/mozilla-central/rev/bc5cc9edd496
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Updated•5 years ago
|
Product: Firefox for Android → GeckoView
Updated•5 years ago
|
Target Milestone: Firefox 59 → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•