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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
Details
Attachments
(9 files)
(deleted),
text/x-review-board-request
|
xidorn
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
bholley
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details |
Just moving things around so it's more sane.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•7 years ago
|
||
Not exactly sure of what Mozreview did there.
Comment 20•7 years ago
|
||
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)
Comment 21•7 years ago
|
||
Or maybe you have tried that once?
Comment 22•7 years ago
|
||
And it seems I cannot review either... HTTP 500 INTERNAL SERVER ERROR :/
Comment 23•7 years ago
|
||
Filed bug 1436880 for this issue.
Assignee | ||
Comment 24•7 years ago
|
||
Yeah, let me try to rebase and do that.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=034cbed5b26b42b833851b28a18ad24e3c383cd4
Is a try run btw.
Flags: needinfo?(emilio)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 34•7 years ago
|
||
mozreview-review |
Comment on attachment 8949494 [details]
style: Rename StylesheetSet to DocumentStylesheetSet.
https://reviewboard.mozilla.org/r/218816/#review224670
Attachment #8949494 -
Flags: review?(xidorn+moz) → review+
Assignee | ||
Comment 35•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
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 36•7 years ago
|
||
mozreview-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
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+
Assignee | ||
Comment 37•7 years ago
|
||
mozreview-review-reply |
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 ;)
Assignee | ||
Comment 38•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 39•7 years ago
|
||
mozreview-review |
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.
Comment 40•7 years ago
|
||
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
Comment 41•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•7 years ago
|
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.
Description
•