Closed
Bug 1228125
Opened 8 years ago
Closed 8 years ago
Remove nsIWidget::Get{,Screen,Client}BoundsUntyped()
Categories
(Core :: Widget, defect)
Core
Widget
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
Attachments
(3 files)
26.33 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
4.05 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
6.25 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
nsIWidget has typed and untyped versions of three bounds-getting functions. We're now at a point where we can get rid of the untyped versions.
Assignee | ||
Comment 1•8 years ago
|
||
This requires adding a new overloading of LayoutDevicePixel::ToAppUnits and a new PixelCastJustification: LayoutDeviceIsParentLayerForRCDRSF.
Attachment #8692175 -
Flags: review?(botond)
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8692178 -
Flags: review?(botond)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8692179 -
Flags: review?(botond)
Comment 4•8 years ago
|
||
Comment on attachment 8692175 [details] [diff] [review] (part 1) - Remove nsIWidget::GetBoundsUntyped() Review of attachment 8692175 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/UnitTransforms.h @@ +44,5 @@ > // Similar to LayoutDeviceIsScreenForUntransformedEvent, PBrowser handles > // some widget/tab dimension information as the OS does -- in screen units. > + LayoutDeviceIsScreenForTabDims, > + // Since the root scroll frame of the root content document is outside > + // any resolution that may be in effect, the LayoutDevicePixel is equivalent I think this conversion is sound, but I find the description a bit misleading; the RCD-RSF isn't outside the resolution, its ParentLayer space is. I would be happy describing this as just a combination of LayoutDeviceIsScreenForBounds and ScreenIsParentLayerForRoot, which is how we're using it. ::: layout/base/nsPresContext.cpp @@ +3089,3 @@ > for (uint32_t i = 0; i < aClipRects1.Length(); ++i) { > for (uint32_t j = 0; j < aClipRects2.Length(); ++j) { > + if ((aClipRects1[i] + offsetDelta.ToUnknownPoint()).Intersects(aClipRects2[j])) Maybe keep offsetDelta as untyped, instead of calling ToUnknownPoint() on each loop iteration? ::: widget/windows/nsWindow.cpp @@ +2678,5 @@ > > // Find the largest rectangle and use that to calculate the inset. Our top > // priority is to include the bounds of all plugins. > + LayoutDeviceIntRect largest = > + aOpaqueRegion.GetLargestRectangle(pluginBounds); I don't understand how this works - if aOpaqueRegion is an nsIntRegion, doesn't it expect an nsIntRect as its argument and return an nsIntRect?
Attachment #8692175 -
Flags: review?(botond) → review+
Comment 5•8 years ago
|
||
Comment on attachment 8692175 [details] [diff] [review] (part 1) - Remove nsIWidget::GetBoundsUntyped() Review of attachment 8692175 [details] [diff] [review]: ----------------------------------------------------------------- ::: view/nsView.cpp @@ +802,5 @@ > if (nullptr != mWindow) { > nscoord p2a = mViewManager->AppUnitsPerDevPixel(); > + LayoutDeviceIntRect rect; > + mWindow->GetClientBounds(rect); > + nsRect windowBounds = LayoutDevicePixel::ToAppUnits(rect, p2a); General comment: we generally prefer to call things like this as LayoutDeviceIntRect::ToAppUnits(). (LayoutDeviceIntRect derives from LayoutDevicePixel).
Comment 6•8 years ago
|
||
Comment on attachment 8692178 [details] [diff] [review] (part 2) - Remove nsIWidget::GetScreenBoundsUntyped() Review of attachment 8692178 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/xul/PopupBoxObject.cpp @@ +288,3 @@ > > int32_t pp = menuPopupFrame->PresContext()->AppUnitsPerDevPixel(); > + rect->SetLayoutRect(LayoutDevicePixel::ToAppUnits(screenRect, pp)); LayoutDeviceIntRect::
Attachment #8692178 -
Flags: review?(botond) → review+
Updated•8 years ago
|
Attachment #8692179 -
Flags: review?(botond) → review+
Comment 7•8 years ago
|
||
Thanks for continuing with this work!
Assignee | ||
Comment 8•8 years ago
|
||
> ::: widget/windows/nsWindow.cpp
> @@ +2678,5 @@
> >
> > // Find the largest rectangle and use that to calculate the inset. Our top
> > // priority is to include the bounds of all plugins.
> > + LayoutDeviceIntRect largest =
> > + aOpaqueRegion.GetLargestRectangle(pluginBounds);
>
> I don't understand how this works - if aOpaqueRegion is an nsIntRegion,
> doesn't it expect an nsIntRect as its argument and return an nsIntRect?
Good catch. I had to insert a FromUnknownRegion() in there.
Assignee | ||
Comment 9•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cc1efabc37f8879530fe19aeed25b6cebaef5f2 Bug 1228125 (part 1) - Remove nsIWidget::GetBoundsUntyped(). r=botond. https://hg.mozilla.org/integration/mozilla-inbound/rev/71bf6683f1013c62aed6653228cf1260e9574a26 Bug 1228125 (part 2) - Remove nsIWidget::GetScreenBoundsUntyped(). r=botond. https://hg.mozilla.org/integration/mozilla-inbound/rev/ae9f5883e654131f73236505da292faff9fc06be Bug 1228125 (part 3) - Remove nsIWidget::GetClientBoundsUntyped(). r=botond.
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0cc1efabc37f https://hg.mozilla.org/mozilla-central/rev/71bf6683f101 https://hg.mozilla.org/mozilla-central/rev/ae9f5883e654
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•