Expose hardware-accelerated layers state in about:support

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: Joe Drew (not getting mail))

Tracking

(Blocks: 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
Created attachment 469361 [details] [diff] [review]
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.
Assignee: nobody → joe
Attachment #469361 - Flags: superreview?(roc)
Attachment #469361 - Flags: review?(gavin.sharp)
(Assignee)

Comment 2

7 years ago
Also, pretend that the bundle variables were correctly moved out of the try scope and converted into lets, rather than just copied out.
(Assignee)

Updated

7 years ago
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.
(Assignee)

Updated

7 years ago
blocking2.0: --- → beta6+
(Assignee)

Comment 5

7 years ago
Created attachment 470063 [details] [diff] [review]
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.
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...

Updated

7 years ago
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)
(Assignee)

Comment 9

7 years ago
Created attachment 471602 [details] [diff] [review]
unbitrotted, exposes layer manager type in about:support
Attachment #470063 - Attachment is obsolete: true
Attachment #471602 - Flags: superreview?(roc)
Attachment #471602 - Flags: review?(gavin.sharp)
Attachment #470063 - Flags: superreview?(roc)
(Assignee)

Comment 10

7 years ago
Created attachment 471607 [details] [diff] [review]
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.
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+
What about comment #6?
(Assignee)

Comment 13

7 years ago
Created attachment 471737 [details] [diff] [review]
address roc's earlier comments

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
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 15

7 years ago
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"?

Updated

7 years ago
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"?
Created attachment 472745 [details] [diff] [review]
fix the string [checked in]
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]

Updated

7 years ago
Blocks: 594464

Updated

7 years ago
Blocks: 598339
You need to log in before you can comment on or make changes to this bug.