MD Settings: Bluetooth: Use CrScrollableBehavior

MD Settings: Bluetooth: Use CrScrollableBehavior

BUG=602538
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[stevenjb, 2016-08-18 23:59:13]

Description was changed from

==========
MD Settings: Bluetooth: Use CrScrollableBehavior

BUG=602538
==========

to

==========
MD Settings: Bluetooth: Use CrScrollableBehavior

BUG=602538
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[stevenjb, 2016-08-22 22:10:43]

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

[dschuyler, 2016-08-23 02:33:12]

https://codereview.chromium.org/2256773005/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.html
(right):

https://codereview.chromium.org/2256773005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.html:10:
<link rel="import"
href="chrome://resources/cr_elements/cr_scrollable_behavior.html">
Can these be in sort order?

https://codereview.chromium.org/2256773005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.html:96:
scroll-target="container" tabindex$="0">
The $ doesn't look needed (on the tabindex).

https://codereview.chromium.org/2256773005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.js
(right):

https://codereview.chromium.org/2256773005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.js:368:
}.bind(this), 100);
nit: since this an interval instead of a timeout, we can use
a small value for the time delay. imo 10ms would be snappy
and still not impose any serious overhead. This is just an 
opinion though, feel free to use another value or leave it
as is.

https://codereview.chromium.org/2256773005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.html
(right):

https://codereview.chromium.org/2256773005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.html:9: <link
rel="import" href="chrome://resources/cr_elements/cr_scrollable_behavior.html">
sort in the import list?

[stevenjb, 2016-08-23 15:31:43]

https://codereview.chromium.org/2256773005/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.html
(right):

https://codereview.chromium.org/2256773005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.html:10:
<link rel="import"
href="chrome://resources/cr_elements/cr_scrollable_behavior.html">
On 2016/08/23 02:33:11, dschuyler wrote:
> Can these be in sort order?

Yeah, I guess so? We're inconsistent; we used to always do polymer first, then
cr_elements, then local. I assume we still should do local last, even though
they would be first if sorted with the others?

https://codereview.chromium.org/2256773005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.html:96:
scroll-target="container" tabindex$="0">
On 2016/08/23 02:33:11, dschuyler wrote:
> The $ doesn't look needed (on the tabindex).

It doesn't need $= here, but it does below when it is set to [[tabIndex]]. I
kind of prefer the consistency of using $= when assigning to an attribute, but I
don't feel strongly. We can bring it up at Wednesday's standup and add something
to the style guide.

https://codereview.chromium.org/2256773005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.js
(right):

https://codereview.chromium.org/2256773005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.js:368:
}.bind(this), 100);
On 2016/08/23 02:33:11, dschuyler wrote:
> nit: since this an interval instead of a timeout, we can use
> a small value for the time delay. imo 10ms would be snappy
> and still not impose any serious overhead. This is just an 
> opinion though, feel free to use another value or leave it
> as is.

Actually, a small delay could have a noticible performance impact. If we are
waiting for the list to fill, which can in practice take a while, executing even
a small amount of JS (which is slooooow) every 10ms can chew up a lot of time.
At some point I think we should measure it, but I was going to suggest that we
increase the interval the one other place where we use setInterval.

https://codereview.chromium.org/2256773005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.html
(right):

https://codereview.chromium.org/2256773005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.html:9: <link
rel="import" href="chrome://resources/cr_elements/cr_scrollable_behavior.html">
On 2016/08/23 02:33:11, dschuyler wrote:
> sort in the import list?

Done.

[dschuyler, 2016-08-23 22:16:29]

https://codereview.chromium.org/2256773005/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.html
(right):

https://codereview.chromium.org/2256773005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.html:96:
scroll-target="container" tabindex$="0">
On 2016/08/23 15:31:43, stevenjb wrote:
> On 2016/08/23 02:33:11, dschuyler wrote:
> > The $ doesn't look needed (on the tabindex).
> 
> It doesn't need $= here, but it does below when it is set to [[tabIndex]]. I
> kind of prefer the consistency of using $= when assigning to an attribute, but
I
> don't feel strongly. We can bring it up at Wednesday's standup and add
something
> to the style guide.

Yeah, cool with me. For this CL we can agree to disagree. It can change later if
it's decided that we should go without the $.

https://codereview.chromium.org/2256773005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.js
(right):

https://codereview.chromium.org/2256773005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.js:368:
}.bind(this), 100);
On 2016/08/23 15:31:43, stevenjb wrote:
> On 2016/08/23 02:33:11, dschuyler wrote:
> > nit: since this an interval instead of a timeout, we can use
> > a small value for the time delay. imo 10ms would be snappy
> > and still not impose any serious overhead. This is just an 
> > opinion though, feel free to use another value or leave it
> > as is.
> 
> Actually, a small delay could have a noticible performance impact. If we are
> waiting for the list to fill, which can in practice take a while, executing
even
> a small amount of JS (which is slooooow) every 10ms can chew up a lot of time.
> At some point I think we should measure it, but I was going to suggest that we
> increase the interval the one other place where we use setInterval.

I think this is another, "agree to disagree". This would be another good thing
to 
get a call on (or consensus) from the team. It also seems like something that
can have some real world data applied to the decision :)

https://codereview.chromium.org/2256773005/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.html
(right):

https://codereview.chromium.org/2256773005/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.html:84:
<!-- Note: dialog + dom-if + iron-list makes sizing complicated so use
Thanks for this comment

[dschuyler, 2016-08-23 22:16:33]

lgtm

[stevenjb, 2016-08-24 20:38:35]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-08-24 20:38:35]

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/2256773005/#ps80001
(title: "Rebase, fix tabindex")

[commit-bot: I haz the power, 2016-08-24 20:39:53]

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-08-24 22:23:25]

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

[commit-bot: I haz the power, 2016-08-24 22:23:26]

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

[stevenjb, 2016-08-26 20:57:26]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-08-26 20:57:26]

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/2256773005/#ps100001
(title: "Improve item focus and fix tests")

[commit-bot: I haz the power, 2016-08-26 20:58:10]

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-08-26 22:27:59]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-26 22:31:03]

Description was changed from

==========
MD Settings: Bluetooth: Use CrScrollableBehavior

BUG=602538
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Bluetooth: Use CrScrollableBehavior

BUG=602538
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2016-08-26 22:31:04]

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