Closed
Bug 1229237
Opened 9 years ago
Closed 9 years ago
Use LayoutDevicePixel type still more in widget/
Categories
(Core :: Widget, defect)
Core
Widget
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(5 files, 2 obsolete files)
57.33 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
6.00 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
9.44 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
39.85 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
11.87 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
I have yet more patches to increase the usage of LayoutDevicePixel coordinates in widget/.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8693961 -
Flags: review?(botond)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8693963 -
Flags: review?(botond)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8693975 -
Flags: review?(botond)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8693976 -
Flags: review?(botond)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8694015 -
Flags: review?(botond)
Assignee | ||
Comment 6•9 years ago
|
||
(Fixes some OS X compile errors.)
Attachment #8694483 -
Flags: review?(botond)
Assignee | ||
Updated•9 years ago
|
Attachment #8693975 -
Attachment is obsolete: true
Attachment #8693975 -
Flags: review?(botond)
Assignee | ||
Comment 7•9 years ago
|
||
(Fixes some OS X compile errors.)
Attachment #8694485 -
Flags: review?(botond)
Assignee | ||
Updated•9 years ago
|
Attachment #8693976 -
Attachment is obsolete: true
Attachment #8693976 -
Flags: review?(botond)
Comment 8•9 years ago
|
||
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 9•9 years ago
|
||
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+
Comment 10•9 years ago
|
||
(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•9 years ago
|
Attachment #8694015 -
Flags: review?(botond) → review+
Comment 11•9 years ago
|
||
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•9 years ago
|
Attachment #8694485 -
Flags: review?(botond) → review+
Assignee | ||
Comment 12•9 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•9 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
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 14•9 years ago
|
||
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•9 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.
Description
•