1425769 - Add a base class for ShadowRoot and Document to share style state

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

This is a pre-requisite for multiple bugs, and allows to eventually remove the proto binding.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1425769: Base class for ShadowRoot and Document to manage style state.

I don't know if there's a more elegant way to do the cycle collection stuff, for
now I've kept calling it directly from nsIDocument / ShadowRoot.

This also removes some confusing comments around nsIDocument regarding some kind
of "special" stylesheets, which don't seem to exist anymore, and consolidates
StyleSheetList so that we only have one implementation.

I think that fixes a potential leak on the shadow root code (even though the API
is v0 only), given the pointer from the ShadowRootStyleSheetList to the
ShadowRoot wasn't being CCd.

Also, more stuff could be renamed, methods removed, etc, feel free to suggest
more cleanup, I've done mostly the minimal.

Next steps are moving the stylesets there and stop using the proto binding sheet
list / resources.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8937370 [details]
Bug 1425769: Base class for ShadowRoot and Document to manage style state.

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

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8937370 [details]
Bug 1425769: Base class for ShadowRoot and Document to manage style state.

https://reviewboard.mozilla.org/r/208048/#review213898

::: commit-message-cd829:3
(Diff revision 2)
> +Bug 1425769: Base class for ShadowRoot and Document to manage style state. r?smaug
> +
> +I don't know if there's a more elegant way to do the cycle collection stuff, for

FWIW, this kinds of comments shouldn't be committed to hg. Commit messages should explain the change.

::: dom/base/StyleSheetList.h:55
(Diff revision 2)
> +    if (!mStyleScope) {
> +      aFound = false;
> +      return nullptr;
> +    }
>  
> -  virtual uint32_t Length() = 0;
> +    auto* sheet = mStyleScope->SheetAt(aIndex);

No 'auto' here. Reader can't know from SheetAt call what the type of sheet variable is.

::: dom/base/nsDocument.h:1143
(Diff revision 2)
>    // caches its result here.
>    mozilla::Maybe<bool> mIsThirdParty;
>  
>  public:
>    RefPtr<mozilla::EventListenerManager> mListenerManager;
>    RefPtr<mozilla::dom::StyleSheetList> mDOMStyleSheets;

Er, don't you need to remove this mDOMStyleSheets member variable, now that same named member variable is inherited from mozilla::dom::StyleScope

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8937370 [details]
Bug 1425769: Base class for ShadowRoot and Document to manage style state.

https://reviewboard.mozilla.org/r/208048/#review213898

> FWIW, this kinds of comments shouldn't be committed to hg. Commit messages should explain the change.

Yeah, usually I remove them before landing, I whish there was a way for dropping a "note to reviewer" or something :)

> No 'auto' here. Reader can't know from SheetAt call what the type of sheet variable is.

Yeah, I thought it was deductible since we return it. But yeah, fair enough.

> Er, don't you need to remove this mDOMStyleSheets member variable, now that same named member variable is inherited from mozilla::dom::StyleScope

Blerg, messed it up when squashing / cleaning up the patch.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8937370 [details]
Bug 1425769: Base class for ShadowRoot and Document to manage style state.

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

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Notes to reviewer could be bugzilla comments, especially when not using MozReview ;)

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] from comment #6)
> Notes to reviewer could be bugzilla comments, especially when not using
> MozReview ;)

Heh, fair enough, though not all reviewers go through the bug before they go ahead and look at the patch... Will note down for next patches I send to you :)

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8937370 [details]
Bug 1425769: Base class for ShadowRoot and Document to manage style state.

https://reviewboard.mozilla.org/r/208048/#review213992

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13faabcf8e96
Base class for ShadowRoot and Document to manage style state. r=smaug

Comment 10

[Dorel Luca [:dluca]]

Backed out changeset 13faabcf8e96 (bug 1425769) for Linux bustage on build/src/dom/base/nsLineBreaker.h

https://treeherder.mozilla.org/logviewer.html#?job_id=152299361&repo=autoland&lineNumber=14546

https://hg.mozilla.org/integration/autoland/rev/d32719cf7ea713ea4432982c9cd7c57ebde78eda

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=13faabcf8e965cca857e3cc8fdbea289a09c7ea0

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4f5ec55f5e2
Add missing include. r=me
https://hg.mozilla.org/integration/autoland/rev/f42c0478255d
Base class for ShadowRoot and Document to manage style state. r=smaug

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/e4f5ec55f5e2
https://hg.mozilla.org/mozilla-central/rev/f42c0478255d