Add pixel units to FrameMetrics.h fields

RESOLVED FIXED in Firefox 24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

23 Branch
mozilla24
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:hd+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 wontfix, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

Attachments

(7 attachments, 3 obsolete attachments)

3.70 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
14.73 KB, patch
BenWa
: review+
ajones
: review+
Details | Diff | Splinter Review
10.74 KB, patch
ajones
: review+
Details | Diff | Splinter Review
15.15 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
19.78 KB, patch
ajones
: review+
Details | Diff | Splinter Review
34.33 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
3.00 KB, patch
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #877728 +++

Follow-up from bug 865735 - this is an incremental propagation of pixel-units information to some uses of gfx::Point, gfx::Size, and gfx::Rect.
I've pushed these patches to try at https://tbpl.mozilla.org/?tree=Try&rev=58e43e6104bb. I'm a little concerned about the changes in parts 1 and 3 that touch nsDisplayList.cpp and RenderFrameParent.cpp, because the old code was using auPerDevPixel to convert from app units to CSS pixels. I think the old code was broken and the new code is correct, but my patch might cause breakage because of a "two wrongs make a right" situation. I'll flag the patches for review once the tests are green.
The try run was green but I found some more code that is probably affected by this and so will spend some more time to stare at that. (http://hg.mozilla.org/mozilla-central/file/tip/layout/ipc/RenderFrameParent.cpp#l414 if anybody cares)
Attachment #758790 - Flags: review?(bgirard)
Attachment #758791 - Flags: review?(bgirard)
Attachment #758792 - Attachment is obsolete: true
Attachment #759314 - Flags: review?(bgirard)
Attachment #759314 - Attachment description: Convert FrameMetrics.mViewport → Part 3 - Convert FrameMetrics.mViewport
Attachment #758793 - Flags: review?(bgirard)
Attachment #758796 - Flags: review?(ajones)
Comment on attachment 758790 [details] [diff] [review]
Part 1 - Convert mDisplayPort and mCriticalDisplayPort in FrameMetrics.h

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +939,5 @@
>    CSSPoint scrollOffset = aFrameMetrics.mScrollOffset;
>  
> +  CSSRect displayPort(0, 0,
> +                      compositionBounds.width * gXStationarySizeMultiplier,
> +                      compositionBounds.height * gYStationarySizeMultiplier);

compositionBounds is already CSSIntRect. This should be a safe conversion. Also is CSSIntRect * StationSize = CSSRect the right thing or shouldn't that be mapped to layer units?
Comment on attachment 758791 [details] [diff] [review]
Part 2 - Move template bodies to header file

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

What the motivation for this change?
Comment on attachment 759314 [details] [diff] [review]
Part 3 - Convert FrameMetrics.mViewport

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

r+ with a better comment for the last hunk.

::: dom/ipc/TabChild.cpp
@@ +346,5 @@
>          // Calculate a really simple resolution that we probably won't
>          // be keeping, as well as putting the scroll offset back to
>          // the top-left of the page.
>          mLastMetrics.mZoom = gfxSize(1.0, 1.0);
> +        mLastMetrics.mViewport = CSSRect(CSSPoint(), kDefaultViewportSize);

I guess these constructor are still unsafe? That's sad :(

@@ +559,2 @@
>    }
> +  NS_ENSURE_TRUE_VOID(pageSize.width); // (return early rather than divide by 0)

not your fault but eww. if (blah) return; is so much easier to read.

::: layout/ipc/RenderFrameParent.cpp
@@ +407,5 @@
>        view->mParentScaleX = aAccConfigXScale;
>        view->mParentScaleY = aAccConfigYScale;
>      }
>  
> +    // I have no idea what units mViewportSize is supposed to be in

These comments are a bit of a pet-peeve of mine. They provide no value in the code and I always wonder why they made it in the code base. Can we put something more informative here? Otherwise having ViewportSize in unknown units should present convey this information.
Attachment #759314 - Flags: review?(bgirard) → review+
Comment on attachment 758793 [details] [diff] [review]
Part 4 - Cleanups for coding style consistency and documentation

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

::: layout/base/Units.h
@@ +45,5 @@
>  typedef gfx::RectTyped<CSSPixel> CSSRect;
>  typedef gfx::IntRectTyped<CSSPixel> CSSIntRect;
>  
> +/*
> + * The pixels that layout rasterizes and delivers to the graphics code.

Nice definitions. Wouldn't hurt to get someone else to verify them.
Attachment #758793 - Flags: review?(bgirard) → review+
(In reply to Benoit Girard (:BenWa) from comment #9)
> > +  CSSRect displayPort(0, 0,
> > +                      compositionBounds.width * gXStationarySizeMultiplier,
> > +                      compositionBounds.height * gYStationarySizeMultiplier);
> 
> compositionBounds is already CSSIntRect. This should be a safe conversion.
> Also is CSSIntRect * StationSize = CSSRect the right thing or shouldn't that
> be mapped to layer units?

gXStationarySizeMultiplier and gYStationarySizeMultiplier are unitless multipliers. Ideally I would want to write this something like

CSSRect displayPort = CSSRect(compositionBounds).Scale(gXStationarySizeMultiplier, gYStationarySizeMultiplier).MoveTo(0, 0)

but for that I would need to add a Scale function to BaseRect. If you're ok with that I can make the change.

(In reply to Benoit Girard (:BenWa) from comment #10)
> Comment on attachment 758791 [details] [diff] [review]
> Part 2 - Move template bodies to header file
> 
> Review of attachment 758791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What the motivation for this change?

Link error. Templated things have to live in .h files for the compiler to generate all the necessary expansions. In this case specifically it was kind of an interesting problem. Up until the changes in part 3, it would work fine because the only expansion required of the AppendToString(... gfx::RectTyped<T> ...) function was the one with T=UnknownUnits. However in part 3, I change FrameMetrics.mViewport from UnknownUnits to CSSPixel, and so now there are two expansions of AppendToString needed (one for CSSPixel and one for UnknownUnits). The one for CSSPixel was getting generated properly because of other code in LayersLogging.cpp which was invoking it, but the one for UnknownUnits was not getting generated. It would compile because of the .h file but then fail to link because it couldn't find the implementation with T=UnknownUnits.

(In reply to Benoit Girard (:BenWa) from comment #11)
> > +        mLastMetrics.mViewport = CSSRect(CSSPoint(), kDefaultViewportSize);
> 
> I guess these constructor are still unsafe? That's sad :(

What would you prefer to see here? I think this is relatively safe, because if kDefaultViewportSize were (for example) a LayerSize then this wouldn't compile. It has to be a CSSSize for this to work.

> @@ +559,2 @@
> > +  NS_ENSURE_TRUE_VOID(pageSize.width); // (return early rather than divide by 0)
> 
> not your fault but eww. if (blah) return; is so much easier to read.

Agreed, I can change this.

> > +    // I have no idea what units mViewportSize is supposed to be in
> 
> These comments are a bit of a pet-peeve of mine. They provide no value in
> the code and I always wonder why they made it in the code base. Can we put
> something more informative here? Otherwise having ViewportSize in unknown
> units should present convey this information.

Ok, I will update the comment.
Comment on attachment 758793 [details] [diff] [review]
Part 4 - Cleanups for coding style consistency and documentation

Adding Anthony Jones to the review for this one to double-check that the definitions make sense.
Attachment #758793 - Flags: review?(ajones)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> but for that I would need to add a Scale function to BaseRect. If you're ok
> with that I can make the change.

I'll leave that up to you. It's fine without it.

> 
> (In reply to Benoit Girard (:BenWa) from comment #10)
> > Comment on attachment 758791 [details] [diff] [review]
> > Part 2 - Move template bodies to header file
> > 
> > Review of attachment 758791 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > What the motivation for this change?
> 
> Link error.

Ok, I suspected that but I don't have the template limitation memorized.

> 
> (In reply to Benoit Girard (:BenWa) from comment #11)
> > > +        mLastMetrics.mViewport = CSSRect(CSSPoint(), kDefaultViewportSize);
> > 
> > I guess these constructor are still unsafe? That's sad :(
> 
> What would you prefer to see here? I think this is relatively safe, because
> if kDefaultViewportSize were (for example) a LayerSize then this wouldn't
> compile. It has to be a CSSSize for this to work.

Ohh I didn't think that would throw a compile error. Nice. This is fine as is.
Comment on attachment 758791 [details] [diff] [review]
Part 2 - Move template bodies to header file

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

What the motivation for this change?
Attachment #758791 - Flags: review?(bgirard) → review+
Might as well do the rest of GeckoContentController in this bug too.
Attachment #759454 - Flags: review?(ajones)
Updated part 1 to add a Scale function to BaseRect so that I can create that displayport instance in a more type-safe way.
Attachment #758790 - Attachment is obsolete: true
Attachment #758790 - Flags: review?(bgirard)
Attachment #759455 - Flags: review?(bgirard)
Comment on attachment 759455 [details] [diff] [review]
Part 1 - Convert mDisplayPort and mCriticalDisplayPort in FrameMetrics.h (v2)

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

::: gfx/2d/BaseRect.h
@@ +351,5 @@
> +  // Scale 'this' by aXScale and aYScale, without doing any explicit rounding.
> +  // Note: It is inadvisable to use this when T is an integer type because the
> +  // result will get static_cast'ed from a double to an int, which may give
> +  // undesirable results.
> +  void Scale(double aXScale, double aYScale)

What about using T instead of double then?
Comment on attachment 758793 [details] [diff] [review]
Part 4 - Cleanups for coding style consistency and documentation

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

::: layout/base/Units.h
@@ +56,2 @@
>  struct LayerPixel {
> +  static gfx::IntPointTyped<LayerPixel> FromCSSPointRounded(const CSSPoint& aPoint, float aResolutionX, float aResolutionY) {

Resolution should be a class. We could use BaseSize<Scale> perhaps?
Attachment #758796 - Flags: review?(ajones) → review+
(In reply to Benoit Girard (:BenWa) from comment #19)
> 
> What about using T instead of double then?

Good call, done.
Attachment #759455 - Attachment is obsolete: true
Attachment #759455 - Flags: review?(bgirard)
Attachment #759723 - Flags: review?(bgirard)
(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #20)
>
> Resolution should be a class. We could use BaseSize<Scale> perhaps?

Yeah, I think I've been coming to the same conclusion. I'm not sure if BaseSize<Scale> is the right one to use, we might need to define a new class. I'd like to think about this more and bump it to another bug, since the changes will be more involved and are outside the scope of this bug.
Filed bug 880676 to track that change.
Attachment #759723 - Flags: review?(bgirard) → review+
Attachment #759454 - Flags: review?(ajones) → review+
Attachment #758793 - Flags: review?(ajones) → review+

Updated

6 years ago
Blocks: 898389

Updated

6 years ago
blocking-b2g: --- → hd?
blocks hd+
blocking-b2g: hd? → hd+
Attachment #785118 - Attachment description: Backport for v1.1.0hd (uncompiled, untested) → Backport for v1.1.0hd
You need to log in before you can comment on or make changes to this bug.