If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Use LayoutDevicePixel type still more in widget/

RESOLVED FIXED in Firefox 45

Status

()

Core
Widget
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
I have yet more patches to increase the usage of LayoutDevicePixel coordinates in widget/.
(Assignee)

Updated

2 years ago
No longer blocks: 1224482
(Assignee)

Comment 1

2 years ago
Created attachment 8693961 [details] [diff] [review]
(part 1) - Make nsIWidget::{Create,CreateChildren}() take a LayoutDeviceIntRect
Attachment #8693961 - Flags: review?(botond)
(Assignee)

Comment 2

2 years ago
Created attachment 8693963 [details] [diff] [review]
(part 2) - Make nsIWidget::DrawWindowUnderlay() take a LayoutDeviceIntRect
Attachment #8693963 - Flags: review?(botond)
(Assignee)

Comment 3

2 years ago
Created attachment 8693975 [details] [diff] [review]
(part 3) - Make nsIWidget::Invalidate() take a LayoutDeviceIntRect
Attachment #8693975 - Flags: review?(botond)
(Assignee)

Comment 4

2 years ago
Created attachment 8693976 [details] [diff] [review]
(part 4) - Make ThemeGeometry::mRect a LayoutDeviceIntRect
Attachment #8693976 - Flags: review?(botond)
(Assignee)

Comment 5

2 years ago
Created attachment 8694015 [details] [diff] [review]
(part 5) - Make Update{Opaque,WindowDragging}Region() take a LayoutDeviceIntRegion
Attachment #8694015 - Flags: review?(botond)
(Assignee)

Comment 6

2 years ago
Created attachment 8694483 [details] [diff] [review]
(part 3) - Make nsIWidget::Invalidate() take a LayoutDeviceIntRect

(Fixes some OS X compile errors.)
Attachment #8694483 - Flags: review?(botond)
(Assignee)

Updated

2 years ago
Attachment #8693975 - Attachment is obsolete: true
Attachment #8693975 - Flags: review?(botond)
(Assignee)

Comment 7

2 years ago
Created attachment 8694485 [details] [diff] [review]
(part 4) - Make ThemeGeometry::mRect a LayoutDeviceIntRect

(Fixes some OS X compile errors.)
Attachment #8694485 - Flags: review?(botond)
(Assignee)

Updated

2 years ago
Attachment #8693976 - Attachment is obsolete: true
Attachment #8693976 - Flags: review?(botond)
(Assignee)

Updated

2 years ago
Blocks: 1229665
Comment on attachment 8693961 [details] [diff] [review]
(part 1) - Make nsIWidget::{Create,CreateChildren}() take a LayoutDeviceIntRect

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

General note: |LayoutDeviceIntRect()| does the same thing as |LayoutDeviceIntRect(0, 0, 0, 0)|. I'll leave it up to you which you prefer as a stylistic choice.

::: widget/PuppetWidget.cpp
@@ -146,5 @@
>  
>  already_AddRefed<nsIWidget>
> -PuppetWidget::CreateChild(const nsIntRect  &aRect,
> -                          nsWidgetInitData *aInitData,
> -                          bool             aForceUseIWidgetParent)

I liked the alignment, but I guess strictly speaking it's not provided for in the style guide.

::: widget/nsBaseWidget.cpp
@@ +308,5 @@
>  // Basic create.
>  //
>  //-------------------------------------------------------------------------
>  void nsBaseWidget::BaseCreate(nsIWidget *aParent,
> +                              const LayoutDeviceIntRect &aRect,

Might as well move the & next to the type here too.
Attachment #8693961 - Flags: review?(botond) → review+
Comment on attachment 8693963 [details] [diff] [review]
(part 2) - Make nsIWidget::DrawWindowUnderlay() take a LayoutDeviceIntRect

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

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +809,5 @@
>  
>    // Allow widget to render a custom background.
> +  mCompositor->GetWidget()->DrawWindowUnderlay(
> +    this, LayoutDeviceIntRect(actualBounds.x, actualBounds.y,
> +                              actualBounds.width, actualBounds.height));

Consider |LayoutDeviceIntRect::FromUnknownRect(TruncatedToInt(actualBounds))|.

(Truncating from floats to ints is what we're doing here. In addition to being less verbose, this form makes that explicit.)
Attachment #8693963 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #9)
> Comment on attachment 8693963 [details] [diff] [review]
> (part 2) - Make nsIWidget::DrawWindowUnderlay() take a LayoutDeviceIntRect
> 
> Review of attachment 8693963 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/LayerManagerComposite.cpp
> @@ +809,5 @@
> >  
> >    // Allow widget to render a custom background.
> > +  mCompositor->GetWidget()->DrawWindowUnderlay(
> > +    this, LayoutDeviceIntRect(actualBounds.x, actualBounds.y,
> > +                              actualBounds.width, actualBounds.height));
> 
> Consider
> |LayoutDeviceIntRect::FromUnknownRect(TruncatedToInt(actualBounds))|.
> 
> (Truncating from floats to ints is what we're doing here. In addition to
> being less verbose, this form makes that explicit.)

Oh, I guess we don't actually have a TruncatedToInt() for rects, only for points [1]. We do have RoundedToInt() for both points [2] and rects [3], so this is probably a good opportunity to add a TruncatedToInt() for rects.

[1] https://dxr.mozilla.org/mozilla-central/rev/470f4f8c2b2d6f82e56e161a4b05262c85f55b59/gfx/2d/Point.h#107
[2] https://dxr.mozilla.org/mozilla-central/rev/470f4f8c2b2d6f82e56e161a4b05262c85f55b59/gfx/2d/Point.h#101
[3] https://dxr.mozilla.org/mozilla-central/rev/470f4f8c2b2d6f82e56e161a4b05262c85f55b59/gfx/2d/Rect.h#213

Updated

2 years ago
Attachment #8694015 - Flags: review?(botond) → review+
Comment on attachment 8694483 [details] [diff] [review]
(part 3) - Make nsIWidget::Invalidate() take a LayoutDeviceIntRect

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

::: widget/gtk/nsWindow.cpp
@@ +2451,5 @@
>      // of toplevels.)
>      if (mBounds.width < size.width) {
>          GdkRectangle rect = DevicePixelsToGdkRectRoundOut(
> +            LayoutDeviceIntRect(mBounds.width, 0,
> +                                size.width - mBounds.width, size.height));

(You should still be able to use the { ... } syntax. If you're choosing not to for stylistic reasons, that's fine.)
Attachment #8694483 - Flags: review?(botond) → review+

Updated

2 years ago
Attachment #8694485 - Flags: review?(botond) → review+
(Assignee)

Comment 12

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/dccb1af2b609e003e6dcc9726ea73070eadb30a3
Bug 1229237 (part 1) - Make nsIWidget::{Create,CreateChildren}() take a LayoutDeviceIntRect. r=botond.

https://hg.mozilla.org/integration/mozilla-inbound/rev/81cb37b5d061b7e51ab465482a219315867b2535
Bug 1229237 (part 2) - Make nsIWidget::DrawWindowUnderlay() take a LayoutDeviceIntRect. r=botond.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e091d14c936c948a490457a6420261abe8c16cc6
Bug 1229237 (part 3) - Make nsIWidget::Invalidate() take a LayoutDeviceIntRect. r=botond.

https://hg.mozilla.org/integration/mozilla-inbound/rev/de467557ee9ef4d31e1674bc5595dbe9c9cc8834
Bug 1229237 (part 4) - Make ThemeGeometry::mRect a LayoutDeviceIntRect. r=botond.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1167d19ab51196d6e075017c949dea459e21abf
Bug 1229237 (part 5) - Make Update{Opaque,WindowDragging}Region() take a LayoutDeviceIntRegion. r=botond.

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dccb1af2b609
https://hg.mozilla.org/mozilla-central/rev/81cb37b5d061
https://hg.mozilla.org/mozilla-central/rev/e091d14c936c
https://hg.mozilla.org/mozilla-central/rev/de467557ee9e
https://hg.mozilla.org/mozilla-central/rev/d1167d19ab51
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment on attachment 8693961 [details] [diff] [review]
(part 1) - Make nsIWidget::{Create,CreateChildren}() take a LayoutDeviceIntRect

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

::: widget/cocoa/nsCocoaWindow.mm
@@ +254,2 @@
>                                 nsNativeWidget aNativeParent,
> +                               const LayoutDeviceIntRect& aRect,

FWIW, this isn't correct; as the comment above says, the rect here is in "global display pixels", which are not the same as "layout device pixels". This applies to several of the changes here (e.g. in the static functions above)... not every use of nsIntRect was in fact for "device pixels".

And that means the caller of this function must be wrong, too.

(I happened to notice this because it conflicts with patches I have in progress for bug 890156, where I introduce a separate DesktopPixel type to address this. I'll try to disentangle things there...)
(Assignee)

Comment 15

2 years ago
> (I happened to notice this because it conflicts with patches I have in
> progress for bug 890156, where I introduce a separate DesktopPixel type to
> address this. I'll try to disentangle things there...)

Sorry for the errors, and thank you for the future distentangling.
You need to log in before you can comment on or make changes to this bug.