Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10827130:
Refactoring the SysInfoProvider. (Closed)
|
Created:
8 years, 4 months ago by tudalex(Chromium) Modified:
8 years, 4 months ago CC:
chromium-reviews, oshima+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org, Elly Fong-Jones Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionRefactoring the SysInfoProvider to include logs from SysLogsProvider.
Refactoring the SysInfoProvider in order to support multiple data sources and also implementing the data sources from SysLogsProvider, that were specific to system_info_ui.
Replaced SysInfoProvider and SysLogsProvider classes in the system_info_ui just with one class the SysLogsFetcher that implements both classes.
BUG=138582
TEST=You navigate to chrome://system and confirm that the data is there. Some of the keys that should be present and have data should be: CHROMEOS_RELEASE_VERSION, mem_usage, cras, cpuinfo.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152856
Patch Set 1 #Patch Set 2 : #
Total comments: 12
Patch Set 3 : #
Total comments: 70
Patch Set 4 : #
Total comments: 26
Patch Set 5 : #
Total comments: 20
Patch Set 6 : #Patch Set 7 : #
Total comments: 13
Patch Set 8 : #Patch Set 9 : #
Total comments: 20
Patch Set 10 : #
Total comments: 53
Patch Set 11 : #
Total comments: 2
Patch Set 12 : #
Total comments: 44
Patch Set 13 : #
Total comments: 37
Patch Set 14 : #Patch Set 15 : #
Total comments: 35
Patch Set 16 : #
Total comments: 41
Patch Set 17 : #
Total comments: 11
Patch Set 18 : #Patch Set 19 : #
Total comments: 4
Patch Set 20 : #Patch Set 21 : #Patch Set 22 : #
Total comments: 27
Patch Set 23 : #
Total comments: 2
Patch Set 24 : #
Total comments: 2
Patch Set 25 : #Messages
Total messages: 72 (0 generated)
http://codereview.chromium.org/10827130/diff/6001/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system/sysinfo_provider.cc (right): http://codereview.chromium.org/10827130/diff/6001/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/sysinfo_provider.cc:7: #include "vector" Already included in the header + should be "#include <vector>" http://codereview.chromium.org/10827130/diff/6001/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/sysinfo_provider.cc:28: Too many LF, need just 1 between the includes ending and the using. http://codereview.chromium.org/10827130/diff/6001/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/sysinfo_provider.cc:50: SysLogsFetcher * fetcher = SysLogsFetcherFactory::GetInstance()->GetFetcher(); When is this freed? http://codereview.chromium.org/10827130/diff/6001/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system/sysinfo_provider.h (right): http://codereview.chromium.org/10827130/diff/6001/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/sysinfo_provider.h:22: typedef std::map<std::string, std::string> SysInfoResponse; This would make more sense as SystemInfoData http://codereview.chromium.org/10827130/diff/6001/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system/syslogs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/6001/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher.cc:50: request_->ForwardResult(response_); Do we really need to use CancelableRequest here? Instead, in the Fetch method, we can take a Callback as a parameter, and simply call the callback here. http://codereview.chromium.org/10827130/diff/6001/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher.cc:56: BackendCollection t; Rename the 't' http://codereview.chromium.org/10827130/diff/6001/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system/syslogs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/6001/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher.h:28: typedef std::vector <SysLogsInterface*> BackendCollection; A more appropriate name would be something like SystemLogsDataSources or something as descriptive. http://codereview.chromium.org/10827130/diff/6001/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher.h:30: class SysLogsFetcher { SystemLogsFetcher http://codereview.chromium.org/10827130/diff/6001/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher.h:35: scoped_refptr<CancelableRequest<SysInfoProvider::FetchCompleteCallback> > I thought we were moving away from using CancelableRequest? http://codereview.chromium.org/10827130/diff/6001/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher.h:48: class SysLogsFetcherFactory { SystemLogsFetcherFactory and place it in a separate file.
https://chromiumcodereview.appspot.com/10827130/diff/6001/chrome/browser/chro... File chrome/browser/chromeos/system/sysinfo_provider.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/6001/chrome/browser/chro... chrome/browser/chromeos/system/sysinfo_provider.cc:50: SysLogsFetcher * fetcher = SysLogsFetcherFactory::GetInstance()->GetFetcher(); On 2012/08/02 18:40:54, Rahul Chaturvedi wrote: > When is this freed? After it responds to the request it calls BrowserThread::DeleteSoon(BrowserThread::UI, FROM_HERE, this); in syslogs_fetcher.cc https://chromiumcodereview.appspot.com/10827130/diff/6001/chrome/browser/chro... File chrome/browser/chromeos/system/syslogs_fetcher.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/6001/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher.cc:56: BackendCollection t; On 2012/08/02 18:40:54, Rahul Chaturvedi wrote: > Rename the 't' Done.
https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... File chrome/browser/chromeos/system/commandline_fetcher.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/commandline_fetcher.cc:22: if (!file_util::CreateTemporaryFile(&temp_filename)) Change this to use base::GetAppOutput. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... File chrome/browser/chromeos/system/commandline_fetcher.h (right): https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/commandline_fetcher.h:18: CommandLineFetcher(std::string key, std::string command):key_(key), ) : key_ https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... File chrome/browser/chromeos/system/debugd_log_fetcher.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/debugd_log_fetcher.cc:42: : response_(new SysInfoResponse()), weak_ptr_factory_(this) { } {} (check other places also in the CL) https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/debugd_log_fetcher.cc:53: base::Unretained(this); Get rid of this; not sure if it's needed. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... File chrome/browser/chromeos/system/debugd_log_fetcher.h (right): https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/debugd_log_fetcher.h:34: void GotLogs(bool succeeded, const std::map<std::string, std::string>& logs); std::string,std::string https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... File chrome/browser/chromeos/system/lsbrelease_fetcher.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/lsbrelease_fetcher.cc:19: // The following code is copied form SysLogsProvider Put a TODO somewhere - "Fix this really crappy code" https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... File chrome/browser/chromeos/system/lsbrelease_fetcher.h (right): https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/lsbrelease_fetcher.h:24: DISALLOW_COPY_AND_ASSIGN; here and other classes. Also add default constructors. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... File chrome/browser/chromeos/system/memorydetails_fetcher.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/memorydetails_fetcher.cc:40: SysLogsFetcherCallback request_; Indentation. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/memorydetails_fetcher.cc:41: DISALLOW_COPY_AND_ASSIGN(SysLogsFetcherMemoryHandler); Newline above. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... File chrome/browser/chromeos/system/syslogs_fetcher.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher.cc:38: LOG(ERROR)<<"Called fetcher."; Not error; maybe info? and ) << " https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher.cc:49: if (--responses_) Return but also delete all your data source instances. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... File chrome/browser/chromeos/system/syslogs_fetcher.h (right): https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher.h:16: #include "base/synchronization/lock.h" Clean up the includes you don't need. Do this for other includes too; make sure that only include if you are instantiating an actual object from a class within that include. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher.h:21: typedef std::map<std::string, std::string> SysInfoResponse; Nit: Rename. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher.h:30: typedef std::vector <SysLogsInterface*> SysLogsDataSources; vector< https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher.h:32: class SysLogsFetcher { Add a header comment explaining, a.) Class usage b.) Examples c.) Object lifetimes https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher.h:36: void Fetch(SysLogsFetcherCallback); New line after. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher.h:38: SysLogsFetcherCallback request_; Add comments, lots of them :) https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher.h:40: SysInfoResponse* response_; Group logically then alphabetically, separate with newlines. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher.h:44: void AddData(SysInfoResponse*); DISALLOW_COPY_AND_ASSIGN https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher.h:44: void AddData(SysInfoResponse*); Move methods to above the fields, separate with a newline. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... File chrome/browser/chromeos/system/syslogs_fetcher_factory.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher_factory.cc:24: SysLogsFetcherFactory * SysLogsFetcherFactory::GetInstance() { Nit: Might be easier to use if you just called this Get https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher_factory.cc:28: SysLogsFetcher * SysLogsFetcherFactory::GetFetcher() { SysLogsFetcher* https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher_factory.cc:28: SysLogsFetcher * SysLogsFetcherFactory::GetFetcher() { Nit: GetFetcher->CreateFetcher https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher_factory.cc:31: // Debug Daemon data source Nit: Missing period, here and below. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher_factory.cc:32: data_sources.push_back(static_cast <SysLogsInterface*>( static_cast< Here and below. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher_factory.cc:33: new DebugDaemonLogFetcher())); Make sure these get deleted in SyslogFetcher. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher_factory.cc:36: data_sources.push_back(static_cast <SysLogsInterface*>( Try this without the static cast. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher_factory.cc:37: new CommandLineFetcher("cras", Merge into one class. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... File chrome/browser/chromeos/system/syslogs_fetcher_factory.h (right): https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher_factory.h:15: #include "base/synchronization/lock.h" Don't need several of these headers. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher_factory.h:16: #include "chrome/browser/chromeos/system/syslogs_fetcher.h" Replace this include with, class SysLogFetcher; move the include to the .cc file. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher_factory.h:19: namespace system { This namespace is unnecessary. Also, move the logs code to chrome/browser/chromeos/syslogs https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher_factory.h:24: SysLogsFetcher * GetFetcher(); SysLogsFetcher* https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/chro... chrome/browser/chromeos/system/syslogs_fetcher_factory.h:28: ~SysLogsFetcherFactory() {} Run Clang bots to make sure this is kosher. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/ui/w... File chrome/browser/ui/webui/chromeos/system_info_ui.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/ui/w... chrome/browser/ui/webui/chromeos/system_info_ui.cc:96: weak_ptr_factory_(this) { use ALLOW_THIS_IN_INITIALIZER_LIST everywhere you initialize weak pointers with the this variable like this. https://chromiumcodereview.appspot.com/10827130/diff/2002/chrome/browser/ui/w... chrome/browser/ui/webui/chromeos/system_info_ui.cc:143: for (it = sys_info_->begin(); it != sys_info_->end(); ++it) { Add a check for sys_info_ being NULL.
Sorry for the delay. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system/commandline_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/commandline_fetcher.cc:22: if (!file_util::CreateTemporaryFile(&temp_filename)) On 2012/08/04 01:15:05, rkc wrote: > Change this to use base::GetAppOutput. Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system/commandline_fetcher.h (right): http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/commandline_fetcher.h:18: CommandLineFetcher(std::string key, std::string command):key_(key), On 2012/08/04 01:15:05, rkc wrote: > ) : key_ Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system/debugd_log_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/debugd_log_fetcher.cc:42: : response_(new SysInfoResponse()), weak_ptr_factory_(this) { } On 2012/08/04 01:15:05, rkc wrote: > {} > (check other places also in the CL) Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/debugd_log_fetcher.cc:53: base::Unretained(this); On 2012/08/04 01:15:05, rkc wrote: > Get rid of this; not sure if it's needed. Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system/debugd_log_fetcher.h (right): http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/debugd_log_fetcher.h:34: void GotLogs(bool succeeded, const std::map<std::string, std::string>& logs); On 2012/08/04 01:15:05, rkc wrote: > std::string,std::string Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system/lsbrelease_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/lsbrelease_fetcher.cc:19: // The following code is copied form SysLogsProvider On 2012/08/04 01:15:05, rkc wrote: > Put a TODO somewhere - "Fix this really crappy code" Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system/lsbrelease_fetcher.h (right): http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/lsbrelease_fetcher.h:24: On 2012/08/04 01:15:05, rkc wrote: > DISALLOW_COPY_AND_ASSIGN; here and other classes. > Also add default constructors. Should these be done on Singleton classes also? Done everywhere except SysLogsFetcherFactory. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system/memorydetails_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/memorydetails_fetcher.cc:40: SysLogsFetcherCallback request_; On 2012/08/04 01:15:05, rkc wrote: > Indentation. Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/memorydetails_fetcher.cc:41: DISALLOW_COPY_AND_ASSIGN(SysLogsFetcherMemoryHandler); On 2012/08/04 01:15:05, rkc wrote: > Newline above. Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system/syslogs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher.cc:38: LOG(ERROR)<<"Called fetcher."; On 2012/08/04 01:15:05, rkc wrote: > Not error; maybe info? > and ) << " Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher.cc:49: if (--responses_) On 2012/08/04 01:15:05, rkc wrote: > Return but also delete all your data source instances. Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system/syslogs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher.h:16: #include "base/synchronization/lock.h" On 2012/08/04 01:15:05, rkc wrote: > Clean up the includes you don't need. > Do this for other includes too; make sure that only include if you are > instantiating an actual object from a class within that include. Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher.h:21: typedef std::map<std::string, std::string> SysInfoResponse; On 2012/08/04 01:15:05, rkc wrote: > Nit: Rename. Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher.h:30: typedef std::vector <SysLogsInterface*> SysLogsDataSources; On 2012/08/04 01:15:05, rkc wrote: > vector< Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher.h:32: class SysLogsFetcher { On 2012/08/04 01:15:05, rkc wrote: > Add a header comment explaining, > a.) Class usage > b.) Examples > c.) Object lifetimes Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher.h:36: void Fetch(SysLogsFetcherCallback); On 2012/08/04 01:15:05, rkc wrote: > New line after. Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher.h:38: SysLogsFetcherCallback request_; On 2012/08/04 01:15:05, rkc wrote: > Add comments, lots of them :) Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher.h:40: SysInfoResponse* response_; On 2012/08/04 01:15:05, rkc wrote: > Group logically then alphabetically, separate with newlines. Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher.h:44: void AddData(SysInfoResponse*); On 2012/08/04 01:15:05, rkc wrote: > Move methods to above the fields, separate with a newline. Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher.h:44: void AddData(SysInfoResponse*); On 2012/08/04 01:15:05, rkc wrote: > DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system/syslogs_fetcher_factory.cc (right): http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher_factory.cc:24: SysLogsFetcherFactory * SysLogsFetcherFactory::GetInstance() { On 2012/08/04 01:15:05, rkc wrote: > Nit: Might be easier to use if you just called this Get If I rename it to something else the Singleton template will not work anymore. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher_factory.cc:28: SysLogsFetcher * SysLogsFetcherFactory::GetFetcher() { On 2012/08/04 01:15:05, rkc wrote: > SysLogsFetcher* Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher_factory.cc:28: SysLogsFetcher * SysLogsFetcherFactory::GetFetcher() { On 2012/08/04 01:15:05, rkc wrote: > Nit: GetFetcher->CreateFetcher Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher_factory.cc:31: // Debug Daemon data source On 2012/08/04 01:15:05, rkc wrote: > Nit: Missing period, here and below. Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher_factory.cc:32: data_sources.push_back(static_cast <SysLogsInterface*>( On 2012/08/04 01:15:05, rkc wrote: > static_cast< > Here and below. Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher_factory.cc:33: new DebugDaemonLogFetcher())); On 2012/08/04 01:15:05, rkc wrote: > Make sure these get deleted in SyslogFetcher. Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher_factory.cc:36: data_sources.push_back(static_cast <SysLogsInterface*>( On 2012/08/04 01:15:05, rkc wrote: > Try this without the static cast. Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher_factory.cc:37: new CommandLineFetcher("cras", On 2012/08/04 01:15:05, rkc wrote: > Merge into one class. Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system/syslogs_fetcher_factory.h (right): http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher_factory.h:15: #include "base/synchronization/lock.h" On 2012/08/04 01:15:05, rkc wrote: > Don't need several of these headers. Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher_factory.h:16: #include "chrome/browser/chromeos/system/syslogs_fetcher.h" On 2012/08/04 01:15:05, rkc wrote: > Replace this include with, > > class SysLogFetcher; > > move the include to the .cc file. Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher_factory.h:19: namespace system { On 2012/08/04 01:15:05, rkc wrote: > This namespace is unnecessary. > Also, move the logs code to chrome/browser/chromeos/syslogs Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher_factory.h:24: SysLogsFetcher * GetFetcher(); On 2012/08/04 01:15:05, rkc wrote: > SysLogsFetcher* Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/syslogs_fetcher_factory.h:28: ~SysLogsFetcherFactory() {} On 2012/08/04 01:15:05, rkc wrote: > Run Clang bots to make sure this is kosher. Need rights first. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/system_info_ui.cc (right): http://codereview.chromium.org/10827130/diff/2002/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/system_info_ui.cc:96: weak_ptr_factory_(this) { On 2012/08/04 01:15:05, rkc wrote: > use ALLOW_THIS_IN_INITIALIZER_LIST everywhere you initialize weak pointers with > the this variable like this. Done. http://codereview.chromium.org/10827130/diff/2002/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/system_info_ui.cc:143: for (it = sys_info_->begin(); it != sys_info_->end(); ++it) { On 2012/08/04 01:15:05, rkc wrote: > Add a check for sys_info_ being NULL. Done.
http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/commandline_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/commandline_fetcher.cc:20: void CommandLineFetcher::Fetch(SysLogsFetcherCallback request) Add a TODO here to mention that we need to move the commands to an array (or something easier to add to than straight code) that is iterated over to run all the command lines. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/commandline_fetcher.h (right): http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/commandline_fetcher.h:12: class CommandLineFetcher:public SysLogsInterface { CommandLineFetcher : public http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/commandline_fetcher.h:14: void Fetch(SysLogsFetcherCallback); Move fetch to below ctor and dtor, add a newline after the ctor and dtor. Make these changes in the other headers too. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/commandline_fetcher.h:16: ~CommandLineFetcher() {} Newline before private: http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc:7: #include "vector" This should be #include <vector> and separated by a newline from the rest of the includes. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/lsbrelease_fetcher.h (right): http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/lsbrelease_fetcher.h:8: #include <vector> Not used. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/memorydetails_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/memorydetails_fetcher.cc:23: // This will call the callback on the calling thread. How? http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/memorydetails_fetcher.h (right): http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/memorydetails_fetcher.h:9: #include <vector> Not used. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.cc (right): http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.cc:23: data_sources.push_back(static_cast <SysLogsInterface*>( no static cast needed. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.cc:26: // Chrome data source. nit: sources. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.h (right): http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.h:25: friend struct DefaultSingletonTraits<SysLogsFetcherFactory>; Move the friend above the ctor/dtor, add a newline after; do this anywhere else friend is used also. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.h:26: DISALLOW_COPY_AND_ASSIGN? friend goes right below the private: and newline after http://codereview.chromium.org/10827130/diff/6019/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/system_info_ui.cc (right): http://codereview.chromium.org/10827130/diff/6019/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/system_info_ui.cc:149: delete sys_info_; Even though a delete null is a NOP, it's move efficiient to move the delete inside the if block; if !sysinfo, we don't need to delete.
http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/commandline_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/commandline_fetcher.cc:20: void CommandLineFetcher::Fetch(SysLogsFetcherCallback request) On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > Add a TODO here to mention that we need to move the commands to an array (or > something easier to add to than straight code) that is iterated over to run all > the command lines. Done. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/commandline_fetcher.h (right): http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/commandline_fetcher.h:12: class CommandLineFetcher:public SysLogsInterface { On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > CommandLineFetcher : public Done. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/commandline_fetcher.h:14: void Fetch(SysLogsFetcherCallback); On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > Move fetch to below ctor and dtor, add a newline after the ctor and dtor. > Make these changes in the other headers too. Done. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/commandline_fetcher.h:16: ~CommandLineFetcher() {} On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > Newline before private: Done. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc:7: #include "vector" On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > This should be #include <vector> and separated by a newline from the rest of the > includes. Acutally it doesn't even need to be there, it's already included in the header. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/lsbrelease_fetcher.h (right): http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/lsbrelease_fetcher.h:8: #include <vector> On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > Not used. Done. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/memorydetails_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/memorydetails_fetcher.cc:23: // This will call the callback on the calling thread. On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > How? Left comment from the SysLogsProvider code when I copied it. Deleted the comment. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/memorydetails_fetcher.h (right): http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/memorydetails_fetcher.h:9: #include <vector> On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > Not used. Done. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.cc (right): http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.cc:23: data_sources.push_back(static_cast <SysLogsInterface*>( On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > no static cast needed. Done. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.cc:26: // Chrome data source. On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > nit: sources. Done. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.h (right): http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.h:25: friend struct DefaultSingletonTraits<SysLogsFetcherFactory>; On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > Move the friend above the ctor/dtor, add a newline after; do this anywhere else > friend is used also. Done. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.h:26: On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > DISALLOW_COPY_AND_ASSIGN? > friend goes right below the private: and newline after Done. http://codereview.chromium.org/10827130/diff/6019/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/system_info_ui.cc (right): http://codereview.chromium.org/10827130/diff/6019/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/system_info_ui.cc:149: delete sys_info_; On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > Even though a delete null is a NOP, it's move efficiient to move the delete > inside the if block; if !sysinfo, we don't need to delete. It is inside the if block, it's not inside the for loop.
LGTM. On 2012/08/06 19:14:22, tudalex1 wrote: > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > File chrome/browser/chromeos/syslogs/commandline_fetcher.cc (right): > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > chrome/browser/chromeos/syslogs/commandline_fetcher.cc:20: void > CommandLineFetcher::Fetch(SysLogsFetcherCallback request) > On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > > Add a TODO here to mention that we need to move the commands to an array (or > > something easier to add to than straight code) that is iterated over to run > all > > the command lines. > > Done. > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > File chrome/browser/chromeos/syslogs/commandline_fetcher.h (right): > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > chrome/browser/chromeos/syslogs/commandline_fetcher.h:12: class > CommandLineFetcher:public SysLogsInterface { > On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > > CommandLineFetcher : public > > Done. > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > chrome/browser/chromeos/syslogs/commandline_fetcher.h:14: void > Fetch(SysLogsFetcherCallback); > On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > > Move fetch to below ctor and dtor, add a newline after the ctor and dtor. > > Make these changes in the other headers too. > > Done. > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > chrome/browser/chromeos/syslogs/commandline_fetcher.h:16: ~CommandLineFetcher() > {} > On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > > Newline before private: > > Done. > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > File chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc (right): > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc:7: #include "vector" > On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > > This should be #include <vector> and separated by a newline from the rest of > the > > includes. > Acutally it doesn't even need to be there, it's already included in the header. > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > File chrome/browser/chromeos/syslogs/lsbrelease_fetcher.h (right): > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > chrome/browser/chromeos/syslogs/lsbrelease_fetcher.h:8: #include <vector> > On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > > Not used. > > Done. > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > File chrome/browser/chromeos/syslogs/memorydetails_fetcher.cc (right): > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > chrome/browser/chromeos/syslogs/memorydetails_fetcher.cc:23: // This will call > the callback on the calling thread. > On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > > How? > Left comment from the SysLogsProvider code when I copied it. Deleted the > comment. > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > File chrome/browser/chromeos/syslogs/memorydetails_fetcher.h (right): > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > chrome/browser/chromeos/syslogs/memorydetails_fetcher.h:9: #include <vector> > On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > > Not used. > > Done. > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > File chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.cc (right): > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.cc:23: > data_sources.push_back(static_cast <SysLogsInterface*>( > On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > > no static cast needed. > > Done. > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.cc:26: // Chrome data > source. > On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > > nit: sources. > > Done. > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > File chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.h (right): > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.h:25: friend struct > DefaultSingletonTraits<SysLogsFetcherFactory>; > On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > > Move the friend above the ctor/dtor, add a newline after; do this anywhere > else > > friend is used also. > > Done. > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/chromeos/sys... > chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.h:26: > On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > > DISALLOW_COPY_AND_ASSIGN? > > friend goes right below the private: and newline after > > Done. > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/ui/webui/chr... > File chrome/browser/ui/webui/chromeos/system_info_ui.cc (right): > > http://codereview.chromium.org/10827130/diff/6019/chrome/browser/ui/webui/chr... > chrome/browser/ui/webui/chromeos/system_info_ui.cc:149: delete sys_info_; > On 2012/08/06 18:30:18, Rahul Chaturvedi wrote: > > Even though a delete null is a NOP, it's move efficiient to move the delete > > inside the if block; if !sysinfo, we don't need to delete. > It is inside the if block, it's not inside the for loop.
Added Zel for owner's review.
Added satorux
There seems to be a number of style issues. Please fix them first. http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/commandline_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/commandline_fetcher.cc:21: { move { to the previous line http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/commandline_fetcher.cc:24: std::vector< std::pair<std::string, CommandLine> > commands; no space after < http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/commandline_fetcher.h (right): http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/commandline_fetcher.h:12: class CommandLineFetcher : public SysLogsInterface { Class comment is missing. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Class_Comments http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/commandline_fetcher.h:20: SysLogsResponse * response_; SysLogsResponse* response_; http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/debugd_log_fetcher.h (right): http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/debugd_log_fetcher.h:18: class DebugDaemonLogFetcher : public SysLogsInterface { class comment is missing. http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/debugd_log_fetcher.h:26: void Fetch(SysLogsFetcherCallback); const SysLogsFetcherCallback& http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/debugd_log_fetcher.h:30: void GotRoutes(bool succeeded, const std::vector<std::string>& routes); function comments are missing. http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/lsbrelease_fetcher.h (right): http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/lsbrelease_fetcher.h:11: class LSBReleaseFetcher : public SysLogsInterface { class comment is missing. http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/lsbrelease_fetcher.h:16: void Fetch(SysLogsFetcherCallback); const SysLogsFetcherCallback& http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/lsbrelease_fetcher.h:16: void Fetch(SysLogsFetcherCallback); is this implementing a virtu function in SysLogsInterface? If so, make it virtual void Fetch(SysLogsFetcherCallback) OVERRIDE;
http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/commandline_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/commandline_fetcher.cc:21: { On 2012/08/06 23:47:31, satorux1 wrote: > move { to the previous line Done. http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/commandline_fetcher.cc:24: std::vector< std::pair<std::string, CommandLine> > commands; On 2012/08/06 23:47:31, satorux1 wrote: > no space after < Done. http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/commandline_fetcher.h (right): http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/commandline_fetcher.h:12: class CommandLineFetcher : public SysLogsInterface { On 2012/08/06 23:47:31, satorux1 wrote: > Class comment is missing. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Class_Comments Done. http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/commandline_fetcher.h:20: SysLogsResponse * response_; On 2012/08/06 23:47:31, satorux1 wrote: > SysLogsResponse* response_; Done. http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/debugd_log_fetcher.h (right): http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/debugd_log_fetcher.h:18: class DebugDaemonLogFetcher : public SysLogsInterface { On 2012/08/06 23:47:31, satorux1 wrote: > class comment is missing. Done. http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/debugd_log_fetcher.h:26: void Fetch(SysLogsFetcherCallback); On 2012/08/06 23:47:31, satorux1 wrote: > const SysLogsFetcherCallback& Done. http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/debugd_log_fetcher.h:30: void GotRoutes(bool succeeded, const std::vector<std::string>& routes); On 2012/08/06 23:47:31, satorux1 wrote: > function comments are missing. Done. Here and in other classes. http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/lsbrelease_fetcher.h (right): http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/lsbrelease_fetcher.h:11: class LSBReleaseFetcher : public SysLogsInterface { On 2012/08/06 23:47:31, satorux1 wrote: > class comment is missing. Done. http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/lsbrelease_fetcher.h:16: void Fetch(SysLogsFetcherCallback); On 2012/08/06 23:47:31, satorux1 wrote: > is this implementing a virtu function in SysLogsInterface? If so, make it > > virtual void Fetch(SysLogsFetcherCallback) OVERRIDE; Done. http://codereview.chromium.org/10827130/diff/8023/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/lsbrelease_fetcher.h:16: void Fetch(SysLogsFetcherCallback); On 2012/08/06 23:47:31, satorux1 wrote: > const SysLogsFetcherCallback& Done.
http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/lsbrelease_fetcher.h (right): http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/lsbrelease_fetcher.h:17: void Fetch(const SysLogsFetcherCallback&) OVERRIDE; Please add parameter name. Also add "virtual" http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/syslogs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher.cc:32: base::AutoLock lock(response_lock_); why is Lock needed? On what thread this function is called? This should be called on the same thread where Fetch() is called? Can this be called from another thread? Please add a DCHECK to ensure functions are called on expected threads. I'd recommend to add the following to all functions in the file, assuming the expected thread is UI thread. DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/syslogs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:26: virtual void Fetch(const SysLogsFetcherCallback&) = 0; parameter name is missing. Please fix everywhere. http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:70: base::Lock response_lock_; // The lock for it. why is Lock needed? we usually don't need it. http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.h (right): http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.h:18: static SysLogsFetcherFactory* GetInstance(); the indentation is off. http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.h:28: no blank line here
The patch description seems to be too short for a large patch like this. Per rkc@, this also adds some features in addition to refactoring, right? please add more description.
TEST= line is also missing
http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/lsbrelease_fetcher.h (right): http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/lsbrelease_fetcher.h:17: void Fetch(const SysLogsFetcherCallback&) OVERRIDE; On 2012/08/07 01:15:07, satorux1 wrote: > Please add parameter name. Also add "virtual" Done. http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/syslogs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher.cc:32: base::AutoLock lock(response_lock_); On 2012/08/07 01:15:07, satorux1 wrote: > why is Lock needed? On what thread this function is called? > > This should be called on the same thread where Fetch() is called? Can this be > called from another thread? > > Please add a DCHECK to ensure functions are called on expected threads. > > > I'd recommend to add the following to all functions in the file, assuming the > expected thread is UI thread. > > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); Lock was not needed. Added checks and also moved the execution of some of the data sources on another thread. Added a comment in the header to reflect this. http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/syslogs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:26: virtual void Fetch(const SysLogsFetcherCallback&) = 0; On 2012/08/07 01:15:07, satorux1 wrote: > parameter name is missing. Please fix everywhere. Done. http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:70: base::Lock response_lock_; // The lock for it. On 2012/08/07 01:15:07, satorux1 wrote: > why is Lock needed? we usually don't need it. Done. http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.h (right): http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.h:18: static SysLogsFetcherFactory* GetInstance(); On 2012/08/07 01:15:07, satorux1 wrote: > the indentation is off. Done. http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.h:28: On 2012/08/07 01:15:07, satorux1 wrote: > no blank line here Done.
http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/syslogs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/9005/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher.cc:32: base::AutoLock lock(response_lock_); On 2012/08/07 23:46:54, tudalex1 wrote: > On 2012/08/07 01:15:07, satorux1 wrote: > > why is Lock needed? On what thread this function is called? > > > > This should be called on the same thread where Fetch() is called? Can this be > > called from another thread? > > > > Please add a DCHECK to ensure functions are called on expected threads. > > > > > > I'd recommend to add the following to all functions in the file, assuming the > > expected thread is UI thread. > > > > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); > Lock was not needed. Added checks and also moved the execution of some of the > data sources on another thread. Added a comment in the header to reflect this. I'd suggest add more of these DCHECKs in other files too. Seems to me that your code runs on multiple threads. It's very helpful to clarify on what thread functions are called http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... File chrome/browser/chromeos/syslogs/commandline_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/commandline_fetcher.cc:23: content::BrowserThread::PostTask(content::BrowserThread::FILE, FILE thread is supposedly dead. Please use PostBlockingPoolTask or friends instead http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/commandline_fetcher.cc:26: base::Unretained(this), This looks unsafe. Is this guaranteed that |this| is alive when the callback is run on the FILE thread? Otherwise, |this| can be referenced after it's deleted, which is a use-after-free. AFAIK, the best way to solve this is to use PostTaskAndReply idiom. Result* result = new Result; PostBlockingPoolTaskAndReply( FROM_HERE, base::Bind(&FreeFunction, result), base::Bind(&Foo::MemberFunction, weak_ptr_factory_.GetWeakPtr(), base::Owned(result))); http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/commandline_fetcher.cc:66: FROM_HERE, indentation. http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... File chrome/browser/chromeos/syslogs/commandline_fetcher.h (right): http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/commandline_fetcher.h:17: ~CommandLineFetcher() {} virtual http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/commandline_fetcher.h:19: virtual void Fetch(const SysLogsFetcherCallback& request) OVERRIDE; add a comment line. // SysLogsInterface override. virtual void Fetch(const SysLogsFetcherCallback& request) OVERRIDE; http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... File chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc:100: content::BrowserThread::PostTask(content::BrowserThread::UI, Why is this needed? On what thread this function is called? Please annotate with DCHECK http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... File chrome/browser/chromeos/syslogs/lsbrelease_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/lsbrelease_fetcher.cc:88: base::Unretained(this), as mentioned elsewhere, this looks unsafe, so please don'd do this. http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... File chrome/browser/chromeos/syslogs/memorydetails_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/memorydetails_fetcher.cc:23: content::BrowserThread::PostTask(content::BrowserThread::UI, Why is this needed? On what thread this function is called? Please annotate with DCHECK http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/memorydetails_fetcher.cc:40: base::Unretained(this), please fix this too. http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... File chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.h (right): http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.h:22: friend struct DefaultSingletonTraits<SysLogsFetcherFactory>; singleton should be avoided. can you make this a member of some class?
BTW, the first line in the description looks too long. Here's some relevant bits from "git commit --help" on how to write commit messages: Though not required, it’s a good idea to begin the commit message with a single short (less than 50 character) line summarizing the change, followed by a blank line and then a more thorough description.
http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... File chrome/browser/chromeos/syslogs/commandline_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/commandline_fetcher.cc:23: content::BrowserThread::PostTask(content::BrowserThread::FILE, On 2012/08/08 22:01:03, satorux1 wrote: > FILE thread is supposedly dead. Please use PostBlockingPoolTask or friends > instead Done. http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/commandline_fetcher.cc:26: base::Unretained(this), On 2012/08/08 22:01:03, satorux1 wrote: > This looks unsafe. Is this guaranteed that |this| is alive when the callback is > run on the FILE thread? Otherwise, |this| can be referenced after it's deleted, > which is a use-after-free. > > AFAIK, the best way to solve this is to use PostTaskAndReply idiom. > > > Result* result = new Result; > PostBlockingPoolTaskAndReply( > FROM_HERE, > base::Bind(&FreeFunction, result), > base::Bind(&Foo::MemberFunction, > weak_ptr_factory_.GetWeakPtr(), > base::Owned(result))); Created a anonymous namespace and put the main execution part on a function there. http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/commandline_fetcher.cc:66: FROM_HERE, On 2012/08/08 22:01:03, satorux1 wrote: > indentation. Done. http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... File chrome/browser/chromeos/syslogs/commandline_fetcher.h (right): http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/commandline_fetcher.h:17: ~CommandLineFetcher() {} On 2012/08/08 22:01:03, satorux1 wrote: > virtual Done. http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/commandline_fetcher.h:19: virtual void Fetch(const SysLogsFetcherCallback& request) OVERRIDE; On 2012/08/08 22:01:03, satorux1 wrote: > add a comment line. > > // SysLogsInterface override. > virtual void Fetch(const SysLogsFetcherCallback& request) OVERRIDE; Done. http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... File chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc:100: content::BrowserThread::PostTask(content::BrowserThread::UI, On 2012/08/08 22:01:03, satorux1 wrote: > Why is this needed? On what thread this function is called? Please annotate with > DCHECK Done. http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... File chrome/browser/chromeos/syslogs/lsbrelease_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/lsbrelease_fetcher.cc:88: base::Unretained(this), On 2012/08/08 22:01:03, satorux1 wrote: > as mentioned elsewhere, this looks unsafe, so please don'd do this. Done the same thing as I've done in the CommandLineFetcher. http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... File chrome/browser/chromeos/syslogs/memorydetails_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/memorydetails_fetcher.cc:23: content::BrowserThread::PostTask(content::BrowserThread::UI, On 2012/08/08 22:01:03, satorux1 wrote: > Why is this needed? On what thread this function is called? Please annotate with > DCHECK Not needed actually. Annotated with DCHECK. http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/memorydetails_fetcher.cc:40: base::Unretained(this), On 2012/08/08 22:01:03, satorux1 wrote: > please fix this too. Not needed actually. http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... File chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.h (right): http://codereview.chromium.org/10827130/diff/15026/chrome/browser/chromeos/sy... chrome/browser/chromeos/syslogs/syslogs_fetcher_factory.h:22: friend struct DefaultSingletonTraits<SysLogsFetcherFactory>; On 2012/08/08 22:01:03, satorux1 wrote: > singleton should be avoided. can you make this a member of some class? Talked to rkc@ and decided that we don't need this class anymore, moved it's functionality into SysLogsFetcher constructor.
http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/commandline_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/commandline_fetcher.cc:23: void Execute(const chromeos::SysLogsFetcherCallback& request) { Function comment is missing. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/commandline_fetcher.cc:58: content::BrowserThread::PostTask(content::BrowserThread::UI, This is unusual. Please use PostTaskAndReply idiom. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/commandline_fetcher.cc:67: void CommandLineFetcher::Fetch(const SysLogsFetcherCallback& request) { so you are on the UI thread here, right? Please document with DCHECK http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/commandline_fetcher.h (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/commandline_fetcher.h:23: no blank line here. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc:42: client->GetRoutes(true, false, nit: when parameters cannot fit in one line, it's usually a good idea to align them vertically. More importantly, true/false here are cryptic. Please add some comment to clarify. client->GetRoutes( true, // add comment here false, // add comment here base::Bind(...) http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc:56: const char* kNotAvailable = "<not available>"; please put this in anonymous namespace. also please do: const char[] kNotAvailable = "..." this way is more efficient for an esoteric reason. http://udrepper.livejournal.com/13851.html http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc:60: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); nit: add a blank line after DCHECK. Please fix elsewhere too http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc:94: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); move this to the beginning of the function followed by a blank line. DCHECK()s are usually place at the beginning, followed by a blank line http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/lsbrelease_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/lsbrelease_fetcher.cc:93: { move { to the previous line. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/lsbrelease_fetcher.cc:118: BrowserThread::PostTask(content::BrowserThread::UI, please don't do this. please use PostTaskAndReply idiom. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/memorydetails_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/memorydetails_fetcher.cc:21: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); add a blank line after DCHECK http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/syslogs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.cc:23: ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) { Please be sure to initialize members of primitive types initializer list. Here, responses_ is not initialized. Please check other classes too. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.cc:32: responses_ = data_sources_.size(); maybe responses_ is unnecessary? cannot we just use data_sources_.size()? http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.cc:36: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); blank line here. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.cc:40: for (it = data_sources_.begin(); it != data_sources_.end(); ++it) { for (SysLogsDataSources::iterator it = data_sources_.begin(); http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.cc:47: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); blank line here. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.cc:49: for (it = data->begin(); it != data->end(); ++it){ for (SysLogsResponse::iterator it; ...) http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.cc:57: SysLogsDataSources::iterator ds_it; then, you don't need this weird name. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/syslogs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:22: class SysLogsInterface { I should have noticed earlier but the class name is confusing. In the context of Unix, "syslog" is an established mechanism for data logging: http://en.wikipedia.org/wiki/Syslog Here, this class is nothing to deal with syslog. Besides, names like "SysLogs" should be avoided in favor of less abbreviation. Something like SystemLogFetcherInterface would be a better name for this class. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:52: class SysLogsFetcher { This aggregates the data from multiple sources, right? If so, something like AggregatedSystemLogFetcher may be good. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:64: void AddData(SysLogsResponse* data); AddResponse((SysLogsResponse* response) ? "data" is usually not a good name. Here response looks better. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:70: size_t responses_; // The number of callbacks it should get. Please add num_ prefix to make it clear it's a number.
http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/syslogs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:50: // fetcher->Fetch(base::Bind(&Example::ProcessLogs,this)); Nit: &Example::ProcessLogs, this http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:76: Extra line.
http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/syslogs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:52: class SysLogsFetcher { On 2012/08/09 21:15:07, satorux1 wrote: > This aggregates the data from multiple sources, right? If so, something like > AggregatedSystemLogFetcher may be good. Satoru, AggregatedSystemLogFetcher is not quite an appropriate name for this class; it is the global system log fetcher that fetches from all other 'log sources'. Instead, we could name this SystemLogsFetcher and instead name all the other fetcher classes as LogSources, So the interface would be named, SystemLogsSource from it, we'll derive, for example, LsbReleaseLogSource ChromeLogSource CommandLineLogSource WDYT?
http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/commandline_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/commandline_fetcher.cc:23: void Execute(const chromeos::SysLogsFetcherCallback& request) { On 2012/08/09 21:15:07, satorux1 wrote: > Function comment is missing. Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/commandline_fetcher.cc:58: content::BrowserThread::PostTask(content::BrowserThread::UI, On 2012/08/09 21:15:07, satorux1 wrote: > This is unusual. Please use PostTaskAndReply idiom. Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/commandline_fetcher.cc:67: void CommandLineFetcher::Fetch(const SysLogsFetcherCallback& request) { On 2012/08/09 21:15:07, satorux1 wrote: > so you are on the UI thread here, right? Please document with DCHECK Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/commandline_fetcher.h (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/commandline_fetcher.h:23: On 2012/08/09 21:15:07, satorux1 wrote: > no blank line here. No longer the case. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc:42: client->GetRoutes(true, false, On 2012/08/09 21:15:07, satorux1 wrote: > nit: when parameters cannot fit in one line, it's usually a good idea to align > them vertically. More importantly, true/false here are cryptic. Please add some > comment to clarify. > > client->GetRoutes( > true, // add comment here > false, // add comment here > base::Bind(...) Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc:56: const char* kNotAvailable = "<not available>"; On 2012/08/09 21:15:07, satorux1 wrote: > please put this in anonymous namespace. also please do: > > const char[] kNotAvailable = "..." > > this way is more efficient for an esoteric reason. > http://udrepper.livejournal.com/13851.html Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc:60: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2012/08/09 21:15:07, satorux1 wrote: > nit: add a blank line after DCHECK. Please fix elsewhere too Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc:94: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2012/08/09 21:15:07, satorux1 wrote: > move this to the beginning of the function followed by a blank line. DCHECK()s > are usually place at the beginning, followed by a blank line Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/lsbrelease_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/lsbrelease_fetcher.cc:93: { On 2012/08/09 21:15:07, satorux1 wrote: > move { to the previous line. Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/lsbrelease_fetcher.cc:118: BrowserThread::PostTask(content::BrowserThread::UI, On 2012/08/09 21:15:07, satorux1 wrote: > please don't do this. please use PostTaskAndReply idiom. Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/memorydetails_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/memorydetails_fetcher.cc:21: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2012/08/09 21:15:07, satorux1 wrote: > add a blank line after DCHECK Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/syslogs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.cc:23: ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) { On 2012/08/09 21:15:07, satorux1 wrote: > Please be sure to initialize members of primitive types initializer list. Here, > responses_ is not initialized. > > Please check other classes too. Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.cc:32: responses_ = data_sources_.size(); On 2012/08/09 21:15:07, satorux1 wrote: > maybe responses_ is unnecessary? cannot we just use data_sources_.size()? I'm decrementing responses_ for each response I've recieved I can't do that with data_soruces_.size(). http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.cc:36: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2012/08/09 21:15:07, satorux1 wrote: > blank line here. Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.cc:40: for (it = data_sources_.begin(); it != data_sources_.end(); ++it) { On 2012/08/09 21:15:07, satorux1 wrote: > for (SysLogsDataSources::iterator it = data_sources_.begin(); Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.cc:47: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2012/08/09 21:15:07, satorux1 wrote: > blank line here. Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.cc:49: for (it = data->begin(); it != data->end(); ++it){ On 2012/08/09 21:15:07, satorux1 wrote: > for (SysLogsResponse::iterator it; ...) Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.cc:57: SysLogsDataSources::iterator ds_it; On 2012/08/09 21:15:07, satorux1 wrote: > then, you don't need this weird name. Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/syslogs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:22: class SysLogsInterface { On 2012/08/09 21:15:07, satorux1 wrote: > I should have noticed earlier but the class name is confusing. In the context > of Unix, "syslog" is an established mechanism for data logging: > http://en.wikipedia.org/wiki/Syslog > > Here, this class is nothing to deal with syslog. Besides, names like "SysLogs" > should be avoided in favor of less abbreviation. > > Something like SystemLogFetcherInterface would be a better name for this class. It was named after SysInfoProvider which was named after SysLogsProvider :) Changed it. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:50: // fetcher->Fetch(base::Bind(&Example::ProcessLogs,this)); On 2012/08/09 21:31:55, Rahul Chaturvedi wrote: > Nit: &Example::ProcessLogs, this Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:52: class SysLogsFetcher { On 2012/08/09 23:50:50, Rahul Chaturvedi wrote: > On 2012/08/09 21:15:07, satorux1 wrote: > > This aggregates the data from multiple sources, right? If so, something like > > AggregatedSystemLogFetcher may be good. > > Satoru, AggregatedSystemLogFetcher is not quite an appropriate name for this > class; it is the global system log fetcher that fetches from all other 'log > sources'. > > Instead, we could name this SystemLogsFetcher and instead name all the other > fetcher classes as LogSources, > > So the interface would be named, > > SystemLogsSource > from it, we'll derive, for example, > > LsbReleaseLogSource > ChromeLogSource > CommandLineLogSource > > WDYT? I wrote it as AggregatedSystemLogFetcher for now. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:64: void AddData(SysLogsResponse* data); On 2012/08/09 21:15:07, satorux1 wrote: > AddResponse((SysLogsResponse* response) ? > > "data" is usually not a good name. Here response looks better. Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:70: size_t responses_; // The number of callbacks it should get. On 2012/08/09 21:15:07, satorux1 wrote: > Please add num_ prefix to make it clear it's a number. Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:76: On 2012/08/09 21:31:55, Rahul Chaturvedi wrote: > Extra line. Done.
+cc: ellyjones@ and hshi@
http://codereview.chromium.org/10827130/diff/8044/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/syslogs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/8044/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:26: virtual void Fetch(const SysLogsFetcherCallback& request) = 0; I understand you have plan to allow specification of a subset of data sources. Would that be implemented in the form of an extra argument to the SystemLogFetcherInterface::Fetch()? Would you also be able to extend this plan to allow even finer-grained control, i.e. for the caller to be able to specify a subset of logs within an individual data source? For example I'm particularly interested to know if it is possible to just request a GetNetworkInterfaces log from the DebugDaemonLogFetcher.
http://codereview.chromium.org/10827130/diff/8044/chrome/browser/chromeos/sys... File chrome/browser/chromeos/syslogs/syslogs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/8044/chrome/browser/chromeos/sys... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:26: virtual void Fetch(const SysLogsFetcherCallback& request) = 0; On 2012/08/10 18:43:45, hshi1 wrote: > I understand you have plan to allow specification of a subset of data sources. > Would that be implemented in the form of an extra argument to the > SystemLogFetcherInterface::Fetch()? That is the plan for now. > Would you also be able to extend this plan to allow even finer-grained control, > i.e. for the caller to be able to specify a subset of logs within an individual > data source? > For example I'm particularly interested to know if it is possible to just > request a GetNetworkInterfaces log from the DebugDaemonLogFetcher. I think that will just over complicate things for now, if you just need that information from DebugDaemon we could implement a datasource that does just that. If somebody else has this requirement we might think of a way to allow you to specify this (I imagine it to be like a set of methods that allows you to modify the data sources before calling Fetch, and using a more generic DebugDaemon data source that you can customize before adding it to the AggregatedSystemLogFetcher object).
I think it is fine for the time being. Is the plan that all calls must be routed through the AggregatedSystemLogFetcher and not directly with individual SystemLogFetcherInterface objects (for individual data sources)? If so, then it seems difficult indeed to add a set of methods to a particular data source (say DebugDaemonLogFetcher) to restrict the logs to a subset within that data source. On 2012/08/10 19:03:33, tudalex1 wrote: > http://codereview.chromium.org/10827130/diff/8044/chrome/browser/chromeos/sys... > File chrome/browser/chromeos/syslogs/syslogs_fetcher.h (right): > > http://codereview.chromium.org/10827130/diff/8044/chrome/browser/chromeos/sys... > chrome/browser/chromeos/syslogs/syslogs_fetcher.h:26: virtual void Fetch(const > SysLogsFetcherCallback& request) = 0; > On 2012/08/10 18:43:45, hshi1 wrote: > > I understand you have plan to allow specification of a subset of data sources. > > Would that be implemented in the form of an extra argument to the > > SystemLogFetcherInterface::Fetch()? > That is the plan for now. > > > Would you also be able to extend this plan to allow even finer-grained > control, > > i.e. for the caller to be able to specify a subset of logs within an > individual > > data source? > > For example I'm particularly interested to know if it is possible to just > > request a GetNetworkInterfaces log from the DebugDaemonLogFetcher. > I think that will just over complicate things for now, if you just need that > information from DebugDaemon we could implement a datasource that does just > that. If somebody else has this requirement we might think of a way to allow you > to specify this (I imagine it to be like a set of methods that allows you to > modify the data sources before calling Fetch, and using a more generic > DebugDaemon data source that you can customize before adding it to the > AggregatedSystemLogFetcher object).
http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/syslogs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.cc:32: responses_ = data_sources_.size(); On 2012/08/10 00:30:31, tudalex1 wrote: > On 2012/08/09 21:15:07, satorux1 wrote: > > maybe responses_ is unnecessary? cannot we just use data_sources_.size()? > I'm decrementing responses_ for each response I've recieved I can't do that with > data_soruces_.size(). Then, please name it to something like num_pending_requests_ http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/syslogs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:52: class SysLogsFetcher { On 2012/08/09 23:50:50, Rahul Chaturvedi wrote: > On 2012/08/09 21:15:07, satorux1 wrote: > > This aggregates the data from multiple sources, right? If so, something like > > AggregatedSystemLogFetcher may be good. > > Satoru, AggregatedSystemLogFetcher is not quite an appropriate name for this > class; it is the global system log fetcher that fetches from all other 'log > sources'. > > Instead, we could name this SystemLogsFetcher and instead name all the other > fetcher classes as LogSources, > > So the interface would be named, > > SystemLogsSource > from it, we'll derive, for example, > > LsbReleaseLogSource > ChromeLogSource > CommandLineLogSource > > WDYT? Source sounds good.
http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/syslogs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.cc:32: responses_ = data_sources_.size(); On 2012/08/10 22:58:34, satorux1 wrote: > On 2012/08/10 00:30:31, tudalex1 wrote: > > On 2012/08/09 21:15:07, satorux1 wrote: > > > maybe responses_ is unnecessary? cannot we just use data_sources_.size()? > > I'm decrementing responses_ for each response I've recieved I can't do that > with > > data_soruces_.size(). > > Then, please name it to something like num_pending_requests_ Done. http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... File chrome/browser/chromeos/syslogs/syslogs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/44/chrome/browser/chromeos/syslo... chrome/browser/chromeos/syslogs/syslogs_fetcher.h:52: class SysLogsFetcher { On 2012/08/10 22:58:34, satorux1 wrote: > On 2012/08/09 23:50:50, Rahul Chaturvedi wrote: > > On 2012/08/09 21:15:07, satorux1 wrote: > > > This aggregates the data from multiple sources, right? If so, something like > > > AggregatedSystemLogFetcher may be good. > > > > Satoru, AggregatedSystemLogFetcher is not quite an appropriate name for this > > class; it is the global system log fetcher that fetches from all other 'log > > sources'. > > > > Instead, we could name this SystemLogsFetcher and instead name all the other > > fetcher classes as LogSources, > > > > So the interface would be named, > > > > SystemLogsSource > > from it, we'll derive, for example, > > > > LsbReleaseLogSource > > ChromeLogSource > > CommandLineLogSource > > > > WDYT? > > Source sounds good. Done. Also renamed the folder from syslogs to system_logs and renamed the files accordingly.
http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/commandline_log_source.cc (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/commandline_log_source.cc:26: SystemLogsResponse* response = new SystemLogsResponse; who deletes |response|? will this be deleted by SysLogsFetcherCallback? If so, you should probably make the callback to take scoped_ptr<SystemLogsResponse> to make it clear that you transfer the ownership of the object to the callback. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/commandline_log_source.cc:29: &CommandLineLogSource::Execute, indentation is wrong. four spaces, instead of two. I'd do BrowserThread::PostBlockingPoolTaskAndReply( FROM_HERE, base::Bind(&CommandLineLogSource::Execute, response), base::Bind(request, response)); http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/commandline_log_source.cc:31: base::Bind(request, response)); if you want to delete |response| after the callback is run, you can do: base::Owned(response) http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/commandline_log_source.cc:34: void CommandLineLogSource::Execute(SystemLogsResponse* response) { Could you move this to anonymous namespace in the .cc file? Seems to me that this function doesn't have to belong to CommandLineLogSource class. Less thing to have in .h file is preferable. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/commandline_log_source.h (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/commandline_log_source.h:12: // It gathers logs data from various command line tools which we can not access Usually, we should minimize use of "it" and "this" in comments as these can often make comments ambiguous. Maybe: // This class is used to gather logs data... http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/commandline_log_source.h:23: // Calls the programs and creates log entries with their output. What are "the programs"? Could you make it clear? http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/debugd_log_source.cc (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:36: : response_(new SystemLogsResponse()), where will it be deleted? http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:46: pending_requests_ = 4; 4 looks cryptic. please define a constant with some comment. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:60: void DebugDaemonLogSource::GotRoutes(bool succeeded, OnGetRoutes() would be better. this way, it's easier to find the matching callback for GetRoutes() http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:94: const std::map<std::string, indentation http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/debugd_log_source.h (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.h:41: int pending_requests_; please add num_ prefix. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:95: void LsbReleaseLogSource::Execute(SystemLogsResponse* response) { move this to anonymous namespace?
http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/debugd_log_source.cc (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:46: pending_requests_ = 4; On 2012/08/13 18:29:03, satorux1 wrote: > 4 looks cryptic. please define a constant with some comment. Actually, this should get initialized to 0, then incremented after each client->GetRoutes That way if we add more client->GetXXX calls, it'll be clear that this needs to be incremented.
http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/memorydetails_log_source.cc (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/memorydetails_log_source.cc:19: virtual void OnDetailsAvailable() OVERRIDE { // MemoryDetails override http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:52: delete response; oh i see. so the response is deleted here. as mentioned elsewhere, it'd be probably nicer to take scoped_ptr<SystemLogsResponse> instead. Using base::Owned() in the client code is also an option. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:54: if (--num_pending_requests_) this is hard to read. please do --num_pending_requests; if (num_pending_requests_ > 0) return; http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:58: for (SysLogsDataSources::iterator it = data_sources_.begin(); Please use ScopedVector for data_sources_ instead of deleting contents in this way. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/system_logs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:65: void AddData(SystemLogsResponse* response); AddResponse? http://codereview.chromium.org/10827130/diff/16005/chrome/browser/ui/webui/ch... File chrome/browser/ui/webui/chromeos/system_info_ui.cc (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:68: chromeos::SystemLogsResponse* sys_info_; sys_info_ -> response_ ? http://codereview.chromium.org/10827130/diff/16005/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:135: { move { to the previous line. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:139: chromeos::SystemLogsResponse::iterator it; Please declare inside of for(); http://codereview.chromium.org/10827130/diff/16005/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:146: delete sys_info_; this is not good. please see my comments elsewhere.
http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/commandline_log_source.cc (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/commandline_log_source.cc:26: SystemLogsResponse* response = new SystemLogsResponse; On 2012/08/13 18:29:03, satorux1 wrote: > who deletes |response|? will this be deleted by SysLogsFetcherCallback? > Yes, it is deleted there. > > If so, you should probably make the callback to take > scoped_ptr<SystemLogsResponse> to make it clear that you transfer the ownership > of the object to the callback. Used base::Owned() to do that. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/commandline_log_source.cc:29: &CommandLineLogSource::Execute, On 2012/08/13 18:29:03, satorux1 wrote: > indentation is wrong. four spaces, instead of two. I'd do > > BrowserThread::PostBlockingPoolTaskAndReply( > FROM_HERE, > base::Bind(&CommandLineLogSource::Execute, response), > base::Bind(request, response)); Thanks, I was wondering what the best approach would be. Done. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/commandline_log_source.cc:31: base::Bind(request, response)); On 2012/08/13 18:29:03, satorux1 wrote: > if you want to delete |response| after the callback is run, you can do: > > base::Owned(response) Used base::Owned(). http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/commandline_log_source.cc:34: void CommandLineLogSource::Execute(SystemLogsResponse* response) { On 2012/08/13 18:29:03, satorux1 wrote: > Could you move this to anonymous namespace in the .cc file? > > Seems to me that this function doesn't have to belong to CommandLineLogSource > class. Less thing to have in .h file is preferable. I had it originally in an anonymous namespace in patch set 10. rkc@ suggested that I move it in the class cause it's related to what the class does. Should I move it back to an anonymous namespace? For now I moved it into an anonymous space and changed it's name to ExecuteCommandLines. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/commandline_log_source.h (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/commandline_log_source.h:12: // It gathers logs data from various command line tools which we can not access On 2012/08/13 18:29:03, satorux1 wrote: > Usually, we should minimize use of "it" and "this" in comments as these can > often make comments ambiguous. Maybe: > > // This class is used to gather logs data... Done. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/commandline_log_source.h:23: // Calls the programs and creates log entries with their output. On 2012/08/13 18:29:03, satorux1 wrote: > What are "the programs"? Could you make it clear? Reformulated it. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/debugd_log_source.cc (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:36: : response_(new SystemLogsResponse()), On 2012/08/13 18:29:03, satorux1 wrote: > where will it be deleted? Made it a scoped_ptr so it is going to be deleted when this object is deleted which is after all the log sources have responded. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:46: pending_requests_ = 4; On 2012/08/13 18:50:14, Rahul Chaturvedi wrote: > On 2012/08/13 18:29:03, satorux1 wrote: > > 4 looks cryptic. please define a constant with some comment. > > Actually, this should get initialized to 0, then incremented after each > client->GetRoutes > > That way if we add more client->GetXXX calls, it'll be clear that this needs to > be incremented. Done. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:60: void DebugDaemonLogSource::GotRoutes(bool succeeded, On 2012/08/13 18:29:03, satorux1 wrote: > OnGetRoutes() would be better. this way, it's easier to find the matching > callback for GetRoutes() Done. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:94: const std::map<std::string, On 2012/08/13 18:29:03, satorux1 wrote: > indentation Done. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/debugd_log_source.h (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.h:41: int pending_requests_; On 2012/08/13 18:29:03, satorux1 wrote: > please add num_ prefix. Done. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:95: void LsbReleaseLogSource::Execute(SystemLogsResponse* response) { On 2012/08/13 18:29:03, satorux1 wrote: > move this to anonymous namespace? Same as CommandLineLogSource::Execute. Moved it and renamed it to ParseLSBRelease. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/memorydetails_log_source.cc (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/memorydetails_log_source.cc:19: virtual void OnDetailsAvailable() OVERRIDE { On 2012/08/13 19:33:36, satorux1 wrote: > // MemoryDetails override Done. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:52: delete response; On 2012/08/13 19:33:36, satorux1 wrote: > oh i see. so the response is deleted here. as mentioned elsewhere, it'd be > probably nicer to take scoped_ptr<SystemLogsResponse> instead. > > Using base::Owned() in the client code is also an option. Done. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:54: if (--num_pending_requests_) On 2012/08/13 19:33:36, satorux1 wrote: > this is hard to read. please do > > --num_pending_requests; > if (num_pending_requests_ > 0) > return; Done. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:58: for (SysLogsDataSources::iterator it = data_sources_.begin(); On 2012/08/13 19:33:36, satorux1 wrote: > Please use ScopedVector for data_sources_ instead of deleting contents in this > way. Done. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/system_logs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:65: void AddData(SystemLogsResponse* response); On 2012/08/13 19:33:36, satorux1 wrote: > AddResponse? Done. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/ui/webui/ch... File chrome/browser/ui/webui/chromeos/system_info_ui.cc (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:68: chromeos::SystemLogsResponse* sys_info_; On 2012/08/13 19:33:36, satorux1 wrote: > sys_info_ -> response_ ? Done. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:135: { On 2012/08/13 19:33:36, satorux1 wrote: > move { to the previous line. Done. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:139: chromeos::SystemLogsResponse::iterator it; On 2012/08/13 19:33:36, satorux1 wrote: > Please declare inside of for(); Done. http://codereview.chromium.org/10827130/diff/16005/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:146: delete sys_info_; On 2012/08/13 19:33:36, satorux1 wrote: > this is not good. please see my comments elsewhere. Done. Used scoped_ptr.
http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/commandline_log_source.cc (right): http://codereview.chromium.org/10827130/diff/16005/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/commandline_log_source.cc:34: void CommandLineLogSource::Execute(SystemLogsResponse* response) { On 2012/08/14 03:24:00, tudalex1 wrote: > On 2012/08/13 18:29:03, satorux1 wrote: > > Could you move this to anonymous namespace in the .cc file? > > > > Seems to me that this function doesn't have to belong to CommandLineLogSource > > class. Less thing to have in .h file is preferable. > I had it originally in an anonymous namespace in patch set 10. rkc@ suggested > that I move it in the class cause it's related to what the class does. Should I > move it back to an anonymous namespace? > For now I moved it into an anonymous space and changed it's name to > ExecuteCommandLines. It's probably a matter of taste, but I personally like to have less things in .h file. See also http://en.wikipedia.org/wiki/Don't_repeat_yourself http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/debugd_log_source.cc (right): http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:36: : response_(new SystemLogsResponse()), num_pending_requests_(0), please move num_pending_requests_(0) to the next line. i.e. please align members vertically. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:37: ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {} please also initialize request_(NULL). Be sure to initialize all pointers and primitive types like int. Please check other classes too. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:42: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); DCHECK(!request_); http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc (right): http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:22: // tokenizer, this code was copied over from the SysLogsProvide class. Ugh, copy-and-pasting this large amount of code is bad. Please fix this now rather than later. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:83: void ParseLSBRelease(chromeos::SystemLogsResponse* response) { Add DCHECK(response); followed by a blank line. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:84: FilePath lsb_release_file("/etc/lsb-release"); const http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:94: while (data.length() > 0) { I think what you want to do is something like: in_multi_line_value = false; SplitString(data, '\n', &lines); for each line: if in_multi_line_value: if line ends with """: remove """ from line; in_multi_line_value = false; value += line; else: SplitString(line, '=', &pair); key = pair[0]; value = pair[1]; if value starts with """: in_multi_line_value = true; if !in_multi_line_value: response[key] = value; http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:114: } This function is complicated and can easily be unit tested. Please make it a public static function, and write a unit test (yes, I know I asked you to put this in anonymous namespace, but I take it back) http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/system_logs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:29: typedef ScopedVector<SystemLogsSource> SysLogsDataSources; This typedef looks unnecessary http://codereview.chromium.org/10827130/diff/15043/chrome/browser/ui/webui/ch... File chrome/browser/ui/webui/chromeos/system_info_ui.cc (right): http://codereview.chromium.org/10827130/diff/15043/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:96: request_id_(0), response_(NULL), move response_(NULL) to the next line. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:135: if (response_ != NULL) { It's ok to say if (response_) { http://codereview.chromium.org/10827130/diff/15043/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:138: chromeos::SystemLogsResponse::iterator it; this is not fixed. please move this inside of for for (chromeos::SystemLogsResponse::iterator it = ...; Minimizing scope is a good thing. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:144: no blank line here.
http://codereview.chromium.org/10827130/diff/15043/chrome/browser/ui/webui/ch... File chrome/browser/ui/webui/chromeos/system_info_ui.cc (right): http://codereview.chromium.org/10827130/diff/15043/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:61: void SysInfoComplete(chromeos::SystemLogsResponse* response); I think you can put the code in the file in 'chromeos' namespace, then you can remove all chromeos:: namespace. That'd be cleaner.
http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/debugd_log_source.cc (right): http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:36: : response_(new SystemLogsResponse()), num_pending_requests_(0), On 2012/08/14 05:51:52, satorux1 wrote: > please move num_pending_requests_(0) to the next line. i.e. please align members > vertically. Done. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:37: ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {} On 2012/08/14 05:51:52, satorux1 wrote: > please also initialize request_(NULL). > > Be sure to initialize all pointers and primitive types like int. Please check > other classes too. Request is a callback and can't initialize it to null. It gives an error saying it can't convert from long int to base::Callback. I initialized it with SysLogsFetcherCallback. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:42: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2012/08/14 05:51:52, satorux1 wrote: > DCHECK(!request_); Says error: no match for 'operator!' in '!request'. Guess because it's a callback. Do you know any equivalent to DCHECK(!request_)? http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc (right): http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:22: // tokenizer, this code was copied over from the SysLogsProvide class. On 2012/08/14 05:51:52, satorux1 wrote: > Ugh, copy-and-pasting this large amount of code is bad. Please fix this now > rather than later. Done. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:83: void ParseLSBRelease(chromeos::SystemLogsResponse* response) { On 2012/08/14 05:51:52, satorux1 wrote: > Add DCHECK(response); followed by a blank line. Done. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:84: FilePath lsb_release_file("/etc/lsb-release"); On 2012/08/14 05:51:52, satorux1 wrote: > const Done. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:94: while (data.length() > 0) { On 2012/08/14 05:51:52, satorux1 wrote: > I think what you want to do is something like: > > in_multi_line_value = false; > SplitString(data, '\n', &lines); > for each line: > if in_multi_line_value: > if line ends with """: > remove """ from line; > in_multi_line_value = false; > value += line; > else: > SplitString(line, '=', &pair); > key = pair[0]; > value = pair[1]; > if value starts with """: > in_multi_line_value = true; > if !in_multi_line_value: > response[key] = value; Talked to rkc@ and lsb-release actually doesn't have multi line values. The code that was copied was used to parse all the older log sources. So I used base::SplitStringIntoKeyValuePairs. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:114: } On 2012/08/14 05:51:52, satorux1 wrote: > This function is complicated and can easily be unit tested. Please make it a > public static function, and write a unit test (yes, I know I asked you to put > this in anonymous namespace, but I take it back) Done, but I had to split it into to so that I can unit test it. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/system_logs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:29: typedef ScopedVector<SystemLogsSource> SysLogsDataSources; On 2012/08/14 05:51:52, satorux1 wrote: > This typedef looks unnecessary Done. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/ui/webui/ch... File chrome/browser/ui/webui/chromeos/system_info_ui.cc (right): http://codereview.chromium.org/10827130/diff/15043/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:61: void SysInfoComplete(chromeos::SystemLogsResponse* response); On 2012/08/14 05:53:21, satorux1 wrote: > I think you can put the code in the file in 'chromeos' namespace, then you can > remove all chromeos:: namespace. That'd be cleaner. Done. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:96: request_id_(0), response_(NULL), On 2012/08/14 05:51:52, satorux1 wrote: > move response_(NULL) to the next line. Done. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:135: if (response_ != NULL) { On 2012/08/14 05:51:52, satorux1 wrote: > It's ok to say if (response_) { I can't say it. It gives me an error that it can't convert scoped_ptr to bool. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:138: chromeos::SystemLogsResponse::iterator it; On 2012/08/14 05:51:52, satorux1 wrote: > this is not fixed. please move this inside of for > > for (chromeos::SystemLogsResponse::iterator it = ...; > > Minimizing scope is a good thing. Sorry. I forgot to save the file in eclipse with the fix. Done now. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:144: On 2012/08/14 05:51:52, satorux1 wrote: > no blank line here. Done.
http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/debugd_log_source.cc (right): http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:37: ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {} On 2012/08/15 01:54:33, tudalex1 wrote: > On 2012/08/14 05:51:52, satorux1 wrote: > > please also initialize request_(NULL). > > > > Be sure to initialize all pointers and primitive types like int. Please check > > other classes too. > Request is a callback and can't initialize it to null. It gives an error saying > it can't convert from long int to base::Callback. I initialized it with > SysLogsFetcherCallback. Sorry, I thought request_ was just a pointer. If it's a callback, you don't have to initialize explicitly. BTW, I think the source of the confusion is that request_ is an unusual name for a callback. Please rename it to something like response_handler_ or something
http://codereview.chromium.org/10827130/diff/15043/chrome/browser/ui/webui/ch... File chrome/browser/ui/webui/chromeos/system_info_ui.cc (right): http://codereview.chromium.org/10827130/diff/15043/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:135: if (response_ != NULL) { On 2012/08/15 01:54:33, tudalex1 wrote: > On 2012/08/14 05:51:52, satorux1 wrote: > > It's ok to say if (response_) { > I can't say it. It gives me an error that it can't convert scoped_ptr to bool. Then do this if (response_.get())
http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/debugd_log_source.cc (right): http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:37: ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {} On 2012/08/15 02:11:32, satorux1 wrote: > On 2012/08/15 01:54:33, tudalex1 wrote: > > On 2012/08/14 05:51:52, satorux1 wrote: > > > please also initialize request_(NULL). > > > > > > Be sure to initialize all pointers and primitive types like int. Please > check > > > other classes too. > > Request is a callback and can't initialize it to null. It gives an error > saying > > it can't convert from long int to base::Callback. I initialized it with > > SysLogsFetcherCallback. > > Sorry, I thought request_ was just a pointer. If it's a callback, you don't > have to initialize explicitly. > > BTW, I think the source of the confusion is that request_ is an unusual name for > a callback. Please rename it to something like response_handler_ or something or just callback_
http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/debugd_log_source.cc (right): http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:37: ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {} On 2012/08/15 02:22:43, satorux1 wrote: > On 2012/08/15 02:11:32, satorux1 wrote: > > On 2012/08/15 01:54:33, tudalex1 wrote: > > > On 2012/08/14 05:51:52, satorux1 wrote: > > > > please also initialize request_(NULL). > > > > > > > > Be sure to initialize all pointers and primitive types like int. Please > > check > > > > other classes too. > > > Request is a callback and can't initialize it to null. It gives an error > > saying > > > it can't convert from long int to base::Callback. I initialized it with > > > SysLogsFetcherCallback. > > > > Sorry, I thought request_ was just a pointer. If it's a callback, you don't > > have to initialize explicitly. > > > > BTW, I think the source of the confusion is that request_ is an unusual name > for > > a callback. Please rename it to something like response_handler_ or something > > or just callback_ Done. Renamed everything with callback_. Also did it in all the other classes. Sorry for the confusion. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/ui/webui/ch... File chrome/browser/ui/webui/chromeos/system_info_ui.cc (right): http://codereview.chromium.org/10827130/diff/15043/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/system_info_ui.cc:135: if (response_ != NULL) { On 2012/08/15 02:12:36, satorux1 wrote: > On 2012/08/15 01:54:33, tudalex1 wrote: > > On 2012/08/14 05:51:52, satorux1 wrote: > > > It's ok to say if (response_) { > > I can't say it. It gives me an error that it can't convert scoped_ptr to bool. > > Then do this if (response_.get()) Done.
thank you for writing tests. it's getting closer. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/debugd_log_source.cc (right): http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:42: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2012/08/15 01:54:33, tudalex1 wrote: > On 2012/08/14 05:51:52, satorux1 wrote: > > DCHECK(!request_); > Says error: no match for 'operator!' in '!request'. Guess because it's a > callback. Do you know any equivalent to DCHECK(!request_)? DCHECK(callback.is_null()); http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc (right): http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:94: while (data.length() > 0) { On 2012/08/15 01:54:33, tudalex1 wrote: > On 2012/08/14 05:51:52, satorux1 wrote: > > I think what you want to do is something like: > > > > in_multi_line_value = false; > > SplitString(data, '\n', &lines); > > for each line: > > if in_multi_line_value: > > if line ends with """: > > remove """ from line; > > in_multi_line_value = false; > > value += line; > > else: > > SplitString(line, '=', &pair); > > key = pair[0]; > > value = pair[1]; > > if value starts with """: > > in_multi_line_value = true; > > if !in_multi_line_value: > > response[key] = value; > Talked to rkc@ and lsb-release actually doesn't have multi line values. The code > that was copied was used to parse all the older log sources. So I used > base::SplitStringIntoKeyValuePairs. so while you are working on the similar code here, could you fix the bad code in the other file? Could you file a bug and fix in a separate patch? you don't have to fix this in this patch. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/debugd_log_source.cc (right): http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:112: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); maybe add DCHECK(!callback_.is_null()) followed by a blank line. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc (right): http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:25: typedef std::pair<std::string, std::string> KV_Pair; no abbreviation. KeyValuePair http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:46: &lsb_data); indentation. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:58: std::vector<KV_Pair> keys; |keys| sounds wrong because it's pairs. maybe |pairs|? http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:61: it != keys.end(); btw, it's a matter of taste, but the following is more succinct. for (size_t i = 0; i < keys.size(); ++i) { http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:67: if (IsStringUTF8(it->second)) { it->second -> value ?? shouldn't we also check the key? http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:73: LOG(WARNING) << "Invalid characters in system log entry: " << it->first; value? http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/lsbrelease_log_source.h (right): http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.h:20: // For testing. Parses a key=value string. please describe more. this accepts multiple lines, right? like foo=1 bar=2 http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.h:21: static void ParseLSBRelease(const std::string lsb_data, const std::string& http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc (right): http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc:18: EXPECT_TRUE(response.find("key2") != response.end()); remove this? http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc:19: EXPECT_EQ("value2", response["key2"]); might want to add some comment: // Make sure extra white space is removed. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc:29: EXPECT_FALSE(response.find("key3") != response.end()); you can do: EXPECT_EQ(0U, response.count("key3")); http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc:32: TEST(LSBReleaseLogSource, UTF8) { BrokenUTF8 http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc:33: std::string data("key=value\nkey2=\nkey3="); you could do "key=value\nkey2=\nkey3=\xFC" and remove the next line http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc:42: TEST(LSBReleasLogSource, NoKeyValuePair) { you might also want to test with an empty string. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:39: ScopedVector<SystemLogsSource>::iterator it; move this in 'for', or I'd suggest just do for (size_t i = 0; ... http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/system_logs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:60: void AddResponse(SystemLogsResponse* callback); response. this is not callback, right?
http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/debugd_log_source.cc (right): http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:42: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2012/08/15 22:01:09, satorux1 wrote: > On 2012/08/15 01:54:33, tudalex1 wrote: > > On 2012/08/14 05:51:52, satorux1 wrote: > > > DCHECK(!request_); > > Says error: no match for 'operator!' in '!request'. Guess because it's a > > callback. Do you know any equivalent to DCHECK(!request_)? > > DCHECK(callback.is_null()); Done. http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc (right): http://codereview.chromium.org/10827130/diff/15043/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:94: while (data.length() > 0) { On 2012/08/15 22:01:09, satorux1 wrote: > On 2012/08/15 01:54:33, tudalex1 wrote: > > On 2012/08/14 05:51:52, satorux1 wrote: > > > I think what you want to do is something like: > > > > > > in_multi_line_value = false; > > > SplitString(data, '\n', &lines); > > > for each line: > > > if in_multi_line_value: > > > if line ends with """: > > > remove """ from line; > > > in_multi_line_value = false; > > > value += line; > > > else: > > > SplitString(line, '=', &pair); > > > key = pair[0]; > > > value = pair[1]; > > > if value starts with """: > > > in_multi_line_value = true; > > > if !in_multi_line_value: > > > response[key] = value; > > Talked to rkc@ and lsb-release actually doesn't have multi line values. The > code > > that was copied was used to parse all the older log sources. So I used > > base::SplitStringIntoKeyValuePairs. > > so while you are working on the similar code here, could you fix the bad code in > the other file? Could you file a bug and fix in a separate patch? you don't have > to fix this in this patch. The plan is that the old file is deprecated and the next CL's will remove that file. It is still being used for Feedback and networking and also the code there was handling multi line values. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/debugd_log_source.cc (right): http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/debugd_log_source.cc:112: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2012/08/15 22:01:09, satorux1 wrote: > maybe add DCHECK(!callback_.is_null()) followed by a blank line. Done. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc (right): http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:25: typedef std::pair<std::string, std::string> KV_Pair; On 2012/08/15 22:01:09, satorux1 wrote: > no abbreviation. KeyValuePair Done. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:46: &lsb_data); On 2012/08/15 22:01:09, satorux1 wrote: > indentation. Done. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:58: std::vector<KV_Pair> keys; On 2012/08/15 22:01:09, satorux1 wrote: > |keys| sounds wrong because it's pairs. maybe |pairs|? Done. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:61: it != keys.end(); On 2012/08/15 22:01:09, satorux1 wrote: > btw, it's a matter of taste, but the following is more succinct. > > for (size_t i = 0; i < keys.size(); ++i) { Done. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:67: if (IsStringUTF8(it->second)) { On 2012/08/15 22:01:09, satorux1 wrote: > it->second -> value ?? > > shouldn't we also check the key? Done. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:73: LOG(WARNING) << "Invalid characters in system log entry: " << it->first; On 2012/08/15 22:01:09, satorux1 wrote: > value? I think the key is better than the value, because it is less prone to errors, assuming the key is hardcoded somewhere, so you then know where to look or what produced the error. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/lsbrelease_log_source.h (right): http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.h:20: // For testing. Parses a key=value string. On 2012/08/15 22:01:09, satorux1 wrote: > please describe more. this accepts multiple lines, right? like > > foo=1 > bar=2 Done. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.h:21: static void ParseLSBRelease(const std::string lsb_data, On 2012/08/15 22:01:09, satorux1 wrote: > const std::string& Done. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc (right): http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc:18: EXPECT_TRUE(response.find("key2") != response.end()); On 2012/08/15 22:01:09, satorux1 wrote: > remove this? No. It checks if the spaces we're removed correctly from the key. I know this is covered also by the next line, but it is easier to realize what happened if they both fail. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc:19: EXPECT_EQ("value2", response["key2"]); On 2012/08/15 22:01:09, satorux1 wrote: > might want to add some comment: > > // Make sure extra white space is removed. Done. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc:32: TEST(LSBReleaseLogSource, UTF8) { On 2012/08/15 22:01:09, satorux1 wrote: > BrokenUTF8 Done. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc:33: std::string data("key=value\nkey2=\nkey3="); On 2012/08/15 22:01:09, satorux1 wrote: > you could do "key=value\nkey2=\nkey3=\xFC" and remove the next line Done. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc:42: TEST(LSBReleasLogSource, NoKeyValuePair) { On 2012/08/15 22:01:09, satorux1 wrote: > you might also want to test with an empty string. Done. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:39: ScopedVector<SystemLogsSource>::iterator it; On 2012/08/15 22:01:09, satorux1 wrote: > move this in 'for', or I'd suggest just do for (size_t i = 0; ... Done. http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/system_logs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:60: void AddResponse(SystemLogsResponse* callback); On 2012/08/15 22:01:09, satorux1 wrote: > response. this is not callback, right? Yes. Done.
http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc (right): http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:73: LOG(WARNING) << "Invalid characters in system log entry: " << it->first; On 2012/08/16 01:08:57, tudalex1 wrote: > On 2012/08/15 22:01:09, satorux1 wrote: > > value? > I think the key is better than the value, because it is less prone to errors, > assuming the key is hardcoded somewhere, so you then know where to look or what > produced the error. sounds good http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc (right): http://codereview.chromium.org/10827130/diff/11029/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc:18: EXPECT_TRUE(response.find("key2") != response.end()); On 2012/08/16 01:08:57, tudalex1 wrote: > On 2012/08/15 22:01:09, satorux1 wrote: > > remove this? > No. It checks if the spaces we're removed correctly from the key. I know this is > covered also by the next line, but it is easier to realize what happened if they > both fail. > ah that makes sense. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/commandline_log_source.cc (right): http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/commandline_log_source.cc:53: std::vector< std::pair<std::string, CommandLine> >::iterator it; move this iterator declaration inside of 'for' or just use size_t i. Please check other places too. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/debugd_log_source.cc (right): http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/debugd_log_source.cc:44: DCHECK(!callback.is_null()); might also want to add DCHECK(callback_.is_null()) to ensure that the function is not called twice. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc (right): http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:37: base::Bind(&ReadLSBRelease, response), this should be &LsbReleaseLogSource::ReadLSBRelease http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:65: if (!key.empty()) { you might want to do if (key.empty()) continue; Then, you'll save one indentation level. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:66: if (IsStringUTF8(value) && IsStringUTF8(key)) { Likewise, if (!IsStringUTF8(value) || !IsStringUTF8(key)) continue; Then, you'll save another indentation level. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/lsbrelease_log_source.h (right): http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/lsbrelease_log_source.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. we should probably rename this file to lsb_relese_log_source.h. we usually separate words with _. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc (right): http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc:36: std::string data("key=value\nkey2=\nkey3=\xFCts\nkey4=value4"); Please also test the broken UTF8 in a key. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc:51: std::string contents2(""); Rather than doing this in the same test, it's cleaner to have a separate test. TEST(LSBReleasLogSource, EmptyInput) http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/memorydetails_log_source.cc (right): http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/memorydetails_log_source.cc:13: class SysLogsFetcherMemoryHandler : public MemoryDetails { class comment is missing. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/memorydetails_log_source.cc:16: SysLogsFetcherCallback callback) move this to the previous line. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:21: response_(new SystemLogsResponse), when will it be deleted? make it a scoped_ptr? If you want to transfer the ownership to the client, then pass the scoped_ptr to the callback, by response_.Pass() http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:35: void SystemLogsFetcher::Fetch(const SysLogsFetcherCallback& request) { request -> callback http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:50: ++it) { you might want to use size_t i = 0 here and elsewhere. it's more succinct. IMHO, using an iterator for iterating a vector does not make much sense. it's just longer and harder to read, but some people disagree, so it's a matter of taste. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/system_logs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:17: typedef base::Callback<void(SystemLogsResponse*)> SysLogsFetcherCallback; please add a parameter name when typedefin'g a callback, like: typedef base::Callback<void(SystemLogsResponse* response)> SysLogsFetcherCallback; http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:40: // ProcessLogs(chrome::SystemLogsResponse) { indentation is off. 3 spaces -> 2 spaces. 'void' is missing. void ProcessLogs(chrome::SystemLogsResponse) { http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:44: // chrome::SystemLogsFetcher* fetcher = remove chrome:: http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:45: // chrome::SystemLogsFetcher::GetInstance()->CreateFetcher(); I think this is not correct. this is no longer a singleton, right? http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:63: SysLogsFetcherCallback request_; callback_ http://codereview.chromium.org/10827130/diff/6064/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/system_info_ui.cc (right): http://codereview.chromium.org/10827130/diff/6064/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/system_info_ui.cc:109: SystemLogsFetcher * fetcher = new SystemLogsFetcher(); SystemLogsFetcher* http://codereview.chromium.org/10827130/diff/6064/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/system_info_ui.cc:111: &SystemInfoUIHTMLSource::SysInfoComplete, indentation.
http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/commandline_log_source.cc (right): http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/commandline_log_source.cc:53: std::vector< std::pair<std::string, CommandLine> >::iterator it; On 2012/08/16 10:50:57, satorux1 wrote: > move this iterator declaration inside of 'for' or just use size_t i. Please > check other places too. Done. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/debugd_log_source.cc (right): http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/debugd_log_source.cc:44: DCHECK(!callback.is_null()); On 2012/08/16 10:50:57, satorux1 wrote: > might also want to add > > DCHECK(callback_.is_null()) to ensure that the function is not called twice. Done. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc (right): http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:37: base::Bind(&ReadLSBRelease, response), On 2012/08/16 10:50:57, satorux1 wrote: > this should be &LsbReleaseLogSource::ReadLSBRelease Done. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:65: if (!key.empty()) { On 2012/08/16 10:50:57, satorux1 wrote: > you might want to do > > if (key.empty()) > continue; > > Then, you'll save one indentation level. Done. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc:66: if (IsStringUTF8(value) && IsStringUTF8(key)) { On 2012/08/16 10:50:57, satorux1 wrote: > Likewise, > > if (!IsStringUTF8(value) || !IsStringUTF8(key)) > continue; > > Then, you'll save another indentation level. Done. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/lsbrelease_log_source.h (right): http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/lsbrelease_log_source.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/08/16 10:50:57, satorux1 wrote: > we should probably rename this file to lsb_relese_log_source.h. we usually > separate words with _. Done. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc (right): http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc:36: std::string data("key=value\nkey2=\nkey3=\xFCts\nkey4=value4"); On 2012/08/16 10:50:57, satorux1 wrote: > Please also test the broken UTF8 in a key. Done. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/lsbrelease_log_source_unittest.cc:51: std::string contents2(""); On 2012/08/16 10:50:57, satorux1 wrote: > Rather than doing this in the same test, it's cleaner to have a separate test. > > TEST(LSBReleasLogSource, EmptyInput) Done. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/memorydetails_log_source.cc (right): http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/memorydetails_log_source.cc:13: class SysLogsFetcherMemoryHandler : public MemoryDetails { On 2012/08/16 10:50:57, satorux1 wrote: > class comment is missing. Done. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/memorydetails_log_source.cc:16: SysLogsFetcherCallback callback) On 2012/08/16 10:50:57, satorux1 wrote: > move this to the previous line. Done. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:21: response_(new SystemLogsResponse), On 2012/08/16 10:50:57, satorux1 wrote: > when will it be deleted? make it a scoped_ptr? If you want to transfer the > ownership to the client, then pass the scoped_ptr to the callback, by > response_.Pass() Done. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:35: void SystemLogsFetcher::Fetch(const SysLogsFetcherCallback& request) { On 2012/08/16 10:50:57, satorux1 wrote: > request -> callback Done. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:50: ++it) { On 2012/08/16 10:50:57, satorux1 wrote: > you might want to use size_t i = 0 here and elsewhere. it's more succinct. IMHO, > using an iterator for iterating a vector does not make much sense. it's just > longer and harder to read, but some people disagree, so it's a matter of taste. response is a map and I don't know any ot her nice way to iterate over it. Also realized map had a insert method that I can use to do this automatically. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/system_logs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:17: typedef base::Callback<void(SystemLogsResponse*)> SysLogsFetcherCallback; On 2012/08/16 10:50:57, satorux1 wrote: > please add a parameter name when typedefin'g a callback, like: > > typedef base::Callback<void(SystemLogsResponse* response)> > SysLogsFetcherCallback; Done. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:40: // ProcessLogs(chrome::SystemLogsResponse) { On 2012/08/16 10:50:57, satorux1 wrote: > indentation is off. 3 spaces -> 2 spaces. > > 'void' is missing. > > void ProcessLogs(chrome::SystemLogsResponse) { Done. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:44: // chrome::SystemLogsFetcher* fetcher = On 2012/08/16 10:50:57, satorux1 wrote: > remove chrome:: Done. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:45: // chrome::SystemLogsFetcher::GetInstance()->CreateFetcher(); On 2012/08/16 10:50:57, satorux1 wrote: > I think this is not correct. this is no longer a singleton, right? Right. Fixed. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:63: SysLogsFetcherCallback request_; On 2012/08/16 10:50:57, satorux1 wrote: > callback_ Done. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/system_info_ui.cc (right): http://codereview.chromium.org/10827130/diff/6064/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/system_info_ui.cc:109: SystemLogsFetcher * fetcher = new SystemLogsFetcher(); On 2012/08/16 10:50:57, satorux1 wrote: > SystemLogsFetcher* Done. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/system_info_ui.cc:111: &SystemInfoUIHTMLSource::SysInfoComplete, On 2012/08/16 10:50:57, satorux1 wrote: > indentation. Done.
almost there. http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/6064/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:50: ++it) { On 2012/08/16 22:33:03, tudalex1 wrote: > On 2012/08/16 10:50:57, satorux1 wrote: > > you might want to use size_t i = 0 here and elsewhere. it's more succinct. > IMHO, > > using an iterator for iterating a vector does not make much sense. it's just > > longer and harder to read, but some people disagree, so it's a matter of > taste. > response is a map and I don't know any ot her nice way to iterate over it. Also > realized map had a insert method that I can use to do this automatically. sorry, i wasn't aware this was a map. insert() lgtm. http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/debugd_log_source.h (right): http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/debugd_log_source.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. please rename the file to debug_daemon_log_source.h http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:35: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); like elsewhere, might want to add DCHECK(callback_.is_null()) to ensure this function is not called twice. http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:47: response_->insert(response->begin(), response->end()); I think this code assume that keys are unique. However, looking at the code, this assumption is very fragile. Can you add a prefix for each source, like "routes" -> "DebugDaemonSource:routes". I guess this requires a lot of changes elsewhere, so you might want to do this in a separate patch. What do you think? http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/system_logs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:19: SysLogsSourceCallback; blank line.
http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/debugd_log_source.h (right): http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/debugd_log_source.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/08/17 17:46:29, satorux1 wrote: > please rename the file to debug_daemon_log_source.h Done. http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:35: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2012/08/17 17:46:29, satorux1 wrote: > like elsewhere, might want to add > > DCHECK(callback_.is_null()) to ensure this function is not called twice. Done. http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:47: response_->insert(response->begin(), response->end()); On 2012/08/17 17:46:29, satorux1 wrote: > I think this code assume that keys are unique. However, looking at the code, > this assumption is very fragile. > > Can you add a prefix for each source, like "routes" -> > "DebugDaemonSource:routes". I guess this requires a lot of changes elsewhere, so > you might want to do this in a separate patch. What do you think? > I documented in the SystemLogsFetcher class comment that the keys that are not unique are not added. There was a prefix added for debug which was removed in this CL https://chromiumcodereview.appspot.com/10825260. I think it's up to the person who writes the data source to be careful to specify a unique name for the key, because we don't want to encourage having duplicate data in our logs. rkc@ what do you say? http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/system_logs_fetcher.h (right): http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:19: SysLogsSourceCallback; On 2012/08/17 17:46:29, satorux1 wrote: > blank line. Done.
http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:47: response_->insert(response->begin(), response->end()); On 2012/08/17 19:43:22, tudalex1 wrote: > On 2012/08/17 17:46:29, satorux1 wrote: > > I think this code assume that keys are unique. However, looking at the code, > > this assumption is very fragile. > > > > Can you add a prefix for each source, like "routes" -> > > "DebugDaemonSource:routes". I guess this requires a lot of changes elsewhere, > so > > you might want to do this in a separate patch. What do you think? > > > I documented in the SystemLogsFetcher class comment that the keys that are not > unique are not added. > There was a prefix added for debug which was removed in this CL > https://chromiumcodereview.appspot.com/10825260. > I think it's up to the person who writes the data source to be careful to > specify a unique name for the key, because we don't want to encourage having > duplicate data in our logs. > rkc@ what do you say? > "be careful" is not a good practice in software development. we should make sure it won't happen wherever we can. That said, instead of insert() everything at once, we should add a key-value one-by-one, and DCHECK if there is no duplicate of a key. you can do it by checking the return value of insert().
http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:47: response_->insert(response->begin(), response->end()); If a key appears from another data source, we overwrite the value, that's our intended behavior. Prefixing keys with anything is directly visible to the user which will be bad. We want the user to see the data in the same way that they were getting till now, all this code is only to make our adding sources and maintaining sources much easier and cleaner. There should be no user impact if possible. I'd suggest that we mitigate the duplicate keys issue by DCHECK'ing on it. That way if someone accidentally adds a duplicate key, they'd know about it (hopefully) before they commit :) On 2012/08/17 19:43:22, tudalex1 wrote: > On 2012/08/17 17:46:29, satorux1 wrote: > > I think this code assume that keys are unique. However, looking at the code, > > this assumption is very fragile. > > > > Can you add a prefix for each source, like "routes" -> > > "DebugDaemonSource:routes". I guess this requires a lot of changes elsewhere, > so > > you might want to do this in a separate patch. What do you think? > > > I documented in the SystemLogsFetcher class comment that the keys that are not > unique are not added. > There was a prefix added for debug which was removed in this CL > https://chromiumcodereview.appspot.com/10825260. > I think it's up to the person who writes the data source to be careful to > specify a unique name for the key, because we don't want to encourage having > duplicate data in our logs. > rkc@ what do you say? >
http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/6067/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:47: response_->insert(response->begin(), response->end()); On 2012/08/17 19:48:27, Rahul Chaturvedi wrote: > If a key appears from another data source, we overwrite the value, that's our > intended behavior. > > Prefixing keys with anything is directly visible to the user which will be bad. > We want the user to see the data in the same way that they were getting till > now, all this code is only to make our adding sources and maintaining sources > much easier and cleaner. There should be no user impact if possible. > > I'd suggest that we mitigate the duplicate keys issue by DCHECK'ing on it. That > way if someone accidentally adds a duplicate key, they'd know about it > (hopefully) before they commit :) > > On 2012/08/17 19:43:22, tudalex1 wrote: > > On 2012/08/17 17:46:29, satorux1 wrote: > > > I think this code assume that keys are unique. However, looking at the code, > > > this assumption is very fragile. > > > > > > Can you add a prefix for each source, like "routes" -> > > > "DebugDaemonSource:routes". I guess this requires a lot of changes > elsewhere, > > so > > > you might want to do this in a separate patch. What do you think? > > > > > I documented in the SystemLogsFetcher class comment that the keys that are > not > > unique are not added. > > There was a prefix added for debug which was removed in this CL > > https://chromiumcodereview.appspot.com/10825260. > > I think it's up to the person who writes the data source to be careful to > > specify a unique name for the key, because we don't want to encourage having > > duplicate data in our logs. > > rkc@ what do you say? > > > Done.
LGTM with a nit. Hope the code review was useful for you. http://codereview.chromium.org/10827130/diff/18012/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/18012/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:52: // It is false if the insert didn't work. "didn't work" is not correct. // It is false if the insert if there is already an element with the same key.
http://codereview.chromium.org/10827130/diff/18012/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/commandline_log_source.h (right): http://codereview.chromium.org/10827130/diff/18012/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/commandline_log_source.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. ah, forgot to ask you rename this file to command_line_log_source.h
https://chromiumcodereview.appspot.com/10827130/diff/18012/chrome/browser/chr... File chrome/browser/chromeos/system_logs/commandline_log_source.h (right): https://chromiumcodereview.appspot.com/10827130/diff/18012/chrome/browser/chr... chrome/browser/chromeos/system_logs/commandline_log_source.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/08/17 21:42:12, satorux1 wrote: > ah, forgot to ask you rename this file to command_line_log_source.h done. also renamed memorydetails to memory_details https://chromiumcodereview.appspot.com/10827130/diff/18012/chrome/browser/chr... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/18012/chrome/browser/chr... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:52: // It is false if the insert didn't work. On 2012/08/17 21:41:21, satorux1 wrote: > "didn't work" is not correct. > > // It is false if the insert if there is already an element with the same key. Done.
Renamed the files using eclipse and forgot to add them in git again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tudalex@chromium.org/10827130/10022
Presubmit check for 10827130-10022 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
chrome/browser/ui/webui
Presubmit checks took 2.6s to calculate.
Added James Hawkins for OWNERS review for chrome/ and chrome/browser/ui/webui.
Added Chris Silverberg for owner's review for chrome/browser/ui/webui.
webui owner lgtm
https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... File chrome/browser/chromeos/system_logs/command_line_log_source.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/command_line_log_source.cc:14: #include "base/file_util.h" nit: remove unused #includes. https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... File chrome/browser/chromeos/system_logs/debug_daemon_log_source.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/debug_daemon_log_source.cc:71: (*response_)["routes"] = JoinString(routes, '\n'); nit: make these constants? same with {network,modem}-status. https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... File chrome/browser/chromeos/system_logs/lsb_release_log_source.h (right): https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/lsb_release_log_source.h:32: static void ReadLSBRelease(SystemLogsResponse* response); Put this in an anonymous namespace instead and remove it from the header? https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:11: #include "chrome/browser/chromeos/system_logs/memory_details_log_source.h" nit: alphabetical ordering https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:23: nit: no blank lines at the top of functions. https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:51: for (SystemLogsResponse::iterator it = response->begin(); const_iterator? https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:56: DCHECK(response_->insert(*it).second); If you do this inside a DCHECK(), it won't actually get called in production releases! https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... File chrome/browser/chromeos/system_logs/system_logs_fetcher.h (right): https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:12: #include "base/memory/weak_ptr.h" nit: alphabetical ordering https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:13: #include "base/memory/scoped_vector.h" nit: add a newline after this. https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:56: explicit SystemLogsFetcher(); nit: remove the explicit https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/ui/... File chrome/browser/ui/webui/chromeos/system_info_ui.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/ui/... chrome/browser/ui/webui/chromeos/system_info_ui.cc:140: for (SystemLogsResponse::iterator it = response_->begin(); const_iterator
https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... File chrome/browser/chromeos/system_logs/lsb_release_log_source.h (right): https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/lsb_release_log_source.h:32: static void ReadLSBRelease(SystemLogsResponse* response); On 2012/08/21 19:56:55, Lei Zhang wrote: > Put this in an anonymous namespace instead and remove it from the header? This has been discussed extensively; it really doesn't matter if we leave it in the class and it might be useful for unit testing it later. https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:56: DCHECK(response_->insert(*it).second); On 2012/08/21 19:56:55, Lei Zhang wrote: > If you do this inside a DCHECK(), it won't actually get called in production > releases! That's intended, we don't want to crash in production on this. This is put in here so developers know that there are conflicting keys.
https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... File chrome/browser/chromeos/system_logs/lsb_release_log_source.h (right): https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/lsb_release_log_source.h:32: static void ReadLSBRelease(SystemLogsResponse* response); On 2012/08/21 20:02:33, rkc wrote: > On 2012/08/21 19:56:55, Lei Zhang wrote: > > Put this in an anonymous namespace instead and remove it from the header? > > This has been discussed extensively; it really doesn't matter if we leave it in > the class and it might be useful for unit testing it later. Which I did not read given the number of iterations on this CL. Fine by me. https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:56: DCHECK(response_->insert(*it).second); On 2012/08/21 20:02:33, rkc wrote: > On 2012/08/21 19:56:55, Lei Zhang wrote: > > If you do this inside a DCHECK(), it won't actually get called in production > > releases! > > That's intended, we don't want to crash in production on this. This is put in > here so developers know that there are conflicting keys. In that case, put the entire loop inside a #if !defined(NDEBUG)
https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:56: DCHECK(response_->insert(*it).second); On 2012/08/21 20:08:15, Lei Zhang wrote: > On 2012/08/21 20:02:33, rkc wrote: > > On 2012/08/21 19:56:55, Lei Zhang wrote: > > > If you do this inside a DCHECK(), it won't actually get called in production > > > releases! > > > > That's intended, we don't want to crash in production on this. This is put in > > here so developers know that there are conflicting keys. > > In that case, put the entire loop inside a #if !defined(NDEBUG) I think Lei was right, and it was a great catch. We should not use DCHECK here. Otherwise, the key/value pair is not inserted at all.
https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:56: DCHECK(response_->insert(*it).second); On 2012/08/21 20:48:31, satorux1 wrote: > On 2012/08/21 20:08:15, Lei Zhang wrote: > > On 2012/08/21 20:02:33, rkc wrote: > > > On 2012/08/21 19:56:55, Lei Zhang wrote: > > > > If you do this inside a DCHECK(), it won't actually get called in > production > > > > releases! > > > > > > That's intended, we don't want to crash in production on this. This is put > in > > > here so developers know that there are conflicting keys. > > > > In that case, put the entire loop inside a #if !defined(NDEBUG) > > I think Lei was right, and it was a great catch. We should not use DCHECK here. > Otherwise, the key/value pair is not inserted at all. Yeah, I realized that. tudalex@ is correcting it.
https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... File chrome/browser/chromeos/system_logs/command_line_log_source.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/command_line_log_source.cc:14: #include "base/file_util.h" On 2012/08/21 19:56:55, Lei Zhang wrote: > nit: remove unused #includes. Done. https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... File chrome/browser/chromeos/system_logs/debug_daemon_log_source.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/debug_daemon_log_source.cc:71: (*response_)["routes"] = JoinString(routes, '\n'); On 2012/08/21 19:56:55, Lei Zhang wrote: > nit: make these constants? same with {network,modem}-status. Done. https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:11: #include "chrome/browser/chromeos/system_logs/memory_details_log_source.h" On 2012/08/21 19:56:55, Lei Zhang wrote: > nit: alphabetical ordering Done. https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:23: On 2012/08/21 19:56:55, Lei Zhang wrote: > nit: no blank lines at the top of functions. Done. https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:51: for (SystemLogsResponse::iterator it = response->begin(); On 2012/08/21 19:56:55, Lei Zhang wrote: > const_iterator? Done. https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:56: DCHECK(response_->insert(*it).second); On 2012/08/21 20:08:15, Lei Zhang wrote: > On 2012/08/21 20:02:33, rkc wrote: > > On 2012/08/21 19:56:55, Lei Zhang wrote: > > > If you do this inside a DCHECK(), it won't actually get called in production > > > releases! > > > > That's intended, we don't want to crash in production on this. This is put in > > here so developers know that there are conflicting keys. > > In that case, put the entire loop inside a #if !defined(NDEBUG) Lei was right in the first place. Moved it outside. https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... File chrome/browser/chromeos/system_logs/system_logs_fetcher.h (right): https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:12: #include "base/memory/weak_ptr.h" On 2012/08/21 19:56:55, Lei Zhang wrote: > nit: alphabetical ordering Done. https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:13: #include "base/memory/scoped_vector.h" On 2012/08/21 19:56:55, Lei Zhang wrote: > nit: add a newline after this. Done. https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/chr... chrome/browser/chromeos/system_logs/system_logs_fetcher.h:56: explicit SystemLogsFetcher(); On 2012/08/21 19:56:55, Lei Zhang wrote: > nit: remove the explicit Done. https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/ui/... File chrome/browser/ui/webui/chromeos/system_info_ui.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/10022/chrome/browser/ui/... chrome/browser/ui/webui/chromeos/system_info_ui.cc:140: for (SystemLogsResponse::iterator it = response_->begin(); On 2012/08/21 19:56:55, Lei Zhang wrote: > const_iterator Done.
lgtm with nit below: https://chromiumcodereview.appspot.com/10827130/diff/22020/chrome/browser/chr... File chrome/browser/chromeos/system_logs/command_line_log_source.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/22020/chrome/browser/chr... chrome/browser/chromeos/system_logs/command_line_log_source.cc:66: base::Bind(&ExecuteCommandLines, response), You removed base/bind.h even though you actually use it here. Ditto for base/logging.h for DCHECK().
https://chromiumcodereview.appspot.com/10827130/diff/22020/chrome/browser/chr... File chrome/browser/chromeos/system_logs/command_line_log_source.cc (right): https://chromiumcodereview.appspot.com/10827130/diff/22020/chrome/browser/chr... chrome/browser/chromeos/system_logs/command_line_log_source.cc:66: base::Bind(&ExecuteCommandLines, response), On 2012/08/21 21:42:50, Lei Zhang wrote: > You removed base/bind.h even though you actually use it here. Ditto for > base/logging.h for DCHECK(). Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tudalex@chromium.org/10827130/10026
Try job failure for 10827130-10026 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
http://codereview.chromium.org/10827130/diff/10026/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/10026/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:56: DCHECK(ok); Would be nice to have some message like: DCHECK(ok) << "Duplicate key found: " << it->first;
http://codereview.chromium.org/10827130/diff/10026/chrome/browser/chromeos/sy... File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right): http://codereview.chromium.org/10827130/diff/10026/chrome/browser/chromeos/sy... chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:56: DCHECK(ok); On 2012/08/22 19:55:43, satorux1 wrote: > Would be nice to have some message like: > > > DCHECK(ok) << "Duplicate key found: " << it->first; Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tudalex@chromium.org/10827130/35001
Change committed as 152856 |
