Closed Bug 1364862 Opened 7 years ago Closed 7 years ago

stylo: Make PostRebuildAllStyleDataEvent do the style flush async.

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(1 file)

Bug 1364862: Make PostRebuildAllStyleData async.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
Bug 1364824 implemented it, but the flush there is done synchronously.

We should make it async.
Comment on attachment 8867766 [details]
Bug 1364862: Make PostRebuildAllStyleData async.

https://reviewboard.mozilla.org/r/139306/#review143332

Sorry, I got a bit confused here, and I think better naming would help.  Hopefully you can get something coherent out of these comments. :-)

::: layout/style/ServoStyleSet.h:432
(Diff revision 3)
> +    /**
> +     * The stylist is fully dirty, and we need a stylesheet flush + device
> +     * reset.
> +     */
> +    FullyDirty,
> +    /** The stylist may be dirty, and we may need a stylesheet flush. */

In these comments, can you mention what kinds of changes cause these kinds of dirtiness?  It's not obvious from the description of the work that will happen in response.

Also I don't know why the second one is "maybe".  Don't we know for sure that we will flush sheets?  Or is it that it may or may not be necessary, but we will do it anyway?

In any case, I think we should name these enum values after what specific work needs to be done or what data is out of date.  The important difference between FullyDirty and MaybeDirty seems to be that the former will, apart from the style sheet flush, additionally cause any cached style data to be thrown away (because it depends on something external, which has changed).

So how about we name the first one "AllStyleDataDirty" and the second one "StyleSheetsDirty".  Or happy to entertain other suggestions.

::: layout/style/ServoStyleSet.h:446
(Diff revision 3)
> +  };
> +
> +  /**
> +   * Marks the stylist as maybe dirty due to stylesheet changes.
> +   */
> +  void SetStylistMayNeedRebuild()

Then this can be "SetStylistStyleSheetsDirty" (or maybe just "SetStyleSheetsDirty"?).

::: layout/style/ServoStyleSet.h:454
(Diff revision 3)
> +      mStylistState = StylistState::MaybeDirty;
> +    }
> +  }
> +
> +  /**
>     * Rebuild the stylist.  This should only be called if mStylistMayNeedRebuild

This comment needs updating.

::: layout/style/ServoStyleSet.h:463
(Diff revision 3)
>  
>    /**
>     * Helper for correctly calling RebuildStylist without paying the cost of an
>     * extra function call in the common no-rebuild-needed case.
>     */
>    void MaybeRebuildStylist()

Can we either:

1. rename "MaybeRebuildStylist" to "RebuildStylist" and "RebuildStylist" to "DoRebuildStylist", or
2. rename "MaybeRebuildStlist" to "RebuildStylistIfDirty"

::: layout/style/ServoStyleSet.cpp:268
(Diff revision 3)
>    mPresContext->Document()->GetDocumentState();
>  
>    // Ensure that the @font-face data is not stale
>    mPresContext->Document()->GetUserFontSet();
> +
> +  MaybeRebuildStylist();

(This needs to move here so that @font-face rules are up to date when rebuilding the stylist, yes?)

::: layout/style/ServoStyleSet.cpp:965
(Diff revision 3)
>  void
>  ServoStyleSet::RebuildData()
>  {
>    ClearNonInheritingStyleContexts();
>    Servo_StyleSet_RebuildData(mRawSet.get());
> +  mStylistState = StylistState::NotDirty;
> +}

When do we want to do this, i.e. clear the non-inheriting style contexts (since they depend on some external data that changed), but not clear the Device on the Servo side, which similarly holds style data?

::: layout/style/ServoStyleSet.cpp:1066
(Diff revision 3)
>  void
>  ServoStyleSet::RebuildStylist()
>  {
> -  MOZ_ASSERT(mStylistMayNeedRebuild);
> +  MOZ_ASSERT(mStylistState != StylistState::NotDirty);
> +  if (mStylistState == StylistState::FullyDirty) {
> +    RebuildData();
> +  } else {
> -  Servo_StyleSet_FlushStyleSheets(mRawSet.get());
> +    Servo_StyleSet_FlushStyleSheets(mRawSet.get());
> -  mStylistMayNeedRebuild = false;
> -}
> +  }
> +  mStylistState = StylistState::NotDirty;
> +}
>  

Here I'm confused again. :-)  I think it's I can't tell what the difference is between "RebuildData" and "RebuildStylist" from the name.

WDYT about calling this one "UpdateStylist" instead?
Comment on attachment 8867766 [details]
Bug 1364862: Make PostRebuildAllStyleData async.

https://reviewboard.mozilla.org/r/139306/#review143360

Thanks, I think this looks better!  r=me with these addressed as you see fit.

::: layout/style/ServoStyleSet.h:292
(Diff revision 4)
>    /**
> +   * Clears the style data, and marks the stylist as needing an unconditional
> +   * full rebuild, including a device reset.
> +   */

Here the "style data" is referring to the style contexts stored here on the ServoStyleSet, yeah?  Mention that explicitly.

::: layout/style/ServoStyleSet.h:437
(Diff revision 4)
> +    /**
> +     * All style data is dirty and both stylesheet data and default computed
> +     * values need to be recomputed.
> +     */

BTW I think we want to use this for cases other than default computed values changing, e.g. for theme color changes.  Might be worth mentioning that here too.

::: layout/style/ServoStyleSet.h:444
(Diff revision 4)
> +  /**
> +   * Marks the stylist as maybe dirty due to stylesheet changes.
> +   */

s/maybe dirty/needing a style sheet flush/ ?
Attachment #8867766 - Flags: review?(cam) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ee2658ed3cfb
Make PostRebuildAllStyleData async. r=heycam
https://hg.mozilla.org/mozilla-central/rev/ee2658ed3cfb
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: