Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Description • 11 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 6 • 11 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 7 • 11 years ago

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)

	Benoit Girard (:BenWa)
	Comment 9 • 11 years ago

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?

	Benoit Girard (:BenWa)
	Comment 10 • 11 years ago

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?

	Benoit Girard (:BenWa)
	Comment 11 • 11 years ago

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.

	Benoit Girard (:BenWa)
	Comment 12 • 11 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 13 • 11 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 14 • 11 years ago

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.

	Benoit Girard (:BenWa)
	Comment 15 • 11 years ago

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

	Benoit Girard (:BenWa)
	Comment 16 • 11 years ago

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?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 17 • 11 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 18 • 11 years ago

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

	Benoit Girard (:BenWa)
	Comment 19 • 11 years ago

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?

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)
	Comment 20 • 11 years ago

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?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 21 • 11 years ago

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

Good call, done.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 22 • 11 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 23 • 11 years ago

Filed bug 880676 to track that change.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 24 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 11 years ago

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

	Wayne Chang [:wchang]
	Comment 27 • 11 years ago

blocks hd+

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 11 years ago

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