Closed Bug 1180643 Opened 4 years ago Closed 4 years ago

error in LogicalPoint::SetY() physical setter

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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!
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
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.
Attachment #8629921 - Flags: review?(smontagu)
Attachment #8629917 - Flags: review?(smontagu) → review+
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.
Attachment #8629921 - Flags: review?(smontagu) → review+
(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 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.
Attachment #8629921 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/061a2fa8ebda
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.