Closed Bug 1422985 Opened 7 years ago Closed 6 years ago

Capture webrender prefs for new compat reports, and set a label

Categories

(Web Compatibility :: Tooling & Investigations, enhancement, P1)

Product:
Web Compatibility
Component:
Tooling & Investigations
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: miketaylr, Assigned: miketaylr)

References

Details

Attachments

(4 files)

Bug 1422985. Send JSON encoded preferences via the details param.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1422985. Send webrender prefs & label in webcompat reports (when enabled).
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1422985. Update decoder doctor to JSON encode details param value.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1422985. Add test to ensure details param is JSON object. a=testonly
59 bytes, text/x-review-board-request
miketaylr
: review+
Details 
We want to capture if people have webrender enabled for new compat reports.

Here's the relevant prefs:

“gfx.webrender.enabled”
“gfx.webrender.blob-images”
“image.mem.shared”
(Linux only) “layers.acceleration.force-enabled”

(probably if any of these are enabled, we should add a `type-webrender` label to the issue and dump the value of all of them in the bug description).
Summary: Capture Webrender prefs for new site issues → Capture webrender prefs for new compat reports, and set a label
(In reply to Karl Dubost :karlcow from comment #1)
> type-webrender already exists
> https://github.com/webcompat/web-bugs/labels/type-webrender

Yep, we'll add it programmatically if the prefs are enabled.
On second thought, something like `type-webrender-enabled` is probably better. Because it may or may not be relevant to the issue being reported. People triaging those issues can change it to `type-webrender` if that's actually the issue (or something similar).
Assignee: nobody → miket
Priority: -- → P1
Milan, I'm guessing it only make sense to report on the pref values of 1 - 4 if and only if 1 is true. Is that correct?

1. gfx.webrender.enabled
2. gfx.webrender.blob-images
3. image.mem.shared
4. layers.acceleration.force-enabled (Linux only)
Flags: needinfo?(milan)
(https://github.com/webcompat/webcompat.com/pull/1962 deployed on the webcompat.com side)
Yes, without the #1, there is no webrender.  #3 s #4 do have some effect, but not relevant to this bug.
Flags: needinfo?(milan)
OK, server and frontend changes are deployed on webcompat.com to support this.
(In reply to Mike Taylor [:miketaylr] (58 Regression Engineering Owner) from comment #7)
> OK, server and frontend changes are deployed on webcompat.com to support
> this.

Possibly found a bug, investigating.
As of today, gfx.webrender.all set to true means we get the full webrender behaviour as above, but without having to set those 3(4) preferences.
(In reply to Milan Sreckovic [:milan] from comment #10)
> As of today, gfx.webrender.all set to true means we get the full webrender
> behaviour as above, but without having to set those 3(4) preferences.

Cool, good to know. Will update my patch to be aware of that.
Comment on attachment 8941973 [details]
Bug 1422985. Send JSON encoded preferences via the details param.

https://reviewboard.mozilla.org/r/212166/#review218382

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:13
(Diff revision 2)
> +const PREF_IMAGE_MEM_SHARED = "image.mem.shared";
> +const PREF_LAYERS_ACCEL_ENABLED = "layers.acceleration.force-enabled";
>  const PREF_STYLO_ENABLED = "layout.css.servo.enabled";
> +const PREF_WR_ALL = "gfx.webrender.all";
> +const PREF_WR_BLOB_IMAGES = "gfx.webrender.blob-images";
> +const PREF_WR_ENABLED = "gfx.webrender.enabled";

Can you use XPCOMUtils.defineLazyPreferenceGetter() for these instead?

You can make a special "prefs" object on which you define properties, and you can make the property keys be the pref names themselves.

Then in the url creation, you can just iterate over the object and put things in without caring about which pref is which.

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:28
(Diff revision 2)
> +function getPref(pref) {
> +  return Services.prefs.getBoolPref(pref, false);
> +}

Then you won't need this helper.

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:99
(Diff revision 2)
> +    let webrenderAllEnabled =  getPref(PREF_WR_ALL);
> +
>      let params = new URLSearchParams();
>      params.append("url", `${tabData.url}`);
>      params.append("src", "desktop-reporter");
> -    if (styloEnabled) {
> +    

Nit: trailing whitespace

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:102
(Diff revision 2)
>          params.append("details", "layout.css.servo.enabled: true");
>          params.append("label", "type-stylo");

If we're messing with this anyway, can we correctly indent the block?

Does eslint run for these files or do they get ignored or something?

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:102
(Diff revision 2)
>          params.append("details", "layout.css.servo.enabled: true");
>          params.append("label", "type-stylo");

I gotta admit, I'm confused... this adds label and details strings, and then later we add more label and details strings for webrender.

Can't we just concat all the pref state into a single blob that we send as json? What's the purpose of the "label" strings, whose ordering will be somewhat arbitrary (one before, one after the `details` param) ?

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:119
(Diff revision 2)
> +          `${PREF_IMAGE_MEM_SHARED}: ${getPref(PREF_IMAGE_MEM_SHARED)}`
> +        ];
> +        if (AppConstants.platform == "linux") {
> +          prefs.push(`${PREF_LAYERS_ACCEL_ENABLED}: ${getPref(PREF_LAYERS_ACCEL_ENABLED)}`);
> +        }
> +        params.append("details", prefs.join("\n"));

Newline-separating things in the url string seems... odd. If we're going to do anything here, why not just JSON.stringify() this?
Attachment #8941973 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #13)
> Does eslint run for these files or do they get ignored or something?

(sorry about these changes, been trying out VSCode and I think my eslint isn't properly configured... will fix that)

> ::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:102
> (Diff revision 2)
> >          params.append("details", "layout.css.servo.enabled: true");
> >          params.append("label", "type-stylo");
> 
> I gotta admit, I'm confused... this adds label and details strings, and then
> later we add more label and details strings for webrender.

Yeah. For details, it doesn't matter so much -- a single list of stuff would be fine. 

But for label, the webcompat backend expects `label=a&label=b`, rather than something like `label=a,b`. It also handles details in the same way.

FWIW, we're going to stop caring about stylo very soon (Bug 1399980), but I agree it would be good to re-write this patch in such a way where we just say "tell us about these prefs".

> Can't we just concat all the pref state into a single blob that we send as
> json? What's the purpose of the "label" strings, whose ordering will be
> somewhat arbitrary (one before, one after the `details` param) ?

As mentioned above, order isn't meaningful in this case (the end result is a label is added to a GitHub issue by a webhook).

> ::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:119
> (Diff revision 2)
> > +          `${PREF_IMAGE_MEM_SHARED}: ${getPref(PREF_IMAGE_MEM_SHARED)}`
> > +        ];
> > +        if (AppConstants.platform == "linux") {
> > +          prefs.push(`${PREF_LAYERS_ACCEL_ENABLED}: ${getPref(PREF_LAYERS_ACCEL_ENABLED)}`);
> > +        }
> > +        params.append("details", prefs.join("\n"));
> 
> Newline-separating things in the url string seems... odd. If we're going to
> do anything here, why not just JSON.stringify() this?

We use the newline on the webcompat.com frontend, since we dump `details` in a textarea and it's nice for there to be line break (but I could add newlines to a stringified array before doing that...).

Let me think about what JSON.stringify might look like for the webcompat side of things.

And thanks for the review!
Thanks for the feedback so far Gijs -- I went ahead and added a more generic prefs object we can use to send a JSON blob as a single `details` param.
(I plan on adding a test here as well, but want to get your feedback/review first, so I don't have to re-write it... :))
Comment on attachment 8941973 [details]
Bug 1422985. Send JSON encoded preferences via the details param.

https://reviewboard.mozilla.org/r/212166/#review221962

I think this is fine, though I'm curious why you kept the `pref: val` bit in "custom" syntax, as it were, rather than just making that bit JSON too.

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:56
(Diff revision 3)
>    },
>  
> +  getDetails() {
> +    let details = [];
> +    for (let pref of Object.getOwnPropertyNames(prefs)) {
> +        details.push(`${pref}: ${prefs[pref]}`);

Nit: Indent is 2 spaces, looks like.

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:58
(Diff revision 3)
> +  getDetails() {
> +    let details = [];
> +    for (let pref of Object.getOwnPropertyNames(prefs)) {
> +        details.push(`${pref}: ${prefs[pref]}`);
> +    }
> +    return JSON.stringify(details);

Still an array of strings that needs separate parsing, then? You could just JSON.stringify() the object, right? :-)
Attachment #8941973 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8945948 [details]
Bug 1422985. Send webrender prefs & label in webcompat reports (when enabled).

https://reviewboard.mozilla.org/r/216012/#review221964
Attachment #8945948 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8945949 [details]
Bug 1422985. Update decoder doctor to JSON encode details param value.

https://reviewboard.mozilla.org/r/216014/#review221966
Attachment #8945949 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #20)
> Comment on attachment 8941973 [details]
> Bug 1422985. Add a method to send JSON encoded preferences via the details
> param.
> 
> https://reviewboard.mozilla.org/r/212166/#review221962
> 
> I think this is fine, though I'm curious why you kept the `pref: val` bit in
> "custom" syntax, as it were, rather than just making that bit JSON too.
> 
> ::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:56
> (Diff revision 3)
> >    },
> >  
> > +  getDetails() {
> > +    let details = [];
> > +    for (let pref of Object.getOwnPropertyNames(prefs)) {
> > +        details.push(`${pref}: ${prefs[pref]}`);
> 
> Nit: Indent is 2 spaces, looks like.

Huh, guess eslint doesn't enforce this. I've updated my editor to default 2 for JS.

> 
> ::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm:58
> (Diff revision 3)
> > +  getDetails() {
> > +    let details = [];
> > +    for (let pref of Object.getOwnPropertyNames(prefs)) {
> > +        details.push(`${pref}: ${prefs[pref]}`);
> > +    }
> > +    return JSON.stringify(details);
> 
> Still an array of strings that needs separate parsing, then? You could just
> JSON.stringify() the object, right? :-)

Erm, yeah. Guess my brain is focused on the output, rather than where that parsing bit should live. I'll move that to the webcompat frontend side. ^__^ 

Thanks for review and suggestions as always.
Comment on attachment 8946533 [details]
Bug 1422985. Add test to ensure details param is JSON object.  a=testonly

https://reviewboard.mozilla.org/r/216550/#review222216
Attachment #8946533 - Flags: review?(miket) → review+
Comment on attachment 8946533 [details]
Bug 1422985. Add test to ensure details param is JSON object.  a=testonly

https://reviewboard.mozilla.org/r/216550/#review222218
Pushed by mitaylor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6b65e874ee02
Send JSON encoded preferences via the details param. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/4dd398d383c6
Send webrender prefs & label in webcompat reports (when enabled). r=Gijs
https://hg.mozilla.org/integration/autoland/rev/a5f7ec2df352
Update decoder doctor to JSON encode details param value. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/5087f500aa3b
Add test to ensure details param is JSON object. r=miketaylr a=testonly
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: