MD Settings: Bluetooth: Add error dialog and messages

MD Settings: Bluetooth: Add error dialog and messages

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

Committed: https://crrev.com/148e478e5800f9664b2f89536d2985e8cfd8a95f
Cr-Commit-Position: refs/heads/master@{#405938}

[stevenjb, 2016-07-12 23:20:48]

Description was changed from

==========
MD Settings: Bluetooth: Add error dialog and messages

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

to

==========
MD Settings: Bluetooth: Add error dialog and messages

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

[stevenjb, 2016-07-12 23:25:03]

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

[Dan Beam, 2016-07-13 01:35:11]

looks pretty good to me

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

https://codereview.chromium.org/2146553005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.js:362:
return this.i18n('bluetoothPairDevicePageTitle');
what's the functional difference here?

https://codereview.chromium.org/2146553005/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html
(right):

https://codereview.chromium.org/2146553005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html:26:
on-tap="itemTapped_" actionable>
isn't the entire area already actionable?

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

https://codereview.chromium.org/2146553005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:464: if
(this.i18nExists(id))
you can add this but why not just use loadTimeData.valueExists() until we need
it from HTML somewhere?

https://codereview.chromium.org/2146553005/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/2146553005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:259:
{"bluetooth_confirmPasskey", IDS_SETTINGS_BLUETOOTH_CONFIRM_PASSKEY},
hmmmmm, why do identifiers do bluetooth_request instead of bluetoothRequest? 
i.e. unix_hacker vs camelCase?

https://codereview.chromium.org/2146553005/diff/20001/ui/webui/resources/js/i...
File ui/webui/resources/js/i18n_behavior.js (right):

https://codereview.chromium.org/2146553005/diff/20001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:66: i18nExists: function(id) {
if you do add this, can you update
chrome/test/data/webui/i18n_behavior_test.html ?

[stevenjb, 2016-07-13 17:26:43]

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

https://codereview.chromium.org/2146553005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.js:362:
return this.i18n('bluetoothPairDevicePageTitle');
On 2016/07/13 01:35:11, Dan Beam wrote:
> what's the functional difference here?

None, it just seems more clear to me now that there are more than two options
(pairDevice and connectError both use this title). Added a comment.

https://codereview.chromium.org/2146553005/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html
(right):

https://codereview.chromium.org/2146553005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html:26:
on-tap="itemTapped_" actionable>
On 2016/07/13 01:35:11, Dan Beam wrote:
> isn't the entire area already actionable?

Yes. I'm not sure why I added this, but confirmed it's not necessary, removed.

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

https://codereview.chromium.org/2146553005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:464: if
(this.i18nExists(id))
On 2016/07/13 01:35:11, Dan Beam wrote:
> you can add this but why not just use loadTimeData.valueExists() until we need
> it from HTML somewhere?

Then we have to add a dependency on loadTimeData to any modules that need it.
It's not a big deal but this seemed cleaner to me. It's not obvious to a dev
unfamiliar with the code that i18n uses loadTimeData under the hood.

https://codereview.chromium.org/2146553005/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/2146553005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:259:
{"bluetooth_confirmPasskey", IDS_SETTINGS_BLUETOOTH_CONFIRM_PASSKEY},
On 2016/07/13 01:35:11, Dan Beam wrote:
> hmmmmm, why do identifiers do bluetooth_request instead of bluetoothRequest? 
> i.e. unix_hacker vs camelCase?
As the comment above explains, these are generated ('bluetooth_' + enum value).
It seemed of little value to add an extra function call to convert
'requestPincode' (a constant provided by the IDL) to 'RequestPincode' just to
appease a styling convention, and bluetoothrequestPincode appears less readable.

https://codereview.chromium.org/2146553005/diff/20001/ui/webui/resources/js/i...
File ui/webui/resources/js/i18n_behavior.js (right):

https://codereview.chromium.org/2146553005/diff/20001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:66: i18nExists: function(id) {
On 2016/07/13 01:35:11, Dan Beam wrote:
> if you do add this, can you update
> chrome/test/data/webui/i18n_behavior_test.html ?

Done.

[Dan Beam, 2016-07-16 00:31:56]

lgtm thanks for the test

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

https://codereview.chromium.org/2146553005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:457: }
nit: \n after each if

https://codereview.chromium.org/2146553005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:468:
console.error('Error connecting to: ' + name + ': ' + this.errorMessage);
nit: i still don't support leaving console messaging in our production code...

[stevenjb, 2016-07-16 01:17:39]

Description was changed from

==========
MD Settings: Bluetooth: Add error dialog and messages

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

to

==========
MD Settings: Bluetooth: Add error dialog and messages

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

[stevenjb, 2016-07-16 01:17:47]

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

https://codereview.chromium.org/2146553005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:457: }
On 2016/07/16 00:31:56, Dan Beam wrote:
> nit: \n after each if
Presuming you mean a blank line here and after 461, done.

https://codereview.chromium.org/2146553005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:468:
console.error('Error connecting to: ' + name + ': ' + this.errorMessage);
On 2016/07/16 00:31:56, Dan Beam wrote:
> nit: i still don't support leaving console messaging in our production code...

Moved the console.error inside the else clause, since that probably shouldn't
happen but might and will be useful for debugging if it does.

One of these days I will add the equivalent of VLOG for JS...

[stevenjb, 2016-07-16 05:31:52]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-07-16 05:31:53]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2146553005/#ps60001
(title: "Feedback")

[commit-bot: I haz the power, 2016-07-16 05:32:06]

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-16 05:32:08]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-16 06:52:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-16 06:54:26]

Description was changed from

==========
MD Settings: Bluetooth: Add error dialog and messages

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

to

==========
MD Settings: Bluetooth: Add error dialog and messages

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

Committed: https://crrev.com/148e478e5800f9664b2f89536d2985e8cfd8a95f
Cr-Commit-Position: refs/heads/master@{#405938}
==========

[commit-bot: I haz the power, 2016-07-16 06:54:27]

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