Closed Bug 843719 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: dholbert, Assigned: dholbert)

Details

Attachments

(4 files, 5 obsolete files)

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.
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.
Attached 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
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.
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.
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
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
Attached patch sub-part 2: fix clients (obsolete) — Splinter Review
(fixing a mis-indented line in part 1a)
Attachment #721392 - Attachment is obsolete: true
Attached 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.
(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")
Attachment #721392 - Attachment description: Part 1a: change API (and add BogusEnumVal) → sub-part 1: change API (and add BogusEnumVal)
Attachment #721393 - Attachment description: Part 1b: fix clients → sub-part 2: fix clients
Attachment #721402 - Attachment description: Part 1a v2: change API (and add BogusEnumVal) → sub-part 1 v2: change API (and add BogusEnumVal)
Attachment #721416 - Attachment description: Part 2: Remove bogus enum val → sub-part 3: Remove bogus enum val
(just reverting one unintentional whitespace-adjustment that I only noticed when looking at the rollup)
Attachment #721443 - Attachment description: sub-part 2: fix clients → sub-part 2 v2: fix clients
Attachment #721393 - Attachment is obsolete: true
(...and fixing the resulting bitrot in sub-part 3)
Attachment #721416 - Attachment is obsolete: true
...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)
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)
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)
Try push w/ the three up-to-date sub-parts:
  https://tbpl.mozilla.org/?tree=Try&rev=f2eb0454baf5
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+
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.
(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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(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!
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.
[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.