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

RESOLVED FIXED in Firefox 14

Status

()

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: jrmuizel, Assigned: joe)

Tracking

(Blocks 1 bug)

unspecified
Firefox 14
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-fennec1.0 beta+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

This will let us avoid having to scale the rounded device pixel size to try to get the CSS pixel size.
Attachment #614528 - Flags: review?(bugmail.mozilla)
Blocks: 742134
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
Attachment #614528 - Flags: review?(bugmail.mozilla) → feedback?
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)
Attachment #614528 - Flags: feedback? → feedback+
Posted patch A version that builds (obsolete) — Splinter Review
Attachment #615064 - Flags: review?(bugmail.mozilla)
Attachment #614528 - Attachment is obsolete: true
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 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).
Attachment #615064 - Flags: review?(bugmail.mozilla) → review-
blocking-fennec1.0: --- → ?
Assignee: nobody → joe
Please do a complete risk assessment of all three pieces before landing, it seems like we could break a lot here.
blocking-fennec1.0: ? → beta+
Blocks: 744939
Depends on: 744901
Attachment #615064 - Attachment is obsolete: true
Attachment #616812 - Flags: review?(bugmail.mozilla)
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 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.
Attachment #616812 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/ec943d586046
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
You need to log in before you can comment on or make changes to this bug.