Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
139923002:
Added VLOG(1) loggings to mdns extensions API code (Closed)
|
Created:
6 years, 11 months ago by imcheng Modified:
6 years, 11 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdded VLOG(1) loggings to mdns extensions API code to help debug.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245267
Patch Set 1 #Patch Set 2 : More logging #Patch Set 3 : #
Total comments: 1
Patch Set 4 #
Total comments: 16
Patch Set 5 : Addressed comments #
Total comments: 29
Patch Set 6 : Addressed second set of comments #
Total comments: 4
Patch Set 7 : Addressed mfoltz's comments #
Messages
Total messages: 15 (0 generated)
Please let me know if it will be helpful to add more logging in other places, thanks. The plan is to submit this and to do testing once it makes it to canary. https://codereview.chromium.org/139923002/diff/50001/chrome/browser/extension... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/139923002/diff/50001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:93: for (base::DictionaryValue::Iterator iter(*filter); What this is for and can it be removed?
most log messages unnecessary long. I believe they has no value for person who does not look in code anyway. can you please make them as shorter as possible? https://codereview.chromium.org/139923002/diff/70001/chrome/browser/extension... File chrome/browser/extensions/api/mdns/dns_sd_registry.cc (right): https://codereview.chromium.org/139923002/diff/70001/chrome/browser/extension... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:56: if (it != service_list_.end()) { How about just one log statement: VLOG(1) << "UpdateService: " << service.service_name << ", added=" << added << ", known=" << (it != service_list_.end()); https://codereview.chromium.org/139923002/diff/70001/chrome/browser/extension... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:133: VLOG(1) << "Registering listener of service type: " << service_type; Just one line in begining with input service_type and IsRegistered(service_type) should be enough https://codereview.chromium.org/139923002/diff/70001/chrome/browser/extension... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:171: VLOG(1) << "ServiceChanged: Service type: " << service_type same https://codereview.chromium.org/139923002/diff/70001/chrome/browser/extension... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:192: << " not registered; ignoring"; same https://codereview.chromium.org/139923002/diff/70001/chrome/browser/extension... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:208: VLOG(1) << "ServicesFlushed: Service type: " << service_type same https://codereview.chromium.org/139923002/diff/70001/chrome/browser/local_dis... File chrome/browser/local_discovery/service_discovery_device_lister.cc (right): https://codereview.chromium.org/139923002/diff/70001/chrome/browser/local_dis... chrome/browser/local_discovery/service_discovery_device_lister.cc:47: VLOG(1) << "In OnServiceUpdated(), service_type: " << service_type_ can be done with less log stataments https://codereview.chromium.org/139923002/diff/70001/chrome/browser/local_dis... chrome/browser/local_discovery/service_discovery_device_lister.cc:94: VLOG(1) << "In OnResolveComplete(), service_type: " << service_type_ can be one statement in beging
And why not DVLOG?
vitalybuka@ is probably better to review this change. https://codereview.chromium.org/139923002/diff/70001/chrome/browser/extension... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/139923002/diff/70001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:92: // TODO(imcheng): Figure out what this is loop for, maybe remove it. Don't add this TODO. It has no use to do code.
Per vitalybuka@'s request, used fewer and shorter loggings. Need to use VLOG because we are doing testing on canary build. https://codereview.chromium.org/139923002/diff/70001/chrome/browser/extension... File chrome/browser/extensions/api/mdns/dns_sd_registry.cc (right): https://codereview.chromium.org/139923002/diff/70001/chrome/browser/extension... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:56: if (it != service_list_.end()) { On 2014/01/15 22:11:42, Vitaly Buka wrote: > How about just one log statement: > > VLOG(1) << "UpdateService: " << service.service_name > << ", added=" << added > << ", known=" << (it != service_list_.end()); Done. https://codereview.chromium.org/139923002/diff/70001/chrome/browser/extension... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:133: VLOG(1) << "Registering listener of service type: " << service_type; On 2014/01/15 22:11:42, Vitaly Buka wrote: > Just one line in begining with input service_type and IsRegistered(service_type) > should be enough Done. https://codereview.chromium.org/139923002/diff/70001/chrome/browser/extension... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:171: VLOG(1) << "ServiceChanged: Service type: " << service_type On 2014/01/15 22:11:42, Vitaly Buka wrote: > same Done. https://codereview.chromium.org/139923002/diff/70001/chrome/browser/extension... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:192: << " not registered; ignoring"; On 2014/01/15 22:11:42, Vitaly Buka wrote: > same Done. https://codereview.chromium.org/139923002/diff/70001/chrome/browser/extension... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:208: VLOG(1) << "ServicesFlushed: Service type: " << service_type On 2014/01/15 22:11:42, Vitaly Buka wrote: > same Done. https://codereview.chromium.org/139923002/diff/70001/chrome/browser/extension... File chrome/browser/extensions/api/mdns/mdns_api.cc (right): https://codereview.chromium.org/139923002/diff/70001/chrome/browser/extension... chrome/browser/extensions/api/mdns/mdns_api.cc:92: // TODO(imcheng): Figure out what this is loop for, maybe remove it. On 2014/01/15 22:34:13, Alpha wrote: > Don't add this TODO. It has no use to do code. Ok, looks like this loop does nothing so I will remove it. https://codereview.chromium.org/139923002/diff/70001/chrome/browser/local_dis... File chrome/browser/local_discovery/service_discovery_device_lister.cc (right): https://codereview.chromium.org/139923002/diff/70001/chrome/browser/local_dis... chrome/browser/local_discovery/service_discovery_device_lister.cc:47: VLOG(1) << "In OnServiceUpdated(), service_type: " << service_type_ On 2014/01/15 22:11:42, Vitaly Buka wrote: > can be done with less log stataments Done. https://codereview.chromium.org/139923002/diff/70001/chrome/browser/local_dis... chrome/browser/local_discovery/service_discovery_device_lister.cc:94: VLOG(1) << "In OnResolveComplete(), service_type: " << service_type_ On 2014/01/15 22:11:42, Vitaly Buka wrote: > can be one statement in beging Done.
lgtm https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... File chrome/browser/extensions/api/mdns/dns_sd_registry.cc (right): https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:71: VLOG(1) << "Updated or added service: " << service.service_name << "? : " function never returns between logs, wot can combine them here https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:130: << ", registered: " << IsRegistered(service_type); missalinged https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:166: << ", added: " << added; missaligned https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:171: VLOG(1) << "Service changed: " << service.service_name; dont need line 171 https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:174: << service_type; missaligned https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:185: << "known: " << IsRegistered(service_type) ", known:... https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:191: if (service_data_map_[service_type]->RemoveService(service_name)) { bool is_removed = service_data_map_[service_type]->RemoveService(service_name); VLOG(1) << "ServiceRemoved: success:" << is_removed; if (is_removed) { no logging }else{ no logging } https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:193: << service_type; missaligned https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:202: << ", known: " << IsRegistered(service_type); missaligned https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:208: VLOG(1) << "Cleared services and dispatching event for service type: " same https://codereview.chromium.org/139923002/diff/180001/chrome/browser/local_di... File chrome/browser/local_discovery/service_discovery_device_lister.cc (right): https://codereview.chromium.org/139923002/diff/180001/chrome/browser/local_di... chrome/browser/local_discovery/service_discovery_device_lister.cc:35: VLOG(1) << "Start: service_type: " << service_type_; Maybe DeviceListerStart https://codereview.chromium.org/139923002/diff/180001/chrome/browser/local_di... chrome/browser/local_discovery/service_discovery_device_lister.cc:40: VLOG(1) << "DiscoverNewDevices, service_type: " << service_type_; Inconsistent separator, should be DiscoverNewDevices: ? https://codereview.chromium.org/139923002/diff/180001/chrome/browser/local_di... chrome/browser/local_discovery/service_discovery_device_lister.cc:49: << ", UpdateType: " << update; missaligned UpdateType-> update https://codereview.chromium.org/139923002/diff/180001/chrome/browser/local_di... chrome/browser/local_discovery/service_discovery_device_lister.cc:78: VLOG(1) << "Resolver for service_name already exists, ignoring: " Resolver already exists, service_name: https://codereview.chromium.org/139923002/diff/180001/chrome/browser/local_di... chrome/browser/local_discovery/service_discovery_device_lister.cc:93: << ", service_name: " << service_name missaligned
https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... File chrome/browser/extensions/api/mdns/dns_sd_registry.cc (right): https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:71: VLOG(1) << "Updated or added service: " << service.service_name << "? : " On 2014/01/15 23:08:51, Vitaly Buka wrote: > function never returns between logs, wot can combine them here Done. https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:166: << ", added: " << added; On 2014/01/15 23:08:51, Vitaly Buka wrote: > missaligned Done. https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:171: VLOG(1) << "Service changed: " << service.service_name; On 2014/01/15 23:08:51, Vitaly Buka wrote: > dont need line 171 Done. https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:174: << service_type; On 2014/01/15 23:08:51, Vitaly Buka wrote: > missaligned Done. https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:185: << "known: " << IsRegistered(service_type) On 2014/01/15 23:08:51, Vitaly Buka wrote: > ", known:... Done. https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:191: if (service_data_map_[service_type]->RemoveService(service_name)) { On 2014/01/15 23:08:51, Vitaly Buka wrote: > bool is_removed = service_data_map_[service_type]->RemoveService(service_name); > VLOG(1) << "ServiceRemoved: success:" << is_removed; > if (is_removed) { > no logging > }else{ > no logging > } Done. https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:193: << service_type; On 2014/01/15 23:08:51, Vitaly Buka wrote: > missaligned Done. https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:202: << ", known: " << IsRegistered(service_type); On 2014/01/15 23:08:51, Vitaly Buka wrote: > missaligned Done. https://codereview.chromium.org/139923002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/mdns/dns_sd_registry.cc:208: VLOG(1) << "Cleared services and dispatching event for service type: " On 2014/01/15 23:08:51, Vitaly Buka wrote: > same Done. https://codereview.chromium.org/139923002/diff/180001/chrome/browser/local_di... File chrome/browser/local_discovery/service_discovery_device_lister.cc (right): https://codereview.chromium.org/139923002/diff/180001/chrome/browser/local_di... chrome/browser/local_discovery/service_discovery_device_lister.cc:35: VLOG(1) << "Start: service_type: " << service_type_; On 2014/01/15 23:08:51, Vitaly Buka wrote: > Maybe DeviceListerStart Done. https://codereview.chromium.org/139923002/diff/180001/chrome/browser/local_di... chrome/browser/local_discovery/service_discovery_device_lister.cc:40: VLOG(1) << "DiscoverNewDevices, service_type: " << service_type_; On 2014/01/15 23:08:51, Vitaly Buka wrote: > Inconsistent separator, should be DiscoverNewDevices: ? > Done. https://codereview.chromium.org/139923002/diff/180001/chrome/browser/local_di... chrome/browser/local_discovery/service_discovery_device_lister.cc:49: << ", UpdateType: " << update; On 2014/01/15 23:08:51, Vitaly Buka wrote: > missaligned > > UpdateType-> update Done. https://codereview.chromium.org/139923002/diff/180001/chrome/browser/local_di... chrome/browser/local_discovery/service_discovery_device_lister.cc:78: VLOG(1) << "Resolver for service_name already exists, ignoring: " On 2014/01/15 23:08:51, Vitaly Buka wrote: > Resolver already exists, service_name: Done. https://codereview.chromium.org/139923002/diff/180001/chrome/browser/local_di... chrome/browser/local_discovery/service_discovery_device_lister.cc:93: << ", service_name: " << service_name On 2014/01/15 23:08:51, Vitaly Buka wrote: > missaligned Done.
LGTM https://codereview.chromium.org/139923002/diff/260001/chrome/browser/local_di... File chrome/browser/local_discovery/service_discovery_device_lister.cc (right): https://codereview.chromium.org/139923002/diff/260001/chrome/browser/local_di... chrome/browser/local_discovery/service_discovery_device_lister.cc:92: << ", service_name: " << service_name Indentation https://codereview.chromium.org/139923002/diff/260001/chrome/browser/local_di... chrome/browser/local_discovery/service_discovery_device_lister.cc:93: << ", status: " << status; Indentation
https://codereview.chromium.org/139923002/diff/260001/chrome/browser/local_di... File chrome/browser/local_discovery/service_discovery_device_lister.cc (right): https://codereview.chromium.org/139923002/diff/260001/chrome/browser/local_di... chrome/browser/local_discovery/service_discovery_device_lister.cc:92: << ", service_name: " << service_name On 2014/01/15 23:57:53, mark a. foltz wrote: > Indentation Done. https://codereview.chromium.org/139923002/diff/260001/chrome/browser/local_di... chrome/browser/local_discovery/service_discovery_device_lister.cc:93: << ", status: " << status; On 2014/01/15 23:57:53, mark a. foltz wrote: > Indentation Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/imcheng@chromium.org/139923002/240002
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/imcheng@chromium.org/139923002/240002
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/imcheng@chromium.org/139923002/240002
Message was sent while issue was closed.
Change committed as 245267 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
