1436798 - Some refactoring in preparation for bug 1436059

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

Just moving things around so it's more sane.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: style: Rename StylesheetSet to DocumentStylesheetSet.

Review commit: https://reviewboard.mozilla.org/r/218816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218816/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: style: Move the dirty bit to SheetCollection.

Review commit: https://reviewboard.mozilla.org/r/218818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218818/

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: style: Assert we don't try to do dumb stuff.

Review commit: https://reviewboard.mozilla.org/r/218820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218820/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: style: Rename OriginValidity to DataValidity.

Review commit: https://reviewboard.mozilla.org/r/218822/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218822/

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: style: Save some work if the origin is not dirty.

Review commit: https://reviewboard.mozilla.org/r/218824/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218824/

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: style: Make AuthorStylesEnabled an enum.

Chances are we need to pass it around in a bit.

Also invert the boolean because I don't want to reason about double negations,
even if they're simple.

Review commit: https://reviewboard.mozilla.org/r/218826/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218826/

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1436798: style: Move author-style-disabled handling to push_applicable_declarations.

This will make it easier to handle it properly for Shadow DOM, though this patch
doesn't do that.

This also makes some method inline and infallible for convenience, since nobody
checks the errors anyway.

Review commit: https://reviewboard.mozilla.org/r/218828/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218828/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: style: Move the stylesheet set methods under a macro to reuse it soon.

Review commit: https://reviewboard.mozilla.org/r/218830/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218830/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: style: Introduce AuthorStyleSheetSet.

Review commit: https://reviewboard.mozilla.org/r/218832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218832/

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949494 [details]
style: Rename StylesheetSet to DocumentStylesheetSet.

Review commit: https://reviewboard.mozilla.org/r/218816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218816/

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949495 [details]
style: Move the dirty bit to SheetCollection.

Review commit: https://reviewboard.mozilla.org/r/218818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218818/

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949496 [details]
style: Assert we don't try to do dumb stuff.

Review commit: https://reviewboard.mozilla.org/r/218820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218820/

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949497 [details]
style: Rename OriginValidity to DataValidity.

Review commit: https://reviewboard.mozilla.org/r/218822/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218822/

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949498 [details]
style: Save some work if the origin is not dirty.

Review commit: https://reviewboard.mozilla.org/r/218824/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218824/

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949499 [details]
style: Make AuthorStylesEnabled an enum.

Chances are we need to pass it around in a bit.

Also invert the boolean because I don't want to reason about double negations,
even if they're simple.

Review commit: https://reviewboard.mozilla.org/r/218826/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218826/

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949500 [details]
Bug 1436798: style: Move author-style-disabled handling to push_applicable_declarations.

This will make it easier to handle it properly for Shadow DOM, though this patch
doesn't do that.

This also makes some method inline and infallible for convenience, since nobody
checks the errors anyway.

Review commit: https://reviewboard.mozilla.org/r/218828/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218828/

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949501 [details]
style: Move the stylesheet set methods under a macro to reuse it soon.

Review commit: https://reviewboard.mozilla.org/r/218830/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218830/

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949502 [details]
style: Introduce AuthorStyleSheetSet.

Review commit: https://reviewboard.mozilla.org/r/218832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218832/

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Not exactly sure of what Mozreview did there.

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Or maybe you have tried that once?

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

And it seems I cannot review either... HTTP 500 INTERNAL SERVER ERROR :/

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

Filed bug 1436880 for this issue.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Yeah, let me try to rebase and do that.

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

Is a try run btw.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949494 [details]
style: Rename StylesheetSet to DocumentStylesheetSet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218816/diff/1-2/

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949495 [details]
style: Move the dirty bit to SheetCollection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218818/diff/1-2/

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949496 [details]
style: Assert we don't try to do dumb stuff.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218820/diff/1-2/

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949497 [details]
style: Rename OriginValidity to DataValidity.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218822/diff/1-2/

Comment 29

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949498 [details]
style: Save some work if the origin is not dirty.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218824/diff/1-2/

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949499 [details]
style: Make AuthorStylesEnabled an enum.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218826/diff/1-2/

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949500 [details]
Bug 1436798: style: Move author-style-disabled handling to push_applicable_declarations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218828/diff/1-2/

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949501 [details]
style: Move the stylesheet set methods under a macro to reuse it soon.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218830/diff/1-2/

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949502 [details]
style: Introduce AuthorStyleSheetSet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218832/diff/1-2/

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8949494 [details]
style: Rename StylesheetSet to DocumentStylesheetSet.

https://reviewboard.mozilla.org/r/218816/#review224670

Comment 35

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 36

[Bobby Holley (:bholley)]

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.

Comment 37

[Emilio Cobos Álvarez (:emilio)]

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 38

[Emilio Cobos Álvarez (:emilio)]

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 39

[Emilio Cobos Álvarez (:emilio)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48cf4fb1409b
style: Move author-style-disabled handling to push_applicable_declarations. r=bholley

Comment 41

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/48cf4fb1409b