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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

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.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: wip (untested)

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.

Comment 3

[Chris Lord [:cwiiis]]

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.

Comment 4

[Daniel Holbert [:dholbert]]

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.

Comment 5

[Chris Lord [:cwiiis]]

(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 :)

Comment 6

[Chris Lord [:cwiiis]]

(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.

Comment 7

[Daniel Holbert [:dholbert]]

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

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: sub-part 1: change API (and add BogusEnumVal)

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".

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: sub-part 2: fix clients

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: sub-part 1 v2: change API (and add BogusEnumVal)

(fixing a mis-indented line in part 1a)

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: sub-part 3: Remove bogus enum val

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: rollup patch

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.

Comment 13

[Daniel Holbert [:dholbert]]

(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")

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: sub-part 2 v2: fix clients

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

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: sub-part 3 v2: Remove bogus enum val

(...and fixing the resulting bitrot in sub-part 3)

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: rollup patch v2 (combining the three sub-parts)

...and here's the updated rollup patch (from applying the three sub-parts in order).

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 18

[Daniel Holbert [:dholbert]]

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

Comment 19

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 721445 [details] [diff] [review]
rollup patch v2 (combining the three sub-parts)

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

Looks good!

Comment 20

[Daniel Holbert [:dholbert]]

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.

Comment 21

[Daniel Holbert [:dholbert]]

Landed rollup patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/968f38210d1d

Comment 22

[Daniel Holbert [:dholbert]]

(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

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/968f38210d1d

Comment 24

[Daniel Holbert [:dholbert]]

(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!

Comment 25

[Daniel Holbert [:dholbert]]

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.

Comment 26

[Daniel Holbert [:dholbert]]

[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.)

Comment 27

[Matt Woodrow (:mattwoodrow)]

Thanks dholbert, merge has been done now without issues.