623446 - Turn the layer prefs into a tri-state

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

direct2d is a good model to follow here.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 2

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

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"..

Comment 3

[Joe Drew (not getting mail)]

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.

Comment 4

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

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..

Comment 5

[Joe Drew (not getting mail)]

Yeah. I don't think it's too important that our hidden prefs be consistent and impossible to put into an "impossible" state.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Attachment: Here's what this could look like

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Attachment: fix up a couple of things

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Attachment: a little better

Comment 9

[Joe Drew (not getting mail)]

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;
> }

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Attachment: fix up comments

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Attachment: actually fix up comments

Comment 12

[Joe Drew (not getting mail)]

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);

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Attachment: more addressing

Comment 14

[(no longer active)]

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.

Comment 15

[Ed Morley [:emorley]]

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 16

[Joe Drew (not getting mail)]

Comment on attachment 502125 [details] [diff] [review]
more addressing

This was reviewed in person.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.