Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1258783009:
Add functionality to Bluetooth settings UI. (Closed)
|
Created:
5 years, 4 months ago by rfrappier Modified:
5 years, 4 months ago CC:
chromium-reviews, hashimoto+watch_chromium.org, stevenjb+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd functionality to Bluetooth settings UI.
* Sync loaded bluetooth devices on startup
* Sync new bluetooth devices after added by the system or another emulator client.
* Allow new bluetooth devices to require a pin or passkey when pairing.
BUG=514290
R=oshima@chromium.org
Committed: https://crrev.com/236c32161b4b455e8c75d8e4e25d0e1f529ec187
Cr-Commit-Position: refs/heads/master@{#341537}
Patch Set 1 #
Total comments: 94
Patch Set 2 : #
Total comments: 16
Patch Set 3 : #
Total comments: 39
Patch Set 4 : #
Total comments: 19
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #
Total comments: 8
Patch Set 7 : #Patch Set 8 : Merge changes from new patch #
Total comments: 23
Patch Set 9 : #Messages
Total messages: 38 (6 generated)
rfrappier@google.com changed reviewers: + afakhry@chromium.org, michaelpg@chromium.org, stevenjb@chromium.org
Ready for review. michaelpg@, can you review the files related to the Web UI stuff? stevenjb@, can you review fake_bluetooth_device_client.*?
rfrappier@google.com changed reviewers: + xiyuan@chromium.org
xiyuan@, since michaelpg@ is OOO until Monday, would you mind reviewing the stuff related to the WebUI?
xiyuan@chromium.org changed reviewers: + armansito@chromium.org
I am not familiar with fake_bluetooth_device_client.cc. armansito@ might want to give it a pass. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.html (right): https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.html:14: <label> Why have this big <label>? https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.html:31: <div class="device-field"> Where is "device-field" defined? https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.html:32: <p> <p> seems redundant and could be removed. Why do we need it? https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.html:47: <p> same here. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.html:52: items="{{deviceAuthenticationMethods}}" as="method" nit: more than 80 chars, wrap it. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.js (right): https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:83: value: function() { return []; } Should we return "{}" for an Object? https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:100: device_list = []; var deviceList = []; https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:113: this.devices = device_list; |device_list| contains only new devices at this point. This would cause all know devices to disappear. Is this intentional? https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:152: getTextForDeviceClass: function(classValue) { We probably should consider move this.deviceClassOptions out of the object and make it a global const and maybe make it a direct map between label and value for easier look up. Make getTextForDeviceClass and getValueForDeviceClass to global helper functions too. e.g. Make them part of BluetoothDevice. BluetoothDevice.DEVICE_CLASS_OPTIONS = { 'Unknown' : 0, 'Mouse': 0x2580, ... }; BluetoothDevice.getTextForDeviceClass = function(classValue) { for (var className in BluetoothDevice.DEVICE_CLASS_OPTIONS) { if (BluetoothDevice.DEVICE_CLASS_OPTIONS[className] == classValue) return className; } return ''; // What should we return here? 'Invalid'? }; BluetoothDevice.getValueForDeviceClass = function(className) { return BluetoothDevice.DEVICE_CLASS_OPTIONS[className]; }; https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:64: std::string DeviceEmulatorMessageHandler::CreateBluetoothDeviceFromListValue( This does not match the declaration order in header file. Please move it accordingly. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:79: device_dict->GetBoolean("isTrusted", &is_trusted); nit: CHECK() all the Getxxx. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:91: scoped_ptr<base::DictionaryValue> DeviceEmulatorMessageHandler::GetDeviceInfo( move this down to match header file order. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:144: scoped_ptr<base::ListValue> devices(new base::ListValue); nit: "base::ListValue devices;" works so not necessary to use a scoped_ptr then new. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:26: // chromeos::FakeBluetoothDeviceClient::BluetoothDeviceClient. chromeos::BluetoothDeviceClient::Observer? https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:72: std::string CreateBluetoothDeviceFromListValue(const base::ListValue* args); nit: document what's is returned. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... File chromeos/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:562: void FakeBluetoothDeviceClient::SimpleErrorCallback( Why move this into FakeBluetoothDeviceClient interface? Seems not used outside this file. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:717: pairing_options_map_[device_path] = options; This leaks. You need to do "STLDeleteValues()" similar to |properties_map_|. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:923: if (iter->second->pairing_method == "None" || nit: Put "None", "PIN Code" etc in const. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... File chromeos/dbus/fake_bluetooth_device_client.h (right): https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.h:56: SimulatedPairingOptions* GetPairingOptions( This is not overridden from BluetoothDeviceClient. Put it somewhere else.
https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.js (right): https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:54: * @type !Array<! { text: string, value: int }} > nit: No ' ' after { or before } https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:79: * Contains keys for all the device paths which have been discovered. @type https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:83: value: function() { return []; } On 2015/07/30 17:21:22, xiyuan wrote: > Should we return "{}" for an Object? It actually looks like this should be type: Array https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:92: // It should only be shown when the pair method is not 'None' or empty. @oaram {string} pairMethod @return {boolean} Whether the PIN/passkey input field should be shown. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:98: // to the main adapter. @param ... https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:100: device_list = []; On 2015/07/30 17:21:21, xiyuan wrote: > var deviceList = []; Or actually: /** @type {!Array<!BluetoothDevice>} */ var device_list = []; https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:103: if (this.devicePaths[devices[i]['path']] == undefined) { if (this.devicePaths[devices[i]['path']] != undefined) continue; Or maybe { device_list.push(devices[i]); continue; } ? https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:141: // the list (i.e. its path has not yet been recorded in |devicePaths|). @param https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:143: if (this.devicePaths[device['path']] == undefined) { if (this.devicePaths[device['path']] != undefined) return; https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:151: // Returns the text for the label that corresponds to |classValue|. @param https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:159: // Returns the integer value which corresponds with the label |classText|. @param @return https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:66: const base::DictionaryValue* device_dict = NULL; s/NULL/nullptr/ https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:20: public chromeos::PowerManagerClient::Observer, We should put this entire class in namespace chromeos since it is chromeos specific and refers to a lot of chromeos:: classes. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:27: void DeviceAdded(const dbus::ObjectPath& object_path) override; What about DeviceRemoved? Also, FWIW, rather than fill this class with PowerManager observer methods, BluetoothDevice observer methods, and anything else we add, it might be better to hide the implementations in file-local embedded classes, e.g: device_emulator_message_handler.h: class DeviceEmulatorMessageHandler { ... private: class BluetoothObserver; scoped_ptr<BluetoothObserver> bluetooth_observer_; ... }; device_emulator_message_handler.cc: class DeviceEmulatorMessageHandler::BluetoothObserver : public BluetoothDeviceClient::Observer { public: explicit BluetoothObserver(DeviceEmulatorMessageHandler* owner) {} ... }; DeviceEmulatorMessageHandler::DeviceEmulatorMessageHandler() : bluetooth_observer_(new BluetoothObserver(this)) ... https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... File chromeos/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:695: device_list_.end()) LOG(ERROR) << "Bluetootl device already exists: " << device_path.value(); Might also be nice to return true/false here so that if we want we can provide feedback to the UI. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:698: Properties* properties = Use a scoped_ptr here so that ownership passing is clear. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:711: SimulatedPairingOptions* options = new SimulatedPairingOptions; scoped_ptr https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:715: properties_map_[device_path] = properties; If we make this a ScopedPtrMap, this will be properties.Pass(), if we leave the map as-is it will be properties.release(), but either way it is more clear that the map owns the pointer. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:717: pairing_options_map_[device_path] = options; On 2015/07/30 17:21:22, xiyuan wrote: > This leaks. You need to do "STLDeleteValues()" similar to |properties_map_|. A good reason to use ScopedPtrMap for both of these. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:943: base::TimeDelta::FromMilliseconds(7 * simulation_interval_ms_)); 7 should be a const and used here and below. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:973: agent_service_provider->DisplayPinCode(object_path, "123456"); "123456" should be declared as a const char[]. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:993: agent_service_provider->DisplayPasskey(object_path, 123456, 0); const int for 123456 https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:1047: base::TimeDelta::FromMilliseconds(3 * simulation_interval_ms_)); const int for 3 https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... File chromeos/dbus/fake_bluetooth_device_client.h (right): https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.h:107: const bool is_trusted); We should put all of the arguments after |device_path| into a struct. That will clean up the DeviceEmulatorMessageHandler implementation as well. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.h:277: PairingOptionsMap pairing_options_map_; Ideally we should use ScopedPtrMap now that it exists for this and PropertiesMap.
https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.js (right): https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:83: value: function() { return []; } On 2015/07/30 18:28:51, stevenjb wrote: > On 2015/07/30 17:21:22, xiyuan wrote: > > Should we return "{}" for an Object? > > It actually looks like this should be type: Array |devicePaths| is used as a dict to look up whether a device path is known. "{}" is better in this sense since path string is used for look up. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:100: device_list = []; On 2015/07/30 18:28:51, stevenjb wrote: > On 2015/07/30 17:21:21, xiyuan wrote: > > var deviceList = []; > > Or actually: > /** @type {!Array<!BluetoothDevice>} */ var device_list = []; device_list -< deviceList, jsdoc is nice.
Patchset #2 (id:20001) has been deleted
Ready for review. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.html (right): https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.html:14: <label> On 2015/07/30 17:21:21, xiyuan wrote: > Why have this big <label>? It was mainly because all this markup is under the "Devices" label. But it's not necessary to keep it in. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.html:32: <p> On 2015/07/30 17:21:21, xiyuan wrote: > <p> seems redundant and could be removed. Why do we need it? The <p> tags were just there to add a bit of vertical margin. I've removed the <p> tags and added a style for .device-field in bluetooth_settings.css https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.html:47: <p> On 2015/07/30 17:21:21, xiyuan wrote: > same here. Acknowledged. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.html:52: items="{{deviceAuthenticationMethods}}" as="method" On 2015/07/30 17:21:21, xiyuan wrote: > nit: more than 80 chars, wrap it. Acknowledged. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.js (right): https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:54: * @type !Array<! { text: string, value: int }} > On 2015/07/30 18:28:51, stevenjb wrote: > nit: No ' ' after { or before } Done. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:79: * Contains keys for all the device paths which have been discovered. On 2015/07/30 18:28:51, stevenjb wrote: > @type Done. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:92: // It should only be shown when the pair method is not 'None' or empty. On 2015/07/30 18:28:51, stevenjb wrote: > @oaram {string} pairMethod > @return {boolean} Whether the PIN/passkey input field should be shown. Done. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:98: // to the main adapter. On 2015/07/30 18:28:51, stevenjb wrote: > @param ... Done. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:100: device_list = []; On 2015/07/30 17:21:21, xiyuan wrote: > var deviceList = []; Done. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:103: if (this.devicePaths[devices[i]['path']] == undefined) { On 2015/07/30 18:28:51, stevenjb wrote: > if (this.devicePaths[devices[i]['path']] != undefined) > continue; > > Or maybe { device_list.push(devices[i]); continue; } ? Done. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:113: this.devices = device_list; On 2015/07/30 17:21:21, xiyuan wrote: > |device_list| contains only new devices at this point. This would cause all know > devices to disappear. Is this intentional? This function is only called in the very beginning after the page loads. Currently, predefined devices do not exist, so |this.devices| is empty before this line. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:141: // the list (i.e. its path has not yet been recorded in |devicePaths|). On 2015/07/30 18:28:51, stevenjb wrote: > @param Done. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:143: if (this.devicePaths[device['path']] == undefined) { On 2015/07/30 18:28:51, stevenjb wrote: > if (this.devicePaths[device['path']] != undefined) > return; Done. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:151: // Returns the text for the label that corresponds to |classValue|. On 2015/07/30 18:28:51, stevenjb wrote: > @param Done. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:152: getTextForDeviceClass: function(classValue) { On 2015/07/30 17:21:22, xiyuan wrote: > We probably should consider move this.deviceClassOptions out of the object and > make it a global const and maybe make it a direct map between label and value > for easier look up. > > Make getTextForDeviceClass and getValueForDeviceClass to global helper functions > too. > > e.g. Make them part of BluetoothDevice. > > BluetoothDevice.DEVICE_CLASS_OPTIONS = { > 'Unknown' : 0, > 'Mouse': 0x2580, > ... > }; > > BluetoothDevice.getTextForDeviceClass = function(classValue) { > for (var className in BluetoothDevice.DEVICE_CLASS_OPTIONS) { > if (BluetoothDevice.DEVICE_CLASS_OPTIONS[className] == classValue) > return className; > } > return ''; // What should we return here? 'Invalid'? > }; > > BluetoothDevice.getValueForDeviceClass = function(className) { > return BluetoothDevice.DEVICE_CLASS_OPTIONS[className]; > }; This may be possible, and I agree that this makes sense from an OO perspective. However, it may make working with Polymer's binding difficult. As far as I know, Polymer's template repeat cannot iterate through keys in an object--it can only do arrays. Also, I cannot find any information saying that Polyemr supports accessing global variables from its binding syntax (and local experimentation seems to suggest that it does not). This means I would have to have a property in the Polymer object which contains key/value pairs for DEVICE_CLASS_OPTIONS anyway. I'm no expert on Polymer, so stevenjb@ or xiyuan@ please correct me if I'm wrong here. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:159: // Returns the integer value which corresponds with the label |classText|. On 2015/07/30 18:28:51, stevenjb wrote: > @param > @return Done. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:64: std::string DeviceEmulatorMessageHandler::CreateBluetoothDeviceFromListValue( On 2015/07/30 17:21:22, xiyuan wrote: > This does not match the declaration order in header file. Please move it > accordingly. Done. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:66: const base::DictionaryValue* device_dict = NULL; On 2015/07/30 18:28:51, stevenjb wrote: > s/NULL/nullptr/ Done. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:79: device_dict->GetBoolean("isTrusted", &is_trusted); On 2015/07/30 17:21:22, xiyuan wrote: > nit: CHECK() all the Getxxx. Done. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:91: scoped_ptr<base::DictionaryValue> DeviceEmulatorMessageHandler::GetDeviceInfo( On 2015/07/30 17:21:22, xiyuan wrote: > move this down to match header file order. Done. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:144: scoped_ptr<base::ListValue> devices(new base::ListValue); On 2015/07/30 17:21:22, xiyuan wrote: > nit: "base::ListValue devices;" works so not necessary to use a scoped_ptr then > new. Done. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:20: public chromeos::PowerManagerClient::Observer, On 2015/07/30 18:28:51, stevenjb wrote: > We should put this entire class in namespace chromeos since it is chromeos > specific and refers to a lot of chromeos:: classes. Done. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:26: // chromeos::FakeBluetoothDeviceClient::BluetoothDeviceClient. On 2015/07/30 17:21:22, xiyuan wrote: > chromeos::BluetoothDeviceClient::Observer? Whoops, don't know where that came from! Fixed. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:27: void DeviceAdded(const dbus::ObjectPath& object_path) override; On 2015/07/30 18:28:51, stevenjb wrote: > What about DeviceRemoved? > > Also, FWIW, rather than fill this class with PowerManager observer methods, > BluetoothDevice observer methods, and anything else we add, it might be better > to hide the implementations in file-local embedded classes, e.g: > > device_emulator_message_handler.h: > > class DeviceEmulatorMessageHandler { > ... > private: > class BluetoothObserver; > > scoped_ptr<BluetoothObserver> bluetooth_observer_; > ... > }; > > device_emulator_message_handler.cc: > > class DeviceEmulatorMessageHandler::BluetoothObserver > : public BluetoothDeviceClient::Observer { > public: > explicit BluetoothObserver(DeviceEmulatorMessageHandler* owner) {} > ... > }; > > DeviceEmulatorMessageHandler::DeviceEmulatorMessageHandler() > : bluetooth_observer_(new BluetoothObserver(this)) > ... Done. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:72: std::string CreateBluetoothDeviceFromListValue(const base::ListValue* args); On 2015/07/30 17:21:22, xiyuan wrote: > nit: document what's is returned. Done. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... File chromeos/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:562: void FakeBluetoothDeviceClient::SimpleErrorCallback( On 2015/07/30 17:21:22, xiyuan wrote: > Why move this into FakeBluetoothDeviceClient interface? Seems not used outside > this file. Forgot to revert this change (I thought I needed it before, but not now). Fixed https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:695: device_list_.end()) On 2015/07/30 18:28:52, stevenjb wrote: > LOG(ERROR) << "Bluetootl device already exists: " << device_path.value(); > > Might also be nice to return true/false here so that if we want we can provide > feedback to the UI. This will probably be removed later to allow for editing a device's properties after it was created. There will probably be client side validation for whether or not the path is unique (next CL :)). https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:698: Properties* properties = On 2015/07/30 18:28:51, stevenjb wrote: > Use a scoped_ptr here so that ownership passing is clear. Done. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:711: SimulatedPairingOptions* options = new SimulatedPairingOptions; On 2015/07/30 18:28:52, stevenjb wrote: > scoped_ptr Done. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:715: properties_map_[device_path] = properties; On 2015/07/30 18:28:52, stevenjb wrote: > If we make this a ScopedPtrMap, this will be properties.Pass(), if we leave the > map as-is it will be properties.release(), but either way it is more clear that > the map owns the pointer. Acknowledged. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:717: pairing_options_map_[device_path] = options; On 2015/07/30 17:21:22, xiyuan wrote: > This leaks. You need to do "STLDeleteValues()" similar to |properties_map_|. Done. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:923: if (iter->second->pairing_method == "None" || On 2015/07/30 17:21:22, xiyuan wrote: > nit: Put "None", "PIN Code" etc in const. Done. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:943: base::TimeDelta::FromMilliseconds(7 * simulation_interval_ms_)); On 2015/07/30 18:28:52, stevenjb wrote: > 7 should be a const and used here and below. Done. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:973: agent_service_provider->DisplayPinCode(object_path, "123456"); On 2015/07/30 18:28:52, stevenjb wrote: > "123456" should be declared as a const char[]. Done. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:993: agent_service_provider->DisplayPasskey(object_path, 123456, 0); On 2015/07/30 18:28:52, stevenjb wrote: > const int for 123456 Done. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.cc:1047: base::TimeDelta::FromMilliseconds(3 * simulation_interval_ms_)); On 2015/07/30 18:28:52, stevenjb wrote: > const int for 3 Done. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... File chromeos/dbus/fake_bluetooth_device_client.h (right): https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.h:107: const bool is_trusted); On 2015/07/30 18:28:52, stevenjb wrote: > We should put all of the arguments after |device_path| into a struct. That will > clean up the DeviceEmulatorMessageHandler implementation as well. Done. https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.h:277: PairingOptionsMap pairing_options_map_; On 2015/07/30 18:28:52, stevenjb wrote: > Ideally we should use ScopedPtrMap now that it exists for this and > PropertiesMap. Done.
https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.js (right): https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:100: device_list = []; On 2015/07/30 22:21:38, rfrappier wrote: > On 2015/07/30 17:21:21, xiyuan wrote: > > var deviceList = []; > > Done. It would be nice to follow stevenjb@'s suggestion to add a jsdoc. /** @type {!Array<!BluetoothDevice>} */ https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:113: this.devices = device_list; On 2015/07/30 22:21:38, rfrappier wrote: > On 2015/07/30 17:21:21, xiyuan wrote: > > |device_list| contains only new devices at this point. This would cause all > know > > devices to disappear. Is this intentional? > > This function is only called in the very beginning after the page loads. > Currently, predefined devices do not exist, so |this.devices| is empty before > this line. If we are sure this is only intended to be called at the beginning of the page load, why bother doing the devicePaths check. It should be empty at the page load too, right? I would suggest either remove the devicePaths check or change the function name to better describe what it does. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:152: getTextForDeviceClass: function(classValue) { On 2015/07/30 22:21:38, rfrappier wrote: > On 2015/07/30 17:21:22, xiyuan wrote: > > We probably should consider move this.deviceClassOptions out of the object and > > make it a global const and maybe make it a direct map between label and value > > for easier look up. > > > > Make getTextForDeviceClass and getValueForDeviceClass to global helper > functions > > too. > > > > e.g. Make them part of BluetoothDevice. > > > > BluetoothDevice.DEVICE_CLASS_OPTIONS = { > > 'Unknown' : 0, > > 'Mouse': 0x2580, > > ... > > }; > > > > BluetoothDevice.getTextForDeviceClass = function(classValue) { > > for (var className in BluetoothDevice.DEVICE_CLASS_OPTIONS) { > > if (BluetoothDevice.DEVICE_CLASS_OPTIONS[className] == classValue) > > return className; > > } > > return ''; // What should we return here? 'Invalid'? > > }; > > > > BluetoothDevice.getValueForDeviceClass = function(className) { > > return BluetoothDevice.DEVICE_CLASS_OPTIONS[className]; > > }; > > This may be possible, and I agree that this makes sense from an OO perspective. > However, it may make working with Polymer's binding difficult. As far as I know, > Polymer's template repeat cannot iterate through keys in an object--it can only > do arrays. Also, I cannot find any information saying that Polyemr supports > accessing global variables from its binding syntax (and local experimentation > seems to suggest that it does not). This means I would have to have a property > in the Polymer object which contains key/value pairs for DEVICE_CLASS_OPTIONS > anyway. > > I'm no expert on Polymer, so stevenjb@ or xiyuan@ please correct me if I'm wrong > here. I don't understand why it would not work with Polymer. We just define a couple global function. BluetoothDevice.getXXX is just a nice name so that we don't pollute the global namespace. You can call them without an instance of BluetoothDevice. Why it is a problem for Polymer? https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... File chromeos/dbus/fake_bluetooth_device_client.h (right): https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.h:56: SimulatedPairingOptions* GetPairingOptions( On 2015/07/30 17:21:22, xiyuan wrote: > This is not overridden from BluetoothDeviceClient. Put it somewhere else. What about this? https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:53: void Init() { Move this after destructor. ctor and dtor should be put together when possible. Also, is it possible to get rid this? A bit prefer to have AddObserver in ctor to be symmetric with RemoveObserver in dtor. https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:69: }; nit: DISALLOW_COPY_AND_ASSIGN(...) https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:73: scoped_ptr<base::DictionaryValue> device = owner_->GetDeviceInfo( nit: wrong indent, use 2 spaces https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:107: }; nit: DISALLOW_COPY_AND_ASSIGN(...) https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:136: bluetooth_observer_->Init(); nit: If we get rid of Init, we can bluetooth_observer_.reset(new BluetoothObserver(this)) here. https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:279: device_dict->GetString("address", &props.device_address); Sorry for the confusion. I meant CHECK(args->GetDictionary(0, &device_dict)); CHECK(device_dict->GetString("path", &path)); ... https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:328: } // namespace chromeos nit: 2 spaces before // https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:82: } // namespace chromeos nit: two space before "//" i.e. } // namespace chromeos
https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.js (right): https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:100: device_list = []; On 2015/07/30 23:30:37, xiyuan wrote: > On 2015/07/30 22:21:38, rfrappier wrote: > > On 2015/07/30 17:21:21, xiyuan wrote: > > > var deviceList = []; > > > > Done. > > It would be nice to follow stevenjb@'s suggestion to add a jsdoc. > /** @type {!Array<!BluetoothDevice>} */ Done. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:113: this.devices = device_list; On 2015/07/30 23:30:37, xiyuan wrote: > On 2015/07/30 22:21:38, rfrappier wrote: > > On 2015/07/30 17:21:21, xiyuan wrote: > > > |device_list| contains only new devices at this point. This would cause all > > know > > > devices to disappear. Is this intentional? > > > > This function is only called in the very beginning after the page loads. > > Currently, predefined devices do not exist, so |this.devices| is empty before > > this line. > > If we are sure this is only intended to be called at the beginning of the page > load, why bother doing the devicePaths check. It should be empty at the page > load too, right? > > I would suggest either remove the devicePaths check or change the function name > to better describe what it does. > Done. https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:152: getTextForDeviceClass: function(classValue) { On 2015/07/30 23:30:37, xiyuan wrote: > On 2015/07/30 22:21:38, rfrappier wrote: > > On 2015/07/30 17:21:22, xiyuan wrote: > > > We probably should consider move this.deviceClassOptions out of the object > and > > > make it a global const and maybe make it a direct map between label and > value > > > for easier look up. > > > > > > Make getTextForDeviceClass and getValueForDeviceClass to global helper > > functions > > > too. > > > > > > e.g. Make them part of BluetoothDevice. > > > > > > BluetoothDevice.DEVICE_CLASS_OPTIONS = { > > > 'Unknown' : 0, > > > 'Mouse': 0x2580, > > > ... > > > }; > > > > > > BluetoothDevice.getTextForDeviceClass = function(classValue) { > > > for (var className in BluetoothDevice.DEVICE_CLASS_OPTIONS) { > > > if (BluetoothDevice.DEVICE_CLASS_OPTIONS[className] == classValue) > > > return className; > > > } > > > return ''; // What should we return here? 'Invalid'? > > > }; > > > > > > BluetoothDevice.getValueForDeviceClass = function(className) { > > > return BluetoothDevice.DEVICE_CLASS_OPTIONS[className]; > > > }; > > > > This may be possible, and I agree that this makes sense from an OO > perspective. > > However, it may make working with Polymer's binding difficult. As far as I > know, > > Polymer's template repeat cannot iterate through keys in an object--it can > only > > do arrays. Also, I cannot find any information saying that Polyemr supports > > accessing global variables from its binding syntax (and local experimentation > > seems to suggest that it does not). This means I would have to have a property > > in the Polymer object which contains key/value pairs for DEVICE_CLASS_OPTIONS > > anyway. > > > > I'm no expert on Polymer, so stevenjb@ or xiyuan@ please correct me if I'm > wrong > > here. > > I don't understand why it would not work with Polymer. We just define a couple > global function. BluetoothDevice.getXXX is just a nice name so that we don't > pollute the global namespace. You can call them without an instance of > BluetoothDevice. Why it is a problem for Polymer? The problem is that Polymer's data binding cannot access the global namespace. If we put things in the global namespace, then we'll have to create a property in the Polymer object that references that value. For example, having BluetoothDevice.DEVICE_CLASS_OPTIONS = { ... } would require a new property or method in the Polymer object for data binding to work. So something like Polymer({ properties: { ... deviceClassOptions: { type: Object, value: function() { return BluetoothDevice.DEVICE_CLASS_OPTIONS; } } } }); The problem with this approach is that deviceClassOptions is an object, which is not iterable in Polymer's data binding. This means that you'd have to create another method to return the keys of the object like so Polymer({ ... deviceClassOptionKeys: function() { return Object.keys(this.deviceClassOptions); } ... }); and then the data binding in the HTML would look like this: <template is="dom-repeat" items="{{ key in deviceClassOptionKeys() }}"> <!-- For printing the label. --> ... {{ key }} ... <!-- For printing the value --> ... {{ deviceClassOptions[key] }} ... </template> This approach works, but I think it's quite a bit of extra code for something so simple. Would you prefer that I implement it this way? https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... File chromeos/dbus/fake_bluetooth_device_client.h (right): https://codereview.chromium.org/1258783009/diff/1/chromeos/dbus/fake_bluetoot... chromeos/dbus/fake_bluetooth_device_client.h:56: SimulatedPairingOptions* GetPairingOptions( On 2015/07/30 23:30:37, xiyuan wrote: > On 2015/07/30 17:21:22, xiyuan wrote: > > This is not overridden from BluetoothDeviceClient. Put it somewhere else. > > What about this? Missed that one, sorry about that. Done https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:53: void Init() { On 2015/07/30 23:30:37, xiyuan wrote: > Move this after destructor. ctor and dtor should be put together when possible. > > Also, is it possible to get rid this? A bit prefer to have AddObserver in ctor > to be symmetric with RemoveObserver in dtor. Done. https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:69: }; On 2015/07/30 23:30:37, xiyuan wrote: > nit: DISALLOW_COPY_AND_ASSIGN(...) Done. https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:73: scoped_ptr<base::DictionaryValue> device = owner_->GetDeviceInfo( On 2015/07/30 23:30:37, xiyuan wrote: > nit: wrong indent, use 2 spaces Done. https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:107: }; On 2015/07/30 23:30:37, xiyuan wrote: > nit: DISALLOW_COPY_AND_ASSIGN(...) Done. https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:136: bluetooth_observer_->Init(); On 2015/07/30 23:30:37, xiyuan wrote: > nit: If we get rid of Init, we can > bluetooth_observer_.reset(new BluetoothObserver(this)) > > here. Done. https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:279: device_dict->GetString("address", &props.device_address); On 2015/07/30 23:30:37, xiyuan wrote: > Sorry for the confusion. I meant > > CHECK(args->GetDictionary(0, &device_dict)); > CHECK(device_dict->GetString("path", &path)); > ... Done. https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:328: } // namespace chromeos On 2015/07/30 23:30:37, xiyuan wrote: > nit: 2 spaces before // Done. https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1258783009/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:82: } // namespace chromeos On 2015/07/30 23:30:37, xiyuan wrote: > nit: two space before "//" > i.e. > } // namespace chromeos Done.
https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.js (right): https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:152: getTextForDeviceClass: function(classValue) { On 2015/07/30 23:30:37, xiyuan wrote: > On 2015/07/30 22:21:38, rfrappier wrote: > > On 2015/07/30 17:21:22, xiyuan wrote: > > > We probably should consider move this.deviceClassOptions out of the object > and > > > make it a global const and maybe make it a direct map between label and > value > > > for easier look up. > > > > > > Make getTextForDeviceClass and getValueForDeviceClass to global helper > > functions > > > too. > > > > > > e.g. Make them part of BluetoothDevice. > > > > > > BluetoothDevice.DEVICE_CLASS_OPTIONS = { > > > 'Unknown' : 0, > > > 'Mouse': 0x2580, > > > ... > > > }; > > > > > > BluetoothDevice.getTextForDeviceClass = function(classValue) { > > > for (var className in BluetoothDevice.DEVICE_CLASS_OPTIONS) { > > > if (BluetoothDevice.DEVICE_CLASS_OPTIONS[className] == classValue) > > > return className; > > > } > > > return ''; // What should we return here? 'Invalid'? > > > }; > > > > > > BluetoothDevice.getValueForDeviceClass = function(className) { > > > return BluetoothDevice.DEVICE_CLASS_OPTIONS[className]; > > > }; > > > > This may be possible, and I agree that this makes sense from an OO > perspective. > > However, it may make working with Polymer's binding difficult. As far as I > know, > > Polymer's template repeat cannot iterate through keys in an object--it can > only > > do arrays. Also, I cannot find any information saying that Polyemr supports > > accessing global variables from its binding syntax (and local experimentation > > seems to suggest that it does not). This means I would have to have a property > > in the Polymer object which contains key/value pairs for DEVICE_CLASS_OPTIONS > > anyway. > > > > I'm no expert on Polymer, so stevenjb@ or xiyuan@ please correct me if I'm > wrong > > here. > > I don't understand why it would not work with Polymer. We just define a couple > global function. BluetoothDevice.getXXX is just a nice name so that we don't > pollute the global namespace. You can call them without an instance of > BluetoothDevice. Why it is a problem for Polymer? Also, I think having this property in the Polyemr object makes more sense when you think about what it is. |deviceClassOptions| stores data that is meant to be displayed in the view (label and value of <paper-radio-button> elements)--this is a part of the view, and therefore should be a part of the Polymer object which is used to construct the view (think of this from a Model-View-ViewModel perspective). Moving this elsewhere seems counter-intuitive to me (as per my other comment).
https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.js (right): https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:152: getTextForDeviceClass: function(classValue) { On 2015/07/31 00:55:59, rfrappier wrote: > The problem is that Polymer's data binding cannot access the global namespace. > If we put things in the global namespace, then we'll have to create a property > in the Polymer object that references that value. For example, having > > BluetoothDevice.DEVICE_CLASS_OPTIONS = { ... } > > would require a new property or method in the Polymer object for data binding to > work. So something like > Why this would not work? Have you tried it? "new BluetoothDeivce()' can be used in the polymer element. Why calling a global function and accessing global var not working? I made a little test here: http://plnkr.co/edit/SLhZzGtvlNyOFeFCxDKD?p=preview Click on the "Update Global" button would invoke a handler in the "proto-element", which would create a message using a global variable and call a global function to set a div's content in global DOM tree.
On 2015/07/31 02:00:00, xiyuan wrote: > https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... > File chrome/browser/resources/chromeos/emulator/bluetooth_settings.js (right): > > https://codereview.chromium.org/1258783009/diff/1/chrome/browser/resources/ch... > chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:152: > getTextForDeviceClass: function(classValue) { > On 2015/07/31 00:55:59, rfrappier wrote: > > The problem is that Polymer's data binding cannot access the global namespace. > > If we put things in the global namespace, then we'll have to create a property > > in the Polymer object that references that value. For example, having > > > > BluetoothDevice.DEVICE_CLASS_OPTIONS = { ... } > > > > would require a new property or method in the Polymer object for data binding > to > > work. So something like > > > > Why this would not work? Have you tried it? "new BluetoothDeivce()' can be used > in the polymer element. Why calling a global function and accessing global var > not working? > > I made a little test here: http://plnkr.co/edit/SLhZzGtvlNyOFeFCxDKD?p=preview > > Click on the "Update Global" button would invoke a handler in the > "proto-element", which would create a message using a global variable and call a > global function to set a div's content in global DOM tree. So I should just add the device class options by injecting into the DOM?
https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.js (right): https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:115: this.getTextForDeviceClass(devices[i]['classValue']); just assign to devices[i].class https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:179: this.splice('devices', foundIndex, 1); Move this inside if block above and remove foundIndex. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:99: const power_manager::PowerSupplyProperties& proto) override; nit :indent https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:127: power_observer_(new PowerObserver(this)) {} it's creating the power observer twice. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:158: for (size_t i = 0; i < paths.size(); ++i) { range based for loop? https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:276: CHECK(device_dict->GetBoolean("isTrusted", &props.is_trusted)); DCHECK https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:308: for (size_t i = 0; i < props->uuids.value().size(); ++i) { ditto https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:9: #include "chromeos/dbus/fake_power_manager_client.h" you can just use forward declarations for FakeXxx. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:66: scoped_ptr<PowerObserver> power_observer_; member variables should come after method declarations. https://codereview.chromium.org/1258783009/diff/60001/chromeos/dbus/fake_blue... File chromeos/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/1258783009/diff/60001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_device_client.cc:336: return NULL; return iter != ....end ? iter->second : nullptr; https://codereview.chromium.org/1258783009/diff/60001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_device_client.cc:1070: // error_callback.Run(kNoResponseError, "No pairing fake"); ? https://codereview.chromium.org/1258783009/diff/60001/chromeos/dbus/fake_blue... File chromeos/dbus/fake_bluetooth_device_client.h (right): https://codereview.chromium.org/1258783009/diff/60001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_device_client.h:279: PropertiesMap; using PropertiesMap = base....; (C++11 style) https://codereview.chromium.org/1258783009/diff/60001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_device_client.h:287: PairingOptionsMap; ditto
On 2015/07/31 14:43:07, rfrappier wrote: > So I should just add the device class options by injecting into the DOM? Discussed offline. The class name/value list is used to generate a list of radio buttons and not just for mapping. I missed that part. So no need to change.
Ready for review. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.js (right): https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:115: this.getTextForDeviceClass(devices[i]['classValue']); On 2015/07/31 14:54:55, oshima wrote: > just assign to devices[i].class At this point, each device object does not have a |.class| attribute, or rather, that value is undefined. Refer to device_emulator_message_handler.cc:287. The back-end does not know about the labels in the front end. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:179: this.splice('devices', foundIndex, 1); On 2015/07/31 14:54:55, oshima wrote: > Move this inside if block above and remove foundIndex. Done. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:99: const power_manager::PowerSupplyProperties& proto) override; On 2015/07/31 14:54:55, oshima wrote: > nit :indent Done. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:127: power_observer_(new PowerObserver(this)) {} On 2015/07/31 14:54:56, oshima wrote: > it's creating the power observer twice. Yes, forgot to remove this. Done. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:158: for (size_t i = 0; i < paths.size(); ++i) { On 2015/07/31 14:54:56, oshima wrote: > range based for loop? Done. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:276: CHECK(device_dict->GetBoolean("isTrusted", &props.is_trusted)); On 2015/07/31 14:54:55, oshima wrote: > DCHECK Done. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:308: for (size_t i = 0; i < props->uuids.value().size(); ++i) { On 2015/07/31 14:54:55, oshima wrote: > ditto Done. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:9: #include "chromeos/dbus/fake_power_manager_client.h" On 2015/07/31 14:54:56, oshima wrote: > you can just use forward declarations for FakeXxx. Done. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:66: scoped_ptr<PowerObserver> power_observer_; On 2015/07/31 14:54:56, oshima wrote: > member variables should come after method declarations. Done. https://codereview.chromium.org/1258783009/diff/60001/chromeos/dbus/fake_blue... File chromeos/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/1258783009/diff/60001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_device_client.cc:336: return NULL; On 2015/07/31 14:54:56, oshima wrote: > return iter != ....end ? iter->second : nullptr; Done. https://codereview.chromium.org/1258783009/diff/60001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_device_client.cc:1070: // error_callback.Run(kNoResponseError, "No pairing fake"); On 2015/07/31 14:54:56, oshima wrote: > ? Done. https://codereview.chromium.org/1258783009/diff/60001/chromeos/dbus/fake_blue... File chromeos/dbus/fake_bluetooth_device_client.h (right): https://codereview.chromium.org/1258783009/diff/60001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_device_client.h:279: PropertiesMap; On 2015/07/31 14:54:56, oshima wrote: > using PropertiesMap = base....; (C++11 style) Done. https://codereview.chromium.org/1258783009/diff/60001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_device_client.h:287: PairingOptionsMap; On 2015/07/31 14:54:56, oshima wrote: > ditto Done.
https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.js (right): https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:115: this.getTextForDeviceClass(devices[i]['classValue']); On 2015/07/31 15:45:09, rfrappier wrote: > On 2015/07/31 14:54:55, oshima wrote: > > just assign to devices[i].class > > At this point, each device object does not have a |.class| attribute, or rather, > that value is undefined. Refer to device_emulator_message_handler.cc:287. The > back-end does not know about the labels in the front end. Correction: device_emulator_message_handler.cc:288. This function is used to get the data for each of the devices in |devices|.
Mostly good. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.js (right): https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:17: this.classValue = 0; class 'Computer' value is 0x104 in the |deviceClassOptions| list. Should we use the same value here? https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:115: this.getTextForDeviceClass(devices[i]['classValue']); On 2015/07/31 15:45:09, rfrappier wrote: > On 2015/07/31 14:54:55, oshima wrote: > > just assign to devices[i].class > > At this point, each device object does not have a |.class| attribute, or rather, > that value is undefined. Refer to device_emulator_message_handler.cc:287. The > back-end does not know about the labels in the front end. I think oshima means get rid of |selectedClass| since it is used only once. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:157: var selectedClass = this.getTextForDeviceClass(device['classValue']); similarly here https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:185: * of a device. nit: 4 more spaces indent, * @param {number} classValue A number representing the bluetooth class * of a device. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:276: CHECK(device_dict->GetBoolean("isTrusted", &props.is_trusted)); On 2015/07/31 15:45:09, rfrappier wrote: > On 2015/07/31 14:54:55, oshima wrote: > > DCHECK > > Done. Be careful with DCHECK. It would be a no-op in NDEBUG build.
Ready for review. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.js (right): https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:17: this.classValue = 0; On 2015/07/31 15:51:38, xiyuan wrote: > class 'Computer' value is 0x104 in the |deviceClassOptions| list. Should we use > the same value here? Yes, we should. Done. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:115: this.getTextForDeviceClass(devices[i]['classValue']); On 2015/07/31 15:51:38, xiyuan wrote: > On 2015/07/31 15:45:09, rfrappier wrote: > > On 2015/07/31 14:54:55, oshima wrote: > > > just assign to devices[i].class > > > > At this point, each device object does not have a |.class| attribute, or > rather, > > that value is undefined. Refer to device_emulator_message_handler.cc:287. The > > back-end does not know about the labels in the front end. > > I think oshima means get rid of |selectedClass| since it is used only once. Oh, okay. Done. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:157: var selectedClass = this.getTextForDeviceClass(device['classValue']); On 2015/07/31 15:51:38, xiyuan wrote: > similarly here Done. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:185: * of a device. On 2015/07/31 15:51:38, xiyuan wrote: > nit: 4 more spaces indent, > > * @param {number} classValue A number representing the bluetooth class > * of a device. Done. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:276: CHECK(device_dict->GetBoolean("isTrusted", &props.is_trusted)); On 2015/07/31 15:51:38, xiyuan wrote: > On 2015/07/31 15:45:09, rfrappier wrote: > > On 2015/07/31 14:54:55, oshima wrote: > > > DCHECK > > > > Done. > > Be careful with DCHECK. It would be a no-op in NDEBUG build. So should I change this back to CHECK, leave as is, or do something else?
https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:276: CHECK(device_dict->GetBoolean("isTrusted", &props.is_trusted)); On 2015/07/31 16:38:53, rfrappier wrote: > On 2015/07/31 15:51:38, xiyuan wrote: > > On 2015/07/31 15:45:09, rfrappier wrote: > > > On 2015/07/31 14:54:55, oshima wrote: > > > > DCHECK > > > > > > Done. > > > > Be careful with DCHECK. It would be a no-op in NDEBUG build. > > So should I change this back to CHECK, leave as is, or do something else? Since we'll exclude this from official build, I'm ok to use CHECK then.
https://codereview.chromium.org/1258783009/diff/100001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/1258783009/diff/100001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_device_client.cc:57: const int kSimulatedPairTimeMultiplierThree = 3; can you give more meaningful name to them rather than Four/Seven etc? https://codereview.chromium.org/1258783009/diff/100001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_device_client.h (right): https://codereview.chromium.org/1258783009/diff/100001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_device_client.h:59: int device_class; actually you should should be able to just initialize inline here like device_class = 0;
This is looking much better, thanks for doing the re-factoring. In particular, thanks for the cleanup of the existing Fake implementation. https://codereview.chromium.org/1258783009/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.html (right): https://codereview.chromium.org/1258783009/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.html:15: <template is="dom-repeat" items="{{devices}}"> nit: [[devices]] since presumably the template isn't modifying devices. https://codereview.chromium.org/1258783009/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.html:35: items="{{deviceClassOptions}}" as="option" nit: [[deviceClassOptions]] https://codereview.chromium.org/1258783009/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.html:38: >{{option.text}}</paper-radio-button> nit: [[option.text]] (and below - anywhere the binding is one-way it is a bit more clear to use [[]]) https://codereview.chromium.org/1258783009/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.html:57: if="{{ showAuthToken(item.pairingMethod) }}"> nit: no ' ' after {{ or before }} https://codereview.chromium.org/1258783009/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.js (right): https://codereview.chromium.org/1258783009/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:71: * @type !Array<!string> nit: ! or ? is only needed for Object and Array types which could be null (i.e. not string). https://codereview.chromium.org/1258783009/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:97: * for a particular device. indent 4 spaces when continuing an @ line, i.e.: * @param {string} pairMethod ... * for a ... https://codereview.chromium.org/1258783009/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1258783009/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:104: DeviceEmulatorMessageHandler * owner_; Handler* https://codereview.chromium.org/1258783009/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:277: DCHECK(device_dict->GetBoolean("isTrusted", &props.is_trusted)); These will all get compiled out in a Release build. These should all look like: if (!args->GetDictionary(0, &device_dict)) NOTREACHED(); https://codereview.chromium.org/1258783009/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:292: fake_bluetooth_device_client_->GetProperties(object_path); nit: blank line here to separate the Get from the new properties. https://codereview.chromium.org/1258783009/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1258783009/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:62: // asynchronously. This comment should now be made specific to RequestPowerInfo. https://codereview.chromium.org/1258783009/diff/80001/chromeos/dbus/fake_blue... File chromeos/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/1258783009/diff/80001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_device_client.cc:57: const int kSimulatedPairTimeMultiplierThree = 3; Instead of using undescriptive names like Four, we should use names associated with where these are used, e.g. kSimulatedPairVanishingTime. We should also put a comment at the top like: // These times are multiplied by kSimulationIntervalMs which can // be changed to slow down or speed up the simulated behavior. https://codereview.chromium.org/1258783009/diff/80001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_device_client.cc:930: iter->second->pairing_method == "") { nit: iter->second->pairing_method.empty() https://codereview.chromium.org/1258783009/diff/80001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_device_client.cc:940: // Display a Pincode, and wait 7 times the interval before acting as nit: We should remove any hard coded values from the comments too, e.g. // Display a Pincode, and wait before acting... https://codereview.chromium.org/1258783009/diff/80001/chromeos/dbus/fake_blue... File chromeos/dbus/fake_bluetooth_device_client.h (right): https://codereview.chromium.org/1258783009/diff/80001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_device_client.h:48: // created. One line https://codereview.chromium.org/1258783009/diff/80001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_device_client.h:60: bool is_trusted; Instead of having a constructor here, in C++11 we can now do: int device_class = 0; bool is_trusted = false; (The destructor is entirely unnecessary for a struct)
Ready for review. https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1258783009/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:276: CHECK(device_dict->GetBoolean("isTrusted", &props.is_trusted)); On 2015/07/31 16:41:06, oshima wrote: > On 2015/07/31 16:38:53, rfrappier wrote: > > On 2015/07/31 15:51:38, xiyuan wrote: > > > On 2015/07/31 15:45:09, rfrappier wrote: > > > > On 2015/07/31 14:54:55, oshima wrote: > > > > > DCHECK > > > > > > > > Done. > > > > > > Be careful with DCHECK. It would be a no-op in NDEBUG build. > > > > So should I change this back to CHECK, leave as is, or do something else? > > Since we'll exclude this from official build, I'm ok to use CHECK then. Done. https://codereview.chromium.org/1258783009/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.html (right): https://codereview.chromium.org/1258783009/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.html:15: <template is="dom-repeat" items="{{devices}}"> On 2015/07/31 16:49:33, stevenjb wrote: > nit: [[devices]] since presumably the template isn't modifying devices. Done. https://codereview.chromium.org/1258783009/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.html:35: items="{{deviceClassOptions}}" as="option" On 2015/07/31 16:49:33, stevenjb wrote: > nit: [[deviceClassOptions]] Done. https://codereview.chromium.org/1258783009/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.html:38: >{{option.text}}</paper-radio-button> On 2015/07/31 16:49:33, stevenjb wrote: > nit: [[option.text]] (and below - anywhere the binding is one-way it is a bit > more clear to use [[]]) Done. https://codereview.chromium.org/1258783009/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/bluetooth_settings.html:57: if="{{ showAuthToken(item.pairingMethod) }}"> On 2015/07/31 16:49:33, stevenjb wrote: > nit: no ' ' after {{ or before }} Done. https://codereview.chromium.org/1258783009/diff/100001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/1258783009/diff/100001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_device_client.cc:57: const int kSimulatedPairTimeMultiplierThree = 3; On 2015/07/31 16:46:10, oshima wrote: > can you give more meaningful name to them rather than Four/Seven etc? Done. https://codereview.chromium.org/1258783009/diff/100001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_device_client.h (right): https://codereview.chromium.org/1258783009/diff/100001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_device_client.h:59: int device_class; On 2015/07/31 16:46:10, oshima wrote: > actually you should should be able to just initialize inline here > like device_class = 0; Done.
LGTM but please wait for stevenjb@ and oshima@. https://codereview.chromium.org/1258783009/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.js (right): https://codereview.chromium.org/1258783009/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:101: return pairMethod != 'None' && pairMethod != undefined && pairMethod != ''; nit: how about return pairMethod && pairMethod != 'None'
lgtm my bits with nits. please wait for stevenjb@. https://codereview.chromium.org/1258783009/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1258783009/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:83: scoped_ptr<PowerObserver> power_observer_; can you sort and group them together ? FakeBlutooth... scoped_ptr<Bluetooth....> FakePowerManagerClient* ... scopedptr<PowerObserver> https://codereview.chromium.org/1258783009/diff/120001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/1258783009/diff/120001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_device_client.cc:57: const int kSimulateNormalPairTimeMultiplier = 3; can you add comment about why these nubmers? https://codereview.chromium.org/1258783009/diff/120001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_device_client.h (right): https://codereview.chromium.org/1258783009/diff/120001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_device_client.h:51: ~IncomingDeviceProperties(); you probably don't need them?
Ready for review. https://codereview.chromium.org/1258783009/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.js (right): https://codereview.chromium.org/1258783009/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:101: return pairMethod != 'None' && pairMethod != undefined && pairMethod != ''; On 2015/07/31 20:09:47, xiyuan wrote: > nit: how about > > return pairMethod && pairMethod != 'None' Done. https://codereview.chromium.org/1258783009/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1258783009/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:83: scoped_ptr<PowerObserver> power_observer_; On 2015/07/31 21:33:58, oshima wrote: > can you sort and group them together ? > > FakeBlutooth... > scoped_ptr<Bluetooth....> > > FakePowerManagerClient* ... > scopedptr<PowerObserver> Done. https://codereview.chromium.org/1258783009/diff/120001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/1258783009/diff/120001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_device_client.cc:57: const int kSimulateNormalPairTimeMultiplier = 3; On 2015/07/31 21:33:58, oshima wrote: > can you add comment about why these nubmers? Done. https://codereview.chromium.org/1258783009/diff/120001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_device_client.h (right): https://codereview.chromium.org/1258783009/diff/120001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_device_client.h:51: ~IncomingDeviceProperties(); On 2015/07/31 21:33:58, oshima wrote: > you probably don't need them? Excluding these yields the compiler error "Complex class/struct needs an explicit out-of-line constructor."
Ready for review once again (merged changes from a new patch).
lgtm with nits https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:18: } nit: } // namespace dbus https://codereview.chromium.org/1258783009/diff/150001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/1258783009/diff/150001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_device_client.cc:258: ~IncomingDeviceProperties() {} nit: You can avoid having to add these by assigning "= default;" to the declarations.
https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.html (right): https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/bluetooth_settings.html:15: <template is="dom-repeat" items="{{devices}}"> [[ ]] ? https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.js (right): https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:71: * @type !Array<!string> !Array<string> https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:97: * for a particular device. indent https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:105: DeviceEmulatorMessageHandler * owner_; Handler* https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:63: // asynchronously. Fix comment https://codereview.chromium.org/1258783009/diff/150001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/1258783009/diff/150001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_device_client.cc:949: iter->second->pairing_method == "") { .empty() https://codereview.chromium.org/1258783009/diff/150001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_device_client.cc:959: // Display a Pincode, and wait 7 times the interval before acting as nit: Remove hard coded values from comments https://codereview.chromium.org/1258783009/diff/150001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_device_client.h (right): https://codereview.chromium.org/1258783009/diff/150001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_device_client.h:48: // created. One line https://codereview.chromium.org/1258783009/diff/150001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_device_client.h:51: ~IncomingDeviceProperties(); Constructor / destructor not needed in struct.
Ready for review. https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.html (right): https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/bluetooth_settings.html:15: <template is="dom-repeat" items="{{devices}}"> On 2015/08/03 15:02:57, stevenjb wrote: > [[ ]] ? Done. https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/bluetooth_settings.js (right): https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:71: * @type !Array<!string> On 2015/08/03 15:02:57, stevenjb wrote: > !Array<string> Done. https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/bluetooth_settings.js:97: * for a particular device. On 2015/08/03 15:02:57, stevenjb wrote: > indent Done. https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:105: DeviceEmulatorMessageHandler * owner_; On 2015/08/03 15:02:57, stevenjb wrote: > Handler* Done. https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:18: } On 2015/08/01 04:53:20, armansito wrote: > nit: } // namespace dbus Done. https://codereview.chromium.org/1258783009/diff/150001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:63: // asynchronously. On 2015/08/03 15:02:57, stevenjb wrote: > Fix comment Done. https://codereview.chromium.org/1258783009/diff/150001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/1258783009/diff/150001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_device_client.cc:258: ~IncomingDeviceProperties() {} On 2015/08/01 04:53:20, armansito wrote: > nit: You can avoid having to add these by assigning "= default;" to the > declarations. Chrome's style checker wants explicit out of line constructors/destructors. " = default;" makes the definition inline, so the style checker still complains. See here: https://www.chromium.org/developers/coding-style/chromium-style-checker-errors https://codereview.chromium.org/1258783009/diff/150001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_device_client.cc:949: iter->second->pairing_method == "") { On 2015/08/03 15:02:57, stevenjb wrote: > .empty() Done. https://codereview.chromium.org/1258783009/diff/150001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_device_client.cc:959: // Display a Pincode, and wait 7 times the interval before acting as On 2015/08/03 15:02:57, stevenjb wrote: > nit: Remove hard coded values from comments Done. https://codereview.chromium.org/1258783009/diff/150001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_device_client.h (right): https://codereview.chromium.org/1258783009/diff/150001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_device_client.h:48: // created. On 2015/08/03 15:02:57, stevenjb wrote: > One line Done. https://codereview.chromium.org/1258783009/diff/150001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_device_client.h:51: ~IncomingDeviceProperties(); On 2015/08/03 15:02:57, stevenjb wrote: > Constructor / destructor not needed in struct. Chrome's style checker requires them in this case because the struct is so large. See here https://www.chromium.org/developers/coding-style/chromium-style-checker-errors
lgtm https://codereview.chromium.org/1258783009/diff/150001/chromeos/dbus/fake_blu... File chromeos/dbus/fake_bluetooth_device_client.h (right): https://codereview.chromium.org/1258783009/diff/150001/chromeos/dbus/fake_blu... chromeos/dbus/fake_bluetooth_device_client.h:51: ~IncomingDeviceProperties(); On 2015/08/03 15:43:15, rfrappier wrote: > On 2015/08/03 15:02:57, stevenjb wrote: > > Constructor / destructor not needed in struct. > > Chrome's style checker requires them in this case because the struct is so > large. See here > https://www.chromium.org/developers/coding-style/chromium-style-checker-errors Ew. OK, nevermind.
The CQ bit was checked by rfrappier@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, oshima@chromium.org, armansito@chromium.org Link to the patchset: https://codereview.chromium.org/1258783009/#ps170001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1258783009/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1258783009/170001
Message was sent while issue was closed.
Committed patchset #9 (id:170001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/236c32161b4b455e8c75d8e4e25d0e1f529ec187 Cr-Commit-Position: refs/heads/master@{#341537} |
