Closed Bug 1229237 Opened 8 years ago Closed 8 years ago

Use LayoutDevicePixel type still more in widget/

Categories

(Core :: Widget, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(5 files, 2 obsolete files)

I have yet more patches to increase the usage of LayoutDevicePixel coordinates in widget/.
No longer blocks: 1224482
(Fixes some OS X compile errors.)
Attachment #8694483 - Flags: review?(botond)
Attachment #8693975 - Attachment is obsolete: true
Attachment #8693975 - Flags: review?(botond)
(Fixes some OS X compile errors.)
Attachment #8694485 - Flags: review?(botond)
Attachment #8693976 - Attachment is obsolete: true
Attachment #8693976 - Flags: review?(botond)
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
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+
Attachment #8694485 - Flags: review?(botond) → review+
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 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...)
> (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.