1305844 - Remove accessors for nsStylePosition mAlignItems, mAlignContent, mJustifyContent (and make them public)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

Up until now, nsStylePosition has made all of its mAlign*** and mJustify*** member-variables private, and only accessible via "Computed***()" getters.  

This was originally important because half of these member-variables had special just-in-time computed-style resolution work to be done.  So we just gave all of these member-variables Computed***() getters, even if they didn't need them, for consistency.

Now that mAlignSelf and mJustifySelf are becoming simpler and losing their Computed*** getters, though, there's only one remaining member-variable that needs to be private & have a magic accessor -- mJustifyItems.  All the others could just be directly accessed now, and their accessors are unnecessary.

Hence, filing this removing all of these accessors and making all of these public (except for mJustifyItems)

(mats, I think you added most of these accessors -- let me know if this sounds good to you, or if you think there's a reason to keep this the way it is.)

Comment 1

[Mats Palmgren (inactive)]

Sounds good to me.

Comment 2

[Daniel Holbert [:dholbert]]

Great, thanks!

Brad, perhaps you'd be up for taking this after bug 1304012?  (At least, a patch for this bug will want to layer on top of your patches in bug 1304012, so no one should work on this until bug 1304012 has landed.)

(This one should be a relatively simple search-and-replace; it's less subtle than bug 1304012, and shouldn't involve any behavior changes.)

(For this bug, it may be simplest to just have a single patch; no need to subdivide this one into separate patches, unless you'd particularly like to. I say that because (a) none of the changes here are functional, and (b) the changes can all be quickly verified as valid using a merge tool like "meld" which highlights the changed parts of each line.)

Comment 3

[Brad Werth [:bradwerth]]

Attachment: nsStylePosition members are now (mostly) public; trivial accessors removed.

Moved member variables to public, with exception of mJustifyItems. "friend" declaration moved immediately before mJustifyItems -- the only member variable that needs the friend declaration.

If it would be clearer, we could also make mJustifyItems public and only use direct access in the nsRuleNode class, and use the accessor elsewhere (similar to how we use direct access to mAlignSelf and mJustifySelf within /layout/style and use the accessors elsewhere).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=983bb88c997f

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8797235 [details] [diff] [review]
nsStylePosition members are now (mostly) public; trivial accessors removed.

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

r=me with commit message updated & a code-comment added (see below).

First, some commit message nits:
> Bug 1305844 -- nsStylePosition members are now (mostly) public; trivial accessors removed.
              ^^
(1) This double-hyphen syntax is uncommon for Mozilla commit messages, and it needlessly makes the message 1 character longer than it needs to be.  Most people just use "Bug NNN: blahblah" or "Bug NNN - blahblah".  Probably best to use one of those, for consistency & brevity.

(2) It's unclear whether you're saying that the members area *already* public (and this commit builds on top of that) vs. whether you're *making* them public in this patch.  To be less ambiguous, we normally phrase commit messages in such a way as to describe the change, in an active tense.

(3) nsStylePosition has a lot of members; this is just focused on the align/justify ones, so let's mention that.

So, to address all of those, please replace the commit message with something like this:
> Bug 1305844 - Make most align/justify nsStylePosition members public, & remove trivial accessors

(Thanks!)

::: layout/style/nsStyleStruct.h
@@ +1748,3 @@
>  private:
> +  friend class nsRuleNode;
> +

While you're cleaning this up, please add a comment to this "private" section, to explain its existence (and the 'friend' declaration).

Something like this:
  // mJustifyItems should only be read via ComputedJustifyItems(), which
  // lazily resolves its "auto" value. (nsRuleNode needs direct access so
  // it can set mJustifyItems' value when populating this struct, though.)

Comment 5

[Brad Werth [:bradwerth]]

Attachment: Make most align/justify nsStylePosition members public, and remove trivial accessors. r=dholbert

Addresses issues in comment 4. Ready for checkin.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59b5094e6e8c
Make most align/justify nsStylePosition members public, and remove trivial accessors. r=dholbert

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/59b5094e6e8c