Some about:support fixes/improvements

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 11 obsolete attachments)

3.50 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
9.33 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
3.42 KB, patch
bjacob
: review+
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
Created attachment 519945 [details] [diff] [review]
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.
Attachment #519945 - Flags: review?(jmuizelaar)
(Assignee)

Comment 1

6 years ago
Created attachment 519949 [details] [diff] [review]
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.
Attachment #519949 - Flags: review?(jmuizelaar)
(Assignee)

Comment 2

6 years ago
Created attachment 519951 [details] [diff] [review]
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.
Attachment #519951 - Flags: review?(jmuizelaar)
(Assignee)

Comment 3

6 years ago
Created attachment 519952 [details] [diff] [review]
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.
Attachment #519952 - Flags: review?(jmuizelaar)
(Assignee)

Comment 4

6 years ago
(... and 10.6.2 for Layers).
(Assignee)

Updated

6 years ago
Depends on: 639842

Updated

6 years ago
Attachment #519945 - Attachment is patch: true
Attachment #519945 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #519945 - Flags: review?(jmuizelaar) → review+
(Assignee)

Updated

6 years ago
Attachment #519949 - Flags: review?(jmuizelaar) → review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Attachment #519951 - Flags: review?(jmuizelaar) → review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Attachment #519952 - Flags: review?(jmuizelaar) → review?(gavin.sharp)
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).
(Assignee)

Comment 7

6 years ago
Created attachment 523038 [details] [diff] [review]
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.
Attachment #519949 - Attachment is obsolete: true
Attachment #519949 - Flags: review?(gavin.sharp)
Attachment #523038 - Flags: review?(gavin.sharp)
(Assignee)

Comment 8

6 years ago
Created attachment 523039 [details] [diff] [review]
part 4 (updated): also report the reason for failure for Layers
Attachment #519951 - Attachment is obsolete: true
Attachment #519951 - Flags: review?(gavin.sharp)
Attachment #523039 - Flags: review?(gavin.sharp)
(Assignee)

Comment 9

6 years ago
(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.
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.
Created 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

Updated according to Gavin's comments
Attachment #523038 - Attachment is obsolete: true
Attachment #523038 - Flags: review?(gavin.sharp)
Attachment #526377 - Flags: review?(gavin.sharp)
Created attachment 526386 [details] [diff] [review]
part 4 (updated): also report the reason for failure for Layers

Applied Gavin's comments.
Attachment #523039 - Attachment is obsolete: true
Attachment #523039 - Flags: review?(gavin.sharp)
Attachment #526386 - Flags: review?(gavin.sharp)
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.
Attachment #526377 - Flags: review?(gavin.sharp) → review-
Created 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

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.
Attachment #526377 - Attachment is obsolete: true
Attachment #527651 - Flags: review?(gavin.sharp)
Filed bug 651981
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.
Created attachment 528321 [details] [diff] [review]
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.
Attachment #519952 - Attachment is obsolete: true
Attachment #526386 - Attachment is obsolete: true
Attachment #527651 - Attachment is obsolete: true
Attachment #519952 - Flags: review?(gavin.sharp)
Attachment #526386 - Flags: review?(gavin.sharp)
Attachment #527651 - Flags: review?(gavin.sharp)
Attachment #528321 - Flags: review?(gavin.sharp)
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
Attachment #528321 - Flags: review?(gavin.sharp) → review-
Created attachment 528409 [details] [diff] [review]
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.
Attachment #528321 - Attachment is obsolete: true
Attachment #528409 - Flags: review?(gavin.sharp)
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).
Attachment #528409 - Flags: review?(gavin.sharp) → review-
Created attachment 528578 [details] [diff] [review]
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.
Attachment #528578 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Attachment #528409 - Attachment is obsolete: true
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 on attachment 528578 [details] [diff] [review]
patch version 394

(I also can't get this to apply, even on top of the older one.)
Attachment #528578 - Flags: review?(gavin.sharp) → review-
Created attachment 529765 [details] [diff] [review]
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+.
Attachment #528578 - Attachment is obsolete: true
Attachment #529765 - Flags: review?(gavin.sharp)
Created attachment 529772 [details] [diff] [review]
part 1 updated

carrying forward r+
Attachment #529772 - Flags: review+
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.
Attachment #529765 - Flags: review?(gavin.sharp) → review+
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.
Well, I assume it wasn't necessary *for me to do*. Someone who knows that logic (jeff? joe?) should presumably review that part.
Comment on attachment 529772 [details] [diff] [review]
part 1 updated

Looks fine!
Attachment #529772 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/14351a8521c3
http://hg.mozilla.org/mozilla-central/rev/ae3c9370cc57
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Backed out due to crashes on Linux64 opt. But it was green on tryserver.
http://hg.mozilla.org/mozilla-central/rev/418b0b9985c6
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
http://hg.mozilla.org/mozilla-central/rev/80c0a924a5b3
http://hg.mozilla.org/mozilla-central/rev/bd95b5dbd8c2
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Assignee: nobody → bjacob
You need to log in before you can comment on or make changes to this bug.