Closed
Bug 590841
Opened 14 years ago
Closed 14 years ago
Expose hardware-accelerated layers state in about:support
Categories
(Core :: Graphics, defect)
Core
Graphics
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)
11.23 KB,
patch
|
roc
:
superreview+
|
Details | Diff | Splinter Review |
2.17 KB,
patch
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
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•14 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.
Comment 3•14 years ago
|
||
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•14 years ago
|
blocking2.0: --- → beta6+
Assignee | ||
Comment 5•14 years ago
|
||
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.
Comment 7•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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 11•14 years ago
|
||
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•14 years ago
|
||
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: 14 years ago
Resolution: --- → FIXED
Comment 15•14 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"?
Comment 17•14 years ago
|
||
(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•14 years ago
|
||
Comment 19•14 years ago
|
||
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•14 years ago
|
Blocks: about:support++
You need to log in
before you can comment on or make changes to this bug.
Description
•