Closed Bug 1425769 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

This is a pre-requisite for multiple bugs, and allows to eventually remove the proto binding.
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
Attachment #8937370 - Flags: review?(bugs) → review-
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.
Notes to reviewer could be bugzilla comments, especially when not using MozReview ;)
(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 on attachment 8937370 [details]
Bug 1425769: Base class for ShadowRoot and Document to manage style state.

https://reviewboard.mozilla.org/r/208048/#review213992
Attachment #8937370 - Flags: review?(bugs) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/13faabcf8e96
Base class for ShadowRoot and Document to manage style state. r=smaug
Pushed by ecoal95@gmail.com:
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
Flags: needinfo?(emilio)
https://hg.mozilla.org/mozilla-central/rev/e4f5ec55f5e2
https://hg.mozilla.org/mozilla-central/rev/f42c0478255d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Assignee: nobody → emilio
You need to log in before you can comment on or make changes to this bug.