Remove nsIWidget::Get{,Screen,Client}BoundsUntyped()

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(3 attachments)

Assignee

Description

4 years ago
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

4 years ago
This requires adding a new overloading of LayoutDevicePixel::ToAppUnits and a
new PixelCastJustification: LayoutDeviceIsParentLayerForRCDRSF.
Attachment #8692175 - Flags: review?(botond)
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 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 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+
Attachment #8692179 - Flags: review?(botond) → review+
Thanks for continuing with this work!
Assignee

Comment 8

4 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.

Comment 10

4 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: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.