Closed Bug 1384542 Opened 7 years ago Closed 7 years ago

stylo: kill GetParentAllowServo.

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(5 files, 3 obsolete files)

      No description provided.
Comment on attachment 8890461 [details]
Bug 1384542: Remove usage of GetParentAllowServo in the frame constructor.

https://reviewboard.mozilla.org/r/161538/#review166892
Attachment #8890461 - Flags: review?(bzbarsky) → review+
Comment on attachment 8890463 [details]
Bug 1384542: Update reftest expectations for bogus first-line support in stylo.

https://reviewboard.mozilla.org/r/161542/#review166906
Attachment #8890463 - Flags: review?(bzbarsky) → review+
See Also: → 1349326
Comment on attachment 8890462 [details]
Bug 1384542: Get rid of GetParentAllowServo in implementation of CSS 'justify-items' property.

https://reviewboard.mozilla.org/r/161540/#review167104

r- for now, since I think the FIXME might be lethal :-/

::: commit-message-8b585:1
(Diff revision 3)
> +Bug 1384542: Get rid of GetParentAllowServo in justify-items. r?dholbert,heycam

s/in justify-items/in implementation of CSS 'justify-items' property/

::: layout/generic/nsBlockFrame.cpp:1532
(Diff revision 3)
>      // that'd only affect rendering if some child has "justify-self:auto".
>      // (It's safe to assume that's likely, since it's the default value that
>      // a child would have.) We also pass in nullptr for the parent style context
>      // because an accurate parameter is slower and only necessary to detect a
>      // narrow edge case with the "legacy" keyword.
>      const nsStylePosition* stylePosition = reflowInput->mStylePosition;
>      if (!IsStyleNormalOrAuto(stylePosition->mJustifyContent) ||
>          !IsStyleNormalOrAuto(stylePosition->mAlignContent) ||
> -        !IsStyleNormalOrAuto(stylePosition->ComputedJustifyItems(nullptr))) {
> +        !IsStyleNormalOrAuto(stylePosition->mJustifyItems)) {

Please update the "We also pass in nullptr" comment above this code.  (You're removing the function call where we passed in nullptr.)

::: layout/style/nsRuleNode.cpp:8522
(Diff revision 3)
> +  if (aSpecifiedJustifiedItems != NS_STYLE_JUSTIFY_AUTO) {
> +    return aSpecifiedJustifiedItems;
> +  }
> +
> +  if (aParentJustifyItems & NS_STYLE_JUSTIFY_LEGACY) {
> +    return aParentJustifyItems;
> +  }
> +
> +  return NS_STYLE_JUSTIFY_NORMAL;

Please preserve the spec-quote from the old version of this function, to explain its behavior a bit.

  // "If the inherited value of justify-items includes the 'legacy' keyword,
  // 'auto' computes to the inherited value."  Otherwise, 'normal'.

::: layout/style/nsRuleNode.cpp:8688
(Diff revision 3)
> +    if (pos->mSpecifiedJustifyItems == NS_STYLE_JUSTIFY_AUTO) {
> +      // FIXME(emilio): This is kind of unfortunate because this is a reset
> +      // property and AUTO is the default value... Can we do better?
> +      conditions.SetUncacheable();
> +    }

I think this is sufficiently-bad problem to block this from landing, with this FIXME unaddressed. :-/

This means this style struct can *never* be shared, basically (except on elements that happen to have justify-items explicitly set).  And I think that's a pretty painful defeat of our memory optimizations here.

(Maybe it's not as bad as I think for some reason, though? I'll let heycam be the authority here.)

::: layout/style/nsStyleStruct.h:1707
(Diff revision 3)
> -private:
> -  friend class nsRuleNode;
> -
> +  // We cascade mSpecifiedJustifyItems, to handle the auto value, but store the
> +  // computed value in mJustifyItems.
> +  uint8_t       mSpecifiedJustifyItems; // [reset] see nsStyleConsts.h

It would be helpful to document how/when they might be expected to differ. 

E.g. maybe something like "They will differ if mSpecifiedJustifyItems is NS_STYLE_JUSTIFY_AUTO, but otherwise they're the same" [is this true? I'm guessing it is, but I haven't read the whole patch in detail].

It might also be handy to sprinkle some assertions to that ^^ effect around, too.

::: layout/style/nsStyleStruct.cpp:1638
(Diff revision 3)
> +  // No need to do anything if mSpecifiedJustifyItems changes.
> +  if (mSpecifiedJustifyItems != aNewData.mSpecifiedJustifyItems) {

Please add something like this to end of the comment here:
", as long as mJustifyItems (tested above) is unchanged."
Attachment #8890462 - Flags: review?(dholbert) → review-
Comment on attachment 8890462 [details]
Bug 1384542: Get rid of GetParentAllowServo in implementation of CSS 'justify-items' property.

https://reviewboard.mozilla.org/r/161540/#review167112

::: layout/style/nsRuleNode.cpp:8688
(Diff revision 3)
> +    if (pos->mSpecifiedJustifyItems == NS_STYLE_JUSTIFY_AUTO) {
> +      // FIXME(emilio): This is kind of unfortunate because this is a reset
> +      // property and AUTO is the default value... Can we do better?
> +      conditions.SetUncacheable();
> +    }

Yeah, I was expecting somebody to tell me this wasn't really needed (that's why the question was there in the comment :P), but I think it is...

I think we may need to do something like whatever we do for relative units (which I don't know)... 

Anyway, I'll discuss with Cam tomorrow :)
Comment on attachment 8890638 [details]
Bug 1384542: Fix dynamic change handling of justify-items legacy values.

https://reviewboard.mozilla.org/r/161796/#review167126

::: layout/style/RuleNodeCacheConditions.h:109
(Diff revision 1)
> +  /**
> +   * Record that the data being computed depends on the writing mode of
> +   * the _parent_ element for which it's being computed.
> +   */
> +  void SetParentJustifyItemsDependency(uint8_t aParentJustifyItems)
> +  {
> +    MOZ_ASSERT(!(mBits & eHaveParentJustifyItems));

Comment tweak: s/writing mode/computed 'justify-items'/ (or somesuch)
(In reply to Daniel Holbert [:dholbert] from comment #24)
> Comment on attachment 8890638 [details]
> Bug 1384542: Add a parentJustifyItems dependency in RuleNodeCacheConditions.
> 
> https://reviewboard.mozilla.org/r/161796/#review167126
> 
> ::: layout/style/RuleNodeCacheConditions.h:109
> (Diff revision 1)
> > +  /**
> > +   * Record that the data being computed depends on the writing mode of
> > +   * the _parent_ element for which it's being computed.
> > +   */
> > +  void SetParentJustifyItemsDependency(uint8_t aParentJustifyItems)
> > +  {
> > +    MOZ_ASSERT(!(mBits & eHaveParentJustifyItems));
> 
> Comment tweak: s/writing mode/computed 'justify-items'/ (or somesuch)

Yup, good catch, thanks! I also think I should null-check GetParent() (though it may not be needed?). 

Anyway, note that these last three patches are undertested (i.e., I verified the browser boots, and pushed to try, because I should sleep), but I want Cam to see them in the morning to see if this is the correct approach to fix the caching issue or not.
Comment on attachment 8890460 [details]
Bug 1384542: Remove the use of GetParentAllowServo in nsFirstLetterFrame.

https://reviewboard.mozilla.org/r/161536/#review167178

::: layout/generic/nsFirstLetterFrame.cpp:386
(Diff revision 2)
> -                                              mStyleContext;
> -      sc = aPresContext->StyleSet()->ResolveStyleForText(kidContent, parentSC);
> +      if (prevInFlow) {
> +        nsIFrame* styleParent =

Can you add a comment in here saying this is for the "rest of the content not in the first line" case, and the other branch is for the "in the first line" case?
Attachment #8890460 - Flags: review?(cam) → review+
Comment on attachment 8890462 [details]
Bug 1384542: Get rid of GetParentAllowServo in implementation of CSS 'justify-items' property.

https://reviewboard.mozilla.org/r/161540/#review167104

> I think this is sufficiently-bad problem to block this from landing, with this FIXME unaddressed. :-/
> 
> This means this style struct can *never* be shared, basically (except on elements that happen to have justify-items explicitly set).  And I think that's a pretty painful defeat of our memory optimizations here.
> 
> (Maybe it's not as bad as I think for some reason, though? I'll let heycam be the authority here.)

Yeah, unfortunately I think we can't do this.  As you say, it means that nearly every element will have its own Position struct (unless we share a whole style context), and they are 520 bytes each.
The later patches to use RuleNodeCacheConditions to handle this case might be OK.  When we have absolutely no rules (as will be the common case for the Position struct), then we should share the style struct that is cached on the root rule node, IIRC.  (That's done in nsRuleNode::WalkRuleTree.)  Emilio, can you verify that when we handle justify-items in RuleNodeCacheConditions, that we can still share the Position struct from there, when we have elements with no Position struct properties specified?  I think I remember we don't look at structs cached with conditions, when choosing one from an ancestor rule node, but I might be wrong.
Flags: needinfo?(emilio+bugs)
(In reply to Cameron McCormack (:heycam) from comment #28)
> The later patches to use RuleNodeCacheConditions to handle this case might
> be OK.  When we have absolutely no rules (as will be the common case for the
> Position struct), then we should share the style struct that is cached on
> the root rule node, IIRC.  (That's done in nsRuleNode::WalkRuleTree.) 
> Emilio, can you verify that when we handle justify-items in
> RuleNodeCacheConditions, that we can still share the Position struct from
> there, when we have elements with no Position struct properties specified? 
> I think I remember we don't look at structs cached with conditions, when
> choosing one from an ancestor rule node, but I might be wrong.

I can try, yeah. I _think_ it will, since we don't even enter the ComputePositionData function for a default struct. But I actually think that that's wrong, actually... And indeed my patch produces incorrect behavior on a very simple test-case like:

<!DOCTYPE html>
<style>
div {
  justify-items: legacy center;
}
</style>
<div><span></span></div>
<script>
  alert(getComputedStyle(document.querySelector('span')).justifyItems);
</script>

So I'll need to find another approach...
Flags: needinfo?(emilio+bugs)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #29)
> (In reply to Cameron McCormack (:heycam) from comment #28)
> > The later patches to use RuleNodeCacheConditions to handle this case might
> > be OK.  When we have absolutely no rules (as will be the common case for the
> > Position struct), then we should share the style struct that is cached on
> > the root rule node, IIRC.  (That's done in nsRuleNode::WalkRuleTree.) 
> > Emilio, can you verify that when we handle justify-items in
> > RuleNodeCacheConditions, that we can still share the Position struct from
> > there, when we have elements with no Position struct properties specified? 
> > I think I remember we don't look at structs cached with conditions, when
> > choosing one from an ancestor rule node, but I might be wrong.
> 
> I can try, yeah. I _think_ it will, since we don't even enter the
> ComputePositionData function for a default struct. But I actually think that
> that's wrong, actually... And indeed my patch produces incorrect behavior on
> a very simple test-case like:
> 
> <!DOCTYPE html>
> <style>
> div {
>   justify-items: legacy center;
> }
> </style>
> <div><span></span></div>
> <script>
>   alert(getComputedStyle(document.querySelector('span')).justifyItems);
> </script>
> 
> So I'll need to find another approach...

I _think_ for servo I can fix this in StyleAdjuster, pretty much like what we do for the border stuff... So we'd keep the similar approach, but without the cache conditions stuff, and resolve it at the end of the cascade, effectively only mutating it if we need to (the legacy flag is on the parent, and mAlignItems != the parent's mAlignItems). That'd solve both the caching (we'd only need to fix up the struct in the very uncommon case) and correctness issues I think.

Cameron, does that sound reasonable?
Flags: needinfo?(cam)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #30)
> I _think_ for servo I can fix this in StyleAdjuster, pretty much like what
> we do for the border stuff... So we'd keep the similar approach, but without
> the cache conditions stuff, and resolve it at the end of the cascade,
> effectively only mutating it if we need to (the legacy flag is on the
> parent, and mAlignItems != the parent's mAlignItems). That'd solve both the
> caching (we'd only need to fix up the struct in the very uncommon case) and
> correctness issues I think.
> 
> Cameron, does that sound reasonable?

So, just to clarify, in Gecko, we would keep the current behavior, and in Servo we do the "eager" resolving down to the real computed style?  And then modify nsComputedDOMStyle::DoGetJustifyItems so that it only does the "lazy" resolving when we're for Gecko?  I think that sounds OK to me.

Though if we ever do bug 1367635, we'll have to come up with something else...
Flags: needinfo?(cam)
Attachment #8890639 - Attachment is obsolete: true
Attachment #8890639 - Flags: review?(cam)
Comment on attachment 8890462 [details]
Bug 1384542: Get rid of GetParentAllowServo in implementation of CSS 'justify-items' property.

https://reviewboard.mozilla.org/r/161540/#review167104

> Please update the "We also pass in nullptr" comment above this code.  (You're removing the function call where we passed in nullptr.)

Done.

> Please preserve the spec-quote from the old version of this function, to explain its behavior a bit.
> 
>   // "If the inherited value of justify-items includes the 'legacy' keyword,
>   // 'auto' computes to the inherited value."  Otherwise, 'normal'.

This went away in the last version of the patch.

> Yeah, unfortunately I think we can't do this.  As you say, it means that nearly every element will have its own Position struct (unless we share a whole style context), and they are 520 bytes each.

I discussed with Cam, and the best way I thought of doing it without doing it separately for Servo and Gecko was handling the resolution in the style fixup phase, which makes us only get a unique struct for elements whose parent has the legacy flag, and in which pulling it from our parent would change the actualy computed value.
Comment on attachment 8890460 [details]
Bug 1384542: Remove the use of GetParentAllowServo in nsFirstLetterFrame.

https://reviewboard.mozilla.org/r/161536/#review167178

> Can you add a comment in here saying this is for the "rest of the content not in the first line" case, and the other branch is for the "in the first line" case?

You mean in the first-letter, right? Done.
Sorry for the bugspam everyone, forgot to address review comments before pushing.

Anyway, the new approach I think is a good compromise that allows us to cache the position struct pretty much always, avoid complexity in the rule node code, and having the same model between Servo and Gecko.

Let me know if you think this wouldn't work for some reason.
Comment on attachment 8890462 [details]
Bug 1384542: Get rid of GetParentAllowServo in implementation of CSS 'justify-items' property.

https://reviewboard.mozilla.org/r/161540/#review167104

> This went away in the last version of the patch.

I don't think it went away -- it looks to me like it's still there.  https://reviewboard.mozilla.org/r/161540/diff/7 still creates a new function called "ComputedJustifyItems" in nsRuleNode.cpp, and that function would benefit from this spec-quote (snipped from the old ComputedJustifyItems function).
Comment on attachment 8890462 [details]
Bug 1384542: Get rid of GetParentAllowServo in implementation of CSS 'justify-items' property.

https://reviewboard.mozilla.org/r/161540/#review167104

> I don't think it went away -- it looks to me like it's still there.  https://reviewboard.mozilla.org/r/161540/diff/7 still creates a new function called "ComputedJustifyItems" in nsRuleNode.cpp, and that function would benefit from this spec-quote (snipped from the old ComputedJustifyItems function).

Arg, I squashed my changes against the wrong version of the patch... It goes away in the next one right now, will fix.
Comment on attachment 8890462 [details]
Bug 1384542: Get rid of GetParentAllowServo in implementation of CSS 'justify-items' property.

https://reviewboard.mozilla.org/r/161540/#review167488

I think this might still have the memory-optimization-defeating issue :(

::: layout/generic/nsBlockFrame.cpp:1534
(Diff revision 8)
>      // First check this frame for non-default values of the css-align properties
>      // that apply to block containers.
>      // Note: we check here for non-default "justify-items", though technically
>      // that'd only affect rendering if some child has "justify-self:auto".
>      // (It's safe to assume that's likely, since it's the default value that
> -    // a child would have.) We also pass in nullptr for the parent style context
> +    // a child would have.).

Please drop the final period on this line. There is no sentence that it is terminating. The parenthetical sentence stands alone and has its own period inside the parens. :)

::: layout/style/GeckoStyleContext.cpp:869
(Diff revision 8)
> +  // Fixup the justify-items: auto value based on our parent style here if
> +  // needed.
> +  if (mParent &&

Please put quotes around "justify-items: auto", so that this sentence is easier to parse.

(Otherwise, it's easy to accidentally misinterpret the colon and read this like...
 //  Fixup the justify-items:
 //  ------------------------
 //  auto value based on our parent [...]

...which doesn't make sense & is clearly not how you intend for this to be read.)

::: layout/style/GeckoStyleContext.cpp:871
(Diff revision 8)
> +  if (mParent &&
> +      mParent->StylePosition()->mJustifyItems & NS_STYLE_JUSTIFY_LEGACY &&
> +      StylePosition()->mSpecifiedJustifyItems == NS_STYLE_JUSTIFY_AUTO &&
> +      StylePosition()->mJustifyItems != mParent->StylePosition()->mJustifyItems) {

The order of checks here seems a little odd.

We only need to care about our parent's style (let alone whether it happens to include "legacy") if our own specified style is "auto" (which, granted, is the default).

So it seems like that's the very first thing we should be checking here. *shrug* Maybe not a big deal though.

Also, might be worth resurrecting that spec-quote I asked about above, and putting the resurrected text here?  Since this is where that logic lives, and this logic is a bit magic if there isn't a bit of documentation explaining it.  (Probably good to include a hint that "normal" is what will already be in mJustifyItems if we leave it alone -- with that, the spec-quote explains the logic here, IIUC)

::: layout/style/GeckoStyleContext.cpp:875
(Diff revision 8)
> +    nsStylePosition* uniquePosition = GET_UNIQUE_STYLE_DATA(Position);
> +    uniquePosition->mJustifyItems = mParent->StylePosition()->mJustifyItems;

GET_UNIQUE_STYLE_DATA sounds like we're preventing ourselves from sharing this style struct.  So I think this still has the same memory-optimization-defeating downfall that the last version had... And I think this all still happens *by default* for every element (i.e. it happens with the default value of "justify-items")

Note that this calls into this API:
https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/layout/style/GeckoStyleContext.cpp#308-312
which is documented as "evil evil... since it forces you to alloc your own separate copy of style data".

I'm guessing that this is less of an issue for the other ApplyStyleFixups cases because they don't exercise this code path *by default* (I hope).   But here, we would be exercising it *by default*, IIUC...
Attachment #8890462 - Flags: review?(dholbert) → review-
Comment on attachment 8890462 [details]
Bug 1384542: Get rid of GetParentAllowServo in implementation of CSS 'justify-items' property.

https://reviewboard.mozilla.org/r/161540/#review167488

> GET_UNIQUE_STYLE_DATA sounds like we're preventing ourselves from sharing this style struct.  So I think this still has the same memory-optimization-defeating downfall that the last version had... And I think this all still happens *by default* for every element (i.e. it happens with the default value of "justify-items")
> 
> Note that this calls into this API:
> https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/layout/style/GeckoStyleContext.cpp#308-312
> which is documented as "evil evil... since it forces you to alloc your own separate copy of style data".
> 
> I'm guessing that this is less of an issue for the other ApplyStyleFixups cases because they don't exercise this code path *by default* (I hope).   But here, we would be exercising it *by default*, IIUC...

Oh, I missed the significance of this caveat from comment 44:
 "only get a unique struct [...if] pulling it from our parent would change the actualy computed value"

I think that check does indeed let us skip the unique struct by default, then, right?  If so, hooray!!  This is good news and *very* important -- please make sure to add documentation to this chunk of code, to explain this check (and that without it, we'd be requesting a unique struct by default).
Comment on attachment 8890462 [details]
Bug 1384542: Get rid of GetParentAllowServo in implementation of CSS 'justify-items' property.

https://reviewboard.mozilla.org/r/161540/#review167488

> The order of checks here seems a little odd.
> 
> We only need to care about our parent's style (let alone whether it happens to include "legacy") if our own specified style is "auto" (which, granted, is the default).
> 
> So it seems like that's the very first thing we should be checking here. *shrug* Maybe not a big deal though.
> 
> Also, might be worth resurrecting that spec-quote I asked about above, and putting the resurrected text here?  Since this is where that logic lives, and this logic is a bit magic if there isn't a bit of documentation explaining it.  (Probably good to include a hint that "normal" is what will already be in mJustifyItems if we leave it alone -- with that, the spec-quote explains the logic here, IIUC)

I pull the style of the parent first to avoid pulling the style struct unconditionally for the child if the LEGACY flag is not specified.

Perhaps it's worth a comment, will add that along with the spec quote.

> Oh, I missed the significance of this caveat from comment 44:
>  "only get a unique struct [...if] pulling it from our parent would change the actualy computed value"
> 
> I think that check does indeed let us skip the unique struct by default, then, right?  If so, hooray!!  This is good news and *very* important -- please make sure to add documentation to this chunk of code, to explain this check (and that without it, we'd be requesting a unique struct by default).

As discussed on IRC, there are two bits that matter here.

The first is that the `LEGACY` modifier is not the default (the default value is `AUTO`), and not only that but I'd say that specifying the legacy modifier would be quite weird.

The second is that bit you just mentioned, which is that we only do it iff it affects the result of the computation.

Both deserve a comment I suppose. I'll address your comments tomorrow morning, since it's quite late here already.

Mote also (since you mentioned updating the spec as a possible source of problems here), that we comply with the spec as written right now even though the spec naming and our naming disagree to some extent.

We call `AUTO` to "legacy" without any other keyword, which is the non-observable value. But we use `LEGACY` _only_ when it's along another keyword... The spec wording looks also quite weird to me, but our behavior and way of representing these makes sense to me over all.
Comment on attachment 8890462 [details]
Bug 1384542: Get rid of GetParentAllowServo in implementation of CSS 'justify-items' property.

https://reviewboard.mozilla.org/r/161540/#review167656

r=me on most of the C++ changes, with nits addressed.

(I'll defer to heycam for the rust changes and the GeckoStyleContext.cpp changes [sounds like you've already discussed those with him so I assume he's OK with them])

::: layout/style/nsRuleNode.cpp:8675
(Diff revision 8)
> +  // NOTE(emilio): Even though the AUTO value may depend on the parent style
> +  // context, we handle that uncommon special case in ApplyStyleFixups,
> +  // instead of disabling all caching of all Position structs.
> +  pos->mJustifyItems =

This comment is slightly confusing/mis-stating the facts, RE "may depend" & "uncommon special case".

The "auto" value *by definition* depends on the parent style context -- that is not an uncommon special case at all. (It "depends" in that we do have to check the parent before we know what the correct value is.)

Could you reword this to be clearer about the dependency & the actual special-case? Maybe something like this: "Even though "auto" technically depends on the parent style context, most of the time it'll resolve to "normal".  So, we optimistically assume here that it does resolve to "normal", and we handle the other cases in ApplyStyleFixups. This way, position structs can be cached in the default/common case.

::: layout/style/nsStyleStruct.h:1710
(Diff revision 8)
> -  // 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.
> +  // They're effectively only different in this regard: mJustifyItems is
> +  // effectively mSpecifiedJustifyItems, except the later is converted
> +  // from AUTO to NORMAL, or to the parent style context's mJustifyItems if it

s/later/latter/

But, RE "except the later is converted": this phrasing is ambiguous.  It sounds to me like it's saying mSpecifiedJustifyItems (the latter) *gets the converted value* -- and that's not correct. (I know what you're trying to say, but without that knowledge, I'd be confused by this.)

Maybe reword like so, to remove the ambiguity:

"mJustifyItems is set to mSpecifiedJustifyItems, except when the latter is AUTO -- in that case, mJustifyItems is set to NORMAL, or to the parent style context's [etc]"

::: layout/style/nsStyleStruct.h:1715
(Diff revision 8)
> -  // 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.
> +  // They're effectively only different in this regard: mJustifyItems is
> +  // effectively mSpecifiedJustifyItems, except the later is converted
> +  // from AUTO to NORMAL, or to the parent style context's mJustifyItems if it
> +  // has the legacy flag enabled.
> +  //
> +  // This last bit happens in nsStyleContext::ApplyStyleFixups.

s/bit/part/ -- otherwise it sounds like you might be talking about the last bit of this uint8_t 8-bit value, or something. :)
Attachment #8890462 - Flags: review- → review+
Comment on attachment 8890462 [details]
Bug 1384542: Get rid of GetParentAllowServo in implementation of CSS 'justify-items' property.

https://reviewboard.mozilla.org/r/161540/#review167662

::: servo/components/style/properties/gecko.mako.rs:1375
(Diff revision 8)
> +    pub fn copy_justify_items_from(&mut self, other: &Self) {
> +        self.gecko.mJustifyItems = other.gecko.mJustifyItems;
> +        self.gecko.mSpecifiedJustifyItems = other.gecko.mJustifyItems;
> +    }

Why do we do this instead of copying over mSpecifiedJustifyItems?

::: servo/components/style/properties/properties.mako.rs
(Diff revision 8)
> -        % if property.style_struct.inherited:
>          let inherited_struct =
> +        % if property.style_struct.inherited:
>              self.inherited_style.get_${property.style_struct.name_lower}();
>          % else:
> -        let inherited_struct =
>              self.inherited_style_ignoring_first_line.get_${property.style_struct.name_lower}();
>          % endif
> +

(This belonged in a separate patch.)

::: servo/components/style/style_adjuster.rs:457
(Diff revision 8)
>          if relevant_link_visited {
>              self.style.flags.insert(IS_RELEVANT_LINK_VISITED);
>          }
>      }
>  
> +    /// Resolves justify-items: auto based on the inherited style if needed to

Nit: if you want to follow Daniel's earlier advice, quotes would go around 'justify-items: auto' here too.  (Though here there's less chance to mis-parse the sentence, I think.)

::: servo/components/style/style_adjuster.rs:460
(Diff revision 8)
>      }
>  
> +    /// Resolves justify-items: auto based on the inherited style if needed to
> +    /// comply with:
> +    ///
> +    /// https://drafts.csswg.org/css-align/#valdef-justify-items-auto

Please update the link to https://drafts.csswg.org/css-align/#valdef-justify-items-legacy and maybe mention somewhere how the spec currently has some renamings in flight.

::: servo/components/style/values/computed/mod.rs:403
(Diff revision 8)
>  
> +/// The computed value for the `justify-items` property.
> +///
> +/// Need to carry around both the specified and computed value to handle the
> +/// special legacy keyword. Sigh
> +#[cfg(feature = "gecko")]

Why don't we need all this stuff for Servo too?

::: servo/components/style/values/computed/mod.rs:405
(Diff revision 8)
> +///
> +/// Need to carry around both the specified and computed value to handle the
> +/// special legacy keyword. Sigh
> +#[cfg(feature = "gecko")]
> +#[derive(Debug, Copy, Clone, PartialEq, Eq)]
> +pub struct JustifyItems {

I think this definition would be better in an align.rs file, so that it matches up with specified/align.rs.

::: servo/components/style/values/computed/mod.rs:443
(Diff revision 8)
> -            if inherited.0.contains(align::ALIGN_LEGACY) {
> -                return inherited
> -            }
> -        }
> -        return *self
> +            } else {
> +                // If the inherited value of `justify-items` includes the
> +                // `legacy` keyword, `auto` computes to the inherited value,
> +                // but we handle that special-case in StyleAdjuster.
> +                Self::normal()
> +            };

So this case should never come up, is that right?  Should we unreachable!() it instead?
Comment on attachment 8890464 [details]
Bug 1384542: Move GetParent and IsLinkContext to GeckoStyleContext.

https://reviewboard.mozilla.org/r/161544/#review167664

Looks good!

::: layout/tables/nsTableColGroupFrame.cpp:302
(Diff revision 8)
>  #ifdef DEBUG
>          nsIFrame* providerFrame;
>          nsStyleContext* psc = colFrame->GetParentStyleContext(&providerFrame);
>          if (psc->IsGecko()) {
>            // This check code is useful only in Gecko-backed style system.
> -          if (static_cast<nsStyleContext*>(colFrame->StyleContext()->GetParent()) == psc) {
> +          if (colFrame->StyleContext()->AsGecko()->GetParent() == psc->AsGecko()) {

Nit: probably don't need the "->AsGecko()" after the "psc".
Attachment #8890464 - Flags: review?(cam) → review+
Comment on attachment 8890637 [details]
style: Standardize different methods to inherit and reset properties.

https://reviewboard.mozilla.org/r/161794/#review167666

I guess this is OK but I with all this added code I kind of feel like the special casing isn't that bad...
Attachment #8890637 - Flags: review?(cam) → review+
Comment on attachment 8890638 [details]
Bug 1384542: Fix dynamic change handling of justify-items legacy values.

https://reviewboard.mozilla.org/r/161796/#review167670

::: testing/web-platform/tests/css/css-align-3/default-alignment/justify-items-legacy-001.html:22
(Diff revision 5)
> +    assert_equals(getComputedStyle(child).justifyItems, "legacy center",
> +                  "default justify-items resolves to the parent justify-items value if legacy")
> +    container.style.justifyItems = "legacy left";
> +    assert_equals(getComputedStyle(child).justifyItems, "legacy left",
> +                  "dynamic changes to justify-items propagate to children if needed (legacy)")
> +    container.style.justifyItems = "normal";

Maybe use a value other than "normal" here?
Attachment #8890638 - Flags: review?(cam) → review+
Comment on attachment 8890462 [details]
Bug 1384542: Get rid of GetParentAllowServo in implementation of CSS 'justify-items' property.

https://reviewboard.mozilla.org/r/161540/#review167730

::: servo/components/style/properties/gecko.mako.rs:1375
(Diff revision 8)
> +    pub fn copy_justify_items_from(&mut self, other: &Self) {
> +        self.gecko.mJustifyItems = other.gecko.mJustifyItems;
> +        self.gecko.mSpecifiedJustifyItems = other.gecko.mJustifyItems;
> +    }

That's what Gecko does for the `inherit` case, and it makes sense to me, you really want to inherit the computed value, not the computed and `auto`/`legacy`.

::: servo/components/style/values/computed/mod.rs:403
(Diff revision 8)
>  
> +/// The computed value for the `justify-items` property.
> +///
> +/// Need to carry around both the specified and computed value to handle the
> +/// special legacy keyword. Sigh
> +#[cfg(feature = "gecko")]

The alignment stuff is compiled out for servo right now.

::: servo/components/style/values/computed/mod.rs:405
(Diff revision 8)
> +///
> +/// Need to carry around both the specified and computed value to handle the
> +/// special legacy keyword. Sigh
> +#[cfg(feature = "gecko")]
> +#[derive(Debug, Copy, Clone, PartialEq, Eq)]
> +pub struct JustifyItems {

Yeah, that's fair, will do.

::: servo/components/style/values/computed/mod.rs:443
(Diff revision 8)
> -            if inherited.0.contains(align::ALIGN_LEGACY) {
> -                return inherited
> -            }
> -        }
> -        return *self
> +            } else {
> +                // If the inherited value of `justify-items` includes the
> +                // `legacy` keyword, `auto` computes to the inherited value,
> +                // but we handle that special-case in StyleAdjuster.
> +                Self::normal()
> +            };

Why not? It does come up, we just fix it up on the style adjustment phase if it happens to not compute to `normal`.
Comment on attachment 8890462 [details]
Bug 1384542: Get rid of GetParentAllowServo in implementation of CSS 'justify-items' property.

https://reviewboard.mozilla.org/r/161540/#review167730

> Why not? It does come up, we just fix it up on the style adjustment phase if it happens to not compute to `normal`.

Yeah, I'm not sure what I was thinking with that comment, please ignore.
Comment on attachment 8890462 [details]
Bug 1384542: Get rid of GetParentAllowServo in implementation of CSS 'justify-items' property.

https://reviewboard.mozilla.org/r/161540/#review168168
Attachment #8890462 - Flags: review?(cam) → review+
Attachment #8890463 - Attachment is obsolete: true
Attachment #8890637 - Attachment is obsolete: true
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/05829f3321be
Remove the use of GetParentAllowServo in nsFirstLetterFrame. r=heycam
https://hg.mozilla.org/integration/autoland/rev/266a37b224ff
Remove usage of GetParentAllowServo in the frame constructor. r=bz
https://hg.mozilla.org/integration/autoland/rev/c4f81c52c266
Get rid of GetParentAllowServo in implementation of CSS 'justify-items' property. r=dholbert,heycam
https://hg.mozilla.org/integration/autoland/rev/c48b603e7650
Move GetParent and IsLinkContext to GeckoStyleContext. r=heycam
https://hg.mozilla.org/integration/autoland/rev/a3a199efb743
Fix dynamic change handling of justify-items legacy values. r=heycam
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a2717f579f07
Fix MustReresolveStyle logic. r=me
Depends on: 1390933
Regressions: 1551126
You need to log in before you can comment on or make changes to this bug.