Some refactoring in preparation for bug 1436059

RESOLVED FIXED in Firefox 60

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
15 days ago
11 days ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

unspecified
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(9 attachments)

(Assignee)

Description

15 days ago
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

15 days ago
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.
(Assignee)

Comment 24

15 days 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

14 days 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

14 days 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

14 days 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

14 days 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

14 days 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

14 days 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

14 days 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

14 days 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

13 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/48cf4fb1409b
Status: NEW → RESOLVED
Last Resolved: 13 days ago
status-firefox60: --- → fixed
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.