1422633 - Inline nsDocument::ThreadSafeGetDocumentState.

Status: RESOLVED FIXED

Description

[Emilio Cobos Álvarez (:emilio)]

It's a totally unnecessary function call, that shows up on stylo-chrome profiles.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Created attachment 8934020 [details]
Bug 1422633: Inline nsIDocument::ThreadSafeGetDocumentState().

It's silly to pay a function call for that, and it shows up in stylo-chrome
profiles.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Created attachment 8934021 [details]
Bug 1422633: Inline the Rust bits of GetDocumentState too.

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

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8934021 [details]
Bug 1422633: Inline the Rust bits of GetDocumentState too.

https://reviewboard.mozilla.org/r/204948/#review210388

Comment 4

[Olli Pettay [:smaug] (high review load)]

Comment on attachment 8934020 [details]
Bug 1422633: Inline nsIDocument::ThreadSafeGetDocumentState().

https://reviewboard.mozilla.org/r/204946/#review210548

::: dom/base/nsIDocument.h:2644
(Diff revision 1)
>    /**
>     * Returns the document state.
>     * Document state bits have the form NS_DOCUMENT_STATE_* and are declared in
>     * nsIDocument.h.
>     */
> -  virtual mozilla::EventStates GetDocumentState() = 0;
> +  mozilla::EventStates GetDocumentState() {

{ goes to its own line

::: dom/base/nsIDocument.h:2652
(Diff revision 1)
> +  }
> +
> +  // GetDocumentState() mutates the state due to lazy resolution;
> +  // and can't be used during parallel traversal. Use this instead,
> +  // and ensure GetDocumentState() has been called first.
> +  // This will assert if the state is stale.

Not about this bug, since you just move code, but what asserts anything here?

::: dom/base/nsIDocument.h:2653
(Diff revision 1)
> +
> +  // GetDocumentState() mutates the state due to lazy resolution;
> +  // and can't be used during parallel traversal. Use this instead,
> +  // and ensure GetDocumentState() has been called first.
> +  // This will assert if the state is stale.
> +  mozilla::EventStates ThreadSafeGetDocumentState() const {

{ goes to its own line

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] (ni?s and f?s processed after r?s) from comment #4)
> ::: dom/base/nsIDocument.h:2652
> (Diff revision 1)
> > +  }
> > +
> > +  // GetDocumentState() mutates the state due to lazy resolution;
> > +  // and can't be used during parallel traversal. Use this instead,
> > +  // and ensure GetDocumentState() has been called first.
> > +  // This will assert if the state is stale.
> 
> Not about this bug, since you just move code, but what asserts anything here?

Hmm, yeah, I'd have bet this used to have a `mGotDocumentState` assertion... I think I could break it though... I think keeping it asynchronously is not super-useful... I'll file a followup.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5cf2158f9740
Inline nsIDocument::ThreadSafeGetDocumentState(). r=smaug
https://hg.mozilla.org/integration/autoland/rev/d3afac5121cc
Inline the Rust bits of GetDocumentState too. r=xidorn

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5cf2158f9740
https://hg.mozilla.org/mozilla-central/rev/d3afac5121cc