Closed Bug 1358758 Opened 3 years ago Closed 3 years ago

Use CSSIntRect for nsIFrame::GetScreenRect

Categories

(Core :: Layout, enhancement)

50 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(1 file)

For example, bug 1197497 would have been easier to spot if it had been CSSIntRect. Also I found some actual bugs (mix-up of dev pixels and CSS pixels) when I'm writing a patch for this bug.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
This patch assumes that bug 1197497 will fix nsGlobalWindow::NotifyDefaultButtonLoaded.
Depends on: 1197497
Comment on attachment 8860616 [details]
Bug 1358758 - Use CSSIntRect for nsIFrame::GetScreenRect.

https://reviewboard.mozilla.org/r/132618/#review135552

::: dom/plugins/base/nsPluginInstanceOwner.cpp:108
(Diff revision 1)
> +static inline nsPoint AsNsPoint(const CSSIntPoint &p) {
> +  return nsPoint(p.x, p.y);
> +}
> +
>  static inline nsPoint AsNsPoint(const nsIntPoint &p) {
>    return nsPoint(p.x, p.y);

These functions seem inherently wrong, because nsPoint is supposed to be a value in app units. Converting blindly from a nsIntPoint or a CSSIntPoint to an nsPoint by definition means that there's a coordinate mismatch here. However in this case it's a pre-existing problem so I won't hold your patch up for this. It might be nice to add a comment here to either explain this behaviour (if you know what's going on) or indicate that this might be wrong.

::: layout/generic/nsFrame.cpp:6103
(Diff revision 1)
> -  return GetScreenRectInAppUnits().ToNearestPixels(PresContext()->AppUnitsPerCSSPixel());
> +  return CSSIntRect::FromUnknownRect(
> +    GetScreenRectInAppUnits().ToNearestPixels(AppUnitsPerCSSPixel()));

I would prefer if you added a function to the CSSPixel class in layout/base/Units.h with this signature:

static CSSIntRect FromAppUnitsToNearest(const nsRect& aRect) { ... }

and then use that here to abstract away the "FromUnknownRect" call that you're doing here. There is a similar function in the LayoutDevicePixel class [1], there just isn't one for CSSPixel because we never needed one so far.

[1] http://searchfox.org/mozilla-central/rev/313e5199bf58200f158c6fcbe193e41b88ed58a6/layout/base/Units.h#327

::: layout/generic/nsIFrame.h:2589
(Diff revision 1)
> -   * Get the screen rect of the frame in pixels.
> -   * @return the pixel rect of the frame in screen coordinates.
> +   * Get the screen rect of the frame in CSS pixels.
> +   * @return the CSS pixel rect of the frame in screen coordinates.

This doesn't make much sense to me. "screen pixels" are different from "CSS pixels" so assuming the code change is correct we should really rename the function. But even if we don't want to rename it, please at least update the documentation to more clearly describe what exactly this is returning, and what it means for a "screen rect" to be in CSS pixels.

::: widget/nsBaseDragService.cpp:610
(Diff revision 1)
>        if (frame) {
>          dragRect = frame->GetScreenRect();
>        }
>      }
>  
> -    dragRect = ToAppUnits(dragRect, nsPresContext::AppUnitsPerCSSPixel()).
> +    nsIntRect dragRectApp =

why "dragRectApp"? The RHS of this expression is returning a value in "device pixels", not app units. Please rename this variable to dragRectRoundedOut or something like that.
Attachment #8860616 - Flags: review?(bugmail) → review+
Comment on attachment 8860616 [details]
Bug 1358758 - Use CSSIntRect for nsIFrame::GetScreenRect.

https://reviewboard.mozilla.org/r/132618/#review135552

> These functions seem inherently wrong, because nsPoint is supposed to be a value in app units. Converting blindly from a nsIntPoint or a CSSIntPoint to an nsPoint by definition means that there's a coordinate mismatch here. However in this case it's a pre-existing problem so I won't hold your patch up for this. It might be nice to add a comment here to either explain this behaviour (if you know what's going on) or indicate that this might be wrong.

I changed nsPoint to CSSIntPoint throughout the function because it is the coordinate system that the function expects.

I intentionally kept the rounding behavior because Flash Player might depend on the current behavior.

> I would prefer if you added a function to the CSSPixel class in layout/base/Units.h with this signature:
> 
> static CSSIntRect FromAppUnitsToNearest(const nsRect& aRect) { ... }
> 
> and then use that here to abstract away the "FromUnknownRect" call that you're doing here. There is a similar function in the LayoutDevicePixel class [1], there just isn't one for CSSPixel because we never needed one so far.
> 
> [1] http://searchfox.org/mozilla-central/rev/313e5199bf58200f158c6fcbe193e41b88ed58a6/layout/base/Units.h#327

Done.

> This doesn't make much sense to me. "screen pixels" are different from "CSS pixels" so assuming the code change is correct we should really rename the function. But even if we don't want to rename it, please at least update the documentation to more clearly describe what exactly this is returning, and what it means for a "screen rect" to be in CSS pixels.

Oh, I did not realize that the term "screen pixels" is sometimes used as a synonym of "device pixels". I modified the comment to clarify.

> why "dragRectApp"? The RHS of this expression is returning a value in "device pixels", not app units. Please rename this variable to dragRectRoundedOut or something like that.

I missed ToOutsidePixels due to the strange indent. Renamed the variable to "dragRectDev" and fixed the indent.
Comment on attachment 8860616 [details]
Bug 1358758 - Use CSSIntRect for nsIFrame::GetScreenRect.

https://reviewboard.mozilla.org/r/132618/#review136280

::: dom/plugins/base/nsPluginInstanceOwner.cpp:1077
(Diff revisions 1 - 2)
> -  nsPoint chromeSize = AsNsPoint(rootWidget->GetChromeDimensions()) / scaleFactor;
> +  CSSIntPoint chromeSize =
> +    CSSIntPoint::FromUnknownPoint(rootWidget->GetChromeDimensions()) /
> +    scaleFactor;

The rootWidget->GetChromeDimensions() function returns an nsIntPoint but conceptually it is a LayoutDeviceIntPoint. If you want to be really correct, you could rewrite this something like so (not sure about the rounding):

CSSIntPoint chromeSize = CSSIntPoint::FromAppUnitsRounded(LayoutDeviceIntPoint::ToAppUnits(LayoutDeviceIntPoint::FromUnknownPoint(rootWidget->GetChromeDimensions()), presContext->DeviceContext()->AppUnitsPerDevPixelAtUnitFullZoom()));

Note that the implementation for GetChromeDimensions [1] actually gets a LayoutDeviceIntPoint before calling ToUnknownPoint() on it. Also note that the scaleFactor value is just a conversion from LayoutDevice space (at unit full zoom) to CSS space. These spaces are documented in layout/base/Units.h if you need background. There is also some background in [2] if you need it.

I suspect similar changes could be made below everywhere that `scaleFactor` is used.

[1] http://searchfox.org/mozilla-central/rev/baf47b352e873d4516d7656186d6d7c7447d3873/widget/PuppetWidget.cpp#1282
[2] https://staktrace.com/spout/entry.php?id=800
(The rest of your changes look fine, thanks!)
Comment on attachment 8860616 [details]
Bug 1358758 - Use CSSIntRect for nsIFrame::GetScreenRect.

https://reviewboard.mozilla.org/r/132618/#review136280

> The rootWidget->GetChromeDimensions() function returns an nsIntPoint but conceptually it is a LayoutDeviceIntPoint. If you want to be really correct, you could rewrite this something like so (not sure about the rounding):
> 
> CSSIntPoint chromeSize = CSSIntPoint::FromAppUnitsRounded(LayoutDeviceIntPoint::ToAppUnits(LayoutDeviceIntPoint::FromUnknownPoint(rootWidget->GetChromeDimensions()), presContext->DeviceContext()->AppUnitsPerDevPixelAtUnitFullZoom()));
> 
> Note that the implementation for GetChromeDimensions [1] actually gets a LayoutDeviceIntPoint before calling ToUnknownPoint() on it. Also note that the scaleFactor value is just a conversion from LayoutDevice space (at unit full zoom) to CSS space. These spaces are documented in layout/base/Units.h if you need background. There is also some background in [2] if you need it.
> 
> I suspect similar changes could be made below everywhere that `scaleFactor` is used.
> 
> [1] http://searchfox.org/mozilla-central/rev/baf47b352e873d4516d7656186d6d7c7447d3873/widget/PuppetWidget.cpp#1282
> [2] https://staktrace.com/spout/entry.php?id=800

Good point. I used CSSToLayoutDeviceScale and CSSIntPoint::Truncate instead because FromAppUnitsRounded will change the behavior.
Comment on attachment 8860616 [details]
Bug 1358758 - Use CSSIntRect for nsIFrame::GetScreenRect.

https://reviewboard.mozilla.org/r/132618/#review136868

::: dom/plugins/base/nsPluginInstanceOwner.cpp:108
(Diff revision 4)
> -static inline nsPoint AsNsPoint(const nsIntPoint &p) {
> -  return nsPoint(p.x, p.y);
> +static inline CSSIntPoint AsCSSIntPoint(const nsPoint &p) {
> +  return CSSIntPoint(p.x, p.y);

Calls to this function can be replaced by e.g. CSSIntPoint::Floor(CSSPoint::FromAppUnits(p));

or you can use CSSIntPoint::Truncate or CSSIntPoint::Round as needed. These are all defined around http://searchfox.org/mozilla-central/rev/baf47b352e873d4516d7656186d6d7c7447d3873/gfx/2d/Point.h#184,191,198,205
Note that CSSPoint::FromAppUnits does the division by the app units conversion factor internally [1], so if you use that you wouldn't need to divide it manually at the call sites.

[1] http://searchfox.org/mozilla-central/rev/876c7dd30586f9c6f9c99ef7444f2d73c7acfe7c/layout/base/Units.h#201
Comment on attachment 8860616 [details]
Bug 1358758 - Use CSSIntRect for nsIFrame::GetScreenRect.

https://reviewboard.mozilla.org/r/132618/#review136868

> Calls to this function can be replaced by e.g. CSSIntPoint::Floor(CSSPoint::FromAppUnits(p));
> 
> or you can use CSSIntPoint::Truncate or CSSIntPoint::Round as needed. These are all defined around http://searchfox.org/mozilla-central/rev/baf47b352e873d4516d7656186d6d7c7447d3873/gfx/2d/Point.h#184,191,198,205

Thank you. Removed AsCSSIntPoint in favor of CSSIntPoint::Truncate(CSSPoint::FromAppUnits(p));.
https://hg.mozilla.org/mozilla-central/rev/ce13c8b07ca7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.