Provide a link to restart chrome when changing gpu settings

Provide a link to restart chrome when changing gpu settings

BUG=173130
TEST=about:settings
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189940

[Zhenyao Mo, 2013-03-18 22:52:16]

estade: please review.

This UI change is approved by Glen Murphy.

[Zhenyao Mo, 2013-03-19 19:13:39]

https://codereview.chromium.org/12494033/diff/16001/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/12494033/diff/16001/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.html:710: <if expr="not
pp_ifdef('chromeos')">
This option also should be excluded in android, but I don't see an example of
android specific rule in this file.  Does that mean what we do here won't affect
android?

[Zhenyao Mo, 2013-03-20 17:21:48]

Also add dbeam and jhawkins.

Whoever has time please review this.  Thanks.

[Dan Beam, 2013-03-20 22:17:55]

https://codereview.chromium.org/12494033/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/12494033/diff/28001/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.html:719: <span
id="hardwareAccelerationModeChange">
IDs should use dash-form in new code, hardware-acceleration-mode-change (etc.)

https://codereview.chromium.org/12494033/diff/28001/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.html:729:
i18n-content="hardwareAccelerationModeChangeRestartPostfix">
can you think of smaller names?  also, shouldn't this be suffix?

https://codereview.chromium.org/12494033/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/12494033/diff/28001/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:472:
$('hardwareAccelerationModeChange').style.visibility = 'hidden';
this still takes up space, is that what you want?

https://codereview.chromium.org/12494033/diff/28001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12494033/diff/28001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:375:
IDS_OPTIONS_SYSTEM_ENABLE_HARDWARE_ACCELERATION_MODE_RESTART_POSTFIX },
shouldn't these be in the

  #if !defined(OS_CHROMEOS)

section?

https://codereview.chromium.org/12494033/diff/28001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:615:
base::Unretained(this)));
^ shouldn't this be defined only in

  #if !defined(OS_CHROMEOS)

as well?

[Zhenyao Mo, 2013-03-20 23:21:49]

revised. please have another look.

https://codereview.chromium.org/12494033/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/12494033/diff/28001/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.html:719: <span
id="hardwareAccelerationModeChange">
On 2013/03/20 22:17:55, Dan Beam wrote:
> IDs should use dash-form in new code, hardware-acceleration-mode-change (etc.)

Done.

https://codereview.chromium.org/12494033/diff/28001/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.html:729:
i18n-content="hardwareAccelerationModeChangeRestartPostfix">
On 2013/03/20 22:17:55, Dan Beam wrote:
> can you think of smaller names?  also, shouldn't this be suffix?

Done.

https://codereview.chromium.org/12494033/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/12494033/diff/28001/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:472:
$('hardwareAccelerationModeChange').style.visibility = 'hidden';
On 2013/03/20 22:17:55, Dan Beam wrote:
> this still takes up space, is that what you want?

I think this one is OK.

https://codereview.chromium.org/12494033/diff/28001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12494033/diff/28001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:375:
IDS_OPTIONS_SYSTEM_ENABLE_HARDWARE_ACCELERATION_MODE_RESTART_POSTFIX },
On 2013/03/20 22:17:55, Dan Beam wrote:
> shouldn't these be in the
> 
>   #if !defined(OS_CHROMEOS)
> 
> section?

Done.

https://codereview.chromium.org/12494033/diff/28001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:615:
base::Unretained(this)));
On 2013/03/20 22:17:55, Dan Beam wrote:
> ^ shouldn't this be defined only in
> 
>   #if !defined(OS_CHROMEOS)
> 
> as well?

Done.

[Dan Beam, 2013-03-21 02:03:52]

https://codereview.chromium.org/12494033/diff/36002/chrome/app/chromium_strin...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/12494033/diff/36002/chrome/app/chromium_strin...
chrome/app/chromium_strings.grd:433: ( requires Chromium
splitting this up is more code and not good for translation, AFAIK, why can't
this just be 1 translation like this?

  ( requires &lt;a href="#"&gt;<ph name="LINK_TEXT"><ex>restart</ex></ph>&lt;/a&gt; )

https://codereview.chromium.org/12494033/diff/36002/chrome/app/google_chrome_...
File chrome/app/google_chrome_strings.grd (right):

https://codereview.chromium.org/12494033/diff/36002/chrome/app/google_chrome_...
chrome/app/google_chrome_strings.grd:4: Chrome supports. -->
are we supposed to edit this file?  I thought it was overwritten by
translations, maybe?

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.html:719: <span
id="gpu-mode-reset-restart">
<span id="gpu-mode-reset-restart"
    i18n-values=".innerHTML:gpuModeResetRestart">

when there's 1 translation

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:472:
$('gpu-mode-reset-restart').style.visibility = 'hidden';
you should change this to .hidden = true;

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:473:
$('gpu-mode-checkbox').onclick = function(event) {
it seems the code as it is now is slightly suboptimal as if you double click
this it'll still show "restart now".  something like:

$('gpu-mode-checkbox').onchange = function(event) {
  $('gpu-mode-reset-restart').hidden =
      event.currentTarget.checked == theCurrentValue;
};

might make it so that the "restart" link is only showing when you actually need
to restart...

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:474:
$('gpu-mode-reset-restart').style.visibility = 'visible';
and .hidden = false; to avoid invisible text causing wrapping

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:476:
$('gpu-mode-reset-restart-link').onclick = function(event) {
event.preventDefault();

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:379: #if
!defined(OS_CHROMEOS)
^ combine with this block

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:590:
base::Unretained(this)));
^ combine with this block

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:614: #if
!defined(OS_CHROMEOS)
^ or use an

#else

[Zhenyao Mo, 2013-03-21 21:52:53]

Thanks a lot for the detailed points for improvement.  As this is not my
comfortable zone, so really appreciate it.

Please have another look.

https://codereview.chromium.org/12494033/diff/36002/chrome/app/chromium_strin...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/12494033/diff/36002/chrome/app/chromium_strin...
chrome/app/chromium_strings.grd:433: ( requires Chromium
On 2013/03/21 02:03:52, Dan Beam wrote:
> splitting this up is more code and not good for translation, AFAIK, why can't
> this just be 1 translation like this?
> 
>
&nbsp;&nbsp;(&nbsp;requires&nbsp;&lt;a&nbsp;href="#"&gt;<ph&nbsp;name="LINK_TEXT"><ex>restart</ex></ph>&lt;/a&gt;&nbsp;)

Done.

https://codereview.chromium.org/12494033/diff/36002/chrome/app/google_chrome_...
File chrome/app/google_chrome_strings.grd (right):

https://codereview.chromium.org/12494033/diff/36002/chrome/app/google_chrome_...
chrome/app/google_chrome_strings.grd:4: Chrome supports. -->
On 2013/03/21 02:03:52, Dan Beam wrote:
> are we supposed to edit this file?  I thought it was overwritten by
> translations, maybe?

I assume if we are not supposed to edit this file, there should be some warning.
 Seeing none, I'd rather make the change here.

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.html:719: <span
id="gpu-mode-reset-restart">
On 2013/03/21 02:03:52, Dan Beam wrote:
> <span id="gpu-mode-reset-restart"
>     i18n-values=".innerHTML:gpuModeResetRestart">
> 
> when there's 1 translation

Done.

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:472:
$('gpu-mode-reset-restart').style.visibility = 'hidden';
On 2013/03/21 02:03:52, Dan Beam wrote:
> you should change this to .hidden = true;

Done.

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:473:
$('gpu-mode-checkbox').onclick = function(event) {
On 2013/03/21 02:03:52, Dan Beam wrote:
> it seems the code as it is now is slightly suboptimal as if you double click
> this it'll still show "restart now".  something like:
> 
> $('gpu-mode-checkbox').onchange = function(event) {
>   $('gpu-mode-reset-restart').hidden =
>       event.currentTarget.checked == theCurrentValue;
> };
> 
> might make it so that the "restart" link is only showing when you actually
need
> to restart...

This is more complicated.  Because a user can modify the setting then navigate
to another page and come back, or open a second settings page.  So we need to
query the current state from chrome instead of replying on a local variable. 
I'd like to do that in a separate CL.

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:474:
$('gpu-mode-reset-restart').style.visibility = 'visible';
On 2013/03/21 02:03:52, Dan Beam wrote:
> and .hidden = false; to avoid invisible text causing wrapping

Done.

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:476:
$('gpu-mode-reset-restart-link').onclick = function(event) {
On 2013/03/21 02:03:52, Dan Beam wrote:
> event.preventDefault();

Done.

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:379: #if
!defined(OS_CHROMEOS)
On 2013/03/21 02:03:52, Dan Beam wrote:
> ^ combine with this block

Done.

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:590:
base::Unretained(this)));
On 2013/03/21 02:03:52, Dan Beam wrote:
> ^ combine with this block

Done.

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:614: #if
!defined(OS_CHROMEOS)
On 2013/03/21 02:03:52, Dan Beam wrote:
> ^ or use an
> 
> #else

Done.

[Dan Beam, 2013-03-21 22:19:27]

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/12494033/diff/36002/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:473:
$('gpu-mode-checkbox').onclick = function(event) {
On 2013/03/21 21:52:53, Zhenyao Mo wrote:
> On 2013/03/21 02:03:52, Dan Beam wrote:
> > it seems the code as it is now is slightly suboptimal as if you double click
> > this it'll still show "restart now".  something like:
> > 
> > $('gpu-mode-checkbox').onchange = function(event) {
> >   $('gpu-mode-reset-restart').hidden =
> >       event.currentTarget.checked == theCurrentValue;
> > };
> > 
> > might make it so that the "restart" link is only showing when you actually
> need
> > to restart...
> 
> This is more complicated.  Because a user can modify the setting then navigate
> to another page and come back, or open a second settings page.  So we need to
> query the current state from chrome instead of replying on a local variable. 
> I'd like to do that in a separate CL.

pref notifications should change the checkbox already if you check/uncheck from
a different place, right?

[Zhenyao Mo, 2013-03-21 22:30:49]

On 2013/03/21 22:19:27, Dan Beam wrote:
>
https://codereview.chromium.org/12494033/diff/36002/chrome/browser/resources/...
> File chrome/browser/resources/options/browser_options.js (right):
> 
>
https://codereview.chromium.org/12494033/diff/36002/chrome/browser/resources/...
> chrome/browser/resources/options/browser_options.js:473:
> $('gpu-mode-checkbox').onclick = function(event) {
> On 2013/03/21 21:52:53, Zhenyao Mo wrote:
> > On 2013/03/21 02:03:52, Dan Beam wrote:
> > > it seems the code as it is now is slightly suboptimal as if you double
click
> > > this it'll still show "restart now".  something like:
> > > 
> > > $('gpu-mode-checkbox').onchange = function(event) {
> > >   $('gpu-mode-reset-restart').hidden =
> > >       event.currentTarget.checked == theCurrentValue;
> > > };
> > > 
> > > might make it so that the "restart" link is only showing when you actually
> > need
> > > to restart...
> > 
> > This is more complicated.  Because a user can modify the setting then
navigate
> > to another page and come back, or open a second settings page.  So we need
to
> > query the current state from chrome instead of replying on a local variable.

> > I'd like to do that in a separate CL.
> 
> pref notifications should change the checkbox already if you check/uncheck
from
> a different place, right?

Yes, but whether to restart chrome, you need to compare with the state when
chrome start, not the state when we open the settings page, because the latter
could be the contaminated value.  So we need to pass that information from
chrome browser.

[Dan Beam, 2013-03-21 22:42:44]

https://codereview.chromium.org/12494033/diff/47001/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/12494033/diff/47001/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:474:
$('gpu-mode-reset-restart').hidden = false;
$('gpu-mode-reset-restart').hidden =
    loadTimeData.getBoolean('gpuModeAtStart') ==
    event.currentTarget.checked;

https://codereview.chromium.org/12494033/diff/47001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12494033/diff/47001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:494: #endif
values->SetBoolean("gpuModeAtStart", gpu_thinger::GetStartState());

[Dan Beam, 2013-03-21 22:43:33]

https://codereview.chromium.org/12494033/diff/47001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12494033/diff/47001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:494: #endif
er, I mean

#if !defined(OS_CHROME)
  values->SetBoolean("gpuModeAtStart", gpu_thinger::GetStartState());
#endif

of course ;)

[Dan Beam, 2013-03-21 22:43:50]

https://codereview.chromium.org/12494033/diff/47001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12494033/diff/47001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:494: #endif
On 2013/03/21 22:43:33, Dan Beam wrote:
> er, I mean
> 
> #if !defined(OS_CHROME)
>   values->SetBoolean("gpuModeAtStart", gpu_thinger::GetStartState());
> #endif
> 
> of course ;)
> 

OS_CHROMEOS**

[Zhenyao Mo, 2013-03-21 23:58:15]

Added the logic.  Please have another look.

[Zhenyao Mo, 2013-03-22 00:04:48]

On 2013/03/21 23:58:15, Zhenyao Mo wrote:
> Added the logic.  Please have another look.

Oops, forgot two newly changed files.

[Dan Beam, 2013-03-22 00:34:40]

check out https://codereview.chromium.org/12929018/

https://codereview.chromium.org/12494033/diff/57002/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/12494033/diff/57002/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:472:
$('gpu-mode-reset-restart').hidden = true;
^ update this when you open the page, IMO

https://codereview.chromium.org/12494033/diff/57002/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:473:
$('gpu-mode-checkbox').onclick = function(event) {
s/onclick/onchange/, unfortunately this isn't working when changed via pref
update... lame

https://codereview.chromium.org/12494033/diff/57002/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:475:
loadTimeData.getBoolean('gpuModeAtStart') ==
gpuEnabledAtStart might be a better name (i was just giving you an example)

[Dan Beam, 2013-03-22 22:44:04]

lgtm, though there's a little bit of extra space in front of the now
.link-button

https://codereview.chromium.org/12494033/diff/83004/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/12494033/diff/83004/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:45: * @type {boolean}
thx

https://codereview.chromium.org/12494033/diff/83004/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:1404: 
it's not a big deal, but I'd revert this

https://codereview.chromium.org/12494033/diff/93001/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/12494033/diff/93001/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:476: }
};

https://codereview.chromium.org/12494033/diff/93001/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:478:
'hardware_acceleration_mode.enabled',
can you use

  $('gpu-mode-checkbox').getAttribute('pref'),

instead of

  'hardware_acceleration_mode.enabled',

[Zhenyao Mo, 2013-03-22 23:13:00]

https://codereview.chromium.org/12494033/diff/93001/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/12494033/diff/93001/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:476: }
On 2013/03/22 22:44:04, Dan Beam wrote:
> };

Done.

https://codereview.chromium.org/12494033/diff/93001/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:478:
'hardware_acceleration_mode.enabled',
On 2013/03/22 22:44:04, Dan Beam wrote:
> can you use
> 
>   $('gpu-mode-checkbox').getAttribute('pref'),
> 
> instead of
> 
>   'hardware_acceleration_mode.enabled',

Done.

[Zhenyao Mo, 2013-03-22 23:13:31]

Committed patchset #8 manually as r189940 (presubmit successful).