590841 - Expose hardware-accelerated layers state in about:support

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Joe Drew (not getting mail)]

We should expose whether hardware-accelerated layers is in use in about:support. This patch does that, and also notes if any of your windows are not hardware-accelerated.

This patch builds on some of the things Jeff did in bug 586046.

Comment 1

[Joe Drew (not getting mail)]

Attachment: Add the ability for a DOM window to tell whether it's accelerated, and use that in about:support

I don't know why my patch didn't upload earlier.

Comment 2

[Joe Drew (not getting mail)]

Also, pretend that the bundle variables were correctly moved out of the try scope and converted into lets, rather than just copied out.

Comment 3

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

Comment on attachment 469361 [details] [diff] [review]
Add the ability for a DOM window to tell whether it's accelerated, and use that in about:support

>diff --git a/dom/interfaces/base/nsIDOMWindowUtils.idl b/dom/interfaces/base/nsIDOMWindowUtils.idl

>+   * Is false if there is no widget associated with this window.

Your implementation seems to throw in this case.

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

> function populateGraphicsSection() {

>+    let awindow = windows.getNext().QueryInterface(Ci.nsIDOMWindow);

might as well QI directly to nsIInterfaceRequestor.

>+  let body = {};

s/= {}// (this confused me)

>+  if (acceleratedWindows == 0) {
>+    body = createElement("td", bundle.GetStringFromName("no"));
>+  } else if (acceleratedWindows == totalWindows) {
>+    body = createElement("td", bundle.GetStringFromName("yes"));
>+  } else {
>+    let str = PluralForm.get(acceleratedWindows,
>+                             bundle.GetStringFromName("acceleratedLayersExist"));
>+    body = createElement("td", str.replace("#1", acceleratedWindows)
>+                                  .replace("#2", totalWindows));
>+  }

factor out the "body = createElement()" part and just assign to "msg" variable in the if/else?

>+  appendChildren(document.getElementById("graphics-tbody"), [ header, body ]);

Wouldn't hurt to keep document.getElementById("graphics-tbody") in a local variable given that it's now used twice.

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

>+acceleratedLayersEnabled = GPU Accelerated Layers Enabled

This is rather jargony - perhaps include a note that mentions that this can be localized with a generic terms like "graphics acceleration" or somesuch?

>+# LOCALIZATION NOTE (acceleratedLayersExist): Semi-colon list of plural forms.
>+# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
>+# #1 number of gpu-accelerated windows
>+# #2 total number of windows
>+# example: Yes - 3 accelerated windows, 4 total
>+acceleratedLayersExist = Yes - #1 accelerated window, #2 total;Yes - #1 accelerated windows, #2 total

I think this is overly complicated (especially if you consider that some locales might need a different plural form of "total" depending on the number). How about just:

acceleratedLayers=Yes (#1/#2)

and avoid pluralform stuff entirely?

r=me with all these addressed.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about if we returned the actual accelerated layer type as a string? E.g. nsIDOMWindowUtils::GetLayerManagerType(), returning "Basic", "GL", "D3D9" etc. Seems worth having if we add a D3D10 layer manager, or a GLES layer manager, etc.

Comment 5

[Joe Drew (not getting mail)]

Attachment: Expose current layer manager type as a string

This patch fixes Gavin's review comments, and exposes the current layer type as a string off of nsIDOMWindowUtils. I made quite a few changes, so I'd like Gavin to take another look.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  mozilla::layers::LayerManager *mgr = widget->GetLayerManager();

using namespace mozilla::layers!

+  /**
+   * Return a string representation of the specified backend name.
+   */
+  static void GetBackendName(LayersBackend aBackend, nsAString& aName);

Surely just make it a non-static method without aBackend? Get the backend type internally, or just make it a pure virtual method and override in the implementations. The latter would be best.

Comment 7

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

I thought the point of exposing LayerManagerType as a string was to include it in about:support. Otherwise returning an int type would be simpler...

Comment 8

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

Comment on attachment 470063 [details] [diff] [review]
Expose current layer manager type as a string

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

>+  if (acceleratedWindows == 0) {
>+    msg = false;
>+  } else if (acceleratedWindows == totalWindows) {
>+    msg = true;

Joe and I decided not to localize the value, and just use the ratio and layer name (e.g. "1/2 (Direct3D 9)", "2/2 (Direct3D 9)", "0/2").

Comment 9

[Joe Drew (not getting mail)]

Attachment: unbitrotted, exposes layer manager type in about:support

Comment 10

[Joe Drew (not getting mail)]

Attachment: remove localizability of the value

Gavin points out that there's no good reason to have the accelerated layers value be localizable, and it makes the patch significantly less complicated to do so too.

Comment 11

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

Comment on attachment 471607 [details] [diff] [review]
remove localizability of the value

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

>+  body = createElement("td", msg);
>+
>+  appendChildren(graphics_tbody, [ header, body ]);

get rid of "body" and just put createElement inline (move "header" declaration down here too)?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What about comment #6?

Comment 13

[Joe Drew (not getting mail)]

Attachment: address roc's earlier comments

Sorry, don't know how I forgot about those.

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/c741254fe595

Comment 15

[Nickolay_Ponomarev]

So I have to say that from a power user's perspective "GPU Accelerated Layers Enabled  0/1" is confusing (is it "1" or "0" - was my first thought). How about "Number of windows with GPU acceleration: 0/1"?

Comment 17

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

(In reply to comment #15)
> So I have to say that from a power user's perspective "GPU Accelerated Layers
> Enabled  0/1" is confusing (is it "1" or "0" - was my first thought). How about
> "Number of windows with GPU acceleration: 0/1"?

Good point - "GPU Accelerated windows"?

Comment 18

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

Attachment: fix the string [checked in]

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 472745 [details] [diff] [review]
fix the string [checked in]

Landed the follow-up string fix: http://hg.mozilla.org/mozilla-central/rev/ad48f7d7d4fd