642502 - Some about:support fixes/improvements

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 1: skip fields with 0/empty/null values

This part 1 gets you rid of most of the platform-specific fields that are just 0/empty on your platform. It also makes the code shorter.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Parts 2-3: Don't report D2D/DWrite outside of windows, and extend failure reason reporting to other features

2 parts in one, sorry:

 * introduce Win-Vista-Or-Higher feature test (taken from WebGL mochitest), use it for D2D and DWrite reporting

 * factor the code reporting the reason for failure into a pushFeatureInfoRow function and lets WebGL use it too.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: part 4: also report the reason for failure for Layers

Layers are a bit special, especially as we don't always know which GfxInfo feature governs them. This patch takes the interesting part of pushFeatureInfoRow into a separate function errorMessageForFeature(), and uses it for layers features. On Windows, we use FEATURE_DIRECT3D_9_LAYERS. The assumption made here is that in most cases, this will give the same GfxInfo error message as D3D10 layers. Ideally, GfxInfo itself should generate all these (localized) error messages: I don't see another way.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 5: add 'blocked OS version' message

This one was missing. It's non-negligible, e.g. it's what Mac 10.5 users currently get for WebGL.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

(... and 10.6.2 for Layers).

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 519945 [details] [diff] [review]
Part 1: skip fields with 0/empty/null values


>+    pushInfoRow(trGraphics, "adapterDescription", gfxInfo.adapterDescription);
>+    pushInfoRow(trGraphics, "adapterVendorID", gfxInfo.adapterVendorID
>+                                               ? String('0000'+gfxInfo.adapterVendorID.toString(16)).slice(-4)
>+                                               : null);
>+    pushInfoRow(trGraphics, "adapterDeviceID", gfxInfo.adapterDeviceID
>+                                               ? String('0000'+gfxInfo.adapterDeviceID.toString(16)).slice(-4)
>+                                               : null);

What about adding a function like zeroPad() that returns null for a null input string? That will make this code more understandable too.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

You can use the XUL preprocessor (which is already enabled for this file) to only include code for Windows (#ifdef XP_WIN as you would in C++). I think that'd make the patch simpler. Also you can use Cc/Ci shorthands (defined earlier in the file).

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Parts 2-3 (updated): Don't report D2D/DWrite outside of windows, and extend failure reason reporting to other features

Updated to apply Gavin's comments.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: part 4 (updated): also report the reason for failure for Layers

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #5)
> What about adding a function like zeroPad() that returns null for a null input
> string? That will make this code more understandable too.


OK, done locally.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

You don't need the enablePrivilege calls, this is all privileged JS. I don't think you really want any of the dump() calls in the exception handlers, either - they'll just clutter stdout with mostly useless exceptions.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Parts 2-3 (updated): Don't report D2D/DWrite outside of windows, and extend failure reason reporting to other features

Updated according to Gavin's comments

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: part 4 (updated): also report the reason for failure for Layers

Applied Gavin's comments.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 526377 [details] [diff] [review]
Parts 2-3 (updated): Don't report D2D/DWrite outside of windows, and extend failure reason reporting to other features

>diff --git a/toolkit/content/aboutSupport.js b/toolkit/content/aboutSupport.js

> function populateGraphicsSection() {
>+  var kIsWindowsVistaOrHigher = false;
>+#ifdef XP_WIN
>+    var version = Cc["@mozilla.org/system-info;1"]
>+                            .getService(Ci.nsIPropertyBag2)
>+                            .getProperty("version");
>+    kIsWindowsVistaOrHigher = (parseFloat(version) >= 6.0);
>+#endif

Seems like you could put this closer to where it is used.

>+  function pushFeatureInfoRow(table, name, feature, isEnabled, messageIfEnabled) {

>+          message = bundle.GetStringFromName("tryNewerDriverVersion").replace("%1", suggestedDriverVersion);

This should be using formatStringFromName...

>+      var dwEnabled = false;
>+      var dwriteEnabledStr = dwEnabled.toString();
>+      var dwriteVersion;
>+      try {
>+        dwEnabled = gfxInfo.DWriteEnabled;
>+        dwriteVersion = gfxInfo.DWriteVersion;
>+        dwriteEnabledStr = dwEnabled.toString() + " (" + dwriteVersion + ")";
>+      } catch(e) {}

It's rather annoying that all of these gfxInfo getters can throw. Can't the fallback values be added to the implementation to make them always succeed, rather than having to use try/catches all over this code?

You don't need the explicit toString() calls on the booleans (and you don't need the separate dwriteEnabledStr variable, just always use dwriteEnabled).

It would be nice if the formatting would be consistent in this file. Opening brackets on the same line and omitting brackets around single line then-blocks is conventional.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Parts 2-3 (updated): Don't report D2D/DWrite outside of windows, and extend failure reason reporting to other features

OK, I've tried to apply your review comments. Thanks for your help. Filing follow-up about the GfxInfo API criticism: this is know to need a complete redesign anyway.

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

Filed bug 651981

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 527651 [details] [diff] [review]
Parts 2-3 (updated): Don't report D2D/DWrite outside of windows, and extend failure reason reporting to other features

>diff --git a/toolkit/content/aboutSupport.js b/toolkit/content/aboutSupport.js

>+  function pushFeatureInfoRow(table, name, feature, isEnabled, messageIfEnabled) {
>+    var message = isEnabled ? messageIfEnabled : false;
>+    if (!isEnabled) {

I think this should be:
function pushFeatureInfoRow(table, name, feature, isEnabled, message)

and then initialize message as:
message = message || isEnabled;

That way you only need to pass in message if it's value is different than "isEnabled", and omit it otherwise. That would make the callers:
pushFeatureInfoRow(trGraphics, "direct2DEnabled", gfxInfo.FEATURE_DIRECT2D, d2dEnabled);
pushFeatureInfoRow(trGraphics, "webglRenderer", webglfeature, webglenabled, webglrenderer || false);

(or just intialize webglrendered to false)

>+      var status = -1; // different from any status value defined in the IDL

You can just leave it undefined (no initialization) and omit the comment.

>+    var kIsWindowsVistaOrHigher = false;
>+#ifdef XP_WIN
>+    var version = Cc["@mozilla.org/system-info;1"]
>+                  .getService(Ci.nsIPropertyBag2)
>+                  .getProperty("version");
>+    kIsWindowsVistaOrHigher = (parseFloat(version) >= 6.0);
>+#endif
>+
>+    if (kIsWindowsVistaOrHigher) {

You could just put this entire block inside the #ifdef, and omit the kIsWindowsVistaOrHigher variable (or leave it in for clarity, doesn't matter).

>+      var dwEnabled = false;

= "false";

>+        dwVersion = gfxInfo.DWriteVersion;
>+        dwEnabled = gfxInfo.DWriteEnabled + " (" + dwVersion + ")";

dwEnabled = gfxInfo.DWriteEnabled + " (" + gfxInfo.DWriteVersion + ")";

>     var webglrenderer;
>+    var webglenabled;
>     try {
>       webglrenderer = gfxInfo.getWebGLParameter("full-renderer");
>+      webglenabled = true;
>     } catch (e) {
>       webglrenderer = "(WebGL unavailable)";
>+      webglenabled = false;

Could just initialize to false.

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: updated patch

Thanks a lot Gavin for your review comments; this new version rolls together the 3 remaining patches, as they were touching the same lines of code.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 528321 [details] [diff] [review]
updated patch

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

::: toolkit/content/aboutSupport.js
@@ +175,5 @@
       ]));
     }
   }
 
+  function errorMessageForFeature(feature) {

How about a switch rather than if/else if/etc.?

@@ +276,5 @@
+    var isWebGLonANGLEavailable = gfxInfo.getFeatureStatus(gfxInfo.FEATURE_WEBGL_ANGLE) == gfxInfo.FEATURE_NO_INFO;
+    var webglfeature = isWebGLonANGLEavailable ? gfxInfo.FEATURE_WEBGL_ANGLE : gfxInfo.FEATURE_WEBGL_OPENGL;
+#else
+    var webglfeature = gfxInfo.FEATURE_WEBGL_OPENGL;
+#endif

Seems like this would be clearer as:

var webglfeature = gfxInfo.FEATURE_WEBGL_OPENGL;
#ifdef XP_WIN
if (gfxInfo.getFeatureStatus(gfxInfo.FEATURE_WEBGL_ANGLE) == gfxInfo.FEATURE_NO_INFO)
  webglfeature = gfxInfo.FEATURE_WEBGL_ANGLE;
#endif

@@ +316,5 @@
+#ifdef XP_WIN
+      feature = gfxInfo.FEATURE_DIRECT3D_9_LAYERS;
+#else
+      feature = gfxInfo.FEATURE_OPENGL_LAYERS;
+#endif

Just put the "var" inside both branches of the #ifdef.

@@ +317,5 @@
+      feature = gfxInfo.FEATURE_DIRECT3D_9_LAYERS;
+#else
+      feature = gfxInfo.FEATURE_OPENGL_LAYERS;
+#endif
+    if (!feature) {

This doesn't look right - this will always test true, since both constants are non-zero. I think you want to test the return value of getFeatureStatus for the given feature?

::: toolkit/locales/en-US/chrome/global/aboutSupport.properties
@@ +15,5 @@
 blockedGraphicsCard = Blocked on your graphics card because of unresolved driver issues.
 
+# LOCALIZATION NOTE The verb "blocked" here refers to a graphics feature such as "Direct2D" or "OpenGL layers".
+blockedOperatingSystemVersion = Blocked on your operating system version.
+

nitpick: "Blocked _for_ your driver/OS" seems like better phrasing to me

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: updated again

Thanks a lot, here's a new version applying your comments.

Regarding the WebGL feature ID: the logic was actually a bit too simplistic, see comment in updated patch.

Regarding the Layers if (!feature): thanks a lot, that was an actual bug, somehow I didn't notice it during my testing.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 528409 [details] [diff] [review]
updated again

>diff --git a/toolkit/content/aboutSupport.js b/toolkit/content/aboutSupport.js

>+  function errorMessageForFeature(feature) {

>+      case gfxInfo.FEATURE_BLOCKED_OS_VERSION:
>+        message = bundle.GetStringFromName("blockedOperatingSystemVersion");

This wants to be "errorMessage", I think.

>+          errorMessage = bundle.GetStringFromName("blockedDriverVersion") + " " +
>+                         bundle.formatStringFromName("tryNewerDriverVersion", [suggestedDriverVersion], 1);

This kind of string munging isn't kosher from a localization point of view. I suppose that might not matter much for this page in particular, but since you're already using formatStringFromName, you might as well do it properly.

>diff --git a/toolkit/locales/en-US/chrome/global/aboutSupport.properties b/toolkit/locales/en-US/chrome/global/aboutSupport.properties

>-tryNewerDriverVersion = Blocked on your graphics driver. Try updating your graphics driver to version %1 or newer.
>+tryNewerDriverVersion = Try updating your graphics driver to version %S or newer.

When you change existing strings, you need to change the string name, so that localizers notice the change (it's not a great system but it's what we have).

Comment 21

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: patch version 394

Thanks again, message->errorMessage was indeed a grave bug, also you're right about operating on localized strings, and I have versioned the localized string names.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 528578 [details] [diff] [review]
patch version 394

>diff --git a/toolkit/content/aboutSupport.js b/toolkit/content/aboutSupport.js

>+  function errorMessageForFeature(feature) {

>+      case gfxInfo.FEATURE_BLOCKED_DRIVER_VERSION:
>+        var suggestedDriverVersion = null;

nit: omit the "= null"

>+      default:

No need for this.

>+    var isWindowsVistaOrHigher = false;
>+#ifdef XP_WIN
...
>+#endif

>+    if (isWindowsVistaOrHigher) {

As I mentioned earlier, you could put this entire block inside the ifdef.

>+      var dwEnabled = false;

"false"

>+#ifdef XP_WIN
>+    // If ANGLE is not available but OpenGL is, we want to report on the OpenGL feature, because that's what's going to get used.
>+    // In all other cases we want to report on the ANGLE feature.
>+    var webglfeature = gfxInfo.FEATURE_WEBGL_ANGLE;
>+    if (gfxInfo.getFeatureStatus(gfxInfo.FEATURE_WEBGL_ANGLE)  != gfxInfo.FEATURE_NO_INFO &&
>+        gfxInfo.getFeatureStatus(gfxInfo.FEATURE_WEBGL_OPENGL) == gfxInfo.FEATURE_NO_INFO)
>+      webglfeature = gfxInfo.FEATURE_WEBGL_OPENGL;
>+#else
>+    var webglfeature = gfxInfo.FEATURE_WEBGL_OPENGL;
>+#endif

A slightly more general comment: it seems fragile to have this logic here, because it needs to be manually kept in sync with the back-end logic that actually decides what gets used when. Having about:support do nothing but just unconditionally call GfxInfo APIs and print their return values directly seems like the ideal state that we want to strive for. Maybe that should be the overarching goal for bug 651981?

>   if (acceleratedWindows)
>     msg += " " + mgrType;
>+  else {

nit: brace both blocks if one is braced

>diff --git a/toolkit/locales/en-US/chrome/global/aboutSupport.properties b/toolkit/locales/en-US/chrome/global/aboutSupport.properties

>+# LOCALIZATION NOTE The %S here is a placeholder, leave unchanged, it will get replaced by the driver version string.
>+tryNewerDriverVersion_v2 = Blocked for your graphics driver version. Try updating your graphics driver to version %S or newer.

The _v2 sufixes are confusing, generally we just try to rename arbitrarily. You can use just e.g. "tryNewerDriver" in this case.

>-blockedGraphicsCard = Blocked on your graphics card because of unresolved driver issues.
>+blockedGraphicsCard_v2 = Blocked for your graphics card because of unresolved driver issues.

blockedGfxCard

>+blockedOperatingSystemVersion_v2 = Blocked for your operating system version.
>+blockedDriverVersion_v2 = Blocked for your graphics driver version.

blockedOSVersion / blockedDriverVersion (no need for the v2 here since these are new strings)

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 528578 [details] [diff] [review]
patch version 394

(I also can't get this to apply, even on top of the older one.)

Comment 24

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: patch version 852

(In reply to comment #22)
> >+#ifdef XP_WIN
> >+    // If ANGLE is not available but OpenGL is, we want to report on the OpenGL feature, because that's what's going to get used.
> >+    // In all other cases we want to report on the ANGLE feature.
> >+    var webglfeature = gfxInfo.FEATURE_WEBGL_ANGLE;
> >+    if (gfxInfo.getFeatureStatus(gfxInfo.FEATURE_WEBGL_ANGLE)  != gfxInfo.FEATURE_NO_INFO &&
> >+        gfxInfo.getFeatureStatus(gfxInfo.FEATURE_WEBGL_OPENGL) == gfxInfo.FEATURE_NO_INFO)
> >+      webglfeature = gfxInfo.FEATURE_WEBGL_OPENGL;
> >+#else
> >+    var webglfeature = gfxInfo.FEATURE_WEBGL_OPENGL;
> >+#endif
> 
> A slightly more general comment: it seems fragile to have this logic here,
> because it needs to be manually kept in sync with the back-end logic that
> actually decides what gets used when. Having about:support do nothing but just
> unconditionally call GfxInfo APIs and print their return values directly seems
> like the ideal state that we want to strive for. Maybe that should be the
> overarching goal for bug 651981?
> 

Yes, this is exactly what I wanted to write in bug 651981 and forgot about. Thanks a lot. Updated that bug.

Applied all your other comments. Hopefully this time you will knight this patch with your blessed r+.

Comment 25

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: part 1 updated

carrying forward r+

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 529765 [details] [diff] [review]
patch version 852

I haven't reviewed the correctness of the platform ifdefs or FEATURE_WEBGL_* feature selection logic, but I assume that isn't necessary. r=me on the rest.

Comment 27

[Benoit Jacob [:bjacob] (mostly away)]

Actually it is quite necessary because the return value of getFeatureStatus for features that a unapplicable to the given OS is not clearly defined, and we have different implementations on different OSes that may behave differently. Another thing to add to bug 651981.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

Well, I assume it wasn't necessary *for me to do*. Someone who knows that logic (jeff? joe?) should presumably review that part.

Comment 29

[Joe Drew (not getting mail)]

Comment on attachment 529772 [details] [diff] [review]
part 1 updated

Looks fine!

Comment 30

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/mozilla-central/rev/14351a8521c3
http://hg.mozilla.org/mozilla-central/rev/ae3c9370cc57

Comment 31

[Benoit Jacob [:bjacob] (mostly away)]

Backed out due to crashes on Linux64 opt. But it was green on tryserver.
http://hg.mozilla.org/mozilla-central/rev/418b0b9985c6

Comment 32

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/mozilla-central/rev/80c0a924a5b3
http://hg.mozilla.org/mozilla-central/rev/bd95b5dbd8c2