Rename cr-settings-elements

Rename cr-settings-elements

This CL:
* Renames cr-settings-{checkbox|input|radio-button} -> settings-*
* Renames the directories and files from {checkbox|input|radio_button}
  -> settings_{checkbox|input|radio_button}

BUG=532540
R=michaelpg@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/40976c05e59bac984fc5f5160fee31a1e045a6bf

[stevenjb, 2015-09-29 20:48:45]

stevenjb@chromium.org changed reviewers:
+ dbeam@chromium.org, michaelpg@chromium.org

[stevenjb, 2015-09-29 20:48:46]

https://codereview.chromium.org/1379483003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_input/settings_input.js (left):

https://codereview.chromium.org/1379483003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_input/settings_input.js:77:
this.$.input.inputElement.focus();
This is the only non-trivial change. This wasn't actually correct and adding
closure compilation caught it.

[michaelpg, 2015-09-29 21:47:26]

IDK about having "settings/settings_checkbox/settings_checkbox.html" but also
"settings/a11y_page/a11y_page.html". Mostly I'd like to have an internally
consistent rule we can apply so we can stop talking about naming :-)

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

https://codereview.chromium.org/1379483003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/on_startup_page/on_startup_page.html:6: <link
rel="import"
href="chrome://md-settings/settings_radio_group/settings_radio_group.html">
nit: alphabetize

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

https://codereview.chromium.org/1379483003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_checkbox/settings_checkbox.html:8:
<dom-module id="settings-checkbox">
fix references in settings_page.css and checkbox_tests.js (I'd also advise
running the tests, you'll just need to rename 'DISABLED_CrSettingsTest' to
'CrSettingsTest')

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

https://codereview.chromium.org/1379483003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_input/settings_input.html:7:
<dom-module id="settings-input">
fix ref in downloads_page.css

[stevenjb, 2015-09-29 22:07:32]

On 2015/09/29 21:47:26, michaelpg wrote:
> IDK about having "settings/settings_checkbox/settings_checkbox.html" but also
> "settings/a11y_page/a11y_page.html". Mostly I'd like to have an internally
> consistent rule we can apply so we can stop talking about naming :-)
> 

Well, we have "elements" in the same place as "pages". I think that abbreviating
the page names is (mostly) reasonable (settings_page and settings_main being the
existing exceptions) because the names are mostly unique. 'checkbox.html'
however is not especially unique. 

Frequently we show file names without context (e.g. in an editor) or want to
find a specific file (e.g. with a git find). Having overly simple / duplicated
filenames can be a pain imho.

[stevenjb, 2015-09-29 22:07:47]

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

https://codereview.chromium.org/1379483003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/on_startup_page/on_startup_page.html:6: <link
rel="import"
href="chrome://md-settings/settings_radio_group/settings_radio_group.html">
On 2015/09/29 21:47:26, michaelpg wrote:
> nit: alphabetize

Done.

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

https://codereview.chromium.org/1379483003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_checkbox/settings_checkbox.html:8:
<dom-module id="settings-checkbox">
On 2015/09/29 21:47:26, michaelpg wrote:
> fix references in settings_page.css and checkbox_tests.js (I'd also advise
> running the tests, you'll just need to rename 'DISABLED_CrSettingsTest' to
> 'CrSettingsTest')

Huh, thought I'd grepped for that. Done.

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

https://codereview.chromium.org/1379483003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_input/settings_input.html:7:
<dom-module id="settings-input">
On 2015/09/29 21:47:26, michaelpg wrote:
> fix ref in downloads_page.css

Done.

[stevenjb, 2015-09-29 22:49:29]

PTAL

[Dan Beam, 2015-09-30 22:50:05]

fwiw: if most (or all) of these are simply: element_name.{html,js,css}, why not
just drop the element_name/ dir in front?

[stevenjb, 2015-09-30 23:00:28]

On 2015/09/30 22:50:05, Dan Beam wrote:
> fwiw: if most (or all) of these are simply: element_name.{html,js,css}, why
not
> just drop the element_name/ dir in front?

You mean put the (currently 9, eventually probably more like 15 or more) files
directly in the c/b/resources/settings/ directory? Not really a fan of that.

I would be more inclined to put them all in a c/b/resources/settings_controls/
directory... but I also personally think 3 file directories are also fine (many
of the page specific directories are similarly small).

[michaelpg, 2015-09-30 23:02:19]

On 2015/09/30 23:00:28, stevenjb wrote:
> On 2015/09/30 22:50:05, Dan Beam wrote:
> > fwiw: if most (or all) of these are simply: element_name.{html,js,css}, why
> not
> > just drop the element_name/ dir in front?
> 
> You mean put the (currently 9, eventually probably more like 15 or more) files
> directly in the c/b/resources/settings/ directory? Not really a fan of that.
> 
> I would be more inclined to put them all in a c/b/resources/settings_controls/
> directory... but I also personally think 3 file directories are also fine
(many
> of the page specific directories are similarly small).

Dave suggested a settings_controls directory as well. I think that'd be fine and
it would perhaps alleviate the verbosity of the scheme adopted in this CL :)

[stevenjb, 2015-09-30 23:09:51]

On 2015/09/30 23:02:19, michaelpg wrote:
> On 2015/09/30 23:00:28, stevenjb wrote:
> > On 2015/09/30 22:50:05, Dan Beam wrote:
> > > fwiw: if most (or all) of these are simply: element_name.{html,js,css},
why
> > not
> > > just drop the element_name/ dir in front?
> > 
> > You mean put the (currently 9, eventually probably more like 15 or more)
files
> > directly in the c/b/resources/settings/ directory? Not really a fan of that.
> > 
> > I would be more inclined to put them all in a
c/b/resources/settings_controls/
> > directory... but I also personally think 3 file directories are also fine
> (many
> > of the page specific directories are similarly small).
> 
> Dave suggested a settings_controls directory as well. I think that'd be fine
and
> it would perhaps alleviate the verbosity of the scheme adopted in this CL :)

dbeam@ - are you on board with that? I would prefer to only re-re-factor this
once more if possible.

[Dan Beam, 2015-10-01 07:13:32]

On 2015/09/30 23:09:51, stevenjb wrote:
> On 2015/09/30 23:02:19, michaelpg wrote:
> > On 2015/09/30 23:00:28, stevenjb wrote:
> > > On 2015/09/30 22:50:05, Dan Beam wrote:
> > > > fwiw: if most (or all) of these are simply: element_name.{html,js,css},
> why
> > > not
> > > > just drop the element_name/ dir in front?
> > > 
> > > You mean put the (currently 9, eventually probably more like 15 or more)
> files
> > > directly in the c/b/resources/settings/ directory? Not really a fan of
that.
> > > 
> > > I would be more inclined to put them all in a
> c/b/resources/settings_controls/
> > > directory... but I also personally think 3 file directories are also fine
> > (many
> > > of the page specific directories are similarly small).
> > 
> > Dave suggested a settings_controls directory as well. I think that'd be fine
> and
> > it would perhaps alleviate the verbosity of the scheme adopted in this CL :)
> 
> dbeam@ - are you on board with that? I would prefer to only re-re-factor this
> once more if possible.

i don't really understand making a directory when there's going to be on average
2.5 files with similar names that differ by suffix only

[chromium-reviews, 2015-10-01 07:20:52]

So that's a yes, right?
On Oct 1, 2015 12:13 AM, <dbeam@chromium.org> wrote:

> On 2015/09/30 23:09:51, stevenjb wrote:
>
>> On 2015/09/30 23:02:19, michaelpg wrote:
>> > On 2015/09/30 23:00:28, stevenjb wrote:
>> > > On 2015/09/30 22:50:05, Dan Beam wrote:
>> > > > fwiw: if most (or all) of these are simply:
>> element_name.{html,js,css},
>> why
>> > > not
>> > > > just drop the element_name/ dir in front?
>> > >
>> > > You mean put the (currently 9, eventually probably more like 15 or
>> more)
>> files
>> > > directly in the c/b/resources/settings/ directory? Not really a fan of
>>
> that.
>
>> > >
>> > > I would be more inclined to put them all in a
>> c/b/resources/settings_controls/
>> > > directory... but I also personally think 3 file directories are also
>> fine
>> > (many
>> > > of the page specific directories are similarly small).
>> >
>> > Dave suggested a settings_controls directory as well. I think that'd be
>> fine
>> and
>> > it would perhaps alleviate the verbosity of the scheme adopted in this
>> CL :)
>>
>
> dbeam@ - are you on board with that? I would prefer to only re-re-factor
>> this
>> once more if possible.
>>
>
> i don't really understand making a directory when there's going to be on
> average
> 2.5 files with similar names that differ by suffix only
>
> https://codereview.chromium.org/1379483003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2015-10-01 16:40:04]

PTAL

[Dan Beam, 2015-10-01 17:02:57]

> So that's a yes, right?

it's a "yes" to "don't see a need for directories"... which includes
settings_controls/ as well, but at least it's a step up.

chrome/browser/resources/settings/
 - page.{css,html,js}
 - control.{css,html,js}

seems the best to me.  if we really love directories, maybe
c/b/r/settings/settings_controls -> c/b/r/settings/controls?  (really no reason
for ANOTHER settings)

[Dan Beam, 2015-10-01 17:03:37]

On 2015/10/01 17:02:57, Dan Beam wrote:
> > So that's a yes, right?
> 
> it's a "yes" to "don't see a need for directories"... which includes
> settings_controls/ as well, but at least it's a step up.
> 
> chrome/browser/resources/settings/
>  - page.{css,html,js}
>  - control.{css,html,js}
> 
> seems the best to me.  if we really love directories, maybe
> c/b/r/settings/settings_controls -> c/b/r/settings/controls?  (really no
reason
> for ANOTHER settings)

annnnnnnd I see you did this :)

[michaelpg, 2015-10-02 20:01:27]

lgtm, glad we have an "official" convention for settings controls now. (if
anyone isn't happy with this, speak now or forever hold your patches!)

[stevenjb, 2015-10-02 23:02:29]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2015-10-02 23:02:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/1379483003/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2015-10-02 23:04:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1379483003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1379483003/100001

[commit-bot: I haz the power, 2015-10-03 05:15:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-03 05:15:49]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[stevenjb, 2015-10-05 16:31:02]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-10-05 16:33:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1379483003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1379483003/100001

[commit-bot: I haz the power, 2015-10-05 19:01:18]

Patchset 7 (id:??) landed as
https://crrev.com/40976c05e59bac984fc5f5160fee31a1e045a6bf
Cr-Commit-Position: refs/heads/master@{#352373}

[stevenjb, 2015-10-05 19:01:38]

Committed patchset #7 (id:120001) manually as
40976c05e59bac984fc5f5160fee31a1e045a6bf (presubmit successful).