Closed Bug 1225007 Opened 4 years ago Closed 4 years ago

Use LayoutDevicePixel more in Cocoa widget code

Categories

(Core :: Widget: Cocoa, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
I started unpicking the ball of string at ShowsResizeIndicator() and it didn't
stop unravelling until I'd converted a whole lot of Cocoa code.
Attachment #8687784 - Flags: review?(bugmail.mozilla)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Attached patch Apparent unit mismatch? (obsolete) — Splinter Review
It very much seems like Cocoa "dev pixels" == LayoutDevicePixels. The previous
patch assumes this, for example.

But CocoaPointsToDevPixels(const NSPoint& aPt) is producing points that are
interpreted as LayoutDeviceIntPoint in some places and ScreenPoint in other
places. The ScreenPoint ones don't match how I've been doing things so far and
are marked with "njn:" comments.

kats, any ideas about this?
Attachment #8687786 - Flags: feedback?(bugmail.mozilla)
(In reply to Nicholas Nethercote [:njn] from comment #2)
> Created attachment 8687786 [details] [diff] [review]
> Apparent unit mismatch?
> 
> It very much seems like Cocoa "dev pixels" == LayoutDevicePixels. The
> previous
> patch assumes this, for example.

Yeah, I believe that's correct.

> 
> But CocoaPointsToDevPixels(const NSPoint& aPt) is producing points that are
> interpreted as LayoutDeviceIntPoint in some places and ScreenPoint in other
> places. The ScreenPoint ones don't match how I've been doing things so far
> and
> are marked with "njn:" comments.
> 
> kats, any ideas about this?

I looked over these call sites and they all seem correct. You can use ViewAs to convert from LayoutDevice to Screen with the LayoutDeviceIsScreenForUntransformedEvent as the justification. All of those ScreenPoints are sent into the APZ code for unapplying the async pan/zoom transform, so it makes sense that they are interpreted as screen coordinates.
Attachment #8687786 - Flags: feedback?(bugmail.mozilla) → feedback+
Comment on attachment 8687784 [details] [diff] [review]
Use LayoutDevicePixel more in Cocoa widget code

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

::: gfx/src/nsRegion.h
@@ +798,5 @@
>    // between different types of BaseIntRegions.
>    explicit BaseIntRegion(const nsRegion& aImpl) : mImpl(aImpl) {}
> +  // This must be public so that FromUnknownRegion() works, but it should not
> +  // be otherwise used.
> +public:

It's not clear to me why this is needed. AFAICT FromUnknownRegion is in IntRegionTyped which extends from BaseIntRegion, so it should be able to access protected functions. The old code was calling Impl() as well and that worked just fine, what changed to make this necessary?

@@ +864,1 @@
>    explicit IntRegionTyped(const nsRegion& aRegion) : Super(aRegion) {}

Ditto here, not sure why this constructor needs to be public - am I missing something?

::: widget/cocoa/nsChildView.mm
@@ +4626,3 @@
>  {
>    nsTArray<CGRect> rects;
> +  nsIntRegionRectIterator iter(aRegion.ToUnknownRegion());

We should probably make this iterator strongly typed as well, and produce strongly typed rects. We don't have to do it in this patch/bug though.
Attachment #8687784 - Flags: review?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> ::: gfx/src/nsRegion.h
> @@ +798,5 @@
> >    // between different types of BaseIntRegions.
> >    explicit BaseIntRegion(const nsRegion& aImpl) : mImpl(aImpl) {}
> > +  // This must be public so that FromUnknownRegion() works, but it should not
> > +  // be otherwise used.
> > +public:
> 
> It's not clear to me why this is needed. AFAICT FromUnknownRegion is in
> IntRegionTyped which extends from BaseIntRegion, so it should be able to
> access protected functions. 

The problem is that there are different instances of the templates in play. E.g. IntRegionTyped<CSSPixel> has access to protected members of BaseIntRegion<CSSPixel>, but not to protected members of BaseIntRegion<UnknownUnits>, which is what we're trying to call here.

> The old code was calling Impl() as well and that
> worked just fine, what changed to make this necessary?

Turns out no one was actually instantiating this function thus far :)

> @@ +864,1 @@
> >    explicit IntRegionTyped(const nsRegion& aRegion) : Super(aRegion) {}
> 
> Ditto here, not sure why this constructor needs to be public - am I missing
> something?

Same reason as above.

I actually have a patch in bug 1208829 (the third patch, titled "Make FromUnknownRegion() actually work, and add ToUnknownRegion()") which addresses the same issues by using friendship. I can land that if you'd prefer.
I discussed this with Botond and yes, I would prefer the friendship approach rather than making this public.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> I discussed this with Botond and yes, I would prefer the friendship approach
> rather than making this public.

Ok, I'll wait for that bug to land.

Though it is interesting that IntRegionTyped is held to a higher standard than other types -- e.g. you can create a IntRectTyped from an nsRect because you can extract and pass in the four int values, but you can't do the equivalent thing with IntRegionTyped.
(In reply to Nicholas Nethercote [:njn] from comment #7)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> > I discussed this with Botond and yes, I would prefer the friendship approach
> > rather than making this public.
> 
> Ok, I'll wait for that bug to land.

I landed that patch specifically on m-i already, so you don't need to wait for the rest of the bug to land.

> Though it is interesting that IntRegionTyped is held to a higher standard
> than other types -- e.g. you can create a IntRectTyped from an nsRect
> because you can extract and pass in the four int values, but you can't do
> the equivalent thing with IntRegionTyped.

In my mind, that's because a Rect being composed out of x/y/width/height fields is part of its public interface, while the fact that an IntRegionTyped is implemented using an nsRegion is an implementation detail.
> I landed that patch specifically on m-i already, so you don't need to wait
> for the rest of the bug to land.

Great! Thank you.
Updated patch that doesn't touch nsRegion.h, now that Botond has fixed up
{To,From}UnknownRegion() elsewhere.
Attachment #8688100 - Flags: review?(bugmail.mozilla)
Attachment #8687784 - Attachment is obsolete: true
Comment on attachment 8688100 [details] [diff] [review]
Use LayoutDevicePixel more in Cocoa widget code

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

This patch still has the bit in nsRegion.h that makes the function public. r+ with that file reverted. Also commit message needs updating.
Attachment #8688100 - Flags: review?(bugmail.mozilla) → review+
Attachment #8688145 - Flags: review?(bugmail.mozilla) → review+
Attachment #8687786 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/52ba2412304630898842e4a42c5d5ffaff8853e8
Bug 1225007 (part 1) - Use LayoutDevicePixel more in Cocoa widget code. r=kats.

https://hg.mozilla.org/integration/mozilla-inbound/rev/66768d6a50fa4c24bd851b47b2ae0f69d78ff475
Bug 1225007 (part 2) - Use LayoutDevicePixel more in Cocoa widget code. r=kats.
https://hg.mozilla.org/integration/mozilla-inbound/rev/334fd45beb6c7508189f5c90563e9a10bff4d3e6
Backout 52ba24123046 and 66768d6a50fa (bug 1225007) for causing lots of test assertions on Mac. CLOSED TREE
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a513c70ce7c8542a9ba74bb903fbcd022d9aa14
Bug 1225007 (part 1, attempt 2) - Use LayoutDevicePixel more in Cocoa widget code. r=kats.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5b352c74b3594e984f04b472b70e78f56c2fd9c
Bug 1225007 (part 2, attempt 2) - Use LayoutDevicePixel more in Cocoa widget code. r=kats.
https://hg.mozilla.org/mozilla-central/rev/8a513c70ce7c
https://hg.mozilla.org/mozilla-central/rev/c5b352c74b35
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
BTW, the backout here was due to lots of "Shouldn't return empty rect" assertion failures in nsIntRegionRectIterator::Next(). Partly undoing the changes in NewCGSRegionFromRegion(), so that a ToUnknownRegion() call was avoided, was enough to fix it. I don't understand why this fixed it, though.
(In reply to Nicholas Nethercote [:njn] from comment #18)
> BTW, the backout here was due to lots of "Shouldn't return empty rect"
> assertion failures in nsIntRegionRectIterator::Next(). Partly undoing the
> changes in NewCGSRegionFromRegion(), so that a ToUnknownRegion() call was
> avoided, was enough to fix it. I don't understand why this fixed it, though.

Compare these two links to see the difference:

https://hg.mozilla.org/integration/mozilla-inbound/rev/52ba2412304630898842e4a42c5d5ffaff8853e8#l8.850
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a513c70ce7c8542a9ba74bb903fbcd022d9aa14#l8.849
Depends on: 1226344
This sucks, but you're going to get the chance to find out why it appeared to (partially) fix it. Backed out in https://hg.mozilla.org/mozilla-central/rev/3835b568092a because they were still happening, including one on your landing on m-i, but we just starred them as existing, frequently long-closed, bugs for other assertion failures until we finally filed a bug for them 11 hours later.

We apparently can't be trusted to recognize assertions, so you'll need to retrigger a lot on try to test possible fixes. If you don't know how to easily trigger multiple extra runs, give me a shout, I'll show you.
Status: RESOLVED → REOPENED
No longer depends on: 1226344
Resolution: FIXED → ---
(In reply to Nicholas Nethercote [:njn] from comment #19)
> (In reply to Nicholas Nethercote [:njn] from comment #18)
> > BTW, the backout here was due to lots of "Shouldn't return empty rect"
> > assertion failures in nsIntRegionRectIterator::Next(). Partly undoing the
> > changes in NewCGSRegionFromRegion(), so that a ToUnknownRegion() call was
> > avoided, was enough to fix it. I don't understand why this fixed it, though.
> 
> Compare these two links to see the difference:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 52ba2412304630898842e4a42c5d5ffaff8853e8#l8.850
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 8a513c70ce7c8542a9ba74bb903fbcd022d9aa14#l8.849

Note that typed region classes have their own typed rect iterators which return typed rects, so this code can be written:

const LayoutDeviceIntRegion& aRegion,
...
LayoutDeviceIntRegion::RectIterator iter(aRegion);
...
const LayoutDeviceIntRect* r = iter.Next();

(This doesn't answer the question of why the ToUnknownRegion() call made a difference. That baffles me, too.)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5082370c9ea0fbf99fc81e3a67823d7dcd734bdb
Bug 1225007 (part 1, attempt 3) - Use LayoutDevicePixel more in Cocoa widget code. r=kats.
(In reply to Nicholas Nethercote [:njn] from comment #22)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 5082370c9ea0fbf99fc81e3a67823d7dcd734bdb
> Bug 1225007 (part 1, attempt 3) - Use LayoutDevicePixel more in Cocoa widget
> code. r=kats.

I'm trying just part 1 again because I couldn't get the assertions on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b093ddea5af&selectedJob=14038234

Not sure if it'll work. It's still mystifying to me how this patch would change anything when it's just tweaking some types and inserting some ToUnknown/FromUnknown calls.
https://hg.mozilla.org/mozilla-central/rev/5082370c9ea0
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
One more patch to land here, assuming part 1 doesn't get backed out again.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/integration/mozilla-inbound/rev/a520af2646de99bb999dc17fa284574bed8232a3
Bug 1225007 (part 2, attempt 3) - Use LayoutDevicePixel more in Cocoa widget code. r=kats.
I relanded part 1 last week and there haven't been any more assertion failures, AFAICT. And I just relanded part 2 after a try push looked ok (https://treeherder.mozilla.org/#/jobs?repo=try&revision=72f19683d4a1).
https://hg.mozilla.org/mozilla-central/rev/a520af2646de
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.