[MD Extensions] Implement keyboard shortcuts page

[MD Extensions] Implement keyboard shortcuts page

Add implementation and testing for the keyboard shortcuts subpage.
This includes adding a shortcut-input component to handle much of the
requisite logic.

BUG=529395
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/aeac37b4c68651d3bda3c4aa82dbbd0078df0b60
Cr-Commit-Position: refs/heads/master@{#404551}

[Devlin, 2016-06-29 22:58:21]

Description was changed from

==========
[MD Extensions] Implement keyboard shortcuts page

Add implementation and testing for the keyboard shortcuts subpage.
This includes adding a shortcut-input component to handle much of the
requisite logic.

BUG=529395
==========

to

==========
[MD Extensions] Implement keyboard shortcuts page

Add implementation and testing for the keyboard shortcuts subpage.
This includes adding a shortcut-input component to handle much of the
requisite logic.

BUG=529395
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Devlin, 2016-06-30 14:47:44]

rdevlin.cronin@chromium.org changed reviewers:
+ michaelpg@chromium.org

[Devlin, 2016-06-30 14:47:44]

Michael, wanna take a look?

[michaelpg, 2016-07-01 21:39:04]

https://codereview.chromium.org/2101933005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/shortcut_input.html (right):

https://codereview.chromium.org/2101933005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/shortcut_input.html:9: <style>
remove

https://codereview.chromium.org/2101933005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/shortcut_input.js (right):

https://codereview.chromium.org/2101933005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/shortcut_input.js:25: capturing_: {
/** @private */ here & below

https://codereview.chromium.org/2101933005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/shortcut_input.js:33: pendingIsValid_: {
unused

https://codereview.chromium.org/2101933005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/shortcut_input.js:145: return
this.pendingShortcut_ || 'Type a shortcut';
i18n?

https://codereview.chromium.org/2101933005/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/extensions/extension_shortcut_input_test.js (right):

https://codereview.chromium.org/2101933005/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_shortcut_input_test.js:19:
input.set('commandName', 'Command');
nit... input.foo = bar;

https://codereview.chromium.org/2101933005/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_shortcut_input_test.js:32: var
startCaptureListener = new extension_test_util.ListenerMock();
if you want to block-scope these, don't you need to use let?

https://codereview.chromium.org/2101933005/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_shortcut_input_test.js:34: {});
looking at addListener, you could leave off these {} if you change {?Object}
eventArgs to {Object=} opt_eventArgs

https://codereview.chromium.org/2101933005/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/extensions/extension_test_util.js (right):

https://codereview.chromium.org/2101933005/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_test_util.js:38: /** @private
{<string, {satisfied: boolean, args: !Object}} */
i think you mean:

/** @private {Object<{satisfied: boolean, args: !Object}>} */

lmk if it's unclear why

https://codereview.chromium.org/2101933005/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_test_util.js:65:
this.listeners_[eventName] = {args: eventArgs || {}, satisfied: false};
maybe assert eventName not in this.listeners_?

[Devlin, 2016-07-07 00:33:37]

https://codereview.chromium.org/2101933005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/shortcut_input.html (right):

https://codereview.chromium.org/2101933005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/shortcut_input.html:9: <style>
On 2016/07/01 21:39:02, michaelpg wrote:
> remove

Whoops, done.

https://codereview.chromium.org/2101933005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/shortcut_input.js (right):

https://codereview.chromium.org/2101933005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/shortcut_input.js:25: capturing_: {
On 2016/07/01 21:39:02, michaelpg wrote:
> /** @private */ here & below

Done.

https://codereview.chromium.org/2101933005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/shortcut_input.js:33: pendingIsValid_: {
On 2016/07/01 21:39:02, michaelpg wrote:
> unused

Done.

https://codereview.chromium.org/2101933005/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_extensions/shortcut_input.js:145: return
this.pendingShortcut_ || 'Type a shortcut';
On 2016/07/01 21:39:02, michaelpg wrote:
> i18n?

Done.

https://codereview.chromium.org/2101933005/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/extensions/extension_shortcut_input_test.js (right):

https://codereview.chromium.org/2101933005/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_shortcut_input_test.js:19:
input.set('commandName', 'Command');
On 2016/07/01 21:39:04, michaelpg wrote:
> nit... input.foo = bar;

Done.

https://codereview.chromium.org/2101933005/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_shortcut_input_test.js:32: var
startCaptureListener = new extension_test_util.ListenerMock();
On 2016/07/01 21:39:03, michaelpg wrote:
> if you want to block-scope these, don't you need to use let?

This was as much for readability as scoping, but yes, let would be better. 
Done.

https://codereview.chromium.org/2101933005/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_shortcut_input_test.js:34: {});
On 2016/07/01 21:39:04, michaelpg wrote:
> looking at addListener, you could leave off these {} if you change {?Object}
> eventArgs to {Object=} opt_eventArgs

Done.

https://codereview.chromium.org/2101933005/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/extensions/extension_test_util.js (right):

https://codereview.chromium.org/2101933005/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_test_util.js:38: /** @private
{<string, {satisfied: boolean, args: !Object}} */
On 2016/07/01 21:39:04, michaelpg wrote:
> i think you mean:
> 
> /** @private {Object<{satisfied: boolean, args: !Object}>} */
> 
> lmk if it's unclear why

Yeah, that thing. ;)  Done.

https://codereview.chromium.org/2101933005/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_test_util.js:65:
this.listeners_[eventName] = {args: eventArgs || {}, satisfied: false};
On 2016/07/01 21:39:04, michaelpg wrote:
> maybe assert eventName not in this.listeners_?

Done.

[michaelpg, 2016-07-08 07:12:33]

lgtm

https://codereview.chromium.org/2101933005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/shortcut_input.html (right):

https://codereview.chromium.org/2101933005/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/shortcut_input.html:1: <link rel="import"
href="chrome://resources/html/cr.html">
+assert.html

https://codereview.chromium.org/2101933005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/shortcut_input.js (right):

https://codereview.chromium.org/2101933005/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/shortcut_input.js:37: behaviors:
[I18nBehavior],
nit outside the scope of this CL... if you get the chance, standardize the order
of properties on the Polymer object prototypes in md_extensions:

is
behaviors
properties
(more under Polymer at
https://www.chromium.org/developers/web-development-style-guide)

md_extensions currently seems 50-50 on putting behaviors before/after
properties, so ¯\_(ツ)_/¯

https://codereview.chromium.org/2101933005/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/extensions/extension_test_util.js (right):

https://codereview.chromium.org/2101933005/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_test_util.js:62: * @param {Object=}
eventArgs If omitted, will check that the details are
opt_eventArgs

[Devlin, 2016-07-08 23:14:44]

https://codereview.chromium.org/2101933005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/shortcut_input.html (right):

https://codereview.chromium.org/2101933005/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/shortcut_input.html:1: <link rel="import"
href="chrome://resources/html/cr.html">
On 2016/07/08 07:12:33, michaelpg wrote:
> +assert.html

Done.

https://codereview.chromium.org/2101933005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/shortcut_input.js (right):

https://codereview.chromium.org/2101933005/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/shortcut_input.js:37: behaviors:
[I18nBehavior],
On 2016/07/08 07:12:33, michaelpg wrote:
> nit outside the scope of this CL... if you get the chance, standardize the
order
> of properties on the Polymer object prototypes in md_extensions:
> 
> is
> behaviors
> properties
> (more under Polymer at
> https://www.chromium.org/developers/web-development-style-guide)
> 
> md_extensions currently seems 50-50 on putting behaviors before/after
> properties, so ¯\_(ツ)_/¯

Will do in a followup.

https://codereview.chromium.org/2101933005/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/extensions/extension_test_util.js (right):

https://codereview.chromium.org/2101933005/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_test_util.js:62: * @param {Object=}
eventArgs If omitted, will check that the details are
On 2016/07/08 07:12:33, michaelpg wrote:
> opt_eventArgs

Done.

[Devlin, 2016-07-08 23:14:49]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2016-07-08 23:14:49]

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/2101933005/#ps60001
(title: "nits + latest master")

[commit-bot: I haz the power, 2016-07-08 23:15:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-09 00:44:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-09 00:47:11]

Description was changed from

==========
[MD Extensions] Implement keyboard shortcuts page

Add implementation and testing for the keyboard shortcuts subpage.
This includes adding a shortcut-input component to handle much of the
requisite logic.

BUG=529395
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Extensions] Implement keyboard shortcuts page

Add implementation and testing for the keyboard shortcuts subpage.
This includes adding a shortcut-input component to handle much of the
requisite logic.

BUG=529395
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/aeac37b4c68651d3bda3c4aa82dbbd0078df0b60
Cr-Commit-Position: refs/heads/master@{#404551}
==========

[commit-bot: I haz the power, 2016-07-09 00:47:12]

Patchset 4 (id:??) landed as
https://crrev.com/aeac37b4c68651d3bda3c4aa82dbbd0078df0b60
Cr-Commit-Position: refs/heads/master@{#404551}