Closed Bug 1223639 Opened 5 years ago Closed 5 years ago

use ForceInside instead of intersect on the displayport computation

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

References

Details

Attachments

(2 files)

When I wrote the code for computing a displayport rect from displayport margins I was copying the code in AsyncPanZoomController::CalculatePendingDisplayPort. However it appears I made a mistake when copying the ForceInside line

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=86ffe5dc026c#2630

I likely assumed ForceInside was the same as intersect and didn't check the definition (just like Botond did in bug 1204136, comment 6).

Bug 1191539 fixed this mistake by calling ForceInside on the screen rect. But let's just do it once at the end and get rid of the mistaken Intersect call.

Also, let's rename ForceInside so we don't make this mistake again.
Attachment #8685741 - Flags: review?(botond)
Open to suggestions on the name, but I think this is much better than ForceInside. No one should confuse the new name with a plain intersect.
Attachment #8685742 - Flags: review?(botond)
Comment on attachment 8685741 [details] [diff] [review]
call ForceInside instead of Intersect

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

Thanks for getting to the bottom of this! (I thought, after reading bug 1204136 comment 6, that doing both ForceInside and Intersect is strange, but I didn't pursue the issue as that bug was making an orthogonal change.)
Attachment #8685741 - Flags: review?(botond) → review+
Comment on attachment 8685742 [details] [diff] [review]
rename ForceInside to MoveInsideAndClamp

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

I confess I've had to read the implementation of this function to understand what it does pretty much every time I encountered it. The new name is much better!
Attachment #8685742 - Flags: review?(botond) → review+
https://hg.mozilla.org/mozilla-central/rev/64a9ebe5f2b9
https://hg.mozilla.org/mozilla-central/rev/c5780f2a10e8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Blocks: 1227799
Depends on: 1277814
You need to log in before you can comment on or make changes to this bug.