Closed Bug 1261754 Opened 4 years ago Closed 4 years ago

improve alignment of Gecko and Servo style struct property coverage

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(12 files)

58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
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
I'm working off the suggested moves in: https://public.etherpad-mozilla.org/p/aligning-style-structs
Moving the transform-related properties is a bit trickier than the others so I'll do that in a separate bug.
Review commit: https://reviewboard.mozilla.org/r/44503/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44503/
Attachment #8738398 - Flags: review?(dholbert)
Attachment #8738399 - Flags: review?(dholbert)
Attachment #8738400 - Flags: review?(dholbert)
Attachment #8738401 - Flags: review?(dholbert)
Attachment #8738402 - Flags: review?(dholbert)
Attachment #8738403 - Flags: review?(dholbert)
Attachment #8738404 - Flags: review?(dholbert)
Attachment #8738405 - Flags: review?(dholbert)
Attachment #8738406 - Flags: review?(dholbert)
Attachment #8738407 - Flags: review?(dholbert)
Attachment #8738408 - Flags: review?(dholbert)
Attachment #8738409 - Flags: review?(dholbert)
Blocks: 1262357
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
Attachment #8738398 - Flags: review?(dholbert) → review+
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.)
Attachment #8738399 - Flags: review?(dholbert)
Attachment #8738400 - Flags: review?(dholbert) → review+
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.)
Attachment #8738401 - Flags: review?(dholbert) → review+
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.
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.
Attachment #8738402 - Flags: review?(dholbert) → review+
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
(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 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
Attachment #8738403 - Flags: review?(dholbert) → review+
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.
Attachment #8738404 - Flags: review?(dholbert) → review+
(Heading to bed now; I'll finish this off with the nsStyleEffects stuff tomorrow.)
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.)
Attachment #8738405 - Flags: review?(dholbert) → review+
Attachment #8738406 - Flags: review?(dholbert) → review+
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 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
Attachment #8738407 - Flags: review?(dholbert) → review+
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
Attachment #8738408 - Flags: review?(dholbert) → review+
Attachment #8738409 - Flags: review?(dholbert) → review+
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.
(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 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/
Attachment #8738398 - Attachment description: MozReview Request: Bug 1261754 - Part 1: Improve static assertions for style struct bits. r?dholbert → MozReview Request: Bug 1261754 - Part 1: Improve static assertions for style struct bits. r=dholbert
Attachment #8738400 - Attachment description: MozReview Request: Bug 1261754 - Part 3: Move quotes from nsStyleQuotes to nsStyleList and delete nsStyleQuotes. r?dholbert → MozReview Request: Bug 1261754 - Part 3: Move quotes from nsStyleQuotes to nsStyleList and delete nsStyleQuotes. r=dholbert
Attachment #8738401 - Attachment description: MozReview Request: Bug 1261754 - Part 4: Move image-rendering from nsStyleSVG to nsStyleVisibility. r?dholbert → MozReview Request: Bug 1261754 - Part 4: Move image-rendering from nsStyleSVG to nsStyleVisibility. r=dholbert
Attachment #8738402 - Attachment description: MozReview Request: Bug 1261754 - Part 5: Move text-rendering from nsStyleSVG to nsStyleText. r?dholbert → MozReview Request: Bug 1261754 - Part 5: Move text-rendering from nsStyleSVG to nsStyleText. r=dholbert
Attachment #8738403 - Attachment description: MozReview Request: Bug 1261754 - Part 6: Move vertical-align from nsStyleTextReset to nsStyleDisplay. r?dholbert → MozReview Request: Bug 1261754 - Part 6: Move vertical-align from nsStyleTextReset to nsStyleDisplay. r=dholbert
Attachment #8738404 - Attachment description: MozReview Request: Bug 1261754 - Part 7: Move pointer-events from nsStyleVisibility to nsStyleUserInterface. r?dholbert → MozReview Request: Bug 1261754 - Part 7: Move pointer-events from nsStyleVisibility to nsStyleUserInterface. r=dholbert
Attachment #8738405 - Attachment description: MozReview Request: Bug 1261754 - Part 8: Move box-shadow from nsStyleBorder to a new nsStyleEffects struct. r?dholbert → MozReview Request: Bug 1261754 - Part 8: Move box-shadow from nsStyleBorder to a new nsStyleEffects struct. r=dholbert
Attachment #8738406 - Attachment description: MozReview Request: Bug 1261754 - Part 9: Move clip from nsStyleDisplay to nsStyleEffects. r?dholbert → MozReview Request: Bug 1261754 - Part 9: Move clip from nsStyleDisplay to nsStyleEffects. r=dholbert
Attachment #8738407 - Attachment description: MozReview Request: Bug 1261754 - Part 10: Move mix-blend-mode from nsStyleDisplay to nsStyleEffects. r?dholbert → MozReview Request: Bug 1261754 - Part 10: Move mix-blend-mode from nsStyleDisplay to nsStyleEffects. r=dholbert
Attachment #8738408 - Attachment description: MozReview Request: Bug 1261754 - Part 11: Move opacity from nsStyleDisplay to nsStyleEffects. r?dholbert → MozReview Request: Bug 1261754 - Part 11: Move opacity from nsStyleDisplay to nsStyleEffects. r=dholbert
Attachment #8738409 - Attachment description: MozReview Request: Bug 1261754 - Part 12: Move filter from nsStyleSVGReset to nsStyleEffects. r?dholbert → MozReview Request: Bug 1261754 - Part 12: Move filter from nsStyleSVGReset to nsStyleEffects. r=dholbert
Attachment #8738399 - Flags: review?(dholbert)
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 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 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 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 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 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 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 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 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 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 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/
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 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.
Attachment #8738399 - Flags: review?(dholbert) → review+
(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.
(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.
Thanks for the reviews!
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!
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6891d6a48f5f
Trivial follow-up to fix outdated documentation. r=me and DONTBUILD because NPOTB.
You need to log in before you can comment on or make changes to this bug.