748469 - Move FormAssistPopup viewport math to Java

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

Attachment: patch

We don't need to be passing viewport data from JS to Java, since Java already knows about the viewport. This patch simplifies things, especially if we ever want to be smarter about adjusting where the popup shows up if the viewport changes (part of the problem encountered in bug 736008).

This is a low priority, but I wanted to upload the patch I came up with while investigating bug 736008 before it got forgotten.

Comment 1

[Wesley Johnston (:wesj)]

I didn't realize this was in my queue! I think there are a few other places where we do similar things. At least I know there is LayerController.convertViewPointToLayerPoint() which does the opposite of this. I think we should probably have a helper for this direction as well. Need to look and see if it exists somewhere.

Comment 2

[:Margaret Leibovic]

Oops, looks like this bug lost traction. Maybe Kats also has ideas about the best way to clean up this viewport math.

Comment 3

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

Comment on attachment 617962 [details] [diff] [review]
patch

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

This patch looks fine to me, except for the comment I noted below. Wes is right in that this is sort of the reverse of the convertViewPointToLayerPoint function we have in LayerController, but in this case the thing you're converting is relative to the top-left of the page rather than offset from the scroll position, so it's much simpler. I don't think it needs its own function.

::: mobile/android/base/FormAssistPopup.java
@@ +253,5 @@
>          int height = 0;
>  
>          try {
> +            left = (int) ((rect.getDouble("x") - viewportMetrics.viewportRectLeft) * zoom);
> +            top = (int) ((rect.getDouble("y") - viewportMetrics.viewportRectTop) * zoom);

I don't think these lines are correct. I think it should be
  left = (int)((rect.getDouble("x") * zoom) - viewportMetrics.viewportRectLeft);

If I'm reading the code correctly, the value returned from getBoundingContentRect is in CSS pixels relative to the top-left of the page. the viewportRectLeft/Top values are in device pixels relative to the top-left of the page.

Comment 4

[Wesley Johnston (:wesj)]

Comment on attachment 617962 [details] [diff] [review]
patch

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

Moving off my plate while we wait for the math to be checked?

Comment 5

[:Margaret Leibovic]

Attachment: patch v2

I'm bringing this patch back to life. The comment kats made was correct, so I fixed that, and I also updated this to work with the new way of getting ImmutableViewportMetrics.

Comment 6

[:Margaret Leibovic]

Attachment: (Part 2) Cleanup positionAndShowPopup

Tacking a little cleanup onto this bug. We never actually use a return value from positionAndShowPopup, and eating the JSONException that could happen would probably result in something weird happening, so I think we should avoid showing the popup in that case.

I tested this with my combo autocomplete/validation testcase, and it looks like nothing regressed: http://people.mozilla.com/~mleibovic/test/validation.html

Comment 7

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

Comment on attachment 719271 [details] [diff] [review]
patch v2

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

LGTM

Comment 8

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e5af7df02b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/39b225c25850

Comment 9

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/7e5af7df02b7
https://hg.mozilla.org/mozilla-central/rev/39b225c25850