Closed Bug 1436798 Opened 7 years ago Closed 7 years ago

Some refactoring in preparation for bug 1436059

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(9 files)

Just moving things around so it's more sane.
Not exactly sure of what Mozreview did there.
Err... What's wrong with MozReview... Could you discard the current set and push them again to see if things would work fine with that? You should be able to find it in "Close" menu of the Review Summary page.
Flags: needinfo?(emilio)
Or maybe you have tried that once?
And it seems I cannot review either... HTTP 500 INTERNAL SERVER ERROR :/
Filed bug 1436880 for this issue.
Flags: needinfo?(emilio)
Comment on attachment 8949494 [details] style: Rename StylesheetSet to DocumentStylesheetSet. https://reviewboard.mozilla.org/r/218816/#review224670
Attachment #8949494 - Flags: review?(xidorn+moz) → review+
Xidorn told me that he couldn't get to it, and that Bobby or nox were maybe more adequate reviewers, so I submitted the servo patch to https://github.com/servo/servo/pull/20004, and will drop the requests on the patches that don't need Gecko changes here.
Attachment #8949495 - Flags: review?(xidorn+moz)
Attachment #8949496 - Flags: review?(xidorn+moz)
Attachment #8949497 - Flags: review?(xidorn+moz)
Attachment #8949498 - Flags: review?(xidorn+moz)
Attachment #8949499 - Flags: review?(xidorn+moz)
Attachment #8949500 - Flags: review?(xidorn+moz) → review?(bobbyholley)
Attachment #8949501 - Flags: review?(xidorn+moz)
Attachment #8949502 - Flags: review?(xidorn+moz)
Comment on attachment 8949500 [details] Bug 1436798: style: Move author-style-disabled handling to push_applicable_declarations. https://reviewboard.mozilla.org/r/218828/#review224850 r=me with that sorted out. ::: layout/style/ServoStyleSet.cpp:365 (Diff revision 2) > { > if (mAuthorStyleDisabled == aStyleDisabled) { > - return NS_OK; > + return; > } > > mAuthorStyleDisabled = aStyleDisabled; It's not clear to me how this gets propagated to the stylist now that we're no longer calling Servo_StyleSet_NoteStyleSheetsChanged. Maybe it's on one of the other patches? Please make sure to test that the feature still works, because I don't know how well-tested it is in automation. ::: layout/style/nsStyleSet.cpp:686 (Diff revision 2) > - if (!mBatching) > - return GatherRuleProcessors(aType); > + if (!mBatching) { > + GatherRuleProcessors(aType); > - > - mDirty |= DirtyBit(aType); > - return NS_OK; > + return NS_OK; > -} > + } > > -bool > -nsStyleSet::GetAuthorStyleDisabled() const > + mDirty |= DirtyBit(aType); > + return NS_OK; > -{ > - return mAuthorStyleDisabled; Nit: With the nsresult propagation removed, I think this would be marginally more readable as an if/else with no early-return.
Attachment #8949500 - Flags: review?(bobbyholley) → review+
Comment on attachment 8949500 [details] Bug 1436798: style: Move author-style-disabled handling to push_applicable_declarations. https://reviewboard.mozilla.org/r/218828/#review224850 > It's not clear to me how this gets propagated to the stylist now that we're no longer calling Servo_StyleSet_NoteStyleSheetsChanged. > > Maybe it's on one of the other patches? Please make sure to test that the feature still works, because I don't know how well-tested it is in automation. The key here is that we don't _need_ to notify the stylist. That's key. Instead, the set of rules we keep around in the stylist are always the same, and we just skip matching them. This is useful, because that way we don't need to rebuild the style data of all the shadow roots in the document when the setting changes. Also, yeah, this is well-tested on automation, I remember breaking it in the past and try being angry at it ;)
Comment on attachment 8949500 [details] Bug 1436798: style: Move author-style-disabled handling to push_applicable_declarations. https://reviewboard.mozilla.org/r/218828/#review224850 > Nit: With the nsresult propagation removed, I think this would be marginally more readable as an if/else with no early-return. Will do, agreed.
Comment on attachment 8949500 [details] Bug 1436798: style: Move author-style-disabled handling to push_applicable_declarations. https://reviewboard.mozilla.org/r/218828/#review224882 ::: layout/style/ServoStyleSet.cpp:365 (Diff revision 2) > { > if (mAuthorStyleDisabled == aStyleDisabled) { > - return NS_OK; > + return; > } > > mAuthorStyleDisabled = aStyleDisabled; Hmm, though I see what you mean, I think I can come up with a test-case where the boolean is not set up in the stylist properly in presence of dynamic changes to the pref... Will fix that.
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/48cf4fb1409b style: Move author-style-disabled handling to push_applicable_declarations. r=bholley
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Summary: Some refactoring in preparation for bug 1436059. → Some refactoring in preparation for bug 1436059
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: