744916 - Make SetPageSize() take the page size in CSS pixels in addition to device pixels.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Make SetPageSize() take the page size in CSS pixels in addition to device pixels.

This will let us avoid having to scale the rounded device pixel size to try to get the CSS pixel size.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 614528 [details] [diff] [review]
Make SetPageSize() take the page size in CSS pixels in addition to device pixels.

We also need to deal with SetFirstPaintViewport

Comment 2

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

Comment on attachment 614528 [details] [diff] [review]
Make SetPageSize() take the page size in CSS pixels in addition to device pixels.

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

Looks like a good start. I like the idea of always keeping the two measures of page size together always and updating method signatures to match. That will ensure that we don't accidentally miss a spot where the coordinates are updated. Also I'm not sure if you're aware of the wiki page at https://wiki.mozilla.org/Fennec/NativeUI/Viewport which may be useful (and will probably need to be updated if these patches land)

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: A version that builds

Comment 4

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

Comment on attachment 615064 [details] [diff] [review]
A version that builds

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

What about the other ViewportMetrics functions like interpolate, scaleTo, etc?

Comment 5

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

Comment on attachment 615064 [details] [diff] [review]
A version that builds

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

Minusing to get it out of my queue. To be clear, I think this patch needs to update the other functions in ViewportMetrics as well to update the mCssPageSize, otherwise things will get horribly out of sync. (Also update toString to print out the css page size).

Comment 6

[JP Rosevear [:jpr]]

Please do a complete risk assessment of all three pieces before landing, it seems like we could break a lot here.

Comment 7

[Joe Drew (not getting mail)]

Attachment: A version that works, with review comments fixed

Comment 8

[Joe Drew (not getting mail)]

Comment on attachment 616812 [details] [diff] [review]
A version that works, with review comments fixed

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

::: mobile/android/base/gfx/ViewportMetrics.java
@@ +186,5 @@
>      public void scaleTo(float newZoomFactor, PointF focus) {
> +        // mCssPageSize is invariant, since we're setting the scale factor
> +        // here. The page size is based on the CSS page size.
> +        mPageSize = mCssPageSize.scale(newZoomFactor);
> +

Jeff thinks this hunk should be in a separate patch, but I don't think so, because it makes us more correct.

Comment 9

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

Comment on attachment 616812 [details] [diff] [review]
A version that works, with review comments fixed

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

r=me with the fix below.

::: mobile/android/base/gfx/ImmutableViewportMetrics.java
@@ +74,5 @@
>          return new FloatSize(pageSizeWidth, pageSizeHeight);
>      }
> +
> +    public FloatSize getCssPageSize() {
> +        return new FloatSize(pageSizeWidth, pageSizeHeight);

This should be returning cssPageSizeWidth and cssPageSizeHeight.

::: mobile/android/base/gfx/ViewportMetrics.java
@@ +186,5 @@
>      public void scaleTo(float newZoomFactor, PointF focus) {
> +        // mCssPageSize is invariant, since we're setting the scale factor
> +        // here. The page size is based on the CSS page size.
> +        mPageSize = mCssPageSize.scale(newZoomFactor);
> +

I'm fine with this hunk being in this patch.

Comment 10

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec943d586046

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ec943d586046