1180643 - error in LogicalPoint::SetY() physical setter

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jonathan Kew [:jfkthame]]

I just noticed that the SetY() setter on LogicalPoint is setting the wrong field. (Compare to SetX(), which does the right thing; but SetY() sets the same field.)

Fortunately, we don't currently use this API anywhere, so it's not causing any problems, but we should fix it in case anyone does ever try to call it!

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Fix the SetY() method on LogicalPoint to update the correct field

Oops.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Remove the unused methods to access individual fields of LogicalPoint in physical terms

It turns out that we don't actually use the physical accessors for individual fields of LogicalPoint anywhere. So as a followup to fixing the SetY() bug, I suggest we remove them from the header altogether. In the (unlikely?) event we do ever need them, we can reintroduce them (including the fix above), but I doubt it'll happen.

Comment 3

[Simon Montagu :smontagu]

Comment on attachment 8629921 [details] [diff] [review]
Remove the unused methods to access individual fields of LogicalPoint in physical terms

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

r=me so you can check it in now if you want to, but does it go far enough? I think we can probably remove all or most of the physical setters in the other logical coordinate classes as well, and maybe that's enough scope creep to justify a separate bug.

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #3)
> r=me so you can check it in now if you want to, but does it go far enough? I
> think we can probably remove all or most of the physical setters in the
> other logical coordinate classes as well, and maybe that's enough scope
> creep to justify a separate bug.

Yes, I think we can probably do that fairly easily. Filed as bug 1181087.

Comment 5

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/061a2fa8ebda

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 8629921 [details] [diff] [review]
Remove the unused methods to access individual fields of LogicalPoint in physical terms

Obsoleting the second patch here, superseded by bug 1181087.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/061a2fa8ebda