1270404 - Show nsIGfxInfo failure messages in decision log

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Since nsIGfxInfo now returns a failure string when querying the blocklist, we can attach this string to the decision log. These strings are patterns, so aboutSupport.js can match those patterns to return better error messages. For example a hardcoded GfxInfo.cpp blocklist entry might have a bug number - but a downloadable blocklist entry will instead have a unique identifier.

Both are intended to pinpoint the exact matching rule, but the former allows us a nicer a message.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

Note that gfxConfig currently uses English messages in most places. To differentiate those from the nsIGfxInfo failure IDs, I prefixed them with a '#'.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: screenshot

This is a screenshot of the new UI, when encountering a blocklisted configuration.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8749084 [details] [diff] [review]
patch

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

Nice!

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +1911,5 @@
>    return DoesD3D11TextureSharingWorkInternal(device, DXGI_FORMAT_R8_UNORM, D3D11_BIND_SHADER_RESOURCE);
>  }
>  
>  static inline bool
> +IsGfxInfoStatusOkay(int32_t aFeature, nsCString* aOutMessage)

This is a file static method, so it is fair not to check nsCString* for null pointer, although I personally would have probably used nsCString&.  Anyway, good the way it is, just commenting on why I think it's OK not to worry about aOutMessage being null.

@@ -2464,5 @@
>    Factory::SetDirect3D11Device(nullptr);
>  }
>  
> -static bool
> -IsD2DBlacklisted()

This was just a leftover unused method?

::: toolkit/content/aboutSupport.js
@@ +440,5 @@
>            if (entry.type == "default" && entry.status == "available")
>              continue;
>  
> +          let contents;
> +          if (entry.message[0] == '#') {

There is no chance entry.message is empty?

@@ +443,5 @@
> +          let contents;
> +          if (entry.message[0] == '#') {
> +            // This is a failure ID.
> +            let m;
> +            if (m = /#BLOCKLIST_FEATURE_FAILURE_BUG_(\d+)/.exec(entry.message)) {

Is just "#BLOCKLIST_ enough here?  It seems we are assuming that gfxWindowsPlatform will get FEATURE_FAILURE_BUG_... back as the status, so that when appended to #BLOCKLIST_ we get this.

It is probably enough to put the comment in both places (here, and the place where we set #BLOCKLIST_ and FEATURE_FAILURE_BUG_) where the other places are that assume those values.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Milan Sreckovic [:milan] from comment #3)
> > -static bool
> > -IsD2DBlacklisted()
> 
> This was just a leftover unused method?

Yeah, I forgot to remove it when gfxConfig happened.

> ::: toolkit/content/aboutSupport.js
> @@ +440,5 @@
> >            if (entry.type == "default" && entry.status == "available")
> >              continue;
> >  
> > +          let contents;
> > +          if (entry.message[0] == '#') {
> 
> There is no chance entry.message is empty?

Hrm, it's not supposed to but it could be empty. But in JS that wouldn't throw an exception or anything. I'll add a length check just for sanity.

> 
> @@ +443,5 @@
> > +          let contents;
> > +          if (entry.message[0] == '#') {
> > +            // This is a failure ID.
> > +            let m;
> > +            if (m = /#BLOCKLIST_FEATURE_FAILURE_BUG_(\d+)/.exec(entry.message)) {
> 
> Is just "#BLOCKLIST_ enough here?  It seems we are assuming that
> gfxWindowsPlatform will get FEATURE_FAILURE_BUG_... back as the status, so
> that when appended to #BLOCKLIST_ we get this.
> 
> It is probably enough to put the comment in both places (here, and the place
> where we set #BLOCKLIST_ and FEATURE_FAILURE_BUG_) where the other places
> are that assume those values.

In GfxInfo.cpp most of the blocklist entries have "FEATURE_FAILURE_BUG_n" as the status code. I'm not sure what other standardized code formats Benoit had/has in mind, but it would be good to have them documented in one place - maybe in the IDL for getFeatureStatus. I'll add some notes in this patch.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ebd9fead9c2

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ebd9fead9c2