879004 - Add pixel units to FrameMetrics.h fields

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

+++ 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.

Comment 1

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

Attachment: Part 1 - Convert mDisplayPort and mCriticalDisplayPort in FrameMetrics.h

Comment 2

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

Attachment: Part 2 - Move template bodies to header file

Comment 3

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

Attachment: Part 3 - Convert FrameMetrics.mViewport

Comment 4

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

Attachment: Part 4 - Cleanups for coding style consistency and documentation

Comment 5

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

Attachment: Part 5 - Convert the async scroll offset fields in APZC.h

Comment 6

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

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.

Comment 7

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

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)

Comment 8

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

Attachment: Part 3 - Convert FrameMetrics.mViewport

Comment 9

[Benoit Girard (:BenWa)]

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 10

[Benoit Girard (:BenWa)]

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 11

[Benoit Girard (:BenWa)]

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.

Comment 12

[Benoit Girard (:BenWa)]

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.

Comment 13

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

(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 14

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

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.

Comment 15

[Benoit Girard (:BenWa)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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 16

[Benoit Girard (:BenWa)]

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 17

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

Attachment: Part 6 - Convert GeckoContentController.h

Might as well do the rest of GeckoContentController in this bug too.

Comment 18

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

Attachment: Part 1 - Convert mDisplayPort and mCriticalDisplayPort in FrameMetrics.h (v2)

Updated part 1 to add a Scale function to BaseRect so that I can create that displayport instance in a more type-safe way.

Comment 19

[Benoit Girard (:BenWa)]

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 20

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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?

Comment 21

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

Attachment: Part 1 - Convert mDisplayPort and mCriticalDisplayPort in FrameMetrics.h (v3)

(In reply to Benoit Girard (:BenWa) from comment #19)
> 
> What about using T instead of double then?

Good call, done.

Comment 22

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

(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.

Comment 23

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

Filed bug 880676 to track that change.

Comment 24

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6b0d5080b1d6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2dcce926842b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fe9614a72a31
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ee37be8f24c2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b764370de1f4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/af9197cbecdc

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6b0d5080b1d6
https://hg.mozilla.org/mozilla-central/rev/2dcce926842b
https://hg.mozilla.org/mozilla-central/rev/fe9614a72a31
https://hg.mozilla.org/mozilla-central/rev/ee37be8f24c2
https://hg.mozilla.org/mozilla-central/rev/b764370de1f4
https://hg.mozilla.org/mozilla-central/rev/af9197cbecdc

Comment 26

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

Attachment: Backport for v1.1.0hd

Comment 27

[Wayne Chang [:wchang]]

blocks hd+

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/8cd648ffcfa7