Closed Bug 1341647 Opened 6 years ago Closed 6 years ago

stylo: need to port HTMLBodyElement::WalkContentStyleRules to stylo

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: manishearth)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Bug 1341647 - stylo: Move HTMLBodyElement::WalkContentStyleRules to the mapped attr functionality;
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
Without this we get various test failures due to various attributes on <body> and <iframe> not being mapped into style correctly.
Blocks: stylo-reftest
Getting this onto Manish's plate.
Flags: needinfo?(manishearth)
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Flags: needinfo?(manishearth)
Priority: -- → P1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Reopening so that I can split out patches.
Blocks: 1341714
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Looks like it fails a lot of tests. Not yet sure why.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=056b61f6666a43aa467f6d38f7bcd77311075308&selectedJob=87403528

bz, is this approach correct?
Flags: needinfo?(bzbarsky)
Comment on attachment 8852623 [details]
Bug 1341647 - stylo: Move HTMLBodyElement::WalkContentStyleRules to the mapped attr functionality;

https://reviewboard.mozilla.org/r/124816/#review127444

::: dom/base/nsAttrAndChildArray.cpp:728
(Diff revision 2)
>  
> +nsresult
> +nsAttrAndChildArray::ForceMapped(nsMappedAttributeElement* aContent, nsIDocument* aDocument)
> +{
> +  nsHTMLStyleSheet* sheet = aDocument->GetAttributeStyleSheet();
> +  nsMappedAttributes* mapped = GetModifiableMapped(aContent, sheet, false);

This needs to be RefPtr<nsMappedAttributes> or you will leak.

::: dom/base/nsFrameLoader.cpp:1213
(Diff revision 2)
> +  // that needs to be updated
> +  if (nsIDocument* doc = mDocShell->GetDocument()) {
> +    if (doc->GetStyleBackendType() == StyleBackendType::Servo) {
> +      for (nsINode* cur = doc; cur; cur = cur->GetNextNode()) {
> +        if (cur->IsHTMLElement(nsGkAtoms::body)) {
> +          static_cast<HTMLBodyElement*>(cur)->GetMappedAttributes()->ClearServoStyle();

This needs a comment about how for Gecko we don't do anything here because we plan to RebuildAllStyleData anyway.

And a comment that we RebuildAllStyleData because now the same nsMappedAttributes* will be producing different styles.

::: dom/base/nsMappedAttributes.h:87
(Diff revision 2)
>    const RefPtr<RawServoDeclarationBlock>& GetServoStyle() const
>    {
>      return mServoStyle;
>    }
>  
> +  void ClearServoStyle() const {

This shouldn't be a const method; it's an explicit mutator!  And then you don't need the const_cast.

I realize that elements don't normally hand out non-const references to their mapped attrs.  What this means is that we need to add a method to them, and to nsAttrAndChildArray, that clears the servo style here; the nsAttrAndChildArray has a non-const reference.

::: dom/base/nsMappedAttributes.h:88
(Diff revision 2)
>    {
>      return mServoStyle;
>    }
>  
> +  void ClearServoStyle() const {
> +    // The servo style is lazily resolved, so it's fine to

The key is that it's lazily resolved on the main thread only, right?

Should probably assert mainthread here.

::: dom/html/HTMLBodyElement.h:124
(Diff revision 2)
> +   */
> +  virtual nsresult AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
> +                                const nsAttrValue* aValue, bool aNotify) override;
> +
>  protected:
> -  virtual ~HTMLBodyElement();
> +  virtual ~HTMLBodyElement() {}

Better to leave the destructor out of line.  Especially since it's virtual anyway, so it's not like it _is_ getting inlined.

::: dom/html/HTMLBodyElement.cpp:178
(Diff revision 2)
>  
>  void
> -HTMLBodyElement::UnbindFromTree(bool aDeep, bool aNullParent)
> +HTMLBodyElement::MapAttributesIntoRule(const nsMappedAttributes* aAttributes,
> +                                       GenericSpecifiedValues* aData)
>  {
> -  if (mContentStyleRule) {
> +  if (!(aData->mSIDs & NS_STYLE_INHERIT_BIT(Margin)))

This part is  wrong: it won't map in any other structs.  What you want is to reverse the sense of this check, and put all the margin* attribute code inside the resulting block.  I spot-checked a few of the test failures on try, and they were all due to this issue at first glance.

::: dom/html/HTMLBodyElement.cpp:188
(Diff revision 2)
> +  int32_t bodyTopMargin = -1;
> +  int32_t bodyBottomMargin = -1;
> +  int32_t bodyLeftMargin = -1;
> +  int32_t bodyRightMargin = -1;
> +
> +  // check the mode (fortunately, the ruleData has a presContext for us to use!)

It's not a ruledata.  It's a GenericSpecifiedValues.

::: dom/html/HTMLBodyElement.cpp:189
(Diff revision 2)
> +  int32_t bodyBottomMargin = -1;
> +  int32_t bodyLeftMargin = -1;
> +  int32_t bodyRightMargin = -1;
> +
> +  // check the mode (fortunately, the ruleData has a presContext for us to use!)
> +  NS_ASSERTION(aData->mPresContext, "null presContext in ruleNode was unexpected");

This is not in ruleNode.

::: dom/html/HTMLBodyElement.cpp:198
(Diff revision 2)
> +  const nsAttrValue* value;
> +  // if marginwidth/marginheight are set, reflect them as 'margin'
> +  value = aAttributes->GetAttr(nsGkAtoms::marginwidth);
> +  if (value && value->Type() == nsAttrValue::eInteger) {
> +    bodyMarginWidth = value->GetIntegerValue();
> +    if (bodyMarginWidth < 0) bodyMarginWidth = 0;

Curlies around the if body, please.  Yes, I know you just moved this.

::: dom/html/HTMLBodyElement.cpp:206
(Diff revision 2)
>    }
>  
> -  nsGenericHTMLElement::UnbindFromTree(aDeep, aNullParent);  
> +  value = aAttributes->GetAttr(nsGkAtoms::marginheight);
> +  if (value && value->Type() == nsAttrValue::eInteger) {
> +    bodyMarginHeight = value->GetIntegerValue();
> +    if (bodyMarginHeight < 0) bodyMarginHeight = 0;

Curlies around body here too.

::: dom/html/HTMLBodyElement.cpp:211
(Diff revision 2)
> +    if (bodyMarginHeight < 0) bodyMarginHeight = 0;
> +    aData->SetPixelValueIfUnset(eCSSProperty_margin_top, (float)bodyMarginHeight);
> +    aData->SetPixelValueIfUnset(eCSSProperty_margin_bottom, (float)bodyMarginHeight);
> +  }
> +
> +    // topmargin (IE-attribute)

Hmm.  So you're keeping the old behavior, but per spec it looks like the parent iframe's marginwidth is supposed to override the leftmargin attr, and so forth.

This seems like a spec bug to me (attrs in our document should trump the parent), though of course that's how webkit behaves due to _copying_ the marginwidth/marginheight off the parent iframe.  Please file the spec bug?  For now, let's keep our existing behavior, which does not follow the current spec.

::: dom/html/HTMLBodyElement.cpp:215
(Diff revision 2)
> +
> +    // topmargin (IE-attribute)
> +  value = aAttributes->GetAttr(nsGkAtoms::topmargin);
> +  if (value && value->Type() == nsAttrValue::eInteger) {
> +    bodyTopMargin = value->GetIntegerValue();
> +    if (bodyTopMargin < 0) bodyTopMargin = 0;

Curlies.

::: dom/html/HTMLBodyElement.cpp:223
(Diff revision 2)
> +
> +    // bottommargin (IE-attribute)
> +  value = aAttributes->GetAttr(nsGkAtoms::bottommargin);
> +  if (value && value->Type() == nsAttrValue::eInteger) {
> +    bodyBottomMargin = value->GetIntegerValue();
> +    if (bodyBottomMargin < 0) bodyBottomMargin = 0;

Curlies.

::: dom/html/HTMLBodyElement.cpp:231
(Diff revision 2)
> +
> +    // leftmargin (IE-attribute)
> +  value = aAttributes->GetAttr(nsGkAtoms::leftmargin);
> +  if (value && value->Type() == nsAttrValue::eInteger) {
> +    bodyLeftMargin = value->GetIntegerValue();
> +    if (bodyLeftMargin < 0) bodyLeftMargin = 0;

Curlies.

::: dom/html/HTMLBodyElement.cpp:239
(Diff revision 2)
> +
> +    // rightmargin (IE-attribute)
> +  value = aAttributes->GetAttr(nsGkAtoms::rightmargin);
> +  if (value && value->Type() == nsAttrValue::eInteger) {
> +    bodyRightMargin = value->GetIntegerValue();
> +    if (bodyRightMargin < 0) bodyRightMargin = 0;

Curlies.

::: dom/html/HTMLBodyElement.cpp:252
(Diff revision 2)
> +    if (docShell) {
> +      nscoord frameMarginWidth=-1;  // default value
> +      nscoord frameMarginHeight=-1; // default value
> +      docShell->GetMarginWidth(&frameMarginWidth); // -1 indicates not set
> +      docShell->GetMarginHeight(&frameMarginHeight);
> +      if ((frameMarginWidth >= 0) && (bodyMarginWidth == -1)) { // set in <frame> & not in <body>

This is overparenthesized.  Yes, I know it just got moved.

::: dom/html/HTMLBodyElement.cpp:254
(Diff revision 2)
> +      nscoord frameMarginHeight=-1; // default value
> +      docShell->GetMarginWidth(&frameMarginWidth); // -1 indicates not set
> +      docShell->GetMarginHeight(&frameMarginHeight);
> +      if ((frameMarginWidth >= 0) && (bodyMarginWidth == -1)) { // set in <frame> & not in <body>
> +        if (eCompatibility_NavQuirks == mode) {
> +          if ((bodyMarginHeight == -1) && (0 > frameMarginHeight)) // nav quirk

Again, overparenthesized.

Please file a followup to check whether other browsers have this quirk.  If so, we should add it to the quirks standard.  If not, we shoud remove it.

::: dom/html/HTMLBodyElement.cpp:258
(Diff revision 2)
> +        if (eCompatibility_NavQuirks == mode) {
> +          if ((bodyMarginHeight == -1) && (0 > frameMarginHeight)) // nav quirk
> +            frameMarginHeight = 0;
> +        }
> +      }
> +      if ((frameMarginHeight >= 0) && (bodyMarginHeight == -1)) { // set in <frame> & not in <body>

Overparenthesized.

::: dom/html/HTMLBodyElement.cpp:260
(Diff revision 2)
> +            frameMarginHeight = 0;
> +        }
> +      }
> +      if ((frameMarginHeight >= 0) && (bodyMarginHeight == -1)) { // set in <frame> & not in <body>
> +        if (eCompatibility_NavQuirks == mode) {
> +          if ((bodyMarginWidth == -1) && (0 > frameMarginWidth)) // nav quirk

Again, overparenthesized.  And need to check the quirk thing.

::: dom/html/HTMLBodyElement.cpp:265
(Diff revision 2)
> +          if ((bodyMarginWidth == -1) && (0 > frameMarginWidth)) // nav quirk
> +            frameMarginWidth = 0;
> +        }
> +      }
> +
> +      if ((bodyMarginWidth == -1) && (frameMarginWidth >= 0)) {

Overparenthesized.

::: dom/html/HTMLBodyElement.cpp:336
(Diff revision 2)
> -    // These aren't mapped through attribute mapping, but they are
> -    // mapped through a style rule, so it is attribute dependent style.
> -    // XXXldb But we don't actually replace the body rule when we have
> -    // dynamic changes...
>      { &nsGkAtoms::marginwidth },
>      { &nsGkAtoms::marginheight },

You need to add topmargin/bottommargin/leftmargin/rightmargin here, no?  Otherwise dynamic changes to them would be broken.  I expect they're broken in Gecko right now too...

Would be good to add tests for this.

::: dom/html/HTMLBodyElement.cpp:406
(Diff revision 2)
> +{
> +  nsresult rv = nsGenericHTMLElement::AfterSetAttr(aNameSpaceID,
> +                                                   aName, aValue, aNotify);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (aNameSpaceID == kNameSpaceID_None) {
> +    if (aName == nsGkAtoms::marginheight ||

This condition doesn't look right.

I assume the intent is that we have a mapped attr even if we're removing an attr and hence might have dropped the mapped attributes in nsAttrAndChildArray::RemoveAttrAt.  But that can happen on removal of _any_ last mapped attr, not just the ones in this list.

What you probably want is to check for !aValue (meaning removal) and IsAttributeMapped(aName).

And add tests that would have caught this problem, if none of our existing ones do?
Attachment #8852623 - Flags: review-
Comment on attachment 8852623 [details]
Bug 1341647 - stylo: Move HTMLBodyElement::WalkContentStyleRules to the mapped attr functionality;

https://reviewboard.mozilla.org/r/124816/#review127444

> Again, overparenthesized.
> 
> Please file a followup to check whether other browsers have this quirk.  If so, we should add it to the quirks standard.  If not, we shoud remove it.

bug 1352002
the bug for the precedence thing is at https://github.com/whatwg/html/issues/2484

All other comments should be addressed.

r?
Blocks: 1352150
We weren't posting restyle hints when mapped attributes changed. Fixed.

Should be green on try now.
Comment on attachment 8852623 [details]
Bug 1341647 - stylo: Move HTMLBodyElement::WalkContentStyleRules to the mapped attr functionality;

https://reviewboard.mozilla.org/r/124816/#review127754

I'm not seeing a test for the "remove a last mapped attr that's not margin-related" situation I asked for a test for at the end of https://bugzilla.mozilla.org/show_bug.cgi?id=1341647#c7

::: dom/base/nsAttrAndChildArray.cpp:733
(Diff revisions 2 - 5)
> -  nsMappedAttributes* mapped = GetModifiableMapped(aContent, sheet, false);
> +  RefPtr<nsMappedAttributes> mapped = GetModifiableMapped(aContent, sheet, false);
>    return MakeMappedUnique(mapped);
>  }
>  
> +void
> +nsAttrAndChildArray::ClearMappedServoStyle() {

'{' on next line, please.

::: dom/html/HTMLBodyElement.cpp:189
(Diff revisions 2 - 5)
> +    // This is the one place where we try to set the same property
> +    // multiple times in presentation attributes. Servo does not support
> +    // querying if a property is set (because that is O(n) behavior
> +    // in ServoSpecifiedValues). Instead, we use the below values to keep
> +    // track of whether we have already set a property, and if so, what value
> +    // we set it to (which is used by the frame handling)

s/frame/containing frame element/ maybe?

::: dom/html/HTMLBodyElement.cpp:232
(Diff revisions 2 - 5)
> -  if (value && value->Type() == nsAttrValue::eInteger) {
> +    if (value && value->Type() == nsAttrValue::eInteger) {
> -    bodyTopMargin = value->GetIntegerValue();
> +      bodyTopMargin = value->GetIntegerValue();
> -    if (bodyTopMargin < 0) bodyTopMargin = 0;
> +      if (bodyTopMargin < 0) {
> +        bodyTopMargin = 0;
> +      }
> +      if (bodyMarginHeight == -1) {

Shouldn't this go around the entire "look for topmargin" block?  Why bother to even look for the attribute if we plan to ignore it?

::: dom/html/HTMLBodyElement.cpp:244
(Diff revisions 2 - 5)
> -  if (value && value->Type() == nsAttrValue::eInteger) {
> +    if (value && value->Type() == nsAttrValue::eInteger) {
> -    bodyBottomMargin = value->GetIntegerValue();
> +      bodyBottomMargin = value->GetIntegerValue();
> -    if (bodyBottomMargin < 0) bodyBottomMargin = 0;
> +      if (bodyBottomMargin < 0) {
> +        bodyBottomMargin = 0;
> +      }
> +      if (bodyMarginHeight == -1) {

Again, this should probably move out more.

::: dom/html/HTMLBodyElement.cpp:256
(Diff revisions 2 - 5)
> -  if (value && value->Type() == nsAttrValue::eInteger) {
> +    if (value && value->Type() == nsAttrValue::eInteger) {
> -    bodyLeftMargin = value->GetIntegerValue();
> +      bodyLeftMargin = value->GetIntegerValue();
> -    if (bodyLeftMargin < 0) bodyLeftMargin = 0;
> +      if (bodyLeftMargin < 0) {
> +        bodyLeftMargin = 0;
> +      }
> +      if (bodyMarginWidth == -1) {

And move this out more too.

::: dom/html/HTMLBodyElement.cpp:268
(Diff revisions 2 - 5)
> -  if (value && value->Type() == nsAttrValue::eInteger) {
> +    if (value && value->Type() == nsAttrValue::eInteger) {
> -    bodyRightMargin = value->GetIntegerValue();
> +      bodyRightMargin = value->GetIntegerValue();
> -    if (bodyRightMargin < 0) bodyRightMargin = 0;
> +      if (bodyRightMargin < 0) {
> +        bodyRightMargin = 0;
> +      }
> +      if (bodyMarginWidth == -1) {

And move this out too.

::: dom/html/HTMLBodyElement.cpp:291
(Diff revisions 2 - 5)
> -        }
> +            }
> -      }
> +          }
> -      if ((frameMarginHeight >= 0) && (bodyMarginHeight == -1)) { // set in <frame> & not in <body>
> +        }
> +        if (frameMarginHeight >= 0 && bodyMarginHeight == -1) { // set in <frame> & not in <body>
> -        if (eCompatibility_NavQuirks == mode) {
> +          if (eCompatibility_NavQuirks == mode) {
> -          if ((bodyMarginWidth == -1) && (0 > frameMarginWidth)) // nav quirk
> +            if ((bodyMarginWidth == -1) && (0 > frameMarginWidth)) { // nav quirk

Overparenthesized.

::: dom/html/HTMLBodyElement.cpp:451
(Diff revisions 2 - 5)
>  {
>    nsresult rv = nsGenericHTMLElement::AfterSetAttr(aNameSpaceID,
>                                                     aName, aValue, aNotify);
>    NS_ENSURE_SUCCESS(rv, rv);
> -  if (aNameSpaceID == kNameSpaceID_None) {
> -    if (aName == nsGkAtoms::marginheight ||
> +  // if the last mapped attribute was removed, don't clear the
> +  // nsMappedAttributes, our style can still depend on the frame

Again, maybe "containing frame element"?

Really, I want to not have people read "the frame" as "the nsIFrame".

::: dom/base/FragmentOrElement.h:227
(Diff revision 5)
>    static void RemoveBlackMarkedNode(nsINode* aNode);
>    static void MarkNodeChildren(nsINode* aNode);
>    static void InitCCCallbacks();
>    static void MarkUserData(void* aObject, nsIAtom* aKey, void* aChild,
>                             void *aData);
> +  void ClearMappedServoStyle() {

This should be in Element, not FragmentOrElement.  Fragments never have attrs.

::: dom/base/nsMappedAttributes.cpp:81
(Diff revision 5)
>    // aSize will include the mAttrs buffer so subtract that.
> -  void* newAttrs = ::operator new(aSize - sizeof(void*[1]) +
> -                                  aAttrCount * sizeof(InternalAttr));
> +  // We don't want to under-allocate, however, so do not subtract
> +  // if we have zero attributes. The zero attribute case only happens
> +  // for <body>'s mapped attributes
> +  if (aAttrCount != 0) {
> +    size -= sizeof(void*[1]); 

Trailing whitespace after the ';' there/

::: dom/base/nsMappedAttributes.cpp:289
(Diff revision 5)
>  }
>  
>  size_t
>  nsMappedAttributes::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const
>  {
> -  NS_ASSERTION(mAttrCount == mBufferSize,
> +  NS_ASSERTION(mAttrCount == 0 || mAttrCount == mBufferSize,

Why do we need to change this assertion?  I'd expect mBufferSize to be 0 for our forced mapped attrs, since that's what operator new would set it to, no?

::: dom/html/reftests/reftest-stylo.list:46
(Diff revision 5)
>  
>  # Test that image documents taken into account CSS properties like
>  # image-orientation when determining the size of the image.
>  # (Fuzzy necessary due to pixel-wise comparison of different JPEGs.
>  # The vast majority of the fuzziness comes from Linux and WinXP.)
> -fails == bug917595-iframe-1.html bug917595-iframe-1.html # Bug 1341647
> +== bug917595-iframe-1.html bug917595-iframe-1.html # Bug 1341647

The comment at the end of the line can go away.

::: layout/reftests/bugs/reftest-stylo.list:836
(Diff revision 5)
>  == 398797-1c.html 398797-1c.html
>  == 398797-1d.html 398797-1d.html
>  == 399209-1.html 399209-1.html
>  == 399209-2.html 399209-2.html
>  == 399258-1.html 399258-1.html
> -fails == 399384-1.html 399384-1.html
> +== 399384-1.html 399384-1.html

I don't see why this patch would make this test change behavior in any way.  Seems pretty odd.

::: layout/reftests/css-break/reftest-stylo.list:4
(Diff revision 5)
>  # DO NOT EDIT! This is a auto-generated temporary list for Stylo testing
>  default-preferences pref(layout.css.box-decoration-break.enabled,true)
>  
> -fails == box-decoration-break-1.html box-decoration-break-1.html
> +== box-decoration-break-1.html box-decoration-break-1.html

Also not sure why the behavior would change here.

::: layout/reftests/css-break/reftest-stylo.list:6
(Diff revision 5)
>  # DO NOT EDIT! This is a auto-generated temporary list for Stylo testing
>  default-preferences pref(layout.css.box-decoration-break.enabled,true)
>  
> -fails == box-decoration-break-1.html box-decoration-break-1.html
> +== box-decoration-break-1.html box-decoration-break-1.html
>  fails == box-decoration-break-with-inset-box-shadow-1.html box-decoration-break-with-inset-box-shadow-1.html
> -fails == box-decoration-break-with-outset-box-shadow-1.html box-decoration-break-with-outset-box-shadow-1.html
> +== box-decoration-break-with-outset-box-shadow-1.html box-decoration-break-with-outset-box-shadow-1.html

Or why behavior changed here.

::: layout/reftests/first-letter/reftest-stylo.list:37
(Diff revision 5)
>  == 23605-3.html 23605-3.html
>  == 23605-4.html 23605-4.html
>  == 23605-5.html 23605-5.html
>  == 23605-6.html 23605-6.html
>  == 229764-1.html 229764-1.html
> -fails == 229764-2.html 229764-2.html # Bug 1324618
> +== 229764-2.html 229764-2.html # Bug 1324618

And this part is not clear to me either.

::: layout/reftests/first-letter/reftest-stylo.list:55
(Diff revision 5)
>  == 399941-6.html 399941-6.html
>  == 399941-7.html 399941-7.html
>  == 399941-8.html 399941-8.html
>  == 399941-9.html 399941-9.html
>  == 429968-1a.html 429968-1a.html
> -fails == 429968-1b.html 429968-1b.html
> +== 429968-1b.html 429968-1b.html

Or this.

::: layout/reftests/table-anonymous-boxes/reftest-stylo.list:50
(Diff revision 5)
>  == infer-table-around-headers-footers-2.html infer-table-around-headers-footers-2.html
>  == infer-table-around-headers-footers-3.html infer-table-around-headers-footers-3.html
>  == infer-rows-inside-rowgroups.html infer-rows-inside-rowgroups.html
>  == infer-table-row-cell.html infer-table-row-cell.html
>  == infer-table.html infer-table.html
> -fails == 3-tables-ref.html 3-tables-ref.html # Bug 1341775
> +== 3-tables-ref.html 3-tables-ref.html # Bug 1341775

This doesn't make sense either.

::: layout/reftests/text-decoration/reftest-stylo.list:113
(Diff revision 5)
>  fails == underline-button-2.html underline-button-2.html
>  fails == underline-select-1.html underline-select-1.html
>  fails == underline-select-2.html underline-select-2.html
>  fails == 1133392.html 1133392.html
>  == 1159729-offset-adjustment.html 1159729-offset-adjustment.html
> -fails == emphasis-style-dynamic.html emphasis-style-dynamic.html
> +== emphasis-style-dynamic.html emphasis-style-dynamic.html

This doesn't make sense either...

::: layout/reftests/text-shadow/reftest-stylo.list:2
(Diff revision 5)
>  # DO NOT EDIT! This is a auto-generated temporary list for Stylo testing
> -fails == 723669.html 723669.html
> +== 723669.html 723669.html

And this doesn't either.

::: layout/style/test/stylo-failures.md:37
(Diff revision 5)
>  * Media query support:
>    * test_bug1089417.html [1]
>    * test_bug418986-2.html: matchMedia support [3]
>    * test_bug453896_deck.html: &lt;style media&gt; support [8]
>    * test_media_queries.html [657]
> -  * test_media_queries_dynamic.html [11]
> +  * test_media_queries_dynamic.html [15]

Why are there more failures?

::: layout/style/test/stylo-failures.md:466
(Diff revision 5)
>  * test_additional_sheets.html: one sub-test cascade order is wrong [1]
>  * test_flexbox_layout.html: resolved width doesn't match expectation [5]
>  * test_selectors.html `:nth-child`: &lt;an+b&gt; parsing difference [14]
>  * test_selectors_on_anonymous_content.html: xbl and :nth-child [1]
>  * test_variables.html `url`: url in custom property [1]
> -* test_pseudoelement_state.html: doesn't seem to work at all, but only range-thumb fails... [4]
> +* test_pseudoelement_state.html: doesn't seem to work at all, but only range-thumb fails... [8]

Hmm...  I guess there are fewer here?  Did the error message on some of them change or something?  Why would the patches in this bug cause that?
Blocks: 1348723
Comment on attachment 8852623 [details]
Bug 1341647 - stylo: Move HTMLBodyElement::WalkContentStyleRules to the mapped attr functionality;

https://reviewboard.mozilla.org/r/124816/#review127846

::: dom/plugins/test/reftest/reftest-stylo.list:19
(Diff revision 7)
> -fails == plugin-background-1-step.html plugin-background-1-step.html # bug 1348723
> -fails == plugin-background-2-step.html plugin-background-2-step.html # bug 1348723
> -fails == plugin-background-5-step.html plugin-background-5-step.html # bug 1348723
> -fails == plugin-background-10-step.html plugin-background-10-step.html # bug 1348723
> +== plugin-background-1-step.html plugin-background-1-step.html # bug 1348723
> +== plugin-background-2-step.html plugin-background-2-step.html # bug 1348723
> +== plugin-background-5-step.html plugin-background-5-step.html # bug 1348723
> +== plugin-background-10-step.html plugin-background-10-step.html # bug 1348723

Remove the "bug 1348723" annotation from these, please.
The other fishy bit here is this:

>+asserts-if(stylo,1) load 412014-1.html # bug 1324618

Bug 1324618 is not related there.  We're hitting bug 731659, _but_ we're hitting it because we reframe the <body>.  And we should not be reframing the <body> on this testcase.  Something about the ForceMapped call in BindToTree causes that.  Needs to be figured out.  We do not want more <body> reframes on the web.
Flags: needinfo?(bzbarsky)
Comment on attachment 8852623 [details]
Bug 1341647 - stylo: Move HTMLBodyElement::WalkContentStyleRules to the mapped attr functionality;

https://reviewboard.mozilla.org/r/124816/#review127754

> Why do we need to change this assertion?  I'd expect mBufferSize to be 0 for our forced mapped attrs, since that's what operator new would set it to, no?

It was crashing earlier; tracked it down to the default value of `1` for the attr size in the `new` operator for `nsMappedAttributes`. Changed it to 0 for the force case.

> I don't see why this patch would make this test change behavior in any way.  Seems pretty odd.

ditto wrt bindtotree

> Also not sure why the behavior would change here.

(this change dropped out in the rebase)

> And this part is not clear to me either.

On a hunch, I removed the BindToTree behavior and the test went back to failing, the same way regular stylo does.

What I suspect is that *somehow* the BindToTree stuff triggers a ton of extra restyles. These tests essentially have a common defining feature; there are some dynamic changes happening, and they fail because we forget to restyle. If there are suddenly a lot of restyles happening, these tests will just start magically passing.

> Or this.

ditto wrt BindToTree

> This doesn't make sense either.

killed in the rebase

> This doesn't make sense either...

ditto wrt bindtotree. also crashy because we don't handle text-emphasis-position pres attrs, which I'll just fix up.

> And this doesn't either.

ditto wrt BindToTree

> Why are there more failures?

ditto wrt BindToTree. back down to 11 if BindToTree does not force a mapped attr.

> Hmm...  I guess there are fewer here?  Did the error message on some of them change or something?  Why would the patches in this bug cause that?

ditto wrt BindToTree. back down to 4 if BindToTree does not force a mapped attr.
Pushed a try run without bindtotree to check what's going on.

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

Yeah, I have that test WIP, will push up soon.
Comment on attachment 8852623 [details]
Bug 1341647 - stylo: Move HTMLBodyElement::WalkContentStyleRules to the mapped attr functionality;

https://reviewboard.mozilla.org/r/124816/#review128056

OK, but we really do need the BindToTree thing somewhere, right?  Otherwise, why would marginwidth/height on the parent iframe ever work?  And it doesn't, with the BindToTree bit removed.

::: dom/html/HTMLBodyElement.cpp
(Diff revisions 7 - 12)
> -      }
> +        }
> -      if (bodyMarginWidth == -1) {
>          aData->SetPixelValueIfUnset(eCSSProperty_margin_right, (float)bodyRightMargin);
>        }
>      }
> -

Probably best to leave this blank line, not remove it.
Attachment #8852623 - Flags: review?(bzbarsky)
> Otherwise, why would marginwidth/height on the parent iframe ever work? 

Specifically why it would ever work in the situation when the <body> has no attributes.  It's possible we have no such testcases in our tree, or that they're failing for other reasons, of course.
Heh, this comment from last night midaired:

> Yep, it was BindToTree. Removed it, and all the weird UNEXPECTED-PASSes are gone.
>
> I'm not quite sure why this works without BindToTree; I would have expected more crashes since we assume that the attrs always exist. Curious; will investigate tomorrow. Nor am I sure why that caused the extra restyles.
> 
> Still need to finish that test, also for tomorrow.
  

So yeah, bindtotree fixes it, but that can't be the *right* fix, because we need it to happen. From IRC it seems like it's us wrongly applying pres hints to pseudos and then triggering restyles.
Depends on: 1352464
Comment on attachment 8852623 [details]
Bug 1341647 - stylo: Move HTMLBodyElement::WalkContentStyleRules to the mapped attr functionality;

https://reviewboard.mozilla.org/r/124816/#review128162

r=me
Attachment #8852623 - Flags: review?(bzbarsky) → review+
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e212c8b1248b
stylo: Move HTMLBodyElement::WalkContentStyleRules to the mapped attr functionality; r=bz
https://hg.mozilla.org/mozilla-central/rev/e212c8b1248b
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.