1261754 - improve alignment of Gecko and Servo style struct property coverage

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

For the stylo project, it will be easier for Servo code to fill in Gecko style structs with computed property values if properties live on the same structs on both the Gecko and Servo sides.  In this bug we'll shuffle some properties around to increase the alignment of struct property coverage between the two.

WIP: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8a924568442

Comment 1

[Cameron McCormack (:heycam)]

I'm working off the suggested moves in: https://public.etherpad-mozilla.org/p/aligning-style-structs

Comment 2

[Cameron McCormack (:heycam)]

Moving the transform-related properties is a bit trickier than the others so I'll do that in a separate bug.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: MozReview Request: Bug 1261754 - Part 1: Improve static assertions for style struct bits. r=dholbert

Review commit: https://reviewboard.mozilla.org/r/44503/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44503/

Comment 4

[Cameron McCormack (:heycam)]

Attachment: MozReview Request: Bug 1261754 - Part 2: Make quotes computed values shareable between different structs. r?dholbert

Review commit: https://reviewboard.mozilla.org/r/44505/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44505/

Comment 5

[Cameron McCormack (:heycam)]

Attachment: MozReview Request: Bug 1261754 - Part 3: Move quotes from nsStyleQuotes to nsStyleList and delete nsStyleQuotes. r=dholbert

Review commit: https://reviewboard.mozilla.org/r/44507/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44507/

Comment 6

[Cameron McCormack (:heycam)]

Attachment: MozReview Request: Bug 1261754 - Part 4: Move image-rendering from nsStyleSVG to nsStyleVisibility. r=dholbert

Review commit: https://reviewboard.mozilla.org/r/44509/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44509/

Comment 7

[Cameron McCormack (:heycam)]

Attachment: MozReview Request: Bug 1261754 - Part 5: Move text-rendering from nsStyleSVG to nsStyleText. r=dholbert

Review commit: https://reviewboard.mozilla.org/r/44511/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44511/

Comment 8

[Cameron McCormack (:heycam)]

Attachment: MozReview Request: Bug 1261754 - Part 6: Move vertical-align from nsStyleTextReset to nsStyleDisplay. r=dholbert

Review commit: https://reviewboard.mozilla.org/r/44513/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44513/

Comment 9

[Cameron McCormack (:heycam)]

Attachment: MozReview Request: Bug 1261754 - Part 7: Move pointer-events from nsStyleVisibility to nsStyleUserInterface. r=dholbert

Review commit: https://reviewboard.mozilla.org/r/44515/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44515/

Comment 10

[Cameron McCormack (:heycam)]

Attachment: MozReview Request: Bug 1261754 - Part 8: Move box-shadow from nsStyleBorder to a new nsStyleEffects struct. r=dholbert

Review commit: https://reviewboard.mozilla.org/r/44517/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44517/

Comment 11

[Cameron McCormack (:heycam)]

Attachment: MozReview Request: Bug 1261754 - Part 9: Move clip from nsStyleDisplay to nsStyleEffects. r=dholbert

Review commit: https://reviewboard.mozilla.org/r/44519/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44519/

Comment 12

[Cameron McCormack (:heycam)]

Attachment: MozReview Request: Bug 1261754 - Part 10: Move mix-blend-mode from nsStyleDisplay to nsStyleEffects. r=dholbert

Review commit: https://reviewboard.mozilla.org/r/44521/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44521/

Comment 13

[Cameron McCormack (:heycam)]

Attachment: MozReview Request: Bug 1261754 - Part 11: Move opacity from nsStyleDisplay to nsStyleEffects. r=dholbert

Review commit: https://reviewboard.mozilla.org/r/44523/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44523/

Comment 14

[Cameron McCormack (:heycam)]

Attachment: MozReview Request: Bug 1261754 - Part 12: Move filter from nsStyleSVGReset to nsStyleEffects. r=dholbert

Review commit: https://reviewboard.mozilla.org/r/44525/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44525/

Comment 15

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12385ba5ee4c

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8738398 [details]
MozReview Request: Bug 1261754 - Part 1: Improve static assertions for style struct bits. r=dholbert

https://reviewboard.mozilla.org/r/44503/#review41223

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8738399 [details]
MozReview Request: Bug 1261754 - Part 2: Make quotes computed values shareable between different structs. r?dholbert

https://reviewboard.mozilla.org/r/44505/#review41377

::: layout/style/nsRuleNode.cpp:8869
(Diff revision 1)
>      break;
>    case eCSSUnit_Initial:
> -    quotes->SetInitial();
> +    quotes->mQuotes = nullptr;
>      break;
>    case eCSSUnit_None:
> -    quotes->AllocateQuotes(0);
> +    quotes->mQuotes = new nsStyleQuoteValues;

I think this change (going from "AllocateQuotes(0)" to "new nsStyleQuoteValues) for \_None is switching us from not-needing-allocation to needing-allocation in this scenario.

This seems like it could cause a perf regression for this case.  (And a space regression -- if we have a rule like `* { quotes: none }`, then I think we'll now be allocating an extra object that we previously didn't need on every single quotes style-struct.)

It seems like we could optimize this pretty easily by having a single lazy-initialized nsStyleQuoteValues to handle "None", with an empty array -- and we'd always use that singleton for this scenario, since these structs are refcounted & shareable anyway!  Sort of like what you're doing for the initial valus right now, except using an actual nsStyleQuoteValues object. (You might even want to do this for the initial value as well, for consistency / fewer differing special-cases.)

::: layout/style/nsStyleStruct.h:2800
(Diff revision 1)
>  };
>  
>  
>  #define DELETE_ARRAY_IF(array)  if (array) { delete[] array; array = nullptr; }
>  
> +class nsStyleQuoteValues

Please add a brief comment explaining the purpose of this new class. (IIUC it's to allow sharing of quote-pair lists between style struct instances, particularly in cases of inheritance.)

::: layout/style/nsStyleStruct.h:2841
(Diff revision 1)
>    }
>  
> -  uint32_t  QuotesCount(void) const { return mQuotesCount; } // [inherited]
> +  static void Shutdown() { sInitialQuotePairs = nullptr; }
>  
> -  const nsString* OpenQuoteAt(uint32_t aIndex) const
> -  {
> +  void SetInitialQuotes() { mQuotes = nullptr; }
> +  void SetQuotes(already_AddRefed<nsStyleQuoteValues> aValues) {

These SetQuotes & SetInitialQuotes methods seem to be unused right now.  (Looks like nsRuleNode just directly modifies mQuotes instead of using these APIs, since mQuotes is publicly-visible.)

Seems like EITHER: these methods should go away, OR we should actually be using them in nsRuleNode instead of directly touching mQuotes.  (In the latter case, we might want to make mQuotes private to make sure we use these APIs consistently).

(I slightly lean towards the latter option -- keeping the methods & using them -- particularly if you end up adding a SetNoneValue() API like I'm suggesting in another review comment here.)

::: layout/style/nsStyleStruct.h:2849
(Diff revision 1)
> -    }
> -    return NS_OK;
> -  }
>  
> -  nsresult  SetQuotesAt(uint32_t aIndex, const nsString& aOpen, const nsString& aClose) {
> -    if (aIndex < mQuotesCount) {
> +private:
> +  static nsAutoPtr<AutoTArray<std::pair<nsString, nsString>, 2>> sInitialQuotePairs;

This should be StaticAutoPtr, instead of nsAutoPtr, to avoid adding a static constructor (and a small startup-time penalty). See http://mxr.mozilla.org/mozilla-central/source/xpcom/base/StaticPtr.h#15 for more details.

But really, though, you may want to replace this with a `static StaticRefPtr<nsStyleQuoteValues>` (one level of abstraction higher, basically), and have one of these pointers for the initial quote pairs and another one for the "none" value, as noted in other review comments here.

::: layout/style/nsStyleStruct.cpp:3547
(Diff revision 1)
> +nsStyleQuotes::CalcDifference(const nsStyleQuotes& aOther) const
>  {
>    // If the quotes implementation is ever going to change we might not need
>    // a framechange here and a reflow should be sufficient.  See bug 35768.
> -  if (mQuotesCount == aOther.mQuotesCount) {
> -    uint32_t ix = (mQuotesCount * 2);
> +  if (mQuotes != aOther.mQuotes &&
> +      (mQuotes || aOther.mQuotes) &&

This "(mQuotes || aOther.mQuotes)" condition is trivially satisfied and should be removed.

(It's checking that at least one of them is non-null. But we only evaluate it after we've determined that they're not equal -- and if they're not equal, then one of them must necessarily be non-null.)

Also, consider adding a comment to make it clearer why we're doing the two-part comparison here. (I initially thought it might be for some sort of thoroughness, but after looking more carefully I think it's just an optimization, right? We just do a pointer-equality check first, since that's an easy & fast way to determine that they're definitely equal, and that's a common scenario. And if that fails, we fall back to string-comparisons.)

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8738400 [details]
MozReview Request: Bug 1261754 - Part 3: Move quotes from nsStyleQuotes to nsStyleList and delete nsStyleQuotes. r=dholbert

https://reviewboard.mozilla.org/r/44507/#review41473

r=me on part 3, just one nit:

::: layout/style/nsStyleStruct.h:1432
(Diff revision 1)
> +  void SetQuotes(already_AddRefed<nsStyleQuoteValues> aValues) {
> +    mQuotes = aValues;
> +  }
> +  const nsTArray<std::pair<nsString, nsString>>& GetQuotePairs() const;
> +
> +  RefPtr<nsStyleQuoteValues> mQuotes;   // [inherited]

Nit: So, if you agree that in part 2 we should make this "mQuotes" member-var private & only modify it via explicit mutator methods (which would be my preference, per my comments above on part 2), then we can & should move this decl down to a better (IMO) place in the "private" block within its new home here.

I think it probably belongs after "mListStyleImage", because:
 - It's not really related to Lists, so it's a bit awkward for it to be the very first member in nsStyleList.
 - Its type is RefPtr, so it makes sense that it should be alongside other RefPtr variables, instead of alongside a uint8_t, from a packing perspective.

(If you make this change, be sure to move the nsRuleNode::ComputeListData chunk down as well.  Don't reorder nsStyleList::CalcDifference much, though, because we don't want to take an early-return with a weaker change hint.)

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8738401 [details]
MozReview Request: Bug 1261754 - Part 4: Move image-rendering from nsStyleSVG to nsStyleVisibility. r=dholbert

https://reviewboard.mozilla.org/r/44509/#review41475

r=me on part 4. One general thought, though (which this patch made me think of, but which isn't specific to this patch):

::: layout/style/nsCSSPropList.h:4042
(Diff revision 1)
>      "",
>      VARIANT_HN,
>      nullptr,
>      offsetof(nsStyleSVGReset, mFloodOpacity),
>      eStyleAnimType_float)
> -CSS_PROP_SVG(
> +CSS_PROP_VISIBILITY(

So, this is now CSS_PROP_VISIBILITY but it's still in the middle of a bunch of CSS_PROP_SVG / CSS_PROP_SVGRESET stuff in nsCSSPropList.h.

At the end of this patch-stack, it might be nice to add one final patch that reorders nsCSSPropList so that all of these macro invocations are grouped with the other macros for their same style struct.  (Either as a final patch here, or as a followup bug.)  Not a huge deal, though.

Comment 20

[Cameron McCormack (:heycam)]

Hmm, yeah.  So the current grouping is "normal properties" followed by "SVG properties".  Really the SVG ones should just be interspersed so that we have a single list sorted by property name.

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 8738402 [details]
MozReview Request: Bug 1261754 - Part 5: Move text-rendering from nsStyleSVG to nsStyleText. r=dholbert

https://reviewboard.mozilla.org/r/44511/#review41477

r=me on part 5

Comment 22

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #20)
> Hmm, yeah.  So the current grouping is "normal properties" followed by "SVG
> properties".  Really the SVG ones should just be interspersed so that we
> have a single list sorted by property name.

Ah, right, sorry -- I thought I remembered them being grouped by struct, but I misremembered.  Anyway, feel free to file a followup on this or not, as you see fit.

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 8738403 [details]
MozReview Request: Bug 1261754 - Part 6: Move vertical-align from nsStyleTextReset to nsStyleDisplay. r=dholbert

https://reviewboard.mozilla.org/r/44513/#review41479

r=me on part 6

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 8738404 [details]
MozReview Request: Bug 1261754 - Part 7: Move pointer-events from nsStyleVisibility to nsStyleUserInterface. r=dholbert

https://reviewboard.mozilla.org/r/44515/#review41481

r=me on part 7, just one nit:

::: layout/style/nsStyleStructInlines.h:179
(Diff revision 1)
>    NS_ASSERTION(aContextFrame->StyleDisplay() == this, "unexpected aContextFrame");
>    return IsAbsolutelyPositionedStyle() && !aContextFrame->IsSVGText();
>  }
>  
>  uint8_t
> -nsStyleVisibility::GetEffectivePointerEvents(nsIFrame* aFrame) const
> +nsStyleUserInterface::GetEffectivePointerEvents(nsIFrame* aFrame) const

This function impl should probably move up to be with the other nsStyleUserInterface methods.  (either in this patch or in a followup)

It's kind of lost all the way down here, at the end of the nsStyleDisplay stuff.

Comment 25

[Daniel Holbert [:dholbert]]

(Heading to bed now; I'll finish this off with the nsStyleEffects stuff tomorrow.)

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 8738405 [details]
MozReview Request: Bug 1261754 - Part 8: Move box-shadow from nsStyleBorder to a new nsStyleEffects struct. r=dholbert

https://reviewboard.mozilla.org/r/44517/#review41545

r=me on part 8.

::: layout/style/nsStyleStruct.cpp:3984
(Diff revision 1)
> +  : mBoxShadow(aSource.mBoxShadow)
> +{
> +  MOZ_COUNT_CTOR(nsStyleEffects);
> +}
> +
> +nsStyleEffects::~nsStyleEffects(void)

Drop the "void" here, unless there's some reason for it.

(I see a few other stylestruct-destructors do have "void" in their args lists, but it's just a few of them. I'm guessing that's just archeological cruft from a more C-like time.)

::: layout/style/nsStyleStruct.cpp:3998
(Diff revision 1)
> +    // Update overflow regions & trigger DLBI to be sure it's noticed.
> +    // Also request a repaint, since it's possible that only the color
> +    // of the shadow is changing (and UpdateOverflow/SchedulePaint won't
> +    // repaint for that, since they won't know what needs invalidating.)
> +    return nsChangeHint_UpdateOverflow |
> +           nsChangeHint_SchedulePaint |

Nit (and this is perhaps a bit OCD; feel free to ignore):

I see that the next patch after this one rewrites this method to use a local variable "hint", and it rewrites this code here to update "hint" and return it later on, instead of returning directly.

It'd probably be a bit cleaner to just make this patch here already use that structure for this method (using a local variable). That way, your next patch -- which has nothing to do with box-shadow -- won't have to mess with this box-shadow code at all; it'll just add its own hints to the existing local var, as-needed.

(Not a huge deal, since the code ends up in the same state either way.)

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 8738406 [details]
MozReview Request: Bug 1261754 - Part 9: Move clip from nsStyleDisplay to nsStyleEffects. r=dholbert

https://reviewboard.mozilla.org/r/44519/#review41561

r=me on part 9

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 8738407 [details]
MozReview Request: Bug 1261754 - Part 10: Move mix-blend-mode from nsStyleDisplay to nsStyleEffects. r=dholbert

https://reviewboard.mozilla.org/r/44521/#review41563

r=me on part 10

Comment 29

[Daniel Holbert [:dholbert]]

Comment on attachment 8738408 [details]
MozReview Request: Bug 1261754 - Part 11: Move opacity from nsStyleDisplay to nsStyleEffects. r=dholbert

https://reviewboard.mozilla.org/r/44523/#review41569

r=me on part 11

Comment 30

[Daniel Holbert [:dholbert]]

Comment on attachment 8738409 [details]
MozReview Request: Bug 1261754 - Part 12: Move filter from nsStyleSVGReset to nsStyleEffects. r=dholbert

https://reviewboard.mozilla.org/r/44525/#review41573

r=me on part 12, with these nits addressed as you see fit:

::: layout/style/nsStyleStruct.cpp:1339
(Diff revision 1)
> -  if (mClipPath != aOther.mClipPath ||
> -      mFilters != aOther.mFilters) {
>      NS_UpdateHint(hint, nsChangeHint_UpdateEffects);
>      NS_UpdateHint(hint, nsChangeHint_RepaintFrame);
>      // We only actually need to update the overflow area for filter
>      // changes.  However, clip-path changes require that we update

This comment needs updating, since it's talking about filters, which you're not handling here anymore.

(Maybe just delete everything up to & including "However, "...)

::: layout/svg/nsSVGIntegrationUtils.cpp:162
(Diff revision 1)
>    const nsStyleSVGReset *style = aFrame->StyleSVGReset();
>    bool hasValidLayers = style->mMask.HasLayerWithImage();
>  
> -  return (style->HasFilters() || style->HasClipPath() || hasValidLayers);
> +  return aFrame->StyleEffects()->HasFilters() ||
> +         style->HasClipPath() ||
> +         hasValidLayers;

I suspect this "hasValidLayers" variable only existed in the first place so that this return statement would fit on one line.

So: since you're splitting the return statement across multiple lines, consider dropping this var and replacing it with its value -- style->mMask.HasLayerWithImage() -- in this return statement.

Comment 31

[Daniel Holbert [:dholbert]]

(So everything is r+ here except for "part 2", because depending on how much you agree with my review feedback in comment 17, that'll be changing enough that its updated version probably merits another round of review.)

Comment 32

[Cameron McCormack (:heycam)]

Comment on attachment 8738398 [details]
MozReview Request: Bug 1261754 - Part 1: Improve static assertions for style struct bits. r=dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44503/diff/1-2/

Comment 33

[Cameron McCormack (:heycam)]

Comment on attachment 8738399 [details]
MozReview Request: Bug 1261754 - Part 2: Make quotes computed values shareable between different structs. r?dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44505/diff/1-2/

Comment 34

[Cameron McCormack (:heycam)]

Comment on attachment 8738400 [details]
MozReview Request: Bug 1261754 - Part 3: Move quotes from nsStyleQuotes to nsStyleList and delete nsStyleQuotes. r=dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44507/diff/1-2/

Comment 35

[Cameron McCormack (:heycam)]

Comment on attachment 8738401 [details]
MozReview Request: Bug 1261754 - Part 4: Move image-rendering from nsStyleSVG to nsStyleVisibility. r=dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44509/diff/1-2/

Comment 36

[Cameron McCormack (:heycam)]

Comment on attachment 8738402 [details]
MozReview Request: Bug 1261754 - Part 5: Move text-rendering from nsStyleSVG to nsStyleText. r=dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44511/diff/1-2/

Comment 37

[Cameron McCormack (:heycam)]

Comment on attachment 8738403 [details]
MozReview Request: Bug 1261754 - Part 6: Move vertical-align from nsStyleTextReset to nsStyleDisplay. r=dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44513/diff/1-2/

Comment 38

[Cameron McCormack (:heycam)]

Comment on attachment 8738404 [details]
MozReview Request: Bug 1261754 - Part 7: Move pointer-events from nsStyleVisibility to nsStyleUserInterface. r=dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44515/diff/1-2/

Comment 39

[Cameron McCormack (:heycam)]

Comment on attachment 8738405 [details]
MozReview Request: Bug 1261754 - Part 8: Move box-shadow from nsStyleBorder to a new nsStyleEffects struct. r=dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44517/diff/1-2/

Comment 40

[Cameron McCormack (:heycam)]

Comment on attachment 8738406 [details]
MozReview Request: Bug 1261754 - Part 9: Move clip from nsStyleDisplay to nsStyleEffects. r=dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44519/diff/1-2/

Comment 41

[Cameron McCormack (:heycam)]

Comment on attachment 8738407 [details]
MozReview Request: Bug 1261754 - Part 10: Move mix-blend-mode from nsStyleDisplay to nsStyleEffects. r=dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44521/diff/1-2/

Comment 42

[Cameron McCormack (:heycam)]

Comment on attachment 8738408 [details]
MozReview Request: Bug 1261754 - Part 11: Move opacity from nsStyleDisplay to nsStyleEffects. r=dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44523/diff/1-2/

Comment 43

[Cameron McCormack (:heycam)]

Comment on attachment 8738409 [details]
MozReview Request: Bug 1261754 - Part 12: Move filter from nsStyleSVGReset to nsStyleEffects. r=dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44525/diff/1-2/

Comment 44

[Cameron McCormack (:heycam)]

https://reviewboard.mozilla.org/r/44515/#review41481

> This function impl should probably move up to be with the other nsStyleUserInterface methods.  (either in this patch or in a followup)
> 
> It's kind of lost all the way down here, at the end of the nsStyleDisplay stuff.

FWIW in this file, nsStyleStructInlines.h, there aren't any other nsStyleUserInterface methods.

Comment 45

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=80727b72f72e

Comment 46

[Daniel Holbert [:dholbert]]

Comment on attachment 8738399 [details]
MozReview Request: Bug 1261754 - Part 2: Make quotes computed values shareable between different structs. r?dholbert

https://reviewboard.mozilla.org/r/44505/#review42175

r=me on part 2; nits below.

::: layout/style/nsStyleStruct.h:2811
(Diff revisions 1 - 2)
>  class nsStyleQuoteValues
>  {
>  public:
> +  typedef nsTArray<std::pair<nsString, nsString>> Array;
>    NS_INLINE_DECL_REFCOUNTING(nsStyleQuoteValues);
> -  nsTArray<std::pair<nsString, nsString>> mQuotePairs;
> +  Array mQuotePairs;

tl;dr: s/Array/QuotePairArray/ for this typedef, please.

"Array" too generic for a highly-specific typedef like this, IMO. I read this line with "Array mQuotePairs", and I confess I got a bit confused about what "Array" actually was, until several minutes later I noticed "Array" at the end of the the typedef expression 2 lines up. :)

So: I'd prefer a slightly-more-distinctinve (and hence more usefully-greppable) typename, something like `QuotePairArray`.  That still saves you quite a few characters as compared to the longform `nsTArray<std::pair<nsString, nsString>>` -- even when fully-qualified as `nsStyleQuoteValues::QuotePairArray`.

::: layout/style/nsStyleStruct.h:2855
(Diff revisions 1 - 2)
> -  RefPtr<nsStyleQuoteValues> mQuotes;  // [inherited]
> +  const nsStyleQuoteValues::Array& GetQuotePairs() const;
> +
> +  void SetQuotesInherit(const nsStyleQuotes* aOther);
> +  void SetQuotesInitial();
> +  void SetQuotesNone();
> +  void SetQuotes(nsStyleQuoteValues::Array&& aValues);

This decl is inside of the "class nsStyleQuoteValues {...}" scope, so I think you can drop the nsStyleQuoteValues:: prefix on the type here, if you like.  (particularly once the "Array" typedef has a more descriptive name, per above -- then, it'll still be pretty clear what this type actually refers to, even without the prefix.)

::: layout/style/nsStyleStruct.h:2861
(Diff revisions 1 - 2)
>  
>  private:
> -  static nsAutoPtr<AutoTArray<std::pair<nsString, nsString>, 2>> sInitialQuotePairs;
> +  RefPtr<nsStyleQuoteValues> mQuotes;  // [inherited]
> +
> +  static mozilla::StaticRefPtr<nsStyleQuoteValues> sInitialQuotes;
> +  static mozilla::StaticRefPtr<nsStyleQuoteValues> sNoneQuotes;

Please add a code comment to explain these, e.g.:
  // Singletons which mQuotes may point to, to represent a few special values.

Comment 47

[Cameron McCormack (:heycam)]

(In reply to Daniel Holbert [:dholbert] from comment #46)
> ::: layout/style/nsStyleStruct.h:2811
> (Diff revisions 1 - 2)
> >  class nsStyleQuoteValues
> >  {
> >  public:
> > +  typedef nsTArray<std::pair<nsString, nsString>> Array;
> >    NS_INLINE_DECL_REFCOUNTING(nsStyleQuoteValues);
> > -  nsTArray<std::pair<nsString, nsString>> mQuotePairs;
> > +  Array mQuotePairs;
> 
> tl;dr: s/Array/QuotePairArray/ for this typedef, please.
> 
> "Array" too generic for a highly-specific typedef like this, IMO. I read
> this line with "Array mQuotePairs", and I confess I got a bit confused about
> what "Array" actually was, until several minutes later I noticed "Array" at
> the end of the the typedef expression 2 lines up. :)
> 
> So: I'd prefer a slightly-more-distinctinve (and hence more
> usefully-greppable) typename, something like `QuotePairArray`.  That still
> saves you quite a few characters as compared to the longform
> `nsTArray<std::pair<nsString, nsString>>` -- even when fully-qualified as
> `nsStyleQuoteValues::QuotePairArray`.

OK.

> ::: layout/style/nsStyleStruct.h:2855
> (Diff revisions 1 - 2)
> > -  RefPtr<nsStyleQuoteValues> mQuotes;  // [inherited]
> > +  const nsStyleQuoteValues::Array& GetQuotePairs() const;
> > +
> > +  void SetQuotesInherit(const nsStyleQuotes* aOther);
> > +  void SetQuotesInitial();
> > +  void SetQuotesNone();
> > +  void SetQuotes(nsStyleQuoteValues::Array&& aValues);
> 
> This decl is inside of the "class nsStyleQuoteValues {...}" scope, so I
> think you can drop the nsStyleQuoteValues:: prefix on the type here, if you
> like.  (particularly once the "Array" typedef has a more descriptive name,
> per above -- then, it'll still be pretty clear what this type actually
> refers to, even without the prefix.)

It's in nsStyleQuotes (and alter, nsStyleList), so I can't leave it off.

> ::: layout/style/nsStyleStruct.h:2861
> (Diff revisions 1 - 2)
> >  
> >  private:
> > -  static nsAutoPtr<AutoTArray<std::pair<nsString, nsString>, 2>> sInitialQuotePairs;
> > +  RefPtr<nsStyleQuoteValues> mQuotes;  // [inherited]
> > +
> > +  static mozilla::StaticRefPtr<nsStyleQuoteValues> sInitialQuotes;
> > +  static mozilla::StaticRefPtr<nsStyleQuoteValues> sNoneQuotes;
> 
> Please add a code comment to explain these, e.g.:
>   // Singletons which mQuotes may point to, to represent a few special
> values.

OK.

Comment 48

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #47)
> It's in nsStyleQuotes (and alter, nsStyleList), so I can't leave it off.

Oops, right you are. Thanks for the correction.

Comment 49

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03f2fa739af3
https://hg.mozilla.org/integration/mozilla-inbound/rev/c01a5b49b4b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/6af0020d9fea
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b499dd6626c
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b3c9dbed851
https://hg.mozilla.org/integration/mozilla-inbound/rev/538985880d09
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b6439dc1f1d
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecaeb6eaa362
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee8b30286447
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d69c89223a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/62a953a61527
https://hg.mozilla.org/integration/mozilla-inbound/rev/ada58caa17b4

Comment 50

[Cameron McCormack (:heycam)]

Thanks for the reviews!

Comment 51

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/03f2fa739af3
https://hg.mozilla.org/mozilla-central/rev/c01a5b49b4b7
https://hg.mozilla.org/mozilla-central/rev/6af0020d9fea
https://hg.mozilla.org/mozilla-central/rev/2b499dd6626c
https://hg.mozilla.org/mozilla-central/rev/9b3c9dbed851
https://hg.mozilla.org/mozilla-central/rev/538985880d09
https://hg.mozilla.org/mozilla-central/rev/0b6439dc1f1d
https://hg.mozilla.org/mozilla-central/rev/ecaeb6eaa362
https://hg.mozilla.org/mozilla-central/rev/ee8b30286447
https://hg.mozilla.org/mozilla-central/rev/5d69c89223a5
https://hg.mozilla.org/mozilla-central/rev/62a953a61527
https://hg.mozilla.org/mozilla-central/rev/ada58caa17b4

Comment 52

[Joel Maher ( :jmaher ) (UTC -8)]

I see a talos perf win here for linux64 tsvgr_opacity:
https://treeherder.mozilla.org/perf.html#/alerts?id=853

thanks for making Firefox faster!

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6891d6a48f5f
Trivial follow-up to fix outdated documentation. r=me and DONTBUILD because NPOTB.

Comment 54

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6891d6a48f5f