Inline nsDocument::ThreadSafeGetDocumentState.

RESOLVED FIXED in Firefox 59

Status

()

Core
DOM: Core & HTML
P2
normal
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

unspecified
mozilla59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

MozReview Requests

()

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

Attachments

(2 attachments)

(Assignee)

Description

2 months ago
It's a totally unnecessary function call, that shows up on stylo-chrome profiles.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

2 months ago
mozreview-review
Comment on attachment 8934021 [details]
Bug 1422633: Inline the Rust bits of GetDocumentState too.

https://reviewboard.mozilla.org/r/204948/#review210388
Attachment #8934021 - Flags: review?(xidorn+moz) → review+

Comment 4

2 months ago
mozreview-review
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
Attachment #8934020 - Flags: review?(bugs) → review+
(Assignee)

Comment 5

2 months ago
(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.
Priority: -- → P2

Comment 6

2 months ago
Pushed by ecoal95@gmail.com:
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

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5cf2158f9740
https://hg.mozilla.org/mozilla-central/rev/d3afac5121cc
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(Assignee)

Updated

a month ago
Depends on: 1424816
You need to log in before you can comment on or make changes to this bug.