Enhancements to https://codereview.chromium.org/12494033/ for zmo@

Enhancements to https://codereview.chromium.org/12494033/ for zmo@

DO NOT SUBMIT

BUG=none

[Dan Beam, 2013-03-22 00:31:29]

zmo@: see chrome/browser/resources/options/browser_options.js

https://codereview.chromium.org/12929018/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/pref_ui.js (right):

https://codereview.chromium.org/12929018/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/pref_ui.js:185: if (checked != this.checked)
arv@: this is mildly alarming

[Dan Beam, 2013-03-22 00:32:56]

https://codereview.chromium.org/12929018/diff/1/chrome/app/chromium_strings.grd
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/12929018/diff/1/chrome/app/chromium_strings.g...
chrome/app/chromium_strings.grd:433: (requires Chromium <ph
name="BEGIN_LINK">&lt;a id="gpu-mode-reset-restart-link"
href="#"&gt;</ph>restart<ph name="END_LINK">&lt;/a&gt;</ph>)
looking the patch, perhaps rewording to "click here to restart chrom{e,ium}"
would be better as (or anything to make it more clear the user *could* lose some
potential work from restarting chrome)

https://codereview.chromium.org/12929018/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/12929018/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/browser_options.js:482: updateGpuRestartLink();
^ zmo@: you should set this on page load as well

[arv (Not doing code reviews), 2013-03-22 14:35:48]

https://codereview.chromium.org/12929018/diff/1/chrome/app/chromium_strings.grd
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/12929018/diff/1/chrome/app/chromium_strings.g...
chrome/app/chromium_strings.grd:433: (requires Chromium <ph
name="BEGIN_LINK">&lt;a id="gpu-mode-reset-restart-link"
href="#"&gt;</ph>restart<ph name="END_LINK">&lt;/a&gt;</ph>)
In general, it is bad to put HTML in here. Can you rethink your solution?

https://codereview.chromium.org/12929018/diff/1/chrome/app/chromium_strings.g...
chrome/app/chromium_strings.grd:433: (requires Chromium <ph
name="BEGIN_LINK">&lt;a id="gpu-mode-reset-restart-link"
href="#"&gt;</ph>restart<ph name="END_LINK">&lt;/a&gt;</ph>)
This should not be a link. It should be a button since opening this in a new tab
or bookmarking it makes no sense.

https://codereview.chromium.org/12929018/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/12929018/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/browser_options.html:720:
i18n-values=".innerHTML:gpuModeResetRestart">
Whenever you see yourself doing .innerHTML you should take a break and see what
other solutions you have available.

https://codereview.chromium.org/12929018/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/12929018/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/browser_options.js:477:
$('gpu-mode-checkbox').onchange = updateGpuRestartLink;
Do you really want to listen to the user changing this? You should probably just
sync this with the underlying preference state. You can do something like
Preferences.getInstance().addEventListener(...)

https://codereview.chromium.org/12929018/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/browser_options.js:479: event.preventDefault();
Change to <button> and no need for preventDefault.

https://codereview.chromium.org/12929018/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/pref_ui.js (right):

https://codereview.chromium.org/12929018/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/pref_ui.js:185: if (checked != this.checked)
On 2013/03/22 00:31:29, Dan Beam wrote:
> arv@: this is mildly alarming

Yeah, this is wrong. We already have code in place for updating the UI when a
pref changes, we should not simulate user actions.

[Dan Beam, 2013-03-22 19:24:31]

arv@: thanks for Preferences.getInstance().addEventListener(), that's better.

https://chromiumcodereview.appspot.com/12929018/diff/1/chrome/app/chromium_st...
File chrome/app/chromium_strings.grd (right):

https://chromiumcodereview.appspot.com/12929018/diff/1/chrome/app/chromium_st...
chrome/app/chromium_strings.grd:433: (requires Chromium <ph
name="BEGIN_LINK">&lt;a id="gpu-mode-reset-restart-link"
href="#"&gt;</ph>restart<ph name="END_LINK">&lt;/a&gt;</ph>)
On 2013/03/22 14:35:48, arv wrote:
> This should not be a link. It should be a button since opening this in a new
tab
> or bookmarking it makes no sense.

I agree on the link, but I don't agree on the HTML point as previous zmo@ had 3
different translations (i.e. prefix, link, suffix) which doesn't work well.

https://chromiumcodereview.appspot.com/12929018/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options/browser_options.html (right):

https://chromiumcodereview.appspot.com/12929018/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/browser_options.html:720:
i18n-values=".innerHTML:gpuModeResetRestart">
On 2013/03/22 14:35:48, arv wrote:
> Whenever you see yourself doing .innerHTML you should take a break and see
what
> other solutions you have available.

Why? Security? This is a trusted source.

https://chromiumcodereview.appspot.com/12929018/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options/browser_options.js (right):

https://chromiumcodereview.appspot.com/12929018/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/browser_options.js:477:
$('gpu-mode-checkbox').onchange = updateGpuRestartLink;
On 2013/03/22 14:35:48, arv wrote:
> Do you really want to listen to the user changing this? You should probably
just
> sync this with the underlying preference state. You can do something like
> Preferences.getInstance().addEventListener(...)

zmo@: yeah, you could do this instead

https://chromiumcodereview.appspot.com/12929018/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/browser_options.js:479: event.preventDefault();
On 2013/03/22 14:35:48, arv wrote:
> Change to <button> and no need for preventDefault.

That would be a little bit awkward where it currently is in the UI.

[Dan Beam, 2013-03-22 19:25:16]

https://chromiumcodereview.appspot.com/12929018/diff/1/chrome/app/chromium_st...
File chrome/app/chromium_strings.grd (right):

https://chromiumcodereview.appspot.com/12929018/diff/1/chrome/app/chromium_st...
chrome/app/chromium_strings.grd:433: (requires Chromium <ph
name="BEGIN_LINK">&lt;a id="gpu-mode-reset-restart-link"
href="#"&gt;</ph>restart<ph name="END_LINK">&lt;/a&gt;</ph>)
On 2013/03/22 19:24:31, Dan Beam wrote:
> On 2013/03/22 14:35:48, arv wrote:
> > This should not be a link. It should be a button since opening this in a new
> tab
> > or bookmarking it makes no sense.
> 
> I agree on the link, but I don't agree on the HTML point as previous zmo@ had
3
> different translations (i.e. prefix, link, suffix) which doesn't work well.

Is there a better way to do

  Start translation <link in middle> end translation.

?

[arv (Not doing code reviews), 2013-03-22 19:35:02]

https://chromiumcodereview.appspot.com/12929018/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options/browser_options.js (right):

https://chromiumcodereview.appspot.com/12929018/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/browser_options.js:479: event.preventDefault();
On 2013/03/22 19:24:31, Dan Beam wrote:
> On 2013/03/22 14:35:48, arv wrote:
> > Change to <button> and no need for preventDefault.
> 
> That would be a little bit awkward where it currently is in the UI.

We have a class called link-button which styles the button as a link.

[Zhenyao Mo, 2013-03-22 20:07:26]

Guys, really appreciate all the points.  I am preparing a new CL.

However, as button vs link or the text, it is not decided by me.

It is approved by the UI lead Glen Murphy and our GPU PM Tom Wiltzius.

It is a decision they made and I can't change it.

Hey, I am just an engineer. :)

[Zhenyao Mo, 2013-03-22 20:11:23]

On 2013/03/22 20:07:26, Zhenyao Mo wrote:
> Guys, really appreciate all the points.  I am preparing a new CL.
> 
> However, as button vs link or the text, it is not decided by me.
> 
> It is approved by the UI lead Glen Murphy and our GPU PM Tom Wiltzius.
> 
> It is a decision they made and I can't change it.
> 
> Hey, I am just an engineer. :)

Ah, I see your comment of styling the button as a link.  Then I can use the
button instead.  Thank you.

But still, the text has to stay as they decided.