Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1193333017:
Added system log uploader. (Closed)
|
Created:
5 years, 6 months ago by Polina Bondarenko Modified:
5 years, 4 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded system log uploader.
SystemLogUploader schedules log files uploads to server every 12 hours,
in case of error makes single retry attempt after 120 sec.
SystemLogDelegate creates UploadJob for every single log upload and loads log files from the disk.
BUG=471888
Committed: https://crrev.com/9f172048dd6b0e816f13b1aa2abadc2edc6c38d2
Cr-Commit-Position: refs/heads/master@{#341533}
Committed: https://crrev.com/48e1357232e5a5a52e90819f860ec30855ff7d8f
Cr-Commit-Position: refs/heads/master@{#341718}
Patch Set 1 : #Patch Set 2 : Moved SystemLogUploadJob creation to DeviceCloudPolicyManager. #
Total comments: 44
Patch Set 3 : Fixed comments. #
Total comments: 1
Patch Set 4 : Added FileReader - helper class that reads files thread safely. #
Total comments: 41
Patch Set 5 : Fixed comments, moved SystemLogDelegate to system_log_uploader.cc #
Total comments: 28
Patch Set 6 : Fixed comments. #
Total comments: 14
Patch Set 7 : Fixed comments, moved SystemLogDelegate back to system_log_uploader.cc #Patch Set 8 : Small comment-fixes. #
Total comments: 6
Patch Set 9 : Fixed comments. #
Total comments: 8
Patch Set 10 : Fixed the last comments. #Patch Set 11 : Fixed failed browser tests. #
Messages
Total messages: 41 (18 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
pbond@chromium.org changed reviewers: + atwilson@chromium.org
Hi Andrew, please take a look at chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc chrome/browser/chromeos/policy/system_log_delegate.h chrome/browser/chromeos/policy/system_log_delegate.cc chrome/browser/chromeos/policy/system_log_upload_job.h chrome/browser/chromeos/policy/system_log_upload_job.cc chrome/browser/chromeos/policy/system_log_uploader.h chrome/browser/chromeos/policy/system_log_uploader.cc
BTW, WeakPtrFactory is for when you are creating a Callback() and you want to embed a pointer to your object, but it's possible that the Callback may outlive your object. You are using base::Unretained() which means that if your callbacks get fired after your objects are freed, you'll get a crash referencing freed data. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc (right): https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:351: task_runner_, make_scoped_ptr(new SystemLogUploadJob( I don't like having all of these dependencies exposed for everyone to have to be aware of (SystemLogDelegate() and SystemLogUploadJob() really seem like they should be internal implementation details of SystemLogUploader() and not something that DeviceCloudPolicyManager should have to know about. I'd consider one of the following: 1) Move this factory method into SystemLogUploader() (i.e. define a static Create(task_runner) method). This would let you hide SystemLogDelegate entirely and make SystemLogUploadJob an interface. 2) Remove these constructor params, and instead define a CreateSystemLogUploadJob() virtual function on SystemLogUploader() and tests can subclass/override if they need to inject their own SystemLogUploadJob(). https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h (right): https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h:145: // The TaskRunner used to do device status and logs uploads. nit: "log uploads" https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_delegate.cc (right): https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_delegate.cc:19: // File names of uploading system logs. nit: filenames of system logs to upload Put a note here not to add anything to this list without checking for PII in the file. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_delegate.cc:42: base::Bind(&SystemLogDelegate::ReadFiles, base::Unretained(this)), Why are you using base::Unretained() instead of the weak ptr factory? https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_delegate.h (right): https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_delegate.h:27: std::map<std::string, std::string> ReadFiles(); Describe what this is passing back - what are the key-value pairs. Maybe filenames + file data? What encoding is the string? utf-8? only ASCII? Unknown/raw byte values? https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_upload_job.cc (right): https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job.cc:55: &SystemLogUploadJob::StartUploadSystemLogs, base::Unretained(this))); Why use base::Unretained() instead of the weak factory? https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job.cc:60: size_t i = 1; nit: name this something like file_number or something, since it's not being used as an index, but rather as a number injected in a header. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job.cc:77: succeeded_callback_ = succeeded_callback; Should you add a DCHECK(!upload_job_) to ensure nobody calls Run() multiple times? https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job.cc:93: void SystemLogUploadJob::Terminate() { What's the use case for calling this instead of just freeing the object? https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_upload_job.h (right): https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job.h:20: const std::map<std::string, std::string>& system_logs)> LogUploadCallback; Maybe make a typedef for std::map<string, string> and document what each string is? You could then use this typedef elsewhere. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job.h:44: // Terminate system log upload. nit: blank line above https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job.h:45: void Terminate(); Why have a Terminate() function? Can the caller just free the upload job if they want to terminate it? https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_uploader.cc (right): https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.cc:58: ScheduleNextSystemLogUpload( You have a multiple line statement here, so you have to put curly braces for this if statement. i.e. this is OK if (foo) do_foo() else do_bar() But this is not OK: if (foo) do_foo( long_line_of_wrapped_params) else do_bar() You have to do this: if (foo) { do_foo( long_line_of_params) } else { do_bar() } https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.cc:63: last_success_ = !last_success_; This logic works, but I think it'd be easier to read if you just had a retrying_ flag instead of last_success_. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.cc:68: base::Bind(&SystemLogUploader::OnSuccess, base::Unretained(this)), Why use base::Unretained() here? Why do you have a weak_ptr_factory? https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.cc:71: void SystemLogUploader::ScheduleNextSystemLogUpload(base::TimeDelta frequency) { nit: blank line between methods https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.cc:78: base::Bind(&SystemLogUploader::StartLogUpload, base::Unretained(this)), Should use weak_ptr_factory instead of base::unretained() https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_uploader.h (right): https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.h:27: scoped_ptr<SystemLogUploadJob> upload_job); I don't like having a persistent upload_job that you start/stop over and over - much cleaner to just have a class that you call Run() then delete when done. Better to either have this class create its own upload_job, or if tests need to use mock upload jobs, then inject a factory. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.h:29: // Destructor. Remove this comment (and the one above) - no need to comment that these are constructors/destructors since it's clear that's what they are. Only comment on constructors if you are adding information/documentation on their use. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.h:49: // failed. Because if log upload is failed, only one retry log upload should This comment is a bit unclear. Maybe this flag should be called "retrying_" or something, and it's only set to |true| if we are doing a retry upload after an error? https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.h:56: // The time the last upload was performed. nit: "last successful upload"? Or just last upload attempt? If it's just the last upload attempt, then perhaps name this last_uplaod_attempt_ ? https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.h:62: // System log upload job. Delete this comment or make it useful. Maybe talk about when it is set?
Patchset #3 (id:80001) has been deleted
Fixed comments, got rid of Unretained(), moved the file reading to FILE thread. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc (right): https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:351: task_runner_, make_scoped_ptr(new SystemLogUploadJob( On 2015/06/29 15:06:08, Andrew T Wilson wrote: > I don't like having all of these dependencies exposed for everyone to have to be > aware of (SystemLogDelegate() and SystemLogUploadJob() really seem like they > should be internal implementation details of SystemLogUploader() and not > something that DeviceCloudPolicyManager should have to know about. > > I'd consider one of the following: > > 1) Move this factory method into SystemLogUploader() (i.e. define a static > Create(task_runner) method). This would let you hide SystemLogDelegate entirely > and make SystemLogUploadJob an interface. > > 2) Remove these constructor params, and instead define a > CreateSystemLogUploadJob() virtual function on SystemLogUploader() and tests can > subclass/override if they need to inject their own SystemLogUploadJob(). Done, I'd go with 2) https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h (right): https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h:145: // The TaskRunner used to do device status and logs uploads. On 2015/06/29 15:06:08, Andrew T Wilson wrote: > nit: "log uploads" Done. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_delegate.cc (right): https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_delegate.cc:19: // File names of uploading system logs. On 2015/06/29 15:06:08, Andrew T Wilson wrote: > nit: filenames of system logs to upload > > Put a note here not to add anything to this list without checking for PII in the > file. Done. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_delegate.cc:42: base::Bind(&SystemLogDelegate::ReadFiles, base::Unretained(this)), On 2015/06/29 15:06:08, Andrew T Wilson wrote: > Why are you using base::Unretained() instead of the weak ptr factory? The weak ptr can't be used here, because there is unknown return type of ReadFiles in case of failure. Anyway changed to PostTaskAndReply with the weak ptr. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_delegate.h (right): https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_delegate.h:27: std::map<std::string, std::string> ReadFiles(); On 2015/06/29 15:06:09, Andrew T Wilson wrote: > Describe what this is passing back - what are the key-value pairs. Maybe > filenames + file data? What encoding is the string? utf-8? only ASCII? > Unknown/raw byte values? Added comment, changed API. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_upload_job.cc (right): https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job.cc:55: &SystemLogUploadJob::StartUploadSystemLogs, base::Unretained(this))); On 2015/06/29 15:06:09, Andrew T Wilson wrote: > Why use base::Unretained() instead of the weak factory? Done, changed to weak_factory_ https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job.cc:60: size_t i = 1; On 2015/06/29 15:06:09, Andrew T Wilson wrote: > nit: name this something like file_number or something, since it's not being > used as an index, but rather as a number injected in a header. Done. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job.cc:77: succeeded_callback_ = succeeded_callback; On 2015/06/29 15:06:09, Andrew T Wilson wrote: > Should you add a DCHECK(!upload_job_) to ensure nobody calls Run() multiple > times? Done, added a comment. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job.cc:93: void SystemLogUploadJob::Terminate() { On 2015/06/29 15:06:09, Andrew T Wilson wrote: > What's the use case for calling this instead of just freeing the object? Done, removed. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_upload_job.h (right): https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job.h:20: const std::map<std::string, std::string>& system_logs)> LogUploadCallback; On 2015/06/29 15:06:09, Andrew T Wilson wrote: > Maybe make a typedef for std::map<string, string> and document what each string > is? You could then use this typedef elsewhere. Done, changed to std::vector of pairs https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job.h:44: // Terminate system log upload. On 2015/06/29 15:06:09, Andrew T Wilson wrote: > nit: blank line above Done, removed the function. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job.h:45: void Terminate(); On 2015/06/29 15:06:09, Andrew T Wilson wrote: > Why have a Terminate() function? Can the caller just free the upload job if they > want to terminate it? Done. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_uploader.cc (right): https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.cc:58: ScheduleNextSystemLogUpload( On 2015/06/29 15:06:09, Andrew T Wilson wrote: > You have a multiple line statement here, so you have to put curly braces for > this if statement. > > i.e. this is OK > > if (foo) > do_foo() > else > do_bar() > > But this is not OK: > > if (foo) > do_foo( > long_line_of_wrapped_params) > else > do_bar() > > You have to do this: > > if (foo) { > do_foo( > long_line_of_params) > } else { > do_bar() > } Done. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.cc:63: last_success_ = !last_success_; On 2015/06/29 15:06:09, Andrew T Wilson wrote: > This logic works, but I think it'd be easier to read if you just had a retrying_ > flag instead of last_success_. Done. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.cc:68: base::Bind(&SystemLogUploader::OnSuccess, base::Unretained(this)), On 2015/06/29 15:06:09, Andrew T Wilson wrote: > Why use base::Unretained() here? Why do you have a weak_ptr_factory? Understood, changed to weak_factory_ to prevent failures. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.cc:71: void SystemLogUploader::ScheduleNextSystemLogUpload(base::TimeDelta frequency) { On 2015/06/29 15:06:09, Andrew T Wilson wrote: > nit: blank line between methods Done. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.cc:78: base::Bind(&SystemLogUploader::StartLogUpload, base::Unretained(this)), On 2015/06/29 15:06:09, Andrew T Wilson wrote: > Should use weak_ptr_factory instead of base::unretained() Done. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_uploader.h (right): https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.h:27: scoped_ptr<SystemLogUploadJob> upload_job); On 2015/06/29 15:06:09, Andrew T Wilson wrote: > I don't like having a persistent upload_job that you start/stop over and over - > much cleaner to just have a class that you call Run() then delete when done. > Better to either have this class create its own upload_job, or if tests need to > use mock upload jobs, then inject a factory. Done. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.h:29: // Destructor. On 2015/06/29 15:06:09, Andrew T Wilson wrote: > Remove this comment (and the one above) - no need to comment that these are > constructors/destructors since it's clear that's what they are. Only comment on > constructors if you are adding information/documentation on their use. Done. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.h:49: // failed. Because if log upload is failed, only one retry log upload should On 2015/06/29 15:06:09, Andrew T Wilson wrote: > This comment is a bit unclear. Maybe this flag should be called "retrying_" or > something, and it's only set to |true| if we are doing a retry upload after an > error? Done. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.h:56: // The time the last upload was performed. On 2015/06/29 15:06:09, Andrew T Wilson wrote: > nit: "last successful upload"? Or just last upload attempt? > > If it's just the last upload attempt, then perhaps name this > last_uplaod_attempt_ ? Done. Renamed to last_upload_attempt_. https://codereview.chromium.org/1193333017/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.h:62: // System log upload job. On 2015/06/29 15:06:09, Andrew T Wilson wrote: > Delete this comment or make it useful. Maybe talk about when it is set? Done.
Patchset #3 (id:100001) has been deleted
Patchset #4 (id:140001) has been deleted
https://codereview.chromium.org/1193333017/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_delegate.cc (right): https://codereview.chromium.org/1193333017/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_delegate.cc:44: // and return to the current thread. The weak ptr is not thread safe, it should not be used here. Fixed by moving the file read to a new thread safe class.
Patchset #4 (id:160001) has been deleted
https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc (right): https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:348: syslog_uploader_.reset(new SystemLogUploader(task_runner_)); This is OK to leave as-is, but in general I don't usually create factory methods that are one line since they aren't really saving lines of code, especially if they are called only once. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_delegate.cc (right): https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_delegate.cc:53: // Owned by reply callback posted below. Can you add some DCHECKs() for which thread you expect each of these to be invoked on? https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_delegate.h (right): https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_delegate.h:14: class FileReader; Can this be an inner class inside SystemLogDelegate instead of at the top level of policy? (i.e. SystemLogDelegate::FileReader) https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_upload_job.cc (right): https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:34: : upload_url_(GURL(kDefaultUploadUrl)), Why make this a data member? Why not just use GURL(kDefaultUploadUrl) below? https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:43: void SystemLogUploadJob::OnSuccess() { Why are we posting a task here instead of just invoking the callback? And why call base::Bind() on the callback? https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:50: base::Bind(failed_callback_)); Should we pass the error code to the callback? Why are we calling base:Bind()? https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:59: size_t file_number = 1; Can this be an int instead? ints are preferred if they are possible (and they should be possible here) https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:80: // Ensure nobody calls Run() multiple times. Document in header file how this class should be used? Also, maybe pass callbacks in constructor, not in Run()? https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:87: if (!upload_url_.is_valid()) { Why would the upload_url_ be invalid, since it's just GURL(kDefaultUploadUrl)? Should this be a DCHECK instead of handling it at runtime? Since it's a constant, won't it always be either valid or invalid for a given binary? https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_upload_job.h (right): https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:16: // Class responsible for periodically uploading system logs. Is this true? Is this class responsible for doing a single log upload, or for doing multiple ones periodically? https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:23: typedef base::Callback<void(const SystemLogs* system_logs)> LogUploadCallback; Pass read-only objects as const SystemLogs& instead of as a ptr. Also, move this into Delegate. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:25: class Delegate { Useful to explain why you are declaring this Delegate class. Is it because you want to override these for tests? https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:43: virtual void Run(const base::Closure& succeeded_callback, Why is this virtual? https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:59: GURL upload_url_; why are we caching this instead of creating it every time we do an upload? https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.cc (right): https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:31: upload_job_.reset(); Why is this here? Doesn't upload_job_ start as NULL? https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:35: ScheduleNextSystemLogUpload(upload_frequency_); This means that if you have your device setup to reboot every 11 hours (or it crashes frequently), it will never upload logs, correct? Just wonder if maybe we should immediately upload logs after every reboot? https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:39: upload_job_.reset(); Why do this here instead of letting the default destructor handle it? https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:43: return new SystemLogUploadJob(make_scoped_ptr(new SystemLogDelegate())); Is SystemLogDelegate used outside this file? Should we move it to an anonymous namespace in this file? https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:69: retrying_ = !retrying_; The logic with retry is somewhat confusing. And it's weird to set retrying_ to true in the constructor since you haven't even started doing an upload yet so you can't be retrying. Maybe just make it retry_count_, initialize it to zero, reset to zero on every successful upload, increment it on every error, and have a kMaxNumRetries = 1, so the logic in OnFailure looks like: if (retry_count_++ < kMaxNumRetries) { ScheduleNext(kErrorUploadDelayMs); } else { // No more retries retry_count = 0; ScheduleNextSystemLogUpload(upload_frequency); } That's easier to parse the logic I think. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.h (right): https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.h:38: // Handles success and failure results of upload. If these are callbacks, you should say so here.
Patchset #5 (id:200001) has been deleted
Fixed comments. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc (right): https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:348: syslog_uploader_.reset(new SystemLogUploader(task_runner_)); On 2015/07/03 15:40:47, Andrew T Wilson wrote: > This is OK to leave as-is, but in general I don't usually create factory methods > that are one line since they aren't really saving lines of code, especially if > they are called only once. Removed the factory method. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_delegate.cc (right): https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_delegate.cc:53: // Owned by reply callback posted below. On 2015/07/03 15:40:47, Andrew T Wilson wrote: > Can you add some DCHECKs() for which thread you expect each of these to be > invoked on? Done. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_delegate.h (right): https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_delegate.h:14: class FileReader; On 2015/07/03 15:40:47, Andrew T Wilson wrote: > Can this be an inner class inside SystemLogDelegate instead of at the top level > of policy? > > (i.e. SystemLogDelegate::FileReader) Done. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_upload_job.cc (right): https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:34: : upload_url_(GURL(kDefaultUploadUrl)), On 2015/07/03 15:40:47, Andrew T Wilson wrote: > Why make this a data member? Why not just use GURL(kDefaultUploadUrl) below? Removed. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:43: void SystemLogUploadJob::OnSuccess() { On 2015/07/03 15:40:47, Andrew T Wilson wrote: > Why are we posting a task here instead of just invoking the callback? And why > call base::Bind() on the callback? Removed PostTask and Bind. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:50: base::Bind(failed_callback_)); On 2015/07/03 15:40:47, Andrew T Wilson wrote: > Should we pass the error code to the callback? Why are we calling base:Bind()? I don't use the error_code in the system log uploader now. There are 4 errors: server, network, content encoding and authentication. Should the retry-behavior be different according to the error type? Removed PostTask and Bind. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:59: size_t file_number = 1; On 2015/07/03 15:40:47, Andrew T Wilson wrote: > Can this be an int instead? ints are preferred if they are possible (and they > should be possible here) Done. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:80: // Ensure nobody calls Run() multiple times. On 2015/07/03 15:40:47, Andrew T Wilson wrote: > Document in header file how this class should be used? > > Also, maybe pass callbacks in constructor, not in Run()? Moved callbacks to constructor. Added comment. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:87: if (!upload_url_.is_valid()) { On 2015/07/03 15:40:47, Andrew T Wilson wrote: > Why would the upload_url_ be invalid, since it's just GURL(kDefaultUploadUrl)? > > Should this be a DCHECK instead of handling it at runtime? Since it's a > constant, won't it always be either valid or invalid for a given binary? Fixed. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_upload_job.h (right): https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:16: // Class responsible for periodically uploading system logs. On 2015/07/03 15:40:47, Andrew T Wilson wrote: > Is this true? Is this class responsible for doing a single log upload, or for > doing multiple ones periodically? Fixed comment to single log upload. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:23: typedef base::Callback<void(const SystemLogs* system_logs)> LogUploadCallback; On 2015/07/03 15:40:48, Andrew T Wilson wrote: > Pass read-only objects as const SystemLogs& instead of as a ptr. > > Also, move this into Delegate. Yes, having const& instead of const* is more convenient, but I need this ptr to be owned and destroyed by LogUploadCallback. Moved to Delegate. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:25: class Delegate { On 2015/07/03 15:40:48, Andrew T Wilson wrote: > Useful to explain why you are declaring this Delegate class. Is it because you > want to override these for tests? Yes, it is needed for tests and separate some functionality from the system log upload job. Added a comment. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:43: virtual void Run(const base::Closure& succeeded_callback, On 2015/07/03 15:40:48, Andrew T Wilson wrote: > Why is this virtual? Yes, it is overridden in the tests. Removed 'virtual'. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:59: GURL upload_url_; On 2015/07/03 15:40:47, Andrew T Wilson wrote: > why are we caching this instead of creating it every time we do an upload? Fixed, don't need anymore. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.cc (right): https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:31: upload_job_.reset(); On 2015/07/03 15:40:48, Andrew T Wilson wrote: > Why is this here? Doesn't upload_job_ start as NULL? Done. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:35: ScheduleNextSystemLogUpload(upload_frequency_); On 2015/07/03 15:40:48, Andrew T Wilson wrote: > This means that if you have your device setup to reboot every 11 hours (or it > crashes frequently), it will never upload logs, correct? Just wonder if maybe we > should immediately upload logs after every reboot? Sounds good, let's upload immediately. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:39: upload_job_.reset(); On 2015/07/03 15:40:48, Andrew T Wilson wrote: > Why do this here instead of letting the default destructor handle it? Done. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:43: return new SystemLogUploadJob(make_scoped_ptr(new SystemLogDelegate())); On 2015/07/03 15:40:48, Andrew T Wilson wrote: > Is SystemLogDelegate used outside this file? Should we move it to an anonymous > namespace in this file? Yes, fixed. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:69: retrying_ = !retrying_; On 2015/07/03 15:40:48, Andrew T Wilson wrote: > The logic with retry is somewhat confusing. And it's weird to set retrying_ to > true in the constructor since you haven't even started doing an upload yet so > you can't be retrying. > > Maybe just make it retry_count_, initialize it to zero, reset to zero on every > successful upload, increment it on every error, and have a kMaxNumRetries = 1, > so the logic in OnFailure looks like: > > if (retry_count_++ < kMaxNumRetries) { > ScheduleNext(kErrorUploadDelayMs); > } else { > // No more retries > retry_count = 0; > ScheduleNextSystemLogUpload(upload_frequency); > } > > That's easier to parse the logic I think. Done. https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.h (right): https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.h:38: // Handles success and failure results of upload. On 2015/07/03 15:40:48, Andrew T Wilson wrote: > If these are callbacks, you should say so here. Done.
Patchset #5 (id:220001) has been deleted
So, consider whether you can get rid of SystemLogUploadJob::Delegate in favor of pre-loading that stuff in SystemLogUploader and passing it in to Run(). https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_upload_job.h (right): https://codereview.chromium.org/1193333017/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:23: typedef base::Callback<void(const SystemLogs* system_logs)> LogUploadCallback; On 2015/07/08 10:07:23, Polina Bondarenko wrote: > On 2015/07/03 15:40:48, Andrew T Wilson wrote: > > Pass read-only objects as const SystemLogs& instead of as a ptr. > > > > Also, move this into Delegate. > > Yes, having const& instead of const* is more convenient, but I need this ptr to > be owned and destroyed by LogUploadCallback. > > Moved to Delegate. OK, understood. You are passing a pointer and binding it to the callback in a way that will cause the pointer to be freed when the callback is destroyed. Makes sense - perhaps pass it as a scoped_ptr to make it explicit? https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_upload_job.cc (right): https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:16: // String constant defining the upload url. So, I would not call it the "default upload url" because it's not the default, but rather *is* the upload url. Maybe call it the kSystemLogUploadUrl and comment it by saying this is the URL we upload system logs to or something. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:51: base::ResetAndReturn(&succeeded_callback_).Run(); comment that we may be freed now. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:57: base::ResetAndReturn(&failed_callback_).Run(); comment that the callback may free this object so we should not access the object any further. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:67: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); The more I think about this, I think what you really want is for your class to have a base::ThreadChecker member, and then call thread_checker_.CalledOnValidThread() in these various member functions. You don't really care, per se, that you're on the UI thread, just that you aren't accidentally invoked from a different thread than Run() was called on. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_upload_job.h (right): https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:33: virtual void LoadSystemLogs(const LogUploadCallback& upload_callback) = 0; One question - why do we have a Delegate class? Why not just pass in SystemLogs and an UploadJob to Run()? It seems unnecessarily complicated to call back to a delegate. Just leave it to the caller to know the upload URL for creating the upload job and to fetch the logs before calling Run(logs, upload_job)? https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:36: // may be called any number of times. Either document under what situation it may be called multiple times, or else document that it should only be called once (when Run() is called). https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:37: virtual scoped_ptr<UploadJob> CreateUploadJob(const GURL&, nit: this is fine, but I still usually put names with the variables. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:38: UploadJob::Delegate*) = 0; It's vaguely confusing that you pass an UploadJob::Delegate to an UploadJob::Delegate::Delegate. It's fine, though - just amusing that we are defining a Delegate class in a class that is itself an implementation of a Delegate interface. Wonder if this would be clearer if you passed in a SystemLogUploadJob here instead of an UploadJob::Delegate, if you know you are only passing in SystemLogUploadJobs. Maybe the confusion is that you've named the parent class SystemLogUploadJob, when it's not actually an UploadJob, it's an UploadJob::Delegate. So I'd suggest calling this SystemLogUploadJobDelegate, that way when you say things like "Run is called only once for every created upload job", I know whether you are talking about SystemLogUploadJobDelegate, or one of the UploadJobs returned by CreateUploadJob(). https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:47: // Starts the system log upload, could be called only once for every created Nit: should only be called once on any SystemLogUploadJob. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:53: void OnSuccess() override; Think these should be public to match their definition in your derived class, or at most protected? https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:64: // uploaded. It destroys the job. note instead that the callback *may* destroy this object. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:67: // The callback that will be called when system log upload failed. It destroys Ditto - note that invoking this callback can free the object https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.cc (right): https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:27: // The maximum number of consequent retries. nit: maybe use a different word than consequent that's more clear? https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:81: base::CancelableTaskTracker::TaskId SystemLogDelegate::FileReader::StartRead( Why do we return a TaskId here - looks like it's ignored?
pbond@chromium.org changed reviewers: + cschuet@chromium.org
Fixed comments. Rewrote structure: Cuurently we have SystemLogUploader that is UploadJob::Delegate and handles the server responses itself in OnSuccess and OnFailure callbacks. Schedules log uploads, initiates loading system logs and starts log uploads to server. SystemLogDelegate - is SystemLogUploader::Delegate - creates the upload job for every log upload and loads thread safely log files from the disk. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_upload_job.cc (right): https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:16: // String constant defining the upload url. On 2015/07/17 15:43:57, Andrew T Wilson wrote: > So, I would not call it the "default upload url" because it's not the default, > but rather *is* the upload url. Maybe call it the kSystemLogUploadUrl and > comment it by saying this is the URL we upload system logs to or something. Done. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:51: base::ResetAndReturn(&succeeded_callback_).Run(); On 2015/07/17 15:43:57, Andrew T Wilson wrote: > comment that we may be freed now. Done. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:57: base::ResetAndReturn(&failed_callback_).Run(); On 2015/07/17 15:43:57, Andrew T Wilson wrote: > comment that the callback may free this object so we should not access the > object any further. Done. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.cc:67: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2015/07/17 15:43:57, Andrew T Wilson wrote: > The more I think about this, I think what you really want is for your class to > have a base::ThreadChecker member, and then call > thread_checker_.CalledOnValidThread() in these various member functions. You > don't really care, per se, that you're on the UI thread, just that you aren't > accidentally invoked from a different thread than Run() was called on. Done. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_upload_job.h (right): https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:33: virtual void LoadSystemLogs(const LogUploadCallback& upload_callback) = 0; On 2015/07/17 15:43:57, Andrew T Wilson wrote: > One question - why do we have a Delegate class? Why not just pass in SystemLogs > and an UploadJob to Run()? It seems unnecessarily complicated to call back to a > delegate. Just leave it to the caller to know the upload URL for creating the > upload job and to fetch the logs before calling Run(logs, upload_job)? Removed SystemLogUploadJob at all. This Delegate is moved to SystemLogUploader. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:36: // may be called any number of times. On 2015/07/17 15:43:57, Andrew T Wilson wrote: > Either document under what situation it may be called multiple times, or else > document that it should only be called once (when Run() is called). After changing structure this delegate (SystemLogDelegate) is created only once on SystemLogUploader creation. And re-creates UploadJobs on every system log upload. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:37: virtual scoped_ptr<UploadJob> CreateUploadJob(const GURL&, On 2015/07/17 15:43:57, Andrew T Wilson wrote: > nit: this is fine, but I still usually put names with the variables. Done. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:38: UploadJob::Delegate*) = 0; On 2015/07/17 15:43:57, Andrew T Wilson wrote: > It's vaguely confusing that you pass an UploadJob::Delegate to an > UploadJob::Delegate::Delegate. It's fine, though - just amusing that we are > defining a Delegate class in a class that is itself an implementation of a > Delegate interface. > > Wonder if this would be clearer if you passed in a SystemLogUploadJob here > instead of an UploadJob::Delegate, if you know you are only passing in > SystemLogUploadJobs. > > Maybe the confusion is that you've named the parent class SystemLogUploadJob, > when it's not actually an UploadJob, it's an UploadJob::Delegate. So I'd suggest > calling this SystemLogUploadJobDelegate, that way when you say things like "Run > is called only once for every created upload job", I know whether you are > talking about SystemLogUploadJobDelegate, or one of the UploadJobs returned by > CreateUploadJob(). Now it is moved to SystemLogUploader and SystemLogUploader is delegate of UploadJob. It is only for handling UploadJob::Delegate callbacks to receive responses from server. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:47: // Starts the system log upload, could be called only once for every created On 2015/07/17 15:43:57, Andrew T Wilson wrote: > Nit: should only be called once on any SystemLogUploadJob. Fixed comment. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:53: void OnSuccess() override; On 2015/07/17 15:43:57, Andrew T Wilson wrote: > Think these should be public to match their definition in your derived class, or > at most protected? Moved to the public section. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:64: // uploaded. It destroys the job. On 2015/07/17 15:43:57, Andrew T Wilson wrote: > note instead that the callback *may* destroy this object. Removed this class and callbacks at all. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_upload_job.h:67: // The callback that will be called when system log upload failed. It destroys On 2015/07/17 15:43:57, Andrew T Wilson wrote: > Ditto - note that invoking this callback can free the object Removed this class and callbacks at all. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.cc (right): https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:27: // The maximum number of consequent retries. On 2015/07/17 15:43:58, Andrew T Wilson wrote: > nit: maybe use a different word than consequent that's more clear? Changed to 'successive retries'. https://codereview.chromium.org/1193333017/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:81: base::CancelableTaskTracker::TaskId SystemLogDelegate::FileReader::StartRead( On 2015/07/17 15:43:57, Andrew T Wilson wrote: > Why do we return a TaskId here - looks like it's ignored? Yes, removed.
Mostly looks good. I have to run to a meeting before finishing but I wanted to give you my existing comments. https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc (right): https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:137: class FileReader : public base::RefCountedThreadSafe<FileReader> { Since this class doesn't have any actual data members, do you need a class here? Can you just define static StartRead and Read functions? That's always preferable to having a class that has to span threads. https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:140: void StartRead(const LogUploadCallback& upload_callback, Document what this does/what should be passed. https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:173: scoped_ptr<policy::UploadJob> SystemLogDelegate::CreateUploadJob( So, it's weird to me that this code lives here in DeviceCloudPolicyManagerChromeOS - why should this class be responsible for creating upload jobs, reading log files, etc? Why doesn't this all live off in SystemLogUploader? Note that for DeviceStatusCollector, we pass in a bunch of null pointers for the various delegates/callbacks so it knows to use the default implementation. This allows us to still inject implementations for tests, but DeviceCloudPolicyManager doesn't need to know these details. Think we should do that in this case as well. https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:200: FROM_HERE, base::Bind(&FileReader::Read, this, system_logs), I don't understand why we need a FileReader class. Can we just use static functions? https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:212: if (!base::ReadFileToString(base::FilePath(file_path), We also want to add something that scans for common PII (email address, IP address) and strips it. can you log a bug for this and add a TODO() to the appropriate place in the code? https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.h (right): https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.h:44: // should be only called once per every scheduled system log upload. Not "should be" but "will be called exactly once per every scheduled system log upload". BTW, what if the upload fails and we have to retry - will we create a new UploadJob in that case, or re-use the existing one? If the former, then we should get rid of the word "scheduled" in the comment, and say "once per every system log upload". If we re-use the existing one on a retry, then we should explicitly say that.
One more comment https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.cc (right): https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:79: // time from now. nit: This comment will get obsolete if kMaxNumRetries is > 1. Probably better to say "If we have hit the maximum number of retries, terminate this upload attempt and schedule the next one using the normal delay. Otherwise, retry uploading after kErrorUploadDelayMs milliseconds".
Patchset #7 (id:270001) has been deleted
Fixed comments, SystemLogDelegate moved back to system_log_uploader.cc https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc (right): https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:137: class FileReader : public base::RefCountedThreadSafe<FileReader> { On 2015/07/31 12:07:13, Andrew T Wilson wrote: > Since this class doesn't have any actual data members, do you need a class here? > Can you just define static StartRead and Read functions? That's always > preferable to having a class that has to span threads. Done, removed StartRead function, moved its functionality to LoadSystemLogs. https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:140: void StartRead(const LogUploadCallback& upload_callback, On 2015/07/31 12:07:13, Andrew T Wilson wrote: > Document what this does/what should be passed. Removed this function, for LoadSystemLogs function it is documented. https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:173: scoped_ptr<policy::UploadJob> SystemLogDelegate::CreateUploadJob( On 2015/07/31 12:07:13, Andrew T Wilson wrote: > So, it's weird to me that this code lives here in > DeviceCloudPolicyManagerChromeOS - why should this class be responsible for > creating upload jobs, reading log files, etc? Why doesn't this all live off in > SystemLogUploader? > > Note that for DeviceStatusCollector, we pass in a bunch of null pointers for the > various delegates/callbacks so it knows to use the default implementation. This > allows us to still inject implementations for tests, but > DeviceCloudPolicyManager doesn't need to know these details. Think we should do > that in this case as well. Done, mpved to system_log_uploader.cc https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:200: FROM_HERE, base::Bind(&FileReader::Read, this, system_logs), On 2015/07/31 12:07:13, Andrew T Wilson wrote: > I don't understand why we need a FileReader class. Can we just use static > functions? Done. https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:212: if (!base::ReadFileToString(base::FilePath(file_path), On 2015/07/31 12:07:13, Andrew T Wilson wrote: > We also want to add something that scans for common PII (email address, IP > address) and strips it. can you log a bug for this and add a TODO() to the > appropriate place in the code? Done. https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.cc (right): https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:79: // time from now. On 2015/07/31 13:05:06, Andrew T Wilson wrote: > nit: This comment will get obsolete if kMaxNumRetries is > 1. > > Probably better to say "If we have hit the maximum number of retries, terminate > this upload attempt and schedule the next one using the normal delay. Otherwise, > retry uploading after kErrorUploadDelayMs milliseconds". Done. https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.h (right): https://codereview.chromium.org/1193333017/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.h:44: // should be only called once per every scheduled system log upload. On 2015/07/31 12:07:13, Andrew T Wilson wrote: > Not "should be" but "will be called exactly once per every scheduled system log > upload". > > BTW, what if the upload fails and we have to retry - will we create a new > UploadJob in that case, or re-use the existing one? If the former, then we > should get rid of the word "scheduled" in the comment, and say "once per every > system log upload". If we re-use the existing one on a retry, then we should > explicitly say that. Fixed, we create new upload job for every upload attempt.
LGTM with a couple of comments. https://codereview.chromium.org/1193333017/diff/310001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.cc (right): https://codereview.chromium.org/1193333017/diff/310001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:60: policy::SystemLogUploader::SystemLogs system_logs; This is going to make multiple copies of all of the file data, isn't it (when you return this to the caller)? Not a huge issue, but still would be nice to structure this not to do this (pass in a pre-allocated vector to fill in, or else have this return a scoped_ptr<SystemLogs> or something like that)? https://codereview.chromium.org/1193333017/diff/310001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:70: // etc.) and not upload |data| if any is found. http://crbug.com/515879. Better than not uploading data would be to modify the data to remove/obfuscate the PII. https://codereview.chromium.org/1193333017/diff/310001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:211: void SystemLogUploader::ScheduleNextSystemLogUpload(base::TimeDelta frequency) { BTW, would be nice if we had a DCHECK in place to ensure we never have more than one pending delayed task. Is that possible? Alternatively, you could just reset your weak_factory_ in this routine.
Hey Andrew, I fixed your comments, could you please take a last quick look at it? https://codereview.chromium.org/1193333017/diff/310001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.cc (right): https://codereview.chromium.org/1193333017/diff/310001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:60: policy::SystemLogUploader::SystemLogs system_logs; On 2015/08/03 09:55:50, Andrew T Wilson wrote: > This is going to make multiple copies of all of the file data, isn't it (when > you return this to the caller)? Not a huge issue, but still would be nice to > structure this not to do this (pass in a pre-allocated vector to fill in, or > else have this return a scoped_ptr<SystemLogs> or something like that)? Done. https://codereview.chromium.org/1193333017/diff/310001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:70: // etc.) and not upload |data| if any is found. http://crbug.com/515879. On 2015/08/03 09:55:50, Andrew T Wilson wrote: > Better than not uploading data would be to modify the data to remove/obfuscate > the PII. Done. https://codereview.chromium.org/1193333017/diff/310001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:211: void SystemLogUploader::ScheduleNextSystemLogUpload(base::TimeDelta frequency) { On 2015/08/03 09:55:50, Andrew T Wilson wrote: > BTW, would be nice if we had a DCHECK in place to ensure we never have more than > one pending delayed task. Is that possible? Alternatively, you could just reset > your weak_factory_ in this routine. Done, added weak_factory_.reset, because we use the same task_runner_ as the status uploader.
https://codereview.chromium.org/1193333017/diff/330001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.cc (right): https://codereview.chromium.org/1193333017/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:219: weak_ptr_factory_.reset(new base::WeakPtrFactory<SystemLogUploader>(this)); Let's just call InvalidateWeakPtrs() instead of reallocating a new WeakPtrFactory. https://codereview.chromium.org/1193333017/diff/330001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.h (right): https://codereview.chromium.org/1193333017/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.h:35: typedef base::Callback<void(const scoped_ptr<SystemLogs> system_logs)> remove const https://codereview.chromium.org/1193333017/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.h:74: void UploadSystemLogs(const scoped_ptr<SystemLogs> system_logs); remove const https://codereview.chromium.org/1193333017/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.h:102: scoped_ptr<base::WeakPtrFactory<SystemLogUploader>> weak_ptr_factory_; this should not be a scoped_ptr
The CQ bit was checked by pbond@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from atwilson@chromium.org Link to the patchset: https://codereview.chromium.org/1193333017/#ps350001 (title: "Fixed the last comments.")
Thanks for reviewing. https://codereview.chromium.org/1193333017/diff/330001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.cc (right): https://codereview.chromium.org/1193333017/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:219: weak_ptr_factory_.reset(new base::WeakPtrFactory<SystemLogUploader>(this)); On 2015/08/03 13:15:29, Andrew T Wilson wrote: > Let's just call InvalidateWeakPtrs() instead of reallocating a new > WeakPtrFactory. Done. https://codereview.chromium.org/1193333017/diff/330001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.h (right): https://codereview.chromium.org/1193333017/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.h:35: typedef base::Callback<void(const scoped_ptr<SystemLogs> system_logs)> On 2015/08/03 13:15:29, Andrew T Wilson wrote: > remove const Done. https://codereview.chromium.org/1193333017/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.h:74: void UploadSystemLogs(const scoped_ptr<SystemLogs> system_logs); On 2015/08/03 13:15:29, Andrew T Wilson wrote: > remove const Done. https://codereview.chromium.org/1193333017/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.h:102: scoped_ptr<base::WeakPtrFactory<SystemLogUploader>> weak_ptr_factory_; On 2015/08/03 13:15:29, Andrew T Wilson wrote: > this should not be a scoped_ptr Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1193333017/350001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1193333017/350001
Message was sent while issue was closed.
Committed patchset #10 (id:350001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/9f172048dd6b0e816f13b1aa2abadc2edc6c38d2 Cr-Commit-Position: refs/heads/master@{#341533}
Message was sent while issue was closed.
Patchset #11 (id:370001) has been deleted
The CQ bit was checked by pbond@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from atwilson@chromium.org Link to the patchset: https://codereview.chromium.org/1193333017/#ps390001 (title: "Fixed failed browser tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1193333017/390001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1193333017/390001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by pbond@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1193333017/390001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1193333017/390001
Message was sent while issue was closed.
Committed patchset #11 (id:390001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/48e1357232e5a5a52e90819f860ec30855ff7d8f Cr-Commit-Position: refs/heads/master@{#341718} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
