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

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

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.

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.

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

(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 5 works for me; still wouldn't block on it, but would approve the patch if it became available!

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

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

We should just document this.

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

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.

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

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

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

No write access to mozilla's hg system

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

not interested for 5.

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

Limi, ping for ui-review please.

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.

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

(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

(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

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

Milan, is anyone actively working on this issue?

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.

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?

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.

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?

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

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

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

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.