Closed Bug 590841 Opened 10 years ago Closed 10 years ago

Expose hardware-accelerated layers state in about:support

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: joe, Assigned: joe)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

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.
I don't know why my patch didn't upload earlier.
Assignee: nobody → joe
Attachment #469361 - Flags: superreview?(roc)
Attachment #469361 - Flags: review?(gavin.sharp)
Also, pretend that the bundle variables were correctly moved out of the try scope and converted into lets, rather than just copied out.
Blocks: 590844
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.
Attachment #469361 - Flags: review?(gavin.sharp) → review+
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.
blocking2.0: --- → beta6+
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.
Attachment #469361 - Attachment is obsolete: true
Attachment #470063 - Flags: superreview?(roc)
Attachment #470063 - Flags: review?(gavin.sharp)
Attachment #469361 - Flags: superreview?(roc)
+  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.
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...
Blocks: 592406
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").
Attachment #470063 - Flags: review?(gavin.sharp)
Attachment #470063 - Attachment is obsolete: true
Attachment #471602 - Flags: superreview?(roc)
Attachment #471602 - Flags: review?(gavin.sharp)
Attachment #470063 - Flags: superreview?(roc)
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.
Attachment #471602 - Attachment is obsolete: true
Attachment #471607 - Flags: superreview?(roc)
Attachment #471607 - Flags: review?(gavin.sharp)
Attachment #471602 - Flags: superreview?(roc)
Attachment #471602 - Flags: review?(gavin.sharp)
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)?
Attachment #471607 - Flags: review?(gavin.sharp) → review+
Sorry, don't know how I forgot about those.
Attachment #471607 - Attachment is obsolete: true
Attachment #471737 - Flags: superreview?(roc)
Attachment #471607 - Flags: superreview?(roc)
Attachment #471737 - Flags: superreview?(roc) → superreview+
http://hg.mozilla.org/mozilla-central/rev/c741254fe595
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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"?
No longer blocks: 592406
Duplicate of this bug: 592406
(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 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
Attachment #472745 - Attachment description: fix the string → fix the string [checked in]
Blocks: 598339
You need to log in before you can comment on or make changes to this bug.