Make Margin classes' constructor & SizeTo parameter-ordering consistent w/ "margin" CSS property (top,right,bottom,left)

RESOLVED FIXED in mozilla22

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla22
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Assignee)

Description

6 years ago
For some reason, our internal Margin classes take arguments in the order:
 left, top, right, bottom
while the CSS "margin" property (and border and padding) take arguments in the order:
 top, right, bottom, left

Unless there's some strong reason for the current Margin ordering, I think we should make it consistent with the CSS ordering.
(Assignee)

Comment 1

6 years ago
Looks like nsMargin's constructor has had this ordering since it was first added to CVS in 1998:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/src/nsMargin.h&rev=1.1&mark=30-32

So, it probably just predates the CSS side-ordering convention.
(Assignee)

Comment 2

6 years ago
Posted patch wip (untested) (obsolete) — Splinter Review
This reorders the args in BaseMargin, nsMargin, gfxMargin, and all the clients of those class's constructors & SizeTo() methods that I could find.

Not yet tested (beyond compiling); I'll request review once this has gotten a green test-run on try.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
OS: Linux → All
Hardware: x86_64 → All
While I think that nsMargin and nsIntMargin should follow CSS, I'm not convinced that gfxMargin should - the left,top,right,bottom order is the convention in graphics, and matches most external graphics APIs.

That said, a mismatch between gfx and ns margins may well cause bugs/confusion, so I don't have strong feelings about this... Just something to bear in mind.
(Assignee)

Comment 4

6 years ago
Early try run (which I interrupted after noticing a few failures on OS X 10.8 opt): 
 https://tbpl.mozilla.org/?tree=Try&rev=451d334c4cb3

(In reply to Chris Lord [:cwiiis] from comment #3)
> While I think that nsMargin and nsIntMargin should follow CSS, I'm not
> convinced that gfxMargin should - the left,top,right,bottom order is the
> convention in graphics, and matches most external graphics APIs.

That's good to know.  However, unless we directly interact with such an API using a gfxMargin (do we?), I tend to be unpersuaded by that.  The various "margin" classes all inherit from a common base class right now.  To justify making one of those subclasses reorder its constructor-args w.r.t. its parent & sibling-classes, we'd need a very strong demonstration for how that makes things clearer as opposed to more confusing.
(In reply to Daniel Holbert [:dholbert] from comment #4)
> Early try run (which I interrupted after noticing a few failures on OS X
> 10.8 opt): 
>  https://tbpl.mozilla.org/?tree=Try&rev=451d334c4cb3
> 
> (In reply to Chris Lord [:cwiiis] from comment #3)
> > While I think that nsMargin and nsIntMargin should follow CSS, I'm not
> > convinced that gfxMargin should - the left,top,right,bottom order is the
> > convention in graphics, and matches most external graphics APIs.
> 
> That's good to know.  However, unless we directly interact with such an API
> using a gfxMargin (do we?), I tend to be unpersuaded by that.  The various
> "margin" classes all inherit from a common base class right now.  To justify
> making one of those subclasses reorder its constructor-args w.r.t. its
> parent & sibling-classes, we'd need a very strong demonstration for how that
> makes things clearer as opposed to more confusing.

I don't think we do as far as I know, cc'ing Joe in case he has something more insightful to say. I think following CSS order is probably the more obvious thing for a rendering engine to do :)
(In reply to Chris Lord [:cwiiis] from comment #5)
> I think following CSS order is probably the more
> obvious thing for a rendering engine to do :)

* a web rendering engine, that is.
(Assignee)

Comment 7

6 years ago
dbaron had a good suggestion for making sure that this bug fixes all the various constructor and SizeTo invocations -- just add a (temporary) bogus argument, and then any client that gets missed will trigger a compile-failure because it'll be missing that argument.  (And then remove the bogus argument in a final patch.)

Patches coming up that use this strategy.  Try job:
  https://tbpl.mozilla.org/?tree=Try&rev=22feea247cf3
(Assignee)

Comment 8

6 years ago
Here's the first patch, which changes the API.  This shifts all the various XYZMargin constructor & SizeTo() methods' "aLeft" param to the end, and adds a bogus additional parameter to those methods to make sure we catch all the clients (in the next patch).

This patch won't build on its own (by design).  When it ultimately lands, I'll fold it together with the next patch, "Part 1b".
Attachment #716804 - Attachment is obsolete: true
(Assignee)

Comment 9

6 years ago
Posted patch sub-part 2: fix clients (obsolete) — Splinter Review
(Assignee)

Comment 10

6 years ago
(fixing a mis-indented line in part 1a)
Attachment #721392 - Attachment is obsolete: true
(Assignee)

Comment 11

6 years ago
(Assignee)

Comment 12

6 years ago
Posted patch rollup patch (obsolete) — Splinter Review
Here's a single rollup patch that combines Part 1a, Part 1b, Part 2.  (Generated from qimporting those patches and running "hg diff -r qparent"

This rollup is considerably smaller than part 1b and part 2, because a lot of the intermediate patches' modified lines are e.g. "SizeTo(0,0,0,0)" which look the same before & after the patch-stack.
(Assignee)

Comment 13

6 years ago
(I was originally planning on landing the API & client changes as one commit, and then the remove-dummy-enum as a separate commit -- this is why they're named part 1a / part 1b / part 2.

However -- this unnecessarily jostles a lot of lines of code like the SizeTo(0,0,0,0) calls mentioned in the previous line.  So I think I'll actually just land the rollup as a single commit, and I'll post the sub-parts as three separate patches here solely for the point of illustration & as a possible review-aide.

So -- I'm rebranding the patches as "sub-part 1, sub-part 2, sub-part 3")
(Assignee)

Updated

6 years ago
Attachment #721392 - Attachment description: Part 1a: change API (and add BogusEnumVal) → sub-part 1: change API (and add BogusEnumVal)
(Assignee)

Updated

6 years ago
Attachment #721393 - Attachment description: Part 1b: fix clients → sub-part 2: fix clients
(Assignee)

Updated

6 years ago
Attachment #721402 - Attachment description: Part 1a v2: change API (and add BogusEnumVal) → sub-part 1 v2: change API (and add BogusEnumVal)
(Assignee)

Updated

6 years ago
Attachment #721416 - Attachment description: Part 2: Remove bogus enum val → sub-part 3: Remove bogus enum val
(Assignee)

Comment 14

6 years ago
(just reverting one unintentional whitespace-adjustment that I only noticed when looking at the rollup)
(Assignee)

Updated

6 years ago
Attachment #721443 - Attachment description: sub-part 2: fix clients → sub-part 2 v2: fix clients
(Assignee)

Updated

6 years ago
Attachment #721393 - Attachment is obsolete: true
(Assignee)

Comment 15

6 years ago
(...and fixing the resulting bitrot in sub-part 3)
Attachment #721416 - Attachment is obsolete: true
(Assignee)

Comment 16

6 years ago
...and here's the updated rollup patch (from applying the three sub-parts in order).
Attachment #721418 - Attachment is obsolete: true
Attachment #721445 - Flags: review?(dbaron)
(Assignee)

Updated

6 years ago
Attachment #721445 - Attachment description: rollup patch v2 → rollup patch v2 (combining the three sub-parts)
Comment on attachment 721445 [details] [diff] [review]
rollup patch v2 (combining the three sub-parts)

dholbert suggested seth could review this, which is fine with me
Attachment #721445 - Flags: review?(dbaron) → review?(seth)
(Assignee)

Updated

6 years ago
Summary: Consider making Margin, gfxMargin, nsMargin constructor param-ordering consistent w/ "margin" CSS property (top,right,bottom,left) → Make Margin classes' constructor & SizeTo parameter-ordering consistent w/ "margin" CSS property (top,right,bottom,left)
(Assignee)

Comment 18

6 years ago
Try push w/ the three up-to-date sub-parts:
  https://tbpl.mozilla.org/?tree=Try&rev=f2eb0454baf5
(Assignee)

Updated

6 years ago
Component: Graphics → Layout
Comment on attachment 721445 [details] [diff] [review]
rollup patch v2 (combining the three sub-parts)

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

Looks good!
Attachment #721445 - Flags: review?(seth) → review+
(Assignee)

Comment 20

6 years ago
Thanks! I'm going to wait until the try push from comment 18 has (mostly) completed, and then I'll land.

(I want to get this in soon, before someone bitrots it or adds a new un-fixed margin usage.  

After this lands, I'll screen for any newly-added margins instances that need fixing by doing a try push w/ just sub-part 1 and sub-part 2, based off of this bug's commit's parent.  If that try push successfully builds, then that will indicate that there aren't any new unfixed margin instances.
(Assignee)

Comment 21

6 years ago
Landed rollup patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/968f38210d1d
Flags: in-testsuite-
(Assignee)

Comment 22

6 years ago
(In reply to Daniel Holbert [:dholbert] from comment #21)
> Landed rollup patch:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/968f38210d1d

To double-check that this caught all the constructor & SizeTo() invocations in the tree when it landed, here's a try push, based off of 968f38210d1d's parent, with only sub-part 1 and sub-part 2 applied. (as I suggested at end of comment 20)

If this builds correctly, then we caught all of the invocations.
  https://tbpl.mozilla.org/?tree=Try&rev=e57010ba2ada
https://hg.mozilla.org/mozilla-central/rev/968f38210d1d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(Assignee)

Comment 24

6 years ago
(In reply to Daniel Holbert [:dholbert] from comment #22)
> If this builds correctly, then we caught all of the invocations.
>   https://tbpl.mozilla.org/?tree=Try&rev=e57010ba2ada

...and it does build correctly, so we're good!
(Assignee)

Comment 25

6 years ago
mattwoodrow: just a heads-up [to you, as the person who seems to be merging m-c to graphics branch most recently/frequently]:

It'd probably be good to merge m-c to the graphics branch in the near-ish term to pick this up & reduce the chance of bitrot (& new Margin usages that'd need to be converted).

As of now, I don't think there are any such margin-usages or any significant bitrot caused by this bug, based on the following commands (run in an up-to-date graphics branch, w/ 819612fa900e being the last graphics<-->m-c merge in the history):
$ hg diff -r 819612fa900e | grep argin
$ hg diff -r 819612fa900e | grep SizeTo

Both of those produce no output, indicating that no xyzMargin constructors & no SizeTo commands have been added/modified in the graphics branch since the last merge from m-c.
(Assignee)

Comment 26

6 years ago
[toggling needinfo?mattwoodrow to be sure he sees comment 25]
(If you'd prefer, I'd be happy to do a merge, too -- not looking to make work for you. :) I just don't want to step on any toes or mess anything up.)
Flags: needinfo?(matt.woodrow)
Thanks dholbert, merge has been done now without issues.
Flags: needinfo?(matt.woodrow)
You need to log in before you can comment on or make changes to this bug.