593027 - Disable HW acceleration option requires a restart

Status: RESOLVED WONTFIX

(Firefox :: Settings UI, defect)

Description

[Scoobidiver (away)]

Build : Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5) Gecko/20100101 Firefox/4.0b5

In Options, when "Use Hw acceleration when enabled" is unticked, HW acceleration is still enabled. It is disabled at the next start.

Expected results :
Solution 1 : Disable immediately HW acceleration
Solution 2 : Modify the label by adding "(need a restart)"

Comment 1

[Joe Drew (not getting mail)]

I don't know what UI would want in this situation.

Comment 2

[Scoobidiver (away)]

Follow-up of comment 0 :
Solution 3 : Solution 2 + after clicking on "OK" button of option window, display a popup window with a message that looks like : "You changed the use of HW acceleration in your settings. To take into account this modification, Firefox is going to restart and restore your current session." with "OK" and "Cancel" buttons. If clicking is OK then restart, if clicking is Cancel then hide the popup window.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Low-bar: add (restart required) to the label
High-bar: add a sheet/modal that asks to restart the browser

I don't think that this blocks Firefox 4, we need to document it, though.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Durr: high-bar is actually making is so a restart isn't required, now that I think of it. :)

Comment 5

[Bas Schouten (:bas.schouten)]

(In reply to comment #4)
> Durr: high-bar is actually making is so a restart isn't required, now that I
> think of it. :)

I actually did some work to make this possible. It shouldn't be too hard to do anymore. In order to do so the following should happen:

1. Flip the pref
2. Call gfxWindowsPlatform::UpdateRenderMode
3. Null out layer managers on all widgets

We will lose any canvas surface content. Not losing canvas surface content would be something that requires some more work.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment 5 works for me; still wouldn't block on it, but would approve the patch if it became available!

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to comment #5)
> (In reply to comment #4)
> > Durr: high-bar is actually making is so a restart isn't required, now that I
> > think of it. :)
> 
> I actually did some work to make this possible. It shouldn't be too hard to do
> anymore. In order to do so the following should happen:
> 
> 1. Flip the pref
> 2. Call gfxWindowsPlatform::UpdateRenderMode
> 3. Null out layer managers on all widgets

Unless you can make things so that GDI fonts work with HW accel, there's another issue to consider - unless DWrite was preffed on already, we have GDI font objects throughout the system, and would have to replace them with DWrite ones. This looks non-trivial.... see comments in bug 606419.

Personally, I don't think we should attempt this for FF4, at least - it's too complex and risky at this stage. Whether we consider it worth the extra complexity in the longer term is another issue; we could work towards it for FF.next if that's what people want.

Comment 9

[Scoobidiver (away)]

As solution in comment 5 can't be implemented (see comment 8), solution in comment 3 must be implemented.

Comment 10

[Joe Drew (not getting mail)]

We should just document this.

Comment 11

[Scoobidiver (away)]

> We should just document this.
See http://support.mozilla.com/kb/Options window%20-%20Advanced panel

Do I close this bug or let it opened for future releases of Firefox?

Comment 12

[Scoobidiver (away)]

Users don't go to SUMO and think that unchecking the check box is enough to turn off HW acceleration. See bug 645872 comment 6.

Comment 13

[Ed Morley [:emorley]]

I've seen several other comments in bugs along the lines of the one linked in comment 12 above.

Users try disabling hardware acceleration to diagnose issues (without restarting Fx) and then when it (obviously) doesn't make a difference, tick the box again; declaring in the bug that disabling hardware acceleration didn't help - which derails attempts to diagnose.

Surely an easier/quicker temporary solution would just be to suffix "(Restart Required)" onto the "enable hardware acceleration" pref string. 

Making Fx cope with hardware acceleration changes without restarting would be nice, but unnecessarily complex short term, given that a two word string change could avoid most of the user confusion :-)

Comment 14

[Kelley Cook]

Attachment: Adds the words "restart required" to English Hardware acceleration preference

I have no idea how locales work, but this one-liner implements Comment 12 for U.S. English

Comment 15

[Kelley Cook]

Attachment: Version without extra hunk with the non-printable

Comment 16

[Kevin Brosnan [Ex-Mozilla]]

You need to bump the string entity name so that localization knows something changed. You will need to select a reviewer to have the patch included. https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch

Comment 17

[Kelley Cook]

Attachment: Adds the words "restart required" to English Hardware acceleration preference. Bumps entity string

No write access to mozilla's hg system

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 526888 [details] [diff] [review]
Adds the words "restart required" to English Hardware acceleration preference.  Bumps entity string

Instead of "allowHWAccel2", how about "useHWAccel"? r=me with that.

This needs to get ui-review as well, though. I recommend limi or faaborg.

Comment 19

[Kelley Cook]

Attachment: patch v4 [r=gavin] (Adds the words "restart required" to English Hardware acceleration preference. change entity string name)

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 527179 [details] [diff] [review]
patch v4 [r=gavin] (Adds the words "restart required" to English Hardware acceleration preference. change entity string name)

This isn't suitable for the 2.0 branch, because it makes string changes (and isn't a critical security/stability issue). You don't need any "approval" to land it on trunk, once it gets ui-review.

Comment 21

[Asa Dotzler [:asa]]

not interested for 5.

Comment 22

[Ed Morley [:emorley]]

Limi, any luck with the ui-review? :-)

Comment 23

[Ed Morley [:emorley]]

Limi, ping for ui-review please.

Comment 24

[Justin Dolske [:Dolske]]

It's not a big deal since this is buried in Prefs -> Advanced -> General, but I'd be slightly concerned that seeing the default state of...

  [X] Use hardware acceleration when available (restart required)

...might imply that HW accel isn't active until the browser (or OS?) is restarted. In other words, it would be clearer if the "restart" text only was displayed when the pref is changed to a value other than what the browser started up in.

Comment 25

[Kelley Cook]

(In reply to comment #24)
>   [X] Use hardware acceleration when available (restart required)
> 
> ...might imply that HW accel isn't active until the browser (or OS?) is
> restarted. In other words, it would be clearer if the "restart" text only
> was displayed when the pref is changed to a value other than what the
> browser started up in.

I can't disagree with this

The real solution, IMO, is to have a flag available so that changing certain preferences would cause a "This change requires the browser to restart - Now / Later" message to appear like it does when installing many add-ons.

My quick and dirty solution was not really meant to be long term solution.

Comment 26

[Scoobidiver (away)]

(In reply to Justin Dolske [:Dolske] from comment #24)
>   [X] Use hardware acceleration when available (restart required)
In IE 9, you have the same kind of option with an asterisk that says "Takes effect after you restart Internet Explorer": http://support.microsoft.com/kb/2528233

Comment 28

[Wayne Mery (:wsmwk)]

(In reply to Justin Dolske [:Dolske] from comment #24)
> ... it would be clearer if the "restart" text only
> was displayed when the pref is changed to a value other than what the
> browser started up in.

bug 973014 is dealing with a privacy pref that provides such a capability

Comment 31

[Kartikaya Gupta (email:[e-mail])]

This bug is still biting people, see e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1251786#c5

Comment 32

[u279076]

Milan, is anyone actively working on this issue?

Comment 33

[Milan Sreckovic [:milan] (needinfo for best results)]

There is bug 1240198, which covers the points mentioned in this bug, but only for about:support, not for about:preferences.  This may not be a bad follow up.

Comment 34

[Caspy7]

Encountered yet-another-case of a user improperly troubleshooting because they had no idea (how could they) that it was necessary to restart.

Anything holding this bug back or mainly just a matter of priority?

Comment 35

[Milan Sreckovic [:milan] (needinfo for best results)]

Let me put up a patch that matches what we did in bug 973014 - force a restart, or revert the value back.  No strings changes required.

Comment 36

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Prompt the user for a restart when they change HW acceleration preference control

Note that this doesn't prompt when the value is change by setting/closing the "Use recommended performance settings" control.  Follow up patch for that.  It also doesn't prompt when the value is changed in about:config, but that's by design.

:Dolske, who are the right people to review the UX/code pieces?

Comment 37

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Prompt the user for a restart when they change HW acceleration preference control

Comment 38

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Milan Sreckovic [:milan] from comment #37)
> Created attachment 8872630 [details] [diff] [review]
> Prompt the user for a restart when they change HW acceleration preference
> control

This version prompts when the HW acceleration is enabled using the "recommended performance setting" control as well.

Comment 39

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Prompt the user for a restart when they change HW acceleration preference control

Comment 40

[Justin Dolske [:Dolske]]

(In reply to Milan Sreckovic [:milan] from comment #36)

> :Dolske, who are the right people to review the UX/code pieces?

I'd suggest Jared (:jaws) for review, although this would probably be a good one to hand off to the Taipei team that has been actively working on prefs. Tina Hsieh would be a good UX contact.

Comment 41

[Evan Tseng [:evanxd]]

Comment on attachment 8872640 [details] [diff] [review]
Prompt the user for a restart when they change HW acceleration preference control

Hi Jared,

Could you take a look of this patch?

Thank you.

Comment 42

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8872640 [details] [diff] [review]
Prompt the user for a restart when they change HW acceleration preference control

Review of attachment 8872640 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry that this patch sat waiting for needinfo/review for so long.

The patch will need to be rebased. Since it was written, the files you were changing have been moved to /browser/components/preferences/in-content-new/main.js. We also should make the same change to /browser/components/preferences/in-content/main.js.

::: browser/components/preferences/in-content/main.js
@@ +1,1 @@
> +a/* This Source Code Form is subject to the terms of the Mozilla Public

This is a syntax error (the 'a' before the comment).

@@ +660,5 @@
>        }
>      }
>    },
>  
> +   * Track if we need to prompt the user for a restart or not

This is a syntax error. It looks like the start /* is missing?

@@ +667,5 @@
> +
> +  /**
> +   * This gets called whenever layers.acceleration.disabled preference
> +   * is modified (about:preferences or about:config)
> +    */

nit, the indentation is messed up here.

@@ +684,5 @@
> +       // Revert the value...
> +       pref.value = !pref.value;
> +     }
> +   },
> + 

Please run `./mach eslint browser/components/preferences` locally to see what code might need to be cleaned up.

Comment 43

[Milan Sreckovic [:milan] (needinfo for best results)]

I'm going to let the preferences work settle a bit, too much churn in there right now, but will get back to this.

Comment 44

[Francis Lee [:frlee]]

based on comment43, remove whiteboard tag. 
we have landed almost all Fx56 related features, @milan, you may be able to start to work on it.