Closed Bug 1369542 Opened 3 years ago Closed 3 years ago

Create configuration and feature bits for OMTP


(Core :: Graphics: Layers, enhancement)

Not set



Tracking Status
firefox56 --- fixed


(Reporter: dvander, Assigned: domfarolino)


(Blocks 1 open bug)



(1 file, 2 obsolete files)

Bootstrapping OMTP requires two pieces: some boilerplate to create a thread and establish ownership of the thread, and hooking up the feature to the config/about:support/crash annotation systems.

This bug is for the latter part: adding the config pref, adding decision logic about whether or not to enable OMTP, and then communicating the decision to about:support and other processes. The exact steps are roughly:

1. Add a line to gfxFeature.h [2]. "OMTP" and "Off Main Thread Painting" are probably fine for a feature name/description. gfxFeature is used to track the decision logic of enabling/disabling features, and gets communicated automatically to about:support.

2. Add a line to gfxVars [3]. These are variables that are automatically communicated to other processes. We'll want something like "UseOMTP", so content processes know whether or not to create a painting thread.

3. Add some logic to gfxPlatform [4] to set up all this state. InitWebrenderConfig() is a good, albeit complicated example of what we want. The logic should go something like this:
  - Create a "ScopedGfxFeatureReporter" for OMTP.
  - If in a content process, early return.
  - Check whether a "layers.omtp.enabled" pref is set to true - with a default of false.
  - If it's false, for now just early return. We won't show anything in about:support.
  - Otherwise the pref is true. Set the gfxFeature status to DisableByDefault, and then UserEnabled. This will make it appear in about:support that the feature is not normally enabled and the user wants it on.
  - If InSafeMode() is true, set the feature to forcefully disabled.
  - At the end, if the gfxFeature status is still enabled, set the reporter to successful. (This will attach a note to crash reports, so we can correlate crashes with features.)

Attached patch bug1369542.patch (obsolete) — Splinter Review
Attachment #8874587 - Flags: review?(dvander)
Attached patch bug1369542.patch (obsolete) — Splinter Review
Nit: Remove double-space
Attachment #8874587 - Attachment is obsolete: true
Attachment #8874587 - Flags: review?(dvander)
Attachment #8874608 - Flags: review?(dvander)
Comment on attachment 8874608 [details] [diff] [review]

Review of attachment 8874608 [details] [diff] [review]:

Looks good but needs one more revision for the InSafeMode thing.

::: gfx/thebes/gfxPlatform.cpp
@@ +2401,5 @@
> +gfxPlatform::InitOMTP()
> +{
> +  bool prefEnabled = Preferences::GetBool("layers.omtp.enabled", false);
> +
> +  if (!prefEnabled) {

nit: Add a comment saying something like, "We don't want to report anything for this feature when turned off since it is still early in development."

@@ +2431,5 @@
> +    gfxVars::SetUseOMTP(true);
> +    reporter.SetSuccessful();
> +  }
> +
> +  if (InSafeMode()) {

This block should come after the UserEnable, and before the IsEnabled+SetSuccessful block, since it can turn OMTP off.

::: gfx/thebes/gfxPlatform.h
@@ +826,5 @@
>      void InitCompositorAccelerationPrefs();
>      void InitGPUProcessPrefs();
>      void InitWebRenderConfig();
> +    void InitOMTP();

nit: rename to InitOMTPConfig
Attachment #8874608 - Flags: review?(dvander)
Attached patch bug1369542.patchSplinter Review
Address both nits, move the InSafeMode check/block, and actually call the InitOMTPConfig() method.
Attachment #8874608 - Attachment is obsolete: true
Attachment #8875053 - Flags: review?(dvander)
Comment on attachment 8875053 [details] [diff] [review]

Review of attachment 8875053 [details] [diff] [review]:

Looks good!
Attachment #8875053 - Flags: review?(dvander) → review+
Try submission link for bug 1369549 ( also applies to this bug, so when that try finishes (assuming no errors) this should be able to be checked in.
Keywords: checkin-needed
Pushed by
Hook up OMTP to config/about:support/crash annotation system. r=dvander
Keywords: checkin-needed
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.