Turn the layer prefs into a tri-state

RESOLVED FIXED in mozilla2.0b10

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

Trunk
mozilla2.0b10
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

7 years ago
direct2d is a good model to follow here.
(Assignee)

Comment 1

7 years ago
direct2d uses:
- gfx.direct2d.disabled
- gfx.direct2d.force-enabled

layers has:
- layers.accelerate-all
- layers.accelerate-none

The reason for both layer prefs is to support using acceleration in fullscreen windows while not using it for everything else. I think we probably want this on 10.5 and perhaps in other places.

I suggest we change the default value of layers.accelerate-all to false
and perhaps add a pref for layers.accelerate-fullscreen which also defaults to false. Setting layers.accelerate-all to true forces layer acceleration on, setting layers.accelerate-fullscreen forces fullscreen acceleration, and setting accelerate-none disables all acceleration.

accelerate-none should obviously override everything and I think accelerate-fullscreen should override accelerate-all.

Thoughts?
why don't we set "layers.accelerate", having options "auto" (auto) "none" "always" "fullscreen"?  They're string options, but that's not that big of a deal.  Then we can also have "layers.accelerate-type" taking "auto" (default) "d3d9" "d3d10" "gl"..
We had arbitrarily-valued prefs before, and it was a big mess. Strings are slightly better in that they're at least readable, but booleans are easier still.

Benoit's suggestion was basically Jeff's, only renaming the prefs:

layers.acceleration.disabled
layers.acceleration.force-enabled
layers.acceleration.fullscreen

We should enable by default on all platforms but Linux and modulo the blocklist. Disabled means "Nope." Force-enabled means "Ignore blocklist and platform restrictions." Fullscreen means as it does in Jeff's message above.
Dunno, the biggest argument against multi-valued prefs that I heard was that it's hard to remember what 0 1 2 etc are.  Multiple prefs leads into the kind of weird interaction mess that we have now.  But I guess these are hidden things anyway, so I'm just bikeshedding..
Yeah. I don't think it's too important that our hidden prefs be consistent and impossible to put into an "impossible" state.
(Assignee)

Comment 6

7 years ago
Created attachment 501799 [details] [diff] [review]
Here's what this could look like
(Assignee)

Comment 7

7 years ago
Created attachment 501978 [details] [diff] [review]
fix up a couple of things
Attachment #501799 - Attachment is obsolete: true
Attachment #501978 - Flags: review?
(Assignee)

Comment 8

7 years ago
Created attachment 502066 [details] [diff] [review]
a little better
Attachment #501978 - Attachment is obsolete: true
Attachment #501978 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #502066 - Flags: review?(joe)
Comment on attachment 502066 [details] [diff] [review]
a little better

I think we should do the following renaming of the prefs:

layers.accelerate. -> layers.acceleration.

These two are bikesheddy:

force-enabled -> force-enable
disabled -> disable


>diff --git a/gfx/layers/d3d9/LayerManagerD3D9.cpp b/gfx/layers/d3d9/LayerManagerD3D9.cpp

> PRBool
> LayerManagerD3D9::Initialize()
> {
>+

Extra whitespace.

>+  nsCOMPtr<nsIPrefBranch2> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
>+
>+  PRBool forceAccelerate = PR_FALSE;
>+  if (prefs) {
>+    // we should use AddBoolPrefVarCache
>+    prefs->GetBoolPref("layers.accelerate.force-enabled",
>+        &forceAccelerate);

Please align & with " above.

>+  }
>+
>+

Extra whitespace.

>diff --git a/gfx/layers/opengl/LayerManagerOGL.cpp b/gfx/layers/opengl/LayerManagerOGL.cpp

> LayerManagerOGL::Initialize(GLContext *aExistingContext)

>+    PRBool forceAccelerate = PR_FALSE;
>+    if (prefs) {
>+      // we should use AddBoolPrefVarCache
>+      prefs->GetBoolPref("layers.accelerate.force-enabled",
>+          &forceAccelerate);

Align & with " above.

>diff --git a/widget/src/xpwidgets/nsBaseWidget.cpp b/widget/src/xpwidgets/nsBaseWidget.cpp

>   if (prefs) {
>-    prefs->GetBoolPref("layers.accelerate-all",
>-                       &accelerateByDefault);
>-    prefs->GetBoolPref("layers.accelerate-none",
>+    // we should use AddBoolPrefVarCache
>+    prefs->GetBoolPref("layers.accelerate.disabled",
>                        &disableAcceleration);
>   }
> 
>   const char *acceleratedEnv = PR_GetEnv("MOZ_ACCELERATED");
>   accelerateByDefault = accelerateByDefault || 
>                         (acceleratedEnv && (*acceleratedEnv != '0'));
> 
>   nsCOMPtr<nsIXULRuntime> xr = do_GetService("@mozilla.org/xre/runtime;1");
>   PRBool safeMode = PR_FALSE;
>   if (xr)
>     xr->GetInSafeMode(&safeMode);
> 
>   if (disableAcceleration || safeMode)
>     return PR_FALSE;
> 
>-  if (accelerateByDefault)
>-    return PR_TRUE;
>+#if defined(XP_WIN) || defined(XP_MACOSX)
>+  return PR_TRUE;
>+#endif
> 
>+  /* use the window acceleration flag */

Why do we even have accelerateByDefault, then? This patch removes the ability for MOZ_ACCELERATED to control acceleration. (And check with equivalent code in windows/nsWindow.cpp.)

>   return mUseAcceleratedRendering;
> }
Attachment #502066 - Flags: review?(joe) → review-
(Assignee)

Comment 10

7 years ago
Created attachment 502115 [details] [diff] [review]
fix up comments
Attachment #502115 - Flags: review?(joe)
(Assignee)

Updated

7 years ago
Attachment #502066 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #502115 - Flags: review?(ehsan)
(Assignee)

Comment 11

7 years ago
Created attachment 502119 [details] [diff] [review]
actually fix up comments
Assignee: nobody → jmuizelaar
Attachment #502115 - Attachment is obsolete: true
Attachment #502119 - Flags: review?
Attachment #502115 - Flags: review?(joe)
Attachment #502115 - Flags: review?(ehsan)
(Assignee)

Updated

7 years ago
Attachment #502119 - Flags: review?(joe)
Attachment #502119 - Flags: review?(ehsan)
Attachment #502119 - Flags: review?
Comment on attachment 502119 [details] [diff] [review]
actually fix up comments

>diff --git a/browser/components/preferences/advanced.xul b/browser/components/preferences/advanced.xul

>       <preference id="general.smoothScroll"            name="general.smoothScroll"            type="bool"/>
>-      <preference id="layers.accelerate-none"          name="layers.accelerate-none"          type="bool"   inverted="true"
>+      <preference id="layers.acceleration.disabled"          name="layers.acceleration.disabled"          type="bool"   inverted="true"
>                   onchange="gAdvancedPane.updateHardwareAcceleration()"/>
>       <preference id="layout.spellcheckDefault"        name="layout.spellcheckDefault"        type="int"/>

Please to fix up the indentation of name and type here.

>diff --git a/gfx/layers/d3d9/LayerManagerD3D9.cpp b/gfx/layers/d3d9/LayerManagerD3D9.cpp

> PRBool
> LayerManagerD3D9::Initialize()
> {
>+  nsCOMPtr<nsIPrefBranch2> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
>+
>+  PRBool forceAccelerate = PR_FALSE;
>+  if (prefs) {
>+    // we should use AddBoolPrefVarCache
>+    prefs->GetBoolPref("layers.acceleration.force-enabled",
>+                       &forceAccelerate);
>+  }
>+
>+

Extra line of whitespace.

>diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js

> // Default value of acceleration for all widgets.

"Whether to disable acceleration for all widgets."

>+pref("layers.acceleration.disabled", false);

> // Whether to allow acceleration on layers at all.

"Whether to force acceleration on, ignoring blacklists."

>+pref("layers.acceleration.force-enabled", false);
Attachment #502119 - Flags: review?(joe) → review+
(Assignee)

Comment 13

7 years ago
Created attachment 502125 [details] [diff] [review]
more addressing
Attachment #502125 - Flags: review?(ehsan)
(Assignee)

Updated

7 years ago
Attachment #502119 - Attachment is obsolete: true
Attachment #502119 - Flags: review?(ehsan)
FWIW, this patch doesn't handle pref migration, so it has the potential of breaking Firefox for some users who have already disabled hw acceleration through our prefs UI because it was broken for them.  I would say that this needs some nightly coverage, and should not land for beta9.
Attachment #502125 - Flags: review?(ehsan)
Blocks: 467530
Version: unspecified → Trunk
This has landed on m-c and push URL isn't above/patches needing review, intentional?

http://hg.mozilla.org/mozilla-central/rev/58d9f36e3a49
Comment on attachment 502125 [details] [diff] [review]
more addressing

This was reviewed in person.
Attachment #502125 - Flags: review+
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Blocks: 625318
Is there any reason why it has been named layers.acceleration.disabled? It's really the first preference now which breaks our default naming scheme in using .enabled. :/ Also having a double negative meaning, is kinda hard to understand.
No longer blocks: 467530
Target Milestone: --- → mozilla2.0b10
You need to log in before you can comment on or make changes to this bug.