MD Settings: First half of Device > Keyboard sub-page.

MD Settings: First half of Device > Keyboard sub-page.

screenshot: http://i.imgur.com/guvcgOi.png

BUG=546834
TEST=CrSettingsDevicePageTest

Committed: https://crrev.com/35606c90c45cc32aff5ccd2b288e2414c491d6d0
Cr-Commit-Position: refs/heads/master@{#378912}

[michaelpg, 2016-03-01 09:10:40]

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

[michaelpg, 2016-03-01 09:10:40]

Dave, mind doing another review for me?

[michaelpg, 2016-03-01 09:20:34]

Description was changed from

==========
MD Settings: First half of Device > Keyboard sub-page.

BUG=546834
TEST=CrSettingsDevicePageTest
==========

to

==========
MD Settings: First half of Device > Keyboard sub-page.

screenshot: http://i.imgur.com/guvcgOi.png

BUG=546834
TEST=CrSettingsDevicePageTest
==========

[dschuyler, 2016-03-02 01:08:06]

https://codereview.chromium.org/1753653002/diff/20001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1753653002/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:918: <message
name="IDS_SETTINGS_KEYBOARD_KEY_SEARCH_LABEL" desc="In Device Settings, the
label for the Search key.">
optional: I've been asked in other reviews to
drop the _LABEL or _TITLE suffix on the IDS names.
If that's cool with you, please do.

https://codereview.chromium.org/1753653002/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:939: <message
name="IDS_SETTINGS_KEYBOARD_KEY_LEFT_ALT" desc="In Device Settings, the dropdown
list item for the Alt key.">
It seems strange to have IDS names with and without
the _LABEL suffix.  Can we get by with one set of these?

https://codereview.chromium.org/1753653002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/device_page.html (right):

https://codereview.chromium.org/1753653002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.html:4: <link
rel="import" href="chrome://resources/polymer/v1_0/iron-icon/iron-icon.html">
Should we include /polymer/ before /html/ or vice versa?
Can we go either way rather than alternate?

https://codereview.chromium.org/1753653002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.html:25: <iron-icon
icon="image:brightness-1"></iron-icon>
optional: I've learned that the brightness-1 icon
implies a TODO to get the 'real' icon later.  Maybe
we could/should start adding TODO's when adding
brightness-1.  Up to you, just a thought.

https://codereview.chromium.org/1753653002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/keyboard.js (right):

https://codereview.chromium.org/1753653002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/keyboard.js:13: // and everything
that depends on it to compiled_resources2.gyp?
Is this referring to the comment below?
Maybe mention DropdownMenuOption here or integrate this into
or closer to the comment/item below?  Or maybe say "don't 
duplicate these @typedef's".

https://codereview.chromium.org/1753653002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/keyboard_handler.cc (right):

https://codereview.chromium.org/1753653002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/keyboard_handler.cc:19: namespace {
I've usually seen anonymous namespaces placed between
the includes and the named namespaces.  Should/could
this be moved up or does it need to be here (or I'm
I wrong about what is the desired layout)?

https://codereview.chromium.org/1753653002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/keyboard_handler.h (right):

https://codereview.chromium.org/1753653002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/keyboard_handler.h:36: //
ui::InputDeviceEventObserver:
I've been told to use period at the end of the comment
in these cases (even though there are lots of examples
of colon being used).

[michaelpg, 2016-03-02 01:51:20]

Patchset #3 (id:40001) has been deleted

[michaelpg, 2016-03-02 02:42:06]

Thanks! PTAL.

https://codereview.chromium.org/1753653002/diff/20001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1753653002/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:918: <message
name="IDS_SETTINGS_KEYBOARD_KEY_SEARCH_LABEL" desc="In Device Settings, the
label for the Search key.">
On 2016/03/02 01:08:06, dschuyler wrote:
> optional: I've been asked in other reviews to
> drop the _LABEL or _TITLE suffix on the IDS names.
> If that's cool with you, please do.

Done.

https://codereview.chromium.org/1753653002/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:939: <message
name="IDS_SETTINGS_KEYBOARD_KEY_LEFT_ALT" desc="In Device Settings, the dropdown
list item for the Alt key.">
On 2016/03/02 01:08:06, dschuyler wrote:
> It seems strange to have IDS names with and without
> the _LABEL suffix.  Can we get by with one set of these?

Options did this. I took a quick peek at the translations and didn't see
anywhere they differed, but I can't say definitively that they don't. I also dug
back to where these strings were originally introduced (c5419ba7) and there was
no explanation for why they were duplicated.

I'll cut out the duplicates and see if anyone yells at me when we get these
translated.

https://codereview.chromium.org/1753653002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/device_page.html (right):

https://codereview.chromium.org/1753653002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.html:4: <link
rel="import" href="chrome://resources/polymer/v1_0/iron-icon/iron-icon.html">
On 2016/03/02 01:08:06, dschuyler wrote:
> Should we include /polymer/ before /html/ or vice versa?
> Can we go either way rather than alternate?

polymer_config.html needs to be included before polymer.html. But including a
Polymer behavior before polymer.html seems weird. (Turns out it would work,
because we don't actually *call* Polymer when defining a behavior, but ehhhhh)

I think the right solution is to actually move the *polymer* import into
polymer_config.html. I'll send out a CL with my rationalization.

Meantime I moved the i18n_behavior down so at least all the Polymer ones are
together.

https://codereview.chromium.org/1753653002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.html:25: <iron-icon
icon="image:brightness-1"></iron-icon>
On 2016/03/02 01:08:06, dschuyler wrote:
> optional: I've learned that the brightness-1 icon
> implies a TODO to get the 'real' icon later.  Maybe
> we could/should start adding TODO's when adding
> brightness-1.  Up to you, just a thought.

Done.

https://codereview.chromium.org/1753653002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/keyboard.js (right):

https://codereview.chromium.org/1753653002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/keyboard.js:13: // and everything
that depends on it to compiled_resources2.gyp?
On 2016/03/02 01:08:06, dschuyler wrote:
> Is this referring to the comment below?
> Maybe mention DropdownMenuOption here or integrate this into
> or closer to the comment/item below?  Or maybe say "don't 
> duplicate these @typedef's".

Done.

https://codereview.chromium.org/1753653002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/keyboard_handler.cc (right):

https://codereview.chromium.org/1753653002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/keyboard_handler.cc:19: namespace {
On 2016/03/02 01:08:06, dschuyler wrote:
> I've usually seen anonymous namespaces placed between
> the includes and the named namespaces.  Should/could
> this be moved up or does it need to be here (or I'm
> I wrong about what is the desired layout)?

If I understand you correctly, I don't think it matters much, and I see the
pattern used here pretty often.

But in this case, HasExternalKeyboard doesn't actually use the chromeos or
settings namespace, so it's fine and maybe slightly better the other way.

https://codereview.chromium.org/1753653002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/keyboard_handler.h (right):

https://codereview.chromium.org/1753653002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/keyboard_handler.h:36: //
ui::InputDeviceEventObserver:
On 2016/03/02 01:08:06, dschuyler wrote:
> I've been told to use period at the end of the comment
> in these cases (even though there are lots of examples
> of colon being used).

The stronger argument is that the line above uses a period, and we should at
least be consistent in the same file :-)

c/b/ui/webui/settings seems to slightly prefer "Foo override." over "Foo:" (and
so does options) so here it is.

[dschuyler, 2016-03-02 21:49:39]

lgtm

[michaelpg, 2016-03-02 21:53:32]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2016-03-02 21:53:59]

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

[commit-bot: I haz the power, 2016-03-02 22:38:08]

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

[commit-bot: I haz the power, 2016-03-02 22:38:10]

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

[michaelpg, 2016-03-02 22:55:43]

Renamed keyboard_handler.* to device_keyboard_handler.* because of
https://crbug.com/554157.

[michaelpg, 2016-03-02 23:20:14]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2016-03-02 23:20:14]

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

[commit-bot: I haz the power, 2016-03-02 23:21:06]

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

[commit-bot: I haz the power, 2016-03-02 23:54:15]

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

[commit-bot: I haz the power, 2016-03-02 23:54:16]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[michaelpg, 2016-03-03 00:32:53]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2016-03-03 00:32:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/1753653002/#ps120001
(title: "missed one")

[commit-bot: I haz the power, 2016-03-03 00:33:25]

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

[commit-bot: I haz the power, 2016-03-03 01:48:01]

Description was changed from

==========
MD Settings: First half of Device > Keyboard sub-page.

screenshot: http://i.imgur.com/guvcgOi.png

BUG=546834
TEST=CrSettingsDevicePageTest
==========

to

==========
MD Settings: First half of Device > Keyboard sub-page.

screenshot: http://i.imgur.com/guvcgOi.png

BUG=546834
TEST=CrSettingsDevicePageTest
==========

[commit-bot: I haz the power, 2016-03-03 01:48:04]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-03-03 01:49:43]

Description was changed from

==========
MD Settings: First half of Device > Keyboard sub-page.

screenshot: http://i.imgur.com/guvcgOi.png

BUG=546834
TEST=CrSettingsDevicePageTest
==========

to

==========
MD Settings: First half of Device > Keyboard sub-page.

screenshot: http://i.imgur.com/guvcgOi.png

BUG=546834
TEST=CrSettingsDevicePageTest

Committed: https://crrev.com/35606c90c45cc32aff5ccd2b288e2414c491d6d0
Cr-Commit-Position: refs/heads/master@{#378912}
==========

[commit-bot: I haz the power, 2016-03-03 01:49:44]

Patchset 6 (id:??) landed as
https://crrev.com/35606c90c45cc32aff5ccd2b288e2414c491d6d0
Cr-Commit-Position: refs/heads/master@{#378912}