Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
20070002:
Demo UI for device discovery and registration (Closed)
|
Created:
7 years, 5 months ago by Noam Samuel Modified:
7 years, 4 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAn example chrome://devices/ page (behind flag) that lets users register devices with Google Cloud Print. Currently supported features:
- Device listing (addition/removal)
- Registration using Privet
BUG=245375
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217748
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : #Patch Set 4 : Gate devices page on enable_mdns=1 #
Total comments: 18
Patch Set 5 : #
Total comments: 33
Patch Set 6 : #
Total comments: 30
Patch Set 7 : #Patch Set 8 : #
Total comments: 6
Patch Set 9 : Added tests #Patch Set 10 : #
Total comments: 25
Patch Set 11 : #
Total comments: 17
Patch Set 12 : #Patch Set 13 : #Patch Set 14 : #
Total comments: 2
Patch Set 15 : #Patch Set 16 : #Patch Set 17 : Gating the browser test on enable_mdns #Patch Set 18 : Added FILE_PATH_LITERAL macro to fix windows compile problem #Messages
Total messages: 44 (0 generated)
lgtm https://codereview.chromium.org/20070002/diff/2001/chrome/browser/ui/webui/lo... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h (right): https://codereview.chromium.org/20070002/diff/2001/chrome/browser/ui/webui/lo... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:19: // UI Handler for chrome://devices/ you can put this class in local_discovery namespace https://codereview.chromium.org/20070002/diff/2001/chrome/browser/ui/webui/lo... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:72: scoped_ptr<local_discovery::PrivetHTTPClient> current_http_client_; and remove local_discovery::
lgtm
https://codereview.chromium.org/20070002/diff/2001/chrome/browser/ui/webui/lo... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h (right): https://codereview.chromium.org/20070002/diff/2001/chrome/browser/ui/webui/lo... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:19: // UI Handler for chrome://devices/ On 2013/07/24 01:27:54, Vitaly Buka wrote: > you can put this class in local_discovery namespace Done. https://codereview.chromium.org/20070002/diff/2001/chrome/browser/ui/webui/lo... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:72: scoped_ptr<local_discovery::PrivetHTTPClient> current_http_client_; On 2013/07/24 01:27:54, Vitaly Buka wrote: > and remove local_discovery:: Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/20070002/13001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/20070002/13001
Sorry for I got bad news for ya. Compile failed with a clobber build on mac. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
On 2013/07/25 18:00:28, I haz the power (commit-bot) wrote: > Sorry for I got bad news for ya. > Compile failed with a clobber build on mac. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number... > Your code is likely broken or HEAD is junk. Please ensure your > code is not broken then alert the build sheriffs. > Look at the try server FAQ for more details. Looks like the devices page was not gated based on enable_mdns=1, added gating to devices page is not built if mdns is not enabled.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/20070002/43001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/20070002/43001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
+jhawkins for web_ui_controller_factory
https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... File chrome/browser/resources/local_discovery/local_discovery.css (right): https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.css:18: background-color: #f0f0f0; nit: Indentation is off; should be two spaces. https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... File chrome/browser/resources/local_discovery/local_discovery.html (right): https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.html:16: <div id="infoConsole"> </div> nit: IDs should be dash-case. https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.html:16: <div id="infoConsole"> </div> nit: Remove space in div. https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.html:19: <th>Name</th> These strings need to be localized. https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... File chrome/browser/resources/local_discovery/local_discovery.js (right): https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.js:15: * @param {string} name of the device service. nit: The format of the parameter doc should be: @param {string} name The name of the device service. https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.js:16: * @param {string} info - additional info of the device, nit: Remove '-' character and capitalize the doc sentence. https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.js:17: * if empty device need to be deleted. nit: Wrapped documentation should be indented 4 spaces only. https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.js:65: button.textContent = 'Register'; Needs to be localized. https://codereview.chromium.org/20070002/diff/43001/chrome/browser/ui/webui/l... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h (right): https://codereview.chromium.org/20070002/diff/43001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:32: // WebUIMessageHandler implementation: Please revert this change. Comments must end with a period, including interface implementation. Here and PrivetRegisterOperation::Delegate and PrivetDeviceListener::Delegate.
https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... File chrome/browser/resources/local_discovery/local_discovery.html (right): https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.html:19: <th>Name</th> On 2013/07/29 16:27:03, James Hawkins wrote: > These strings need to be localized. This is a provisional UI behind a flag. We plan to localize once we get UX signoff.
https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... File chrome/browser/resources/local_discovery/local_discovery.html (right): https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.html:19: <th>Name</th> On 2013/07/29 16:58:01, Noam Samuel wrote: > On 2013/07/29 16:27:03, James Hawkins wrote: > > These strings need to be localized. > > This is a provisional UI behind a flag. We plan to localize once we get UX > signoff. Let's assume you're going to get signoff (otherwise why bother doing any of this?) and add the proper localization.
https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... File chrome/browser/resources/local_discovery/local_discovery.css (right): https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.css:18: background-color: #f0f0f0; On 2013/07/29 16:27:03, James Hawkins wrote: > nit: Indentation is off; should be two spaces. Done. https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... File chrome/browser/resources/local_discovery/local_discovery.html (right): https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.html:16: <div id="infoConsole"> </div> On 2013/07/29 16:27:03, James Hawkins wrote: > nit: IDs should be dash-case. Done. https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.html:19: <th>Name</th> On 2013/07/29 17:01:57, James Hawkins wrote: > On 2013/07/29 16:58:01, Noam Samuel wrote: > > On 2013/07/29 16:27:03, James Hawkins wrote: > > > These strings need to be localized. > > > > This is a provisional UI behind a flag. We plan to localize once we get UX > > signoff. > > Let's assume you're going to get signoff (otherwise why bother doing any of > this?) and add the proper localization. Done. https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... File chrome/browser/resources/local_discovery/local_discovery.js (right): https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.js:15: * @param {string} name of the device service. On 2013/07/29 16:27:03, James Hawkins wrote: > nit: The format of the parameter doc should be: > > @param {string} name The name of the device service. Done. https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.js:16: * @param {string} info - additional info of the device, On 2013/07/29 16:27:03, James Hawkins wrote: > nit: Remove '-' character and capitalize the doc sentence. Done. https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.js:17: * if empty device need to be deleted. On 2013/07/29 16:27:03, James Hawkins wrote: > nit: Wrapped documentation should be indented 4 spaces only. Done. https://codereview.chromium.org/20070002/diff/43001/chrome/browser/ui/webui/l... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h (right): https://codereview.chromium.org/20070002/diff/43001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:32: // WebUIMessageHandler implementation: On 2013/07/29 16:27:03, James Hawkins wrote: > Please revert this change. Comments must end with a period, including interface > implementation. Here and PrivetRegisterOperation::Delegate and > PrivetDeviceListener::Delegate. Done.
On 2013/08/02 22:20:02, Noam Samuel wrote: > https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... > File chrome/browser/resources/local_discovery/local_discovery.css (right): > > https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... > chrome/browser/resources/local_discovery/local_discovery.css:18: > background-color: #f0f0f0; > On 2013/07/29 16:27:03, James Hawkins wrote: > > nit: Indentation is off; should be two spaces. > > Done. > > https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... > File chrome/browser/resources/local_discovery/local_discovery.html (right): > > https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... > chrome/browser/resources/local_discovery/local_discovery.html:16: <div > id="infoConsole"> </div> > On 2013/07/29 16:27:03, James Hawkins wrote: > > nit: IDs should be dash-case. > > Done. > > https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... > chrome/browser/resources/local_discovery/local_discovery.html:19: <th>Name</th> > On 2013/07/29 17:01:57, James Hawkins wrote: > > On 2013/07/29 16:58:01, Noam Samuel wrote: > > > On 2013/07/29 16:27:03, James Hawkins wrote: > > > > These strings need to be localized. > > > > > > This is a provisional UI behind a flag. We plan to localize once we get UX > > > signoff. > > > > Let's assume you're going to get signoff (otherwise why bother doing any of > > this?) and add the proper localization. > > Done. > > https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... > File chrome/browser/resources/local_discovery/local_discovery.js (right): > > https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... > chrome/browser/resources/local_discovery/local_discovery.js:15: * @param > {string} name of the device service. > On 2013/07/29 16:27:03, James Hawkins wrote: > > nit: The format of the parameter doc should be: > > > > @param {string} name The name of the device service. > > Done. > > https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... > chrome/browser/resources/local_discovery/local_discovery.js:16: * @param > {string} info - additional info of the device, > On 2013/07/29 16:27:03, James Hawkins wrote: > > nit: Remove '-' character and capitalize the doc sentence. > > Done. > > https://codereview.chromium.org/20070002/diff/43001/chrome/browser/resources/... > chrome/browser/resources/local_discovery/local_discovery.js:17: * > if empty device need to be deleted. > On 2013/07/29 16:27:03, James Hawkins wrote: > > nit: Wrapped documentation should be indented 4 spaces only. > > Done. > > https://codereview.chromium.org/20070002/diff/43001/chrome/browser/ui/webui/l... > File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h > (right): > > https://codereview.chromium.org/20070002/diff/43001/chrome/browser/ui/webui/l... > chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:32: // > WebUIMessageHandler implementation: > On 2013/07/29 16:27:03, James Hawkins wrote: > > Please revert this change. Comments must end with a period, including > interface > > implementation. Here and PrivetRegisterOperation::Delegate and > > PrivetDeviceListener::Delegate. > > Done. JHawkins, could you please take another look?
I'm on leave till Aug. 16.
DBeam, could you take a look?
https://codereview.chromium.org/20070002/diff/78001/chrome/browser/resources/... File chrome/browser/resources/local_discovery/local_discovery.js (right): https://codereview.chromium.org/20070002/diff/78001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.js:4: nit: cr.namespace('local_discovery', function() { ... return { registrationFailed: registrationFailed, registrationSuccess: registrationSuccess, }; }); https://codereview.chromium.org/20070002/diff/78001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.js:17: * be deleted. nit: 4 \s wrapping indent * @param {string} info Additional info of the device, if empty device need to * be deleted. https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc (right): https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:57: data->GetString(0, &device_name); nit: CHECK() or bool rv = ...; DCHECK(rv); etc. https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:59: nit: what if |device_name| isn't in |device_descriptions_|? https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:150: LOG(INFO) << "Confirm success."; nit: DLOG https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:170: if (!description.ip_address.empty()) { nit: no curlies https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:187: name_value, *null_value.get()); nit: don't think you need the .get() https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:193: LOG(ERROR) << error; nit: DLOG https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:199: LOG(INFO) << "Registered " << id; nit: DLOG https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h (right): https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:38: nit: remove \n https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:43: const DictionaryValue* json) OVERRIDE; nit: remove \n https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:57: void OnRegisterDevice(const base::ListValue* data); nit: HandleRegister https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:59: void OnStart(const base::ListValue* value); nit: HandleStart https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:73: doc comments for all these members https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:85: device_descriptions_; nit: looks like this would fit in 80 cols https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:92: } // namespace local_discovery nit: 2 spaces, e.g. } // namespace local_discovery
https://codereview.chromium.org/20070002/diff/78001/chrome/browser/resources/... File chrome/browser/resources/local_discovery/local_discovery.js (right): https://codereview.chromium.org/20070002/diff/78001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.js:4: On 2013/08/06 21:21:38, Dan Beam wrote: > nit: cr.namespace('local_discovery', function() { > > ... > > return { > registrationFailed: registrationFailed, > registrationSuccess: registrationSuccess, > }; > }); cr.define**
https://codereview.chromium.org/20070002/diff/78001/chrome/browser/resources/... File chrome/browser/resources/local_discovery/local_discovery.js (right): https://codereview.chromium.org/20070002/diff/78001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.js:4: On 2013/08/06 21:23:18, Dan Beam wrote: > On 2013/08/06 21:21:38, Dan Beam wrote: > > nit: cr.namespace('local_discovery', function() { > > > > ... > > > > return { > > registrationFailed: registrationFailed, > > registrationSuccess: registrationSuccess, > > }; > > }); > > cr.define** Done. https://codereview.chromium.org/20070002/diff/78001/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.js:17: * be deleted. On 2013/08/06 21:21:38, Dan Beam wrote: > nit: 4 \s wrapping indent > > * @param {string} info Additional info of the device, if empty device need to > * be deleted. Done. https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc (right): https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:57: data->GetString(0, &device_name); On 2013/08/06 21:21:38, Dan Beam wrote: > nit: CHECK() or bool rv = ...; DCHECK(rv); etc. Done. https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:59: On 2013/08/06 21:21:38, Dan Beam wrote: > nit: what if |device_name| isn't in |device_descriptions_|? Hm. I shouldn't actually be looking this up in the map, since if the domain is still valid in the cache, resolving it will cause no network hit and will be more accurate (due to things like expiration). https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:150: LOG(INFO) << "Confirm success."; On 2013/08/06 21:21:38, Dan Beam wrote: > nit: DLOG Done. https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:170: if (!description.ip_address.empty()) { On 2013/08/06 21:21:38, Dan Beam wrote: > nit: no curlies Done. https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:187: name_value, *null_value.get()); On 2013/08/06 21:21:38, Dan Beam wrote: > nit: don't think you need the .get() Done. https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:193: LOG(ERROR) << error; On 2013/08/06 21:21:38, Dan Beam wrote: > nit: DLOG Done. https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:199: LOG(INFO) << "Registered " << id; On 2013/08/06 21:21:38, Dan Beam wrote: > nit: DLOG Done. https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h (right): https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:38: On 2013/08/06 21:21:38, Dan Beam wrote: > nit: remove \n Done. https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:43: const DictionaryValue* json) OVERRIDE; On 2013/08/06 21:21:38, Dan Beam wrote: > nit: remove \n Done. https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:57: void OnRegisterDevice(const base::ListValue* data); On 2013/08/06 21:21:38, Dan Beam wrote: > nit: HandleRegister Done. https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:59: void OnStart(const base::ListValue* value); On 2013/08/06 21:21:38, Dan Beam wrote: > nit: HandleStart Done. https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:73: On 2013/08/06 21:21:38, Dan Beam wrote: > doc comments for all these members Done. https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:85: device_descriptions_; On 2013/08/06 21:21:38, Dan Beam wrote: > nit: looks like this would fit in 80 cols Done. https://codereview.chromium.org/20070002/diff/78001/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:92: } // namespace local_discovery On 2013/08/06 21:21:38, Dan Beam wrote: > nit: 2 spaces, e.g. > > } // namespace local_discovery Done.
https://codereview.chromium.org/20070002/diff/90002/chrome/browser/resources/... File chrome/browser/resources/local_discovery/local_discovery.js (right): https://codereview.chromium.org/20070002/diff/90002/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.js:13: cr.define('local_discovery', function() { nit: 'use strict'; https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc (right): https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:42: &LocalDiscoveryUIHandler::HandleRegisterDevice, nit: Handle + "callbackName" == HandleCallbackName, so either HandleRegister or "registerDevice" https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:46: void LocalDiscoveryUIHandler::HandleStart(const base::ListValue* value) { nit: |value| -> |args| https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:56: const base::ListValue* data) { nit: |data| -> |args| https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:61: nit: because accessing device_descriptions_["non-existent-field"] will create new DeviceDescription (and this struct has a default ctor), can this code: DCHECK_NE(0U, device_descriptions_.count(device_name)); here and anywhere else unless this is valid behavior... https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:182: info.SetString("metadata", ""); ^ what's the point of this field? https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h (right): https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:27: public PrivetDeviceLister::Delegate { nit: class LocalDiscoveryUIHandler : public content::WebUIMessageHandler, public PrivetRegisterOperation::Delegate, public PrivetDeviceLister::Delegate { https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:55: void HandleRegisterDevice(const base::ListValue* data); nit: s/data/args/ https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:57: void HandleStart(const base::ListValue* value); nit: s/value/args/ https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:73: scoped_ptr<PrivetHTTPClient> current_http_client_; nit: \n between each https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:76: current_register_operation_; nit: can fit in 80 cols https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:85: service_discovery_client_; nit: can fit in 80 cols https://codereview.chromium.org/20070002/diff/90002/chrome/chrome_browser_ui.... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/20070002/diff/90002/chrome/chrome_browser_ui.... chrome/chrome_browser_ui.gypi:3084: 'sources': [ nit: indent off by 2 https://codereview.chromium.org/20070002/diff/90002/chrome/chrome_browser_ui.... chrome/chrome_browser_ui.gypi:3089: ] nit: ], https://codereview.chromium.org/20070002/diff/90002/chrome/chrome_browser_ui.... chrome/chrome_browser_ui.gypi:3090: }] nit: }],
https://codereview.chromium.org/20070002/diff/90002/chrome/browser/resources/... File chrome/browser/resources/local_discovery/local_discovery.js (right): https://codereview.chromium.org/20070002/diff/90002/chrome/browser/resources/... chrome/browser/resources/local_discovery/local_discovery.js:13: cr.define('local_discovery', function() { On 2013/08/07 00:51:28, Dan Beam wrote: > nit: 'use strict'; Done. https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc (right): https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:42: &LocalDiscoveryUIHandler::HandleRegisterDevice, On 2013/08/07 00:51:28, Dan Beam wrote: > nit: Handle + "callbackName" == HandleCallbackName, so either HandleRegister or > "registerDevice" Done. https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:46: void LocalDiscoveryUIHandler::HandleStart(const base::ListValue* value) { On 2013/08/07 00:51:28, Dan Beam wrote: > nit: |value| -> |args| Done. https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:56: const base::ListValue* data) { On 2013/08/07 00:51:28, Dan Beam wrote: > nit: |data| -> |args| Done. https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:61: On 2013/08/07 00:51:28, Dan Beam wrote: > nit: because accessing device_descriptions_["non-existent-field"] will create > new DeviceDescription (and this struct has a default ctor), can this code: > > DCHECK_NE(0U, device_descriptions_.count(device_name)); > > here and anywhere else unless this is valid behavior... It's actually valid, since a device can actually disappear while you're trying to register it without it being considered incorrect program behavior if its record times out or it sends a goodbye packet. https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:182: info.SetString("metadata", ""); On 2013/08/07 00:51:28, Dan Beam wrote: > ^ what's the point of this field? Was reserved for extra metadata in the TXT field, but it's actually not used for that at the moment, and based on the way further development on this has turned out, it seems unlikely it will be. I'll remove it. https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h (right): https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:27: public PrivetDeviceLister::Delegate { On 2013/08/07 00:51:28, Dan Beam wrote: > nit: > class LocalDiscoveryUIHandler : public content::WebUIMessageHandler, > public PrivetRegisterOperation::Delegate, > public PrivetDeviceLister::Delegate { Done. https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:55: void HandleRegisterDevice(const base::ListValue* data); On 2013/08/07 00:51:28, Dan Beam wrote: > nit: s/data/args/ Done. https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:57: void HandleStart(const base::ListValue* value); On 2013/08/07 00:51:28, Dan Beam wrote: > nit: s/value/args/ Done. https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:73: scoped_ptr<PrivetHTTPClient> current_http_client_; On 2013/08/07 00:51:28, Dan Beam wrote: > nit: \n between each Done. https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:76: current_register_operation_; On 2013/08/07 00:51:28, Dan Beam wrote: > nit: can fit in 80 cols Done. https://codereview.chromium.org/20070002/diff/90002/chrome/browser/ui/webui/l... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:85: service_discovery_client_; On 2013/08/07 00:51:28, Dan Beam wrote: > nit: can fit in 80 cols Done. https://codereview.chromium.org/20070002/diff/90002/chrome/chrome_browser_ui.... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/20070002/diff/90002/chrome/chrome_browser_ui.... chrome/chrome_browser_ui.gypi:3084: 'sources': [ On 2013/08/07 00:51:28, Dan Beam wrote: > nit: indent off by 2 Done. https://codereview.chromium.org/20070002/diff/90002/chrome/chrome_browser_ui.... chrome/chrome_browser_ui.gypi:3089: ] On 2013/08/07 00:51:28, Dan Beam wrote: > nit: ], Done. https://codereview.chromium.org/20070002/diff/90002/chrome/chrome_browser_ui.... chrome/chrome_browser_ui.gypi:3090: }] On 2013/08/07 00:51:28, Dan Beam wrote: > nit: }], Done.
tests? https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... File chrome/browser/resources/local_discovery/local_discovery.js (right): https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... chrome/browser/resources/local_discovery/local_discovery.js:34: if (row.cells[0].textContent == name) { ^ this can be buggy, btw, as (blah.textContent = a) != a // sometimes true but this tends to only happen when \n or \r are involved https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... chrome/browser/resources/local_discovery/local_discovery.js:76: } ^ do you have any tests for this code? https://codereview.chromium.org/20070002/diff/125001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc (right): https://codereview.chromium.org/20070002/diff/125001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:8: #include "base/message_loop/message_loop.h" ^ where is this used?
On 2013/08/07 18:43:03, Dan Beam wrote: > tests? > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... > File chrome/browser/resources/local_discovery/local_discovery.js (right): > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... > chrome/browser/resources/local_discovery/local_discovery.js:34: if > (row.cells[0].textContent == name) { > ^ this can be buggy, btw, as > > (blah.textContent = a) != a // sometimes true > > but this tends to only happen when \n or \r are involved > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... > chrome/browser/resources/local_discovery/local_discovery.js:76: } > ^ do you have any tests for this code? > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/ui/webui/... > File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc > (right): > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/ui/webui/... > chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:8: > #include "base/message_loop/message_loop.h" > ^ where is this used? We don't have tests primarily because we expect to rewrite this UI code, but are using it to bring together the end-to-end of MDns and local discovery with actual printers, debug issues, and then refactor it into smaller testable components.
On 2013/08/07 19:01:54, Noam Samuel wrote: > On 2013/08/07 18:43:03, Dan Beam wrote: > > tests? > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... > > File chrome/browser/resources/local_discovery/local_discovery.js (right): > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... > > chrome/browser/resources/local_discovery/local_discovery.js:34: if > > (row.cells[0].textContent == name) { > > ^ this can be buggy, btw, as > > > > (blah.textContent = a) != a // sometimes true > > > > but this tends to only happen when \n or \r are involved > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... > > chrome/browser/resources/local_discovery/local_discovery.js:76: } > > ^ do you have any tests for this code? > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/ui/webui/... > > File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc > > (right): > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/ui/webui/... > > chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:8: > > #include "base/message_loop/message_loop.h" > > ^ where is this used? > > We don't have tests primarily because we expect to rewrite this UI code, but are > using it to bring together the end-to-end of MDns and local discovery with > actual printers, debug issues, and then refactor it into smaller testable > components. Where's the refactoring CL?
On 2013/08/07 19:05:50, Dan Beam wrote: > On 2013/08/07 19:01:54, Noam Samuel wrote: > > On 2013/08/07 18:43:03, Dan Beam wrote: > > > tests? > > > > > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... > > > File chrome/browser/resources/local_discovery/local_discovery.js (right): > > > > > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... > > > chrome/browser/resources/local_discovery/local_discovery.js:34: if > > > (row.cells[0].textContent == name) { > > > ^ this can be buggy, btw, as > > > > > > (blah.textContent = a) != a // sometimes true > > > > > > but this tends to only happen when \n or \r are involved > > > > > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... > > > chrome/browser/resources/local_discovery/local_discovery.js:76: } > > > ^ do you have any tests for this code? > > > > > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/ui/webui/... > > > File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/ui/webui/... > > > chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:8: > > > #include "base/message_loop/message_loop.h" > > > ^ where is this used? > > > > We don't have tests primarily because we expect to rewrite this UI code, but > are > > using it to bring together the end-to-end of MDns and local discovery with > > actual printers, debug issues, and then refactor it into smaller testable > > components. > > Where's the refactoring CL? Right now I'm working on moving the resolve-and-create-HTTP-object flow into an asynchronous factory that can be mocked, which is the first refactoring CL. Haven't yet uploaded it, though.
On 2013/08/07 19:05:50, Dan Beam wrote: > On 2013/08/07 19:01:54, Noam Samuel wrote: > > On 2013/08/07 18:43:03, Dan Beam wrote: > > > tests? > > > > > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... > > > File chrome/browser/resources/local_discovery/local_discovery.js (right): > > > > > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... > > > chrome/browser/resources/local_discovery/local_discovery.js:34: if > > > (row.cells[0].textContent == name) { > > > ^ this can be buggy, btw, as > > > > > > (blah.textContent = a) != a // sometimes true > > > > > > but this tends to only happen when \n or \r are involved > > > > > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... > > > chrome/browser/resources/local_discovery/local_discovery.js:76: } > > > ^ do you have any tests for this code? > > > > > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/ui/webui/... > > > File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/ui/webui/... > > > chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:8: > > > #include "base/message_loop/message_loop.h" > > > ^ where is this used? > > > > We don't have tests primarily because we expect to rewrite this UI code, but > are > > using it to bring together the end-to-end of MDns and local discovery with > > actual printers, debug issues, and then refactor it into smaller testable > > components. > > Where's the refactoring CL? How about this -- why don't we write a minimal set of tests for now (shouldn't take very long) as you want to distribute this code to a minimal set of users (because it's behind a flag). Really I just want to test the complex C++/JS/HTML interaction in onServiceUpdate (mainly that table rows/cells are created or destroyed correctly from a device change/removal). Does this sound like a reasonable compromise?
On 2013/08/07 19:11:55, Noam Samuel wrote: > On 2013/08/07 19:05:50, Dan Beam wrote: > > On 2013/08/07 19:01:54, Noam Samuel wrote: > > > On 2013/08/07 18:43:03, Dan Beam wrote: > > > > tests? > > > > > > > > > > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... > > > > File chrome/browser/resources/local_discovery/local_discovery.js (right): > > > > > > > > > > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... > > > > chrome/browser/resources/local_discovery/local_discovery.js:34: if > > > > (row.cells[0].textContent == name) { > > > > ^ this can be buggy, btw, as > > > > > > > > (blah.textContent = a) != a // sometimes true > > > > > > > > but this tends to only happen when \n or \r are involved > > > > > > > > > > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... > > > > chrome/browser/resources/local_discovery/local_discovery.js:76: } > > > > ^ do you have any tests for this code? > > > > > > > > > > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/ui/webui/... > > > > File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/ui/webui/... > > > > chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:8: > > > > #include "base/message_loop/message_loop.h" > > > > ^ where is this used? > > > > > > We don't have tests primarily because we expect to rewrite this UI code, but > > are > > > using it to bring together the end-to-end of MDns and local discovery with > > > actual printers, debug issues, and then refactor it into smaller testable > > > components. > > > > Where's the refactoring CL? > > Right now I'm working on moving the resolve-and-create-HTTP-object flow into an > asynchronous factory that can be mocked, which is the first refactoring CL. > Haven't yet uploaded it, though. Good, this shouldn't really affect the area I was hoping you're going to test, AFAICT.
Added browser test for adding/removing devices. https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... File chrome/browser/resources/local_discovery/local_discovery.js (right): https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... chrome/browser/resources/local_discovery/local_discovery.js:34: if (row.cells[0].textContent == name) { On 2013/08/07 18:43:03, Dan Beam wrote: > ^ this can be buggy, btw, as > > (blah.textContent = a) != a // sometimes true > > but this tends to only happen when \n or \r are involved For now added line to enforce lack of '\r' and '\n' in name, but eventually we should move to a better method for removing/updating rows. https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... chrome/browser/resources/local_discovery/local_discovery.js:76: } On 2013/08/07 18:43:03, Dan Beam wrote: > ^ do you have any tests for this code? Note that this function is mostly rewritten in a DIFFBASE-ed CL. https://codereview.chromium.org/20504002. https://codereview.chromium.org/20070002/diff/125001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc (right): https://codereview.chromium.org/20070002/diff/125001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:8: #include "base/message_loop/message_loop.h" On 2013/08/07 18:43:03, Dan Beam wrote: > ^ where is this used? Removed.
DBeam, could you PTAL? On 2013/08/08 18:45:36, Noam Samuel wrote: > Added browser test for adding/removing devices. > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... > File chrome/browser/resources/local_discovery/local_discovery.js (right): > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... > chrome/browser/resources/local_discovery/local_discovery.js:34: if > (row.cells[0].textContent == name) { > On 2013/08/07 18:43:03, Dan Beam wrote: > > ^ this can be buggy, btw, as > > > > (blah.textContent = a) != a // sometimes true > > > > but this tends to only happen when \n or \r are involved > > For now added line to enforce lack of '\r' and '\n' in name, but eventually we > should move to a better method for removing/updating rows. > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/resources... > chrome/browser/resources/local_discovery/local_discovery.js:76: } > On 2013/08/07 18:43:03, Dan Beam wrote: > > ^ do you have any tests for this code? > > Note that this function is mostly rewritten in a DIFFBASE-ed CL. > https://codereview.chromium.org/20504002. > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/ui/webui/... > File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc > (right): > > https://codereview.chromium.org/20070002/diff/125001/chrome/browser/ui/webui/... > chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.cc:8: > #include "base/message_loop/message_loop.h" > On 2013/08/07 18:43:03, Dan Beam wrote: > > ^ where is this used? > > Removed.
https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc (right): https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:6: #include "base/command_line.h" base/message_loop/message_loop.h https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:29: if (waiting_) { nit: no curlies https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:44: discover_devices_called_(discover_devices_called) { FakePrivetDeviceLister::FakePrivetDeviceLister( const base::Closure& discover_deviced_called) : discover_devices_called_(discover_devices_called) { https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:59: owned_privet_lister_.swap(privet_lister); nit: why not owned_privet_lister_ = privet_lister.Pass(); ? seems to make more sense at first glance as you don't use |privet_lister| again here. https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:61: LocalDiscoveryUIHandler::SetFactory(this); indent off https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:65: LocalDiscoveryUIHandler::SetFactory(NULL); indent off https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:106: ASSERT_TRUE(WebUIBrowserTest::RunJavascriptTest("checkTableHasNoRows")); nit: EXPECT_TRUE() (ASSERT_TRUE() should be used to prevent meaningless code from running after it) https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.h (right): https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. where else is are these classes used? why does a browsertest need a .h? generally we just put everything together in a .cc file for this kind of thing. https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.h:7: base/basictypes.h (for DISALLOW_*) base/compiler_specific.h (for OVERRIDE) base/callback.h (for base::Closure) base/memory/scoped_ptr.h https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.h:19: void Wait(); nit: \n https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.h:22: bool waiting_; DISALLOW_COPY_AND_ASSIGN() on all relevant classes https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.h:53: FakePrivetDeviceLister* privet_lister_; ^ denote ownership model differences and how |owned_privet_lister_| is used https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.h:67: TestMessageLoopCondition condition_devices_listed_; private with accessors
https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc (right): https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:29: if (waiting_) { On 2013/08/12 22:33:52, Dan Beam wrote: > nit: no curlies Done. https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:44: discover_devices_called_(discover_devices_called) { On 2013/08/12 22:33:52, Dan Beam wrote: > FakePrivetDeviceLister::FakePrivetDeviceLister( > const base::Closure& discover_deviced_called) > : discover_devices_called_(discover_devices_called) { Done. https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:59: owned_privet_lister_.swap(privet_lister); On 2013/08/12 22:33:52, Dan Beam wrote: > nit: why not > > owned_privet_lister_ = privet_lister.Pass(); > > ? seems to make more sense at first glance as you don't use |privet_lister| > again here. It is used in the accessor privet_lister(). https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:65: LocalDiscoveryUIHandler::SetFactory(NULL); On 2013/08/12 22:33:52, Dan Beam wrote: > indent off Done. https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:106: ASSERT_TRUE(WebUIBrowserTest::RunJavascriptTest("checkTableHasNoRows")); On 2013/08/12 22:33:52, Dan Beam wrote: > nit: EXPECT_TRUE() (ASSERT_TRUE() should be used to prevent meaningless code > from running after it) Done. https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.h (right): https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2013/08/12 22:33:52, Dan Beam wrote: > where else is are these classes used? why does a browsertest need a .h? > generally we just put everything together in a .cc file for this kind of thing. Done. https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.h:7: On 2013/08/12 22:33:52, Dan Beam wrote: > base/basictypes.h (for DISALLOW_*) > base/compiler_specific.h (for OVERRIDE) > base/callback.h (for base::Closure) > base/memory/scoped_ptr.h Done. https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.h:19: void Wait(); On 2013/08/12 22:33:52, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.h:19: void Wait(); On 2013/08/12 22:33:52, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.h:22: bool waiting_; On 2013/08/12 22:33:52, Dan Beam wrote: > DISALLOW_COPY_AND_ASSIGN() on all relevant classes Done. https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.h:53: FakePrivetDeviceLister* privet_lister_; On 2013/08/12 22:33:52, Dan Beam wrote: > ^ denote ownership model differences and how |owned_privet_lister_| is used Done. https://codereview.chromium.org/20070002/diff/139001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.h:67: TestMessageLoopCondition condition_devices_listed_; On 2013/08/12 22:33:52, Dan Beam wrote: > private with accessors Done.
very very close, thanks for your patience https://codereview.chromium.org/20070002/diff/149001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc (right): https://codereview.chromium.org/20070002/diff/149001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:32: void Wait(); doc comments https://codereview.chromium.org/20070002/diff/149001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:35: bool set_; nit: signaled_ or signaling_ https://codereview.chromium.org/20070002/diff/149001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:55: base::Closure discover_devices_called_; indent off https://codereview.chromium.org/20070002/diff/149001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:65: virtual ~FakeLocalDiscoveryUIFactory(); indent off https://codereview.chromium.org/20070002/diff/149001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:126: base::Closure discover_devices_called) nit: const-ref, e.g. const base::Closure& discover_devices_called) https://codereview.chromium.org/20070002/diff/149001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:184: } instead of class ClassName { Method(...) OVERRIDE; }; and then later ClassName::Method(...) { ... } just do class ClassName { MethodImpl(...) OVERRIDE { ... } }; as nobody includes this code in their headers (so inlining doesn't create cruft). this is the most common practice in browser tests, AFAIK. https://codereview.chromium.org/20070002/diff/149001/chrome/test/data/webui/l... File chrome/test/data/webui/local_discovery_ui_test.js (right): https://codereview.chromium.org/20070002/diff/149001/chrome/test/data/webui/l... chrome/test/data/webui/local_discovery_ui_test.js:1: var $ = document.getElementById.bind(document); needs copyright https://codereview.chromium.org/20070002/diff/149001/chrome/test/data/webui/l... chrome/test/data/webui/local_discovery_ui_test.js:1: var $ = document.getElementById.bind(document); nit: what you have now is more generic but you seem to use it for the same purpose both times, an alternative function rows() { return document.querySelectorAll('#devices-table tr'); } https://codereview.chromium.org/20070002/diff/149001/chrome/test/data/webui/l... chrome/test/data/webui/local_discovery_ui_test.js:6: var firstRow = rows.item(1); nit: \n before every "td ="
https://codereview.chromium.org/20070002/diff/149001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc (right): https://codereview.chromium.org/20070002/diff/149001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:32: void Wait(); On 2013/08/13 00:30:43, Dan Beam wrote: > doc comments Done. https://codereview.chromium.org/20070002/diff/149001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:35: bool set_; On 2013/08/13 00:30:43, Dan Beam wrote: > nit: signaled_ or signaling_ Done. https://codereview.chromium.org/20070002/diff/149001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:55: base::Closure discover_devices_called_; On 2013/08/13 00:30:43, Dan Beam wrote: > indent off Done. https://codereview.chromium.org/20070002/diff/149001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:65: virtual ~FakeLocalDiscoveryUIFactory(); On 2013/08/13 00:30:43, Dan Beam wrote: > indent off Done. https://codereview.chromium.org/20070002/diff/149001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:126: base::Closure discover_devices_called) On 2013/08/13 00:30:43, Dan Beam wrote: > nit: const-ref, e.g. > > const base::Closure& discover_devices_called) Done. https://codereview.chromium.org/20070002/diff/149001/chrome/browser/ui/webui/... chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc:184: } On 2013/08/13 00:30:43, Dan Beam wrote: > instead of > > class ClassName { > Method(...) OVERRIDE; > }; > > and then later > > ClassName::Method(...) { > ... > } > > just do > > class ClassName { > MethodImpl(...) OVERRIDE { > ... > } > }; > > as nobody includes this code in their headers (so inlining doesn't create > cruft). this is the most common practice in browser tests, AFAIK. Done. https://codereview.chromium.org/20070002/diff/149001/chrome/test/data/webui/l... File chrome/test/data/webui/local_discovery_ui_test.js (right): https://codereview.chromium.org/20070002/diff/149001/chrome/test/data/webui/l... chrome/test/data/webui/local_discovery_ui_test.js:1: var $ = document.getElementById.bind(document); On 2013/08/13 00:30:43, Dan Beam wrote: > nit: what you have now is more generic but you seem to use it for the same > purpose both times, an alternative > > function rows() { > return document.querySelectorAll('#devices-table tr'); > } Done. https://codereview.chromium.org/20070002/diff/149001/chrome/test/data/webui/l... chrome/test/data/webui/local_discovery_ui_test.js:6: var firstRow = rows.item(1); On 2013/08/13 00:30:43, Dan Beam wrote: > nit: \n before every "td =" Done.
lgtm w/fix https://chromiumcodereview.appspot.com/20070002/diff/161001/chrome/browser/re... File chrome/browser/resources/local_discovery/local_discovery.js (right): https://chromiumcodereview.appspot.com/20070002/diff/161001/chrome/browser/re... chrome/browser/resources/local_discovery/local_discovery.js:23: name = name.replace('\r', '').replace('\n', ''); this doesn't replace all encountered \r or \n :( this does: name = name.replace(/[\r\n]/g, '');
https://chromiumcodereview.appspot.com/20070002/diff/161001/chrome/browser/re... File chrome/browser/resources/local_discovery/local_discovery.js (right): https://chromiumcodereview.appspot.com/20070002/diff/161001/chrome/browser/re... chrome/browser/resources/local_discovery/local_discovery.js:23: name = name.replace('\r', '').replace('\n', ''); On 2013/08/14 20:43:02, Dan Beam wrote: > this doesn't replace all encountered \r or \n :( > > this does: > > name = name.replace(/[\r\n]/g, ''); Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/20070002/178001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/20070002/203001
Sorry for I got bad news for ya. Compile failed with a clobber build on win_x64_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
On 2013/08/15 00:29:52, I haz the power (commit-bot) wrote: > Sorry for I got bad news for ya. > Compile failed with a clobber build on win_x64_rel. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re... > Your code is likely broken or HEAD is junk. Please ensure your > code is not broken then alert the build sheriffs. > Look at the try server FAQ for more details. Whoops. Fixed lack of FILE_PATH_LITERAL macro.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/20070002/222001
Message was sent while issue was closed.
Change committed as 217748 |
