[MD settings] adding isNewTabPage setting radio buttons

[MD settings] adding isNewTabPage setting radio buttons

This CL adds UI to set a preference on whether to use a NewTabPage
for the homepage or whether to use a custom url for the
homepage.

This CL had a bit of extra challenge because the pref is a bool and the UI control are radio buttons.  This is solved by using a local string property to store the radio button state.

BUG=531786

[dschuyler, 2015-09-15 22:30:38]

dschuyler@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Dan Beam, 2015-09-15 23:44:43]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2015-09-15 23:44:44]

t/f is not better than true/false, IMO

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.html:30:
hidden="[[!prefs.browser.show_home_button.value]]"
can we use <template is="dom-if" if="prefs.browser.show_home_button.value">
instead?

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.html:31:
selected="{{isNewTabPageString}}">
can this be {{prefs.browser.show_home_button}} and depend on stringification?

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.html:32:
<paper-radio-button name="t" i18n-content="homePageNtp">
why can't we just bind this to a boolean property like we did with numbers?

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:35: notify:
true,
observer: 'isNewTabPageChanged_',

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:62: },
^ make these private

[Dan Beam, 2015-09-15 23:44:52]

dbeam@chromium.org changed reviewers:
- dbeam@chromium.org

[stevenjb, 2015-09-15 23:55:06]

I *think* we can wrap paper-radio-group as <cr-settings-radio-group
pref="prefs.foo.bar"> and handle String, Boolean, and Number prefs there.

We will need to wrap paper-radio-group regardless so that we can show the
settings indicators.

Give me tomorrow to take a stab at that.

[dschuyler, 2015-09-16 00:14:28]

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.html:30:
hidden="[[!prefs.browser.show_home_button.value]]"
On 2015/09/15 23:44:44, Dan Beam wrote:
> can we use <template is="dom-if" if="prefs.browser.show_home_button.value">
> instead?

hidden should be better, right?  Dom-if is documented as being slow.  I can find
a link about the topic if that will help.  The dom-if template is useful iff the
contents are heavy to initially create.  This holds a radio group, some buttons
and an input.  That doesn't feel heavy, but 'heavy' wasn't explicitly defined in
the docs.

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.html:31:
selected="{{isNewTabPageString}}">
On 2015/09/15 23:44:44, Dan Beam wrote:
> can this be {{prefs.browser.show_home_button}} and depend on stringification?

That may work one way (reading), but the writing would then want to write a
string I believe.

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.html:32:
<paper-radio-button name="t" i18n-content="homePageNtp">
On 2015/09/15 23:44:44, Dan Beam wrote:
> why can't we just bind this to a boolean property like we did with numbers?

I haven't seen that work with bools.  We can set the value to true, but setting
it back to false changes the type.

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:35: notify:
true,
On 2015/09/15 23:44:44, Dan Beam wrote:
> observer: 'isNewTabPageChanged_',

I added String in there.

Done.

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:62: },
On 2015/09/15 23:44:44, Dan Beam wrote:
> ^ make these private

Done.

[Dan Beam, 2015-09-16 00:23:19]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2015-09-16 00:23:19]

> t/f is not better than true/false

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.html:30:
hidden="[[!prefs.browser.show_home_button.value]]"
On 2015/09/16 00:14:28, dschuyler wrote:
> On 2015/09/15 23:44:44, Dan Beam wrote:
> > can we use <template is="dom-if" if="prefs.browser.show_home_button.value">
> > instead?
> 
> hidden should be better, right?

probably not

>  Dom-if is documented as being slow.

I haven't found this to be particularly true in practice

>  I can find
> a link about the topic if that will help.

no, i know what you're talking about

>  The dom-if template is useful iff the
> contents are heavy to initially create.

it probably is

>  This holds a radio group, some buttons
> and an input.  That doesn't feel heavy, but 'heavy' wasn't explicitly defined
in
> the docs.

element.querySelector('* /deep/ *') => how many nodes are in the sub-dom?

https://codereview.chromium.org/1345913002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/1345913002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:57:
this.isNewTabPageString = newValue ? 't' : 'f';
if you change to 'true'/'false' (which is the same as true.toString() and
false.toString()), do you need this method?

[dschuyler, 2015-09-16 01:37:30]

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.html:30:
hidden="[[!prefs.browser.show_home_button.value]]"
On 2015/09/16 00:23:19, Dan Beam wrote:
> On 2015/09/16 00:14:28, dschuyler wrote:
> > On 2015/09/15 23:44:44, Dan Beam wrote:
> > > can we use <template is="dom-if"
if="prefs.browser.show_home_button.value">
> > > instead?
> > 
> > hidden should be better, right?
> 
> probably not
> 
> >  Dom-if is documented as being slow.
> 
> I haven't found this to be particularly true in practice
> 
> >  I can find
> > a link about the topic if that will help.
> 
> no, i know what you're talking about
> 
> >  The dom-if template is useful iff the
> > contents are heavy to initially create.
> 
> it probably is
> 
> >  This holds a radio group, some buttons
> > and an input.  That doesn't feel heavy, but 'heavy' wasn't explicitly
defined
> in
> > the docs.
> 
> element.querySelector('* /deep/ *') => how many nodes are in the sub-dom?

querySelector says 1.  querySelectorAll says 27.

I tried it with the dom-if because I didn't feel that it's big deal to use 
hidden or dom-if, but when I did change to dom-if, the paper radio buttons lost
their
labels.  I didn't go too far into that since I know that hidden works well.
I don't want to get sidetracked on debugging dom-if vs. hidden unless
you feel that it's a priority thing to do.
My first guess about why the labels are missing is that it is something 
related to prefs values coming later in the initial load.

https://codereview.chromium.org/1345913002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/1345913002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:57:
this.isNewTabPageString = newValue ? 't' : 'f';
On 2015/09/16 00:23:19, Dan Beam wrote:
> if you change to 'true'/'false' (which is the same as true.toString() and
> false.toString()), do you need this method?

It kinda works and kinda doesn't.  The part that doesn't work is the initial
value.  The initial value won't be set on load, but after the user clicks either
of the radio buttons, then they work fine.  This may be worth tinkering with
further, unless Steven is really going to replace it all with something that 
handles string/bool/int in a radio group (then I'd feel silly putting a lot of
time into making
something that only handles bools).

[Dan Beam, 2015-09-16 01:56:34]

27 nodes is a lot

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):

https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.html:30:
hidden="[[!prefs.browser.show_home_button.value]]"
On 2015/09/16 01:37:30, dschuyler wrote:
> On 2015/09/16 00:23:19, Dan Beam wrote:
> > On 2015/09/16 00:14:28, dschuyler wrote:
> > > On 2015/09/15 23:44:44, Dan Beam wrote:
> > > > can we use <template is="dom-if"
> if="prefs.browser.show_home_button.value">
> > > > instead?
> > > 
> > > hidden should be better, right?
> > 
> > probably not
> > 
> > >  Dom-if is documented as being slow.
> > 
> > I haven't found this to be particularly true in practice
> > 
> > >  I can find
> > > a link about the topic if that will help.
> > 
> > no, i know what you're talking about
> > 
> > >  The dom-if template is useful iff the
> > > contents are heavy to initially create.
> > 
> > it probably is
> > 
> > >  This holds a radio group, some buttons
> > > and an input.  That doesn't feel heavy, but 'heavy' wasn't explicitly
> defined
> > in
> > > the docs.
> > 
> > element.querySelector('* /deep/ *') => how many nodes are in the sub-dom?
> 
> querySelector says 1.  querySelectorAll says 27.
> 
> I tried it with the dom-if because I didn't feel that it's big deal to use 
> hidden or dom-if, but when I did change to dom-if, the paper radio buttons
lost
> their
> labels.  I didn't go too far into that since I know that hidden works well.
> I don't want to get sidetracked on debugging dom-if vs. hidden unless
> you feel that it's a priority thing to do.
> My first guess about why the labels are missing is that it is something 
> related to prefs values coming later in the initial load.

fix for that is:

  <... i18n-content="homePageNtp"></...>

needs to change to:

  <...>[[i18n_.homePageNtp]]</...>

and you need to make something like:

  properties: {
    i18n_: {
      type: Object,
      value: function() {
        return {
          homePageNtp: loadTimeData.getString('homePageNtp'),
        };
      },
    },
  },

(just value: {homepageNtp: loadTimeData.getString(...)} might also work)

[dschuyler, 2015-09-16 17:06:26]

On 2015/09/16 01:56:34, Dan Beam wrote:
> 27 nodes is a lot
> 
>
https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
> File chrome/browser/resources/settings/appearance_page/appearance_page.html
> (right):
> 
>
https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
> chrome/browser/resources/settings/appearance_page/appearance_page.html:30:
> hidden="[[!prefs.browser.show_home_button.value]]"
> On 2015/09/16 01:37:30, dschuyler wrote:
> > On 2015/09/16 00:23:19, Dan Beam wrote:
> > > On 2015/09/16 00:14:28, dschuyler wrote:
> > > > On 2015/09/15 23:44:44, Dan Beam wrote:
> > > > > can we use <template is="dom-if"
> > if="prefs.browser.show_home_button.value">
> > > > > instead?
> > > > 
> > > > hidden should be better, right?
> > > 
> > > probably not
> > > 
> > > >  Dom-if is documented as being slow.
> > > 
> > > I haven't found this to be particularly true in practice
> > > 
> > > >  I can find
> > > > a link about the topic if that will help.
> > > 
> > > no, i know what you're talking about
> > > 
> > > >  The dom-if template is useful iff the
> > > > contents are heavy to initially create.
> > > 
> > > it probably is
> > > 
> > > >  This holds a radio group, some buttons
> > > > and an input.  That doesn't feel heavy, but 'heavy' wasn't explicitly
> > defined
> > > in
> > > > the docs.
> > > 
> > > element.querySelector('* /deep/ *') => how many nodes are in the sub-dom?
> > 
> > querySelector says 1.  querySelectorAll says 27.
> > 
> > I tried it with the dom-if because I didn't feel that it's big deal to use 
> > hidden or dom-if, but when I did change to dom-if, the paper radio buttons
> lost
> > their
> > labels.  I didn't go too far into that since I know that hidden works well.
> > I don't want to get sidetracked on debugging dom-if vs. hidden unless
> > you feel that it's a priority thing to do.
> > My first guess about why the labels are missing is that it is something 
> > related to prefs values coming later in the initial load.
> 
> fix for that is:
> 
>   <... i18n-content="homePageNtp"></...>
> 
> needs to change to:
> 
>   <...>[[i18n_.homePageNtp]]</...>
> 
> and you need to make something like:
> 
>   properties: {
>     i18n_: {
>       type: Object,
>       value: function() {
>         return {
>           homePageNtp: loadTimeData.getString('homePageNtp'),
>         };
>       },
>     },
>   },
> 
> (just value: {homepageNtp: loadTimeData.getString(...)} might also work)

Since this is a optimization and it's unrelated to the actual change in this CL,
can this be moved to a bug or todo?
I think this is an interesting topic.  It's just a separate thing from this CL
and I'm trying to 'stay focused' :)

[Dan Beam, 2015-09-16 17:38:16]

On 2015/09/16 17:06:26, dschuyler wrote:
> On 2015/09/16 01:56:34, Dan Beam wrote:
> > 27 nodes is a lot
> > 
> >
>
https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
> > File chrome/browser/resources/settings/appearance_page/appearance_page.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
> > chrome/browser/resources/settings/appearance_page/appearance_page.html:30:
> > hidden="[[!prefs.browser.show_home_button.value]]"
> > On 2015/09/16 01:37:30, dschuyler wrote:
> > > On 2015/09/16 00:23:19, Dan Beam wrote:
> > > > On 2015/09/16 00:14:28, dschuyler wrote:
> > > > > On 2015/09/15 23:44:44, Dan Beam wrote:
> > > > > > can we use <template is="dom-if"
> > > if="prefs.browser.show_home_button.value">
> > > > > > instead?
> > > > > 
> > > > > hidden should be better, right?
> > > > 
> > > > probably not
> > > > 
> > > > >  Dom-if is documented as being slow.
> > > > 
> > > > I haven't found this to be particularly true in practice
> > > > 
> > > > >  I can find
> > > > > a link about the topic if that will help.
> > > > 
> > > > no, i know what you're talking about
> > > > 
> > > > >  The dom-if template is useful iff the
> > > > > contents are heavy to initially create.
> > > > 
> > > > it probably is
> > > > 
> > > > >  This holds a radio group, some buttons
> > > > > and an input.  That doesn't feel heavy, but 'heavy' wasn't explicitly
> > > defined
> > > > in
> > > > > the docs.
> > > > 
> > > > element.querySelector('* /deep/ *') => how many nodes are in the
sub-dom?
> > > 
> > > querySelector says 1.  querySelectorAll says 27.
> > > 
> > > I tried it with the dom-if because I didn't feel that it's big deal to use

> > > hidden or dom-if, but when I did change to dom-if, the paper radio buttons
> > lost
> > > their
> > > labels.  I didn't go too far into that since I know that hidden works
well.
> > > I don't want to get sidetracked on debugging dom-if vs. hidden unless
> > > you feel that it's a priority thing to do.
> > > My first guess about why the labels are missing is that it is something 
> > > related to prefs values coming later in the initial load.
> > 
> > fix for that is:
> > 
> >   <... i18n-content="homePageNtp"></...>
> > 
> > needs to change to:
> > 
> >   <...>[[i18n_.homePageNtp]]</...>
> > 
> > and you need to make something like:
> > 
> >   properties: {
> >     i18n_: {
> >       type: Object,
> >       value: function() {
> >         return {
> >           homePageNtp: loadTimeData.getString('homePageNtp'),
> >         };
> >       },
> >     },
> >   },
> > 
> > (just value: {homepageNtp: loadTimeData.getString(...)} might also work)

^ is there something unclear about my instructions?

> 
> Since this is a optimization and it's unrelated to the actual change in this
CL,

you're adding UI; it's related that it doesn't slow down the rest of the page
[more].

> can this be moved to a bug or todo?
> I think this is an interesting topic.  It's just a separate thing from this CL
> and I'm trying to 'stay focused' :)

there's nobody currently focusing on performance.  it's better that everybody
care about things like security, performance, accessibility, and testing as
trying to fix all these things after the code is written is harder.

alternatively, you can create a CL based on this one and upload it if you'd
like.

[dschuyler, 2015-09-16 20:12:21]

On 2015/09/16 17:38:16, Dan Beam wrote:
> On 2015/09/16 17:06:26, dschuyler wrote:
> > On 2015/09/16 01:56:34, Dan Beam wrote:
> > > 27 nodes is a lot
> > > 
> > >
> >
>
https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
> > > File
chrome/browser/resources/settings/appearance_page/appearance_page.html
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1345913002/diff/1/chrome/browser/resources/se...
> > > chrome/browser/resources/settings/appearance_page/appearance_page.html:30:
> > > hidden="[[!prefs.browser.show_home_button.value]]"
> > > On 2015/09/16 01:37:30, dschuyler wrote:
> > > > On 2015/09/16 00:23:19, Dan Beam wrote:
> > > > > On 2015/09/16 00:14:28, dschuyler wrote:
> > > > > > On 2015/09/15 23:44:44, Dan Beam wrote:
> > > > > > > can we use <template is="dom-if"
> > > > if="prefs.browser.show_home_button.value">
> > > > > > > instead?
> > > > > > 
> > > > > > hidden should be better, right?
> > > > > 
> > > > > probably not
> > > > > 
> > > > > >  Dom-if is documented as being slow.
> > > > > 
> > > > > I haven't found this to be particularly true in practice
> > > > > 
> > > > > >  I can find
> > > > > > a link about the topic if that will help.
> > > > > 
> > > > > no, i know what you're talking about
> > > > > 
> > > > > >  The dom-if template is useful iff the
> > > > > > contents are heavy to initially create.
> > > > > 
> > > > > it probably is
> > > > > 
> > > > > >  This holds a radio group, some buttons
> > > > > > and an input.  That doesn't feel heavy, but 'heavy' wasn't
explicitly
> > > > defined
> > > > > in
> > > > > > the docs.
> > > > > 
> > > > > element.querySelector('* /deep/ *') => how many nodes are in the
> sub-dom?
> > > > 
> > > > querySelector says 1.  querySelectorAll says 27.
> > > > 
> > > > I tried it with the dom-if because I didn't feel that it's big deal to
use
> 
> > > > hidden or dom-if, but when I did change to dom-if, the paper radio
buttons
> > > lost
> > > > their
> > > > labels.  I didn't go too far into that since I know that hidden works
> well.
> > > > I don't want to get sidetracked on debugging dom-if vs. hidden unless
> > > > you feel that it's a priority thing to do.
> > > > My first guess about why the labels are missing is that it is something 
> > > > related to prefs values coming later in the initial load.
> > > 
> > > fix for that is:
> > > 
> > >   <... i18n-content="homePageNtp"></...>
> > > 
> > > needs to change to:
> > > 
> > >   <...>[[i18n_.homePageNtp]]</...>
> > > 
> > > and you need to make something like:
> > > 
> > >   properties: {
> > >     i18n_: {
> > >       type: Object,
> > >       value: function() {
> > >         return {
> > >           homePageNtp: loadTimeData.getString('homePageNtp'),
> > >         };
> > >       },
> > >     },
> > >   },
> > > 
> > > (just value: {homepageNtp: loadTimeData.getString(...)} might also work)
> 
> ^ is there something unclear about my instructions?
> 
> > 
> > Since this is a optimization and it's unrelated to the actual change in this
> CL,
> 
> you're adding UI; it's related that it doesn't slow down the rest of the page
> [more].
> 
> > can this be moved to a bug or todo?
> > I think this is an interesting topic.  It's just a separate thing from this
CL
> > and I'm trying to 'stay focused' :)
> 
> there's nobody currently focusing on performance.  it's better that everybody
> care about things like security, performance, accessibility, and testing as
> trying to fix all these things after the code is written is harder.
> 
> alternatively, you can create a CL based on this one and upload it if you'd
> like.

Thanks!  I've uploaded CL 1345253002 to do the dom-if.

[Dan Beam, 2015-09-16 21:51:22]

>> t/f is not better than true/false

(for the third time)

[stevenjb, 2015-09-16 21:55:39]

So, FWIW, I think that the change in https://codereview.chromium.org/1347193003/
is both simpler and doesn't requre repeating this solution if we come across any
other places where we map a boolean pref to a radio group.

[dschuyler, 2015-09-16 22:14:27]

On 2015/09/16 21:51:22, Dan Beam wrote:
> >> t/f is not better than true/false
> 
> (for the third time)

I tried to give an answer in #10.
This latest CL does use true/false strings,
but it doesn't use the pref value directly.
I haven't worked out how to use the pref value
directly (without a local) yet.

[dschuyler, 2015-09-16 22:29:14]

On 2015/09/16 21:55:39, stevenjb wrote:
> So, FWIW, I think that the change in
https://codereview.chromium.org/1347193003/
> is both simpler and doesn't requre repeating this solution if we come across
any
> other places where we map a boolean pref to a radio group.

Nice.  This also looks like it's using 
a local string property.  Though at least it
encapsulates it.  I was trying to eliminate
the local string property and haven't
succeeded with bools yet.  It may be better
to go with encapsulating it :)

[dschuyler, 2015-09-16 22:29:18]

On 2015/09/16 21:55:39, stevenjb wrote:
> So, FWIW, I think that the change in
https://codereview.chromium.org/1347193003/
> is both simpler and doesn't requre repeating this solution if we come across
any
> other places where we map a boolean pref to a radio group.

Nice.  This also looks like it's using 
a local string property.  Though at least it
encapsulates it.  I was trying to eliminate
the local string property and haven't
succeeded with bools yet.  It may be better
to go with encapsulating it :)