Create configuration and feature bits for OMTP

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dvander, Assigned: domfarolino)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.)

[1] http://searchfox.org/mozilla-central/source/gfx/thebes/gfxPrefs.h
[2] http://searchfox.org/mozilla-central/source/gfx/config/gfxFeature.h
[3] http://searchfox.org/mozilla-central/source/gfx/config/gfxVars.h
[4] http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/gfx/thebes/gfxPlatform.cpp#695
(Assignee)

Comment 1

2 years ago
Posted patch bug1369542.patch (obsolete) — Splinter Review
Attachment #8874587 - Flags: review?(dvander)
(Assignee)

Comment 2

2 years ago
Posted 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]
bug1369542.patch

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)
(Assignee)

Comment 4

2 years ago
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]
bug1369542.patch

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

Looks good!
Attachment #8875053 - Flags: review?(dvander) → review+
(Assignee)

Comment 6

2 years ago
Try submission link for bug 1369549 (https://bugzilla.mozilla.org/show_bug.cgi?id=1369549#c12) also applies to this bug, so when that try finishes (assuming no errors) this should be able to be checked in.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 7

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8adb463b2631
Hook up OMTP to config/about:support/crash annotation system. r=dvander
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8adb463b2631
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.