Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2603523002:
Move net-export thread-hopping code into NetLogFileWriter and add IO polled data. (Closed)
DescriptionMove net-export thread-hopping code into NetLogFileWriter and add IO polled data.
(1) Use net::FileNetLogObserver in place of net::WriteToFileNetLogObserver for chrome://net-export (which is a big part of crbug.com/679030).
(2) Move NetLogFileWriter's public interface from the FILE_USER_BLOCKING to the UI thread (internalizing the post tasks to file thread/UI thread to simplify things)
(3) Add plumbing for passing polled data, which will be collected on both the UI thread and IO thread (crbug.com/438656). Some of the refactor done will also facilitate crbug.com/679021, which is just a few extra lines. In this CL, only the polled data from the IO thread will be retrieved; the UI thread polled data will be added in a follow-up CL.
BUG=679030, 438656, 679021
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Review-Url: https://codereview.chromium.org/2603523002
Cr-Commit-Position: refs/heads/master@{#446875}
Committed: https://chromium.googlesource.com/chromium/src/+/8ae679d977a7eb3c784d7bc6483cdf74400b52bd
Patch Set 1 #Patch Set 2 : Updated net_export_ui.cc to work with the updated NetLogFileWriter #Patch Set 3 : Various refactors to make code cleaner #
Total comments: 75
Patch Set 4 : Fixed nits; callbacks are now always executed regardless of failure or no-op #
Total comments: 54
Patch Set 5 : Fixed Eric's comments; all callbacks are now async #
Total comments: 20
Patch Set 6 : Updated NetLogFileWriter unit-test to work with new NetLogFileWriter #Patch Set 7 : Fixed Eric's nits. StartNetLog() callback will now wait til after an already-ongoing initialization. #Patch Set 8 : Updated android cronet and ios net_export_ui.cc #
Total comments: 15
Patch Set 9 : Fixed Eric's comments from patchset 8 #
Total comments: 19
Patch Set 10 : Fixed Eric's comments from patchset 9 about unit-test. #Patch Set 11 : GetState() now waits until after ongoing initialization finishes to run callback #Patch Set 12 : Fixed free-after-return issue FileWriterGetFilePathToCompletedLog() #
Total comments: 18
Patch Set 13 : Fixed Eric's comments from patchset 12 #
Total comments: 4
Patch Set 14 : Fixed tommycli's nits from patchset 13 #Dependent Patchsets: Messages
Total messages: 66 (38 generated)
wangyix@google.com changed reviewers: + eroman@chromium.org
This is a very rough attempt at moving all the thread-hopping code into NetLogFileWriter. It currently does NOT have any code for grabbing the polled_data; the major change is that NetLogFileWriter now has thread-hopping to handle the file-related tasks in its initialization code. PTAL.
Description was changed from ========== Initial attempt at updating NetLogFileWriter to post to file thread BUG= ========== to ========== Initial attempt at updating NetLogFileWriter to handle all the thread-hopping logic. In this implementation, the NetLogFileWriter will live on the UI thread instead of the IO thread as suggested here: https://docs.google.com/document/d/1nhA0PmkDYFGbmc3Ny98IEVQprZTyEcD457Jc4xoLp... The motivation for this is the following: The NetLogFileWriter has three possible states: STATE_UNINITIALIZED, STATE_LOGGING, and STATE_NOT_LOGGING. When the NetLogFileWriter receives a command (e.g. StartNetLog() or StopNetLog()), then depending on its current state and the command received, it will need to carry out a different sequence of thread-hops. For example, if StartNetLog() is called while NetLogFileWriter is in STATE_UNINITIALIZED, it will need to perform initialization steps on the FILE thread and then return to the main thread to create and attach a new observer. If StopNetLog() is called while NetLogFileWriter is in STATE_LOGGING, then it will need to grab the polled_data from the IO thread, and then return to the main thread to stop the observer. Because the sequence of threads required by NetLogFileWriter depends on what state it was in when a command is received, it makes sense to have that state live on the UI thread (where NetExportMessageHandler lives, which controls the NetLogFileWriter). If NetLogFileWriter is put on the IO thread, then every command from NetExportMessageHandler must first be posted to the IO thread in order to check the state of NetLogFileWriter before it can determine which threads it needs to visit to complete the command. This extra visit to the IO thread is unnecessary and will further complicate the code. ==========
wangyix@google.com changed reviewers: + mmenke@chromium.org, xunjieli@chromium.org
Added more reviewers, PTAL. NetLogFileWriter has been refactored, and NetExportMessageHandler has been updated to work with the new NetLogFileWriter. The NetLogFileWriter unit tests have not been updated. As of patchset 2, polled_data has been added to net-export, but it only includes the info from net::GetNetInfo(). The other info (e.g. browser-specific stuff) has yet to be added.
The overall design looks good! I have a number of comments on the implementation details. Can you simplify the changelist description to enumerate all the changes this is making? By my count there are 3 big changes in this CL: (1) Use net::FileNetLogObserver in place of net::WriteToFileNetLogObserver for chrome://net-export (which is a big part of crbug.com/679030) (2) Move NetLogFileWriter's public interface from the FILE_USER_BLOCKING thread to the UI thread (internalizing the post tasks to file thread/UI thread to simplify things) (3) Add plumbing for for passing polled data, collected on both the UI thread and IO thread (crbug.com/438656). Some of the refactor done will also facilitate crbug.com/679021, which is just a few extra lines. https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/net_export_ui.cc (right): https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:65: // functions except SendEmail run on FILE_USER_BLOCKING thread. This comment is no longer true. https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:90: void StartNetLogAndNotifyUI(const base::FilePath& log_path, I suggest naming this "StartNetLogThenNotifyUI" (Replace And with Then to clarify that it happens sequentially rather than in parallel) https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:93: void StopNetLogAndNotifyUI(); Same comment here. https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:95: // Send NetLog data via email. This runs on UI thread. The mention of "UI" thread is probably redundant, now that everything runs on UI thread in here. https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:110: // Call NetExportView.onExportNetLogInfoChanged JavsScript function in the While you are editing this, could you change "Call" to "Calls" ? https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:122: // posted to the FILE_USER_BLOCKING thread. Base::Unretained is used here This comment is no longer accurate. https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:132: // here. Please explain the purpose of this variable, and its validity range (i.e. this is only really valid while the file dialog is open on the desktop UI). An alternative would be to pass the capture mode directly to SelectFile(), and not need this member -- SelectFile takes a params pointer which it will pass back to the delegate in FileSelected(). https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:145: if (!net_log_file_writer_->GetFileTaskRunner()) { Since these will always be set in unison, how about unconditionally doing: net_log_file_writer_->SetTaskRunners(....FILE_USER_BLOCKING, ....IO); And inside of the implementation for SetTaskRunners(), if the task runners have not been assigned yet it does so, otherwise it asserts that they are the same as the ones passed in. https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:203: if (capture_mode_string == "LOG_BYTES") { Can we internalize the conversion from "capture mode string" --> capture mode in the NetLogFileWriter? * NetLogFileWriter has code that does the inverse mapping (from capture mode to string), so makes sense for the code to live side by side * The iOS frontend will need to repeat this code if it lives in the message handler. Can either add a helper function, or make StartNetLog() take the parameter as as string again. https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:242: net_log_file_writer_->SetUpNetExportLogPath(log_path); Rather than a separate method SetUpNetExportLogPath(), can you move this to a parameter to the call below net_log_file_writer_->StartNetLog(...) ? https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:254: // fill polled_data with browser-specific polled data. Please add a "TODO(crbug.com/438656)" here https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... File components/net_log/net_log_file_writer.cc (right): https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:55: thread_checker_.DetachFromThread(); Is this necessary? I believe the creation and destruction happens from the UI thread (as do all the public entry points), so this is no longer needed. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:87: write_to_file_observer_->StartObservingUnbounded(chrome_net_log_, StartLogging() will eventually need to take a URLRequestContextGetter parameter (crbug.com/679021) Don't need to address that in this changelist, but something to be aware of in the design I think StartLogging() will want to eventually take a URLRequestContextGetter, and do a thread-hop to the IO thread in order to start logging here. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:125: DCHECK_EQ(STATE_LOGGING, state_); I don't think this DCHECK holds() -- Let's say StopNetLog() was called at about the same time from 2 different tabs. One of them would hit this DCHECK(). https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:127: base::Bind([] {})); Shouldn't we wait for this to complete before reporting completion of StopLogging? In particular, is there a possible race between stopping logging, transitioning the state to STATE_NOT_LOGGING, and then emailing the file (however the file has not finished writing yet). https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:173: dict->SetString("captureMode", "LOG_BYTES"); Formatting. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:180: state_callback.Run(std::unique_ptr<base::DictionaryValue>(GetState())); See comment about making GetState() return a unique_ptr https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:186: return; Would it make sense to always invoke the callback, but possibly with an empty path to indicate "failure"? https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:211: if (runCallback) hacker_style_notation rather than camelCaseNotation https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... File components/net_log/net_log_file_writer.h (right): https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:36: // NetLogFileWriter logs all the NetLog entries into a specified file. Please add a description for the threading model: * This class is created and destroyed on the UI thread, and all public entry points are to be called on the UI thread * However internally the class may run some code on the file_task_runner_ and net_task_runner_. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:45: class NetLogFileWriter : public base::SupportsWeakPtr<NetLogFileWriter> { Why is SupportsWeakPtr<> needed? (the weak_ptr_factory_ member added should be sufficient). https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:48: virtual ~NetLogFileWriter(); Does this class still need a vtable? The only virtual method (GetNetExportLogBaseDirectory()) has been removed. How are the unit-tests impacted by this change? https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:50: typedef base::Callback<void(std::unique_ptr<base::DictionaryValue>)> For new code we prefer "using" over typedef. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:53: // Start collecting NetLog data into chrome-net-export-log.json file in Can you update this to read "Starts" rather than "Start" ? (I realize this is just moving an existing comment). https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:59: StateCallback state_callback); Can callback can be a const-ref? https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:65: StateCallback state_callback); Can callback can be a const-ref? https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:69: void GetState(StateCallback state_callback); Can callback can be a const-ref? https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:71: typedef base::Callback<void(const base::FilePath&)> FilePathCallback; For new code we prefer "using" over typedef. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:74: // logging, |success_callback| is called with the log filepath as the param. Please update the comment to describer what happens in the other case (i.e. does it call back with empty FilePath(), or not callback at all). https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:80: void SetFileTaskRunner( Please add some explanation for this mechanism (either here, or in the top-level comments for the class). https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:122: void InitAndCallback(base::Closure success_callback); Can callback can be a const-ref? https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:138: void InitStateAndCallback(base::Closure success_callback, Can callback can be a const-ref? https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:143: StateCallback state_callback); Can callback can be a const-ref? https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:151: void StopLogging(StateCallback state_callback, Can callback can be a const-ref? https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:155: // responsible for deleting the returned value. Can this return an std::unique_ptr<> instead? https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:159: void RunStateCallback(StateCallback state_callback); Can callback can be a const-ref? https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:173: scoped_refptr<base::SingleThreadTaskRunner> file_task_runner_; Please add a comment for the data members below explaining what is accessed from which thread. I believe everything is to be accessed solely from the UI thread (i.e. what thread_checker_ is bound to). And file_task_runner_ is just used for shuttling data around for the disk IO. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/reso... File components/net_log/resources/net_export.js (right): https://codereview.chromium.org/2603523002/diff/40001/components/net_log/reso... components/net_log/resources/net_export.js:109: if (!!exportNetLogInfo.logExists) { Agreed these names are clearer!
Oh and also: There is an iOS version of net_export_ui.cc which will need to be updated in the same way. You may want to hold off on that until the non-iOS version has stabilized, as it will largely be duplicated code. See: https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/webui/net_export/n...
wangyix@chromium.org changed reviewers: + wangyix@chromium.org
https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/net_export_ui.cc (right): https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:65: // functions except SendEmail run on FILE_USER_BLOCKING thread. On 2017/01/06 21:46:22, eroman (slow) wrote: > This comment is no longer true. Done. https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:90: void StartNetLogAndNotifyUI(const base::FilePath& log_path, On 2017/01/06 21:46:22, eroman (slow) wrote: > I suggest naming this "StartNetLogThenNotifyUI" > > (Replace And with Then to clarify that it happens sequentially rather than in > parallel) Done. https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:93: void StopNetLogAndNotifyUI(); On 2017/01/06 21:46:22, eroman (slow) wrote: > Same comment here. Done. https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:95: // Send NetLog data via email. This runs on UI thread. On 2017/01/06 21:46:22, eroman (slow) wrote: > The mention of "UI" thread is probably redundant, now that everything runs on UI > thread in here. Done. https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:110: // Call NetExportView.onExportNetLogInfoChanged JavsScript function in the On 2017/01/06 21:46:22, eroman (slow) wrote: > While you are editing this, could you change "Call" to "Calls" ? Done. https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:122: // posted to the FILE_USER_BLOCKING thread. Base::Unretained is used here On 2017/01/06 21:46:22, eroman (slow) wrote: > This comment is no longer accurate. Done. https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:132: // here. On 2017/01/06 21:46:22, eroman (slow) wrote: > Please explain the purpose of this variable, and its validity range (i.e. this > is only really valid while the file dialog is open on the desktop UI). > > An alternative would be to pass the capture mode directly to SelectFile(), and > not need this member -- SelectFile takes a params pointer which it will pass > back to the delegate in FileSelected(). Done. https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:145: if (!net_log_file_writer_->GetFileTaskRunner()) { On 2017/01/06 21:46:22, eroman (slow) wrote: > Since these will always be set in unison, how about unconditionally doing: > > net_log_file_writer_->SetTaskRunners(....FILE_USER_BLOCKING, ....IO); > > And inside of the implementation for SetTaskRunners(), if the task runners have > not been assigned yet it does so, otherwise it asserts that they are the same as > the ones passed in. Done. https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:203: if (capture_mode_string == "LOG_BYTES") { On 2017/01/06 21:46:22, eroman (slow) wrote: > Can we internalize the conversion from "capture mode string" --> capture mode in > the NetLogFileWriter? > > * NetLogFileWriter has code that does the inverse mapping (from capture mode to > string), so makes sense for the code to live side by side > * The iOS frontend will need to repeat this code if it lives in the message > handler. > > Can either add a helper function, or make StartNetLog() take the parameter as as > string again. Done. https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:242: net_log_file_writer_->SetUpNetExportLogPath(log_path); On 2017/01/06 21:46:22, eroman (slow) wrote: > Rather than a separate method SetUpNetExportLogPath(), can you move this to a > parameter to the call below net_log_file_writer_->StartNetLog(...) ? Done. https://codereview.chromium.org/2603523002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:254: // fill polled_data with browser-specific polled data. On 2017/01/06 21:46:22, eroman (slow) wrote: > Please add a "TODO(crbug.com/438656)" here Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... File components/net_log/net_log_file_writer.cc (right): https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:55: thread_checker_.DetachFromThread(); On 2017/01/06 21:46:23, eroman (slow) wrote: > Is this necessary? I believe the creation and destruction happens from the UI > thread (as do all the public entry points), so this is no longer needed. Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:87: write_to_file_observer_->StartObservingUnbounded(chrome_net_log_, On 2017/01/06 21:46:23, eroman (slow) wrote: > StartLogging() will eventually need to take a URLRequestContextGetter parameter > (crbug.com/679021) > > Don't need to address that in this changelist, but something to be aware of in > the design > > I think StartLogging() will want to eventually take a URLRequestContextGetter, > and do a thread-hop to the IO thread in order to start logging here. Acknowledged. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:125: DCHECK_EQ(STATE_LOGGING, state_); On 2017/01/06 21:46:23, eroman (slow) wrote: > I don't think this DCHECK holds() -- Let's say StopNetLog() was called at about > the same time from 2 different tabs. One of them would hit this DCHECK(). Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:127: base::Bind([] {})); On 2017/01/06 21:46:23, eroman (slow) wrote: > Shouldn't we wait for this to complete before reporting completion of > StopLogging? > > In particular, is there a possible race between stopping logging, transitioning > the state to STATE_NOT_LOGGING, and then emailing the file (however the file has > not finished writing yet). Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:173: dict->SetString("captureMode", "LOG_BYTES"); On 2017/01/06 21:46:23, eroman (slow) wrote: > Formatting. Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:180: state_callback.Run(std::unique_ptr<base::DictionaryValue>(GetState())); On 2017/01/06 21:46:22, eroman (slow) wrote: > See comment about making GetState() return a unique_ptr Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:186: return; On 2017/01/06 21:46:23, eroman (slow) wrote: > Would it make sense to always invoke the callback, but possibly with an empty > path to indicate "failure"? Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:211: if (runCallback) On 2017/01/06 21:46:22, eroman (slow) wrote: > hacker_style_notation rather than camelCaseNotation Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... File components/net_log/net_log_file_writer.h (right): https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:36: // NetLogFileWriter logs all the NetLog entries into a specified file. On 2017/01/06 21:46:23, eroman (slow) wrote: > Please add a description for the threading model: > > * This class is created and destroyed on the UI thread, and all public entry > points are to be called on the UI thread > > * However internally the class may run some code on the file_task_runner_ and > net_task_runner_. Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:45: class NetLogFileWriter : public base::SupportsWeakPtr<NetLogFileWriter> { On 2017/01/06 21:46:23, eroman (slow) wrote: > Why is SupportsWeakPtr<> needed? (the weak_ptr_factory_ member added should be > sufficient). Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:48: virtual ~NetLogFileWriter(); On 2017/01/06 21:46:23, eroman (slow) wrote: > Does this class still need a vtable? The only virtual method > (GetNetExportLogBaseDirectory()) has been removed. How are the unit-tests > impacted by this change? Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:50: typedef base::Callback<void(std::unique_ptr<base::DictionaryValue>)> On 2017/01/06 21:46:23, eroman (slow) wrote: > For new code we prefer "using" over typedef. Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:53: // Start collecting NetLog data into chrome-net-export-log.json file in On 2017/01/06 21:46:23, eroman (slow) wrote: > Can you update this to read "Starts" rather than "Start" ? (I realize this is > just moving an existing comment). Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:59: StateCallback state_callback); On 2017/01/06 21:46:23, eroman (slow) wrote: > Can callback can be a const-ref? Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:65: StateCallback state_callback); On 2017/01/06 21:46:23, eroman (slow) wrote: > Can callback can be a const-ref? Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:69: void GetState(StateCallback state_callback); On 2017/01/06 21:46:23, eroman (slow) wrote: > Can callback can be a const-ref? Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:71: typedef base::Callback<void(const base::FilePath&)> FilePathCallback; On 2017/01/06 21:46:23, eroman (slow) wrote: > For new code we prefer "using" over typedef. Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:74: // logging, |success_callback| is called with the log filepath as the param. On 2017/01/06 21:46:23, eroman (slow) wrote: > Please update the comment to describer what happens in the other case (i.e. does > it call back with empty FilePath(), or not callback at all). Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:80: void SetFileTaskRunner( On 2017/01/06 21:46:23, eroman (slow) wrote: > Please add some explanation for this mechanism (either here, or in the top-level > comments for the class). Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:122: void InitAndCallback(base::Closure success_callback); On 2017/01/06 21:46:23, eroman (slow) wrote: > Can callback can be a const-ref? Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:138: void InitStateAndCallback(base::Closure success_callback, On 2017/01/06 21:46:23, eroman (slow) wrote: > Can callback can be a const-ref? Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:143: StateCallback state_callback); On 2017/01/06 21:46:23, eroman (slow) wrote: > Can callback can be a const-ref? Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:151: void StopLogging(StateCallback state_callback, On 2017/01/06 21:46:23, eroman (slow) wrote: > Can callback can be a const-ref? Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:155: // responsible for deleting the returned value. On 2017/01/06 21:46:23, eroman (slow) wrote: > Can this return an std::unique_ptr<> instead? Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:159: void RunStateCallback(StateCallback state_callback); On 2017/01/06 21:46:23, eroman (slow) wrote: > Can callback can be a const-ref? Done. https://codereview.chromium.org/2603523002/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:173: scoped_refptr<base::SingleThreadTaskRunner> file_task_runner_; On 2017/01/06 21:46:23, eroman (slow) wrote: > Please add a comment for the data members below explaining what is accessed from > which thread. > > I believe everything is to be accessed solely from the UI thread (i.e. what > thread_checker_ is bound to). > And file_task_runner_ is just used for shuttling data around for the disk IO. Done.
This is looking good, thanks for iterating on it! This still needs the accompanying modifications to unit-tests and ios, so I expect at least another patchset before I approve this. My earlier comments on the CL description still stand. Concretely: * Change the title from "Initial attempt..." to something more concrete. * Rather than describing the motivation for the change in detail, start by describing what the actual changes are. In my previous comment I summarized 3 bullets for what this change accomplishes. The prose for supporting motivation can be removed, or placed after the overview. https://codereview.chromium.org/2603523002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/net_export_ui.cc (right): https://codereview.chromium.org/2603523002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:111: // Calls NetExportView.onExportNetLogInfoChanged JavsScript function in the typo: javascript https://codereview.chromium.org/2603523002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:231: std::unique_ptr<base::DictionaryValue> polled_data; [optional] I suggest renaming this something like: * ui_polled_data * browser_polled_data * extra_polled_data (to emphasize it doesn't need to contain the NetInfo polled data) https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... File components/net_log/net_log_file_writer.cc (right): https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.cc:60: InitAndCallback(base::Bind(&NetLogFileWriter::StartLogging, Side-comment: A common idiom that we use in //net code is a state-machine using a single-parameter completion callback. For some examples of that search for "DoLoop" To be clear, no action necessary here, the code as written is good; I just wanted to introduce you to that other approach, which you will be seeing a lot more of :) https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.cc:63: } else if (state_ == STATE_NOT_LOGGING) { optional: I suggest using a switch statement rather than if/else. The advantage of switch statements is when you omit a "default" we get compile-time errors whenever a new value is introduced (which ensures that all code handling state_ gets updated correctly). If you make this change, should be done everywhere state_ is used for consistency. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.cc:114: // Before we post that task to the net thread, change state to Can you avoid using "we" pronoun throughout this comment block? https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.cc:276: results.default_log_path_success = false; Can you also initialize log_exists = false ? (as there is a short-circuit which may leave this uninitialized on return). (The downstream code (InitStateAndCallback) doesn't currently care as it only accesses log_exists if default_log_path_success, but it is still good hygiene to do so.) https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.cc:333: } else { Please change the default case. If |capture_mode| has an unknown value we should return "NORMAL" rather than "LOG_BYTES". Moreover, I suggest adding a NOT_REACHED() for that case (as it implies our conversion code is incomplete). https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.cc:346: DCHECK_EQ(capture_mode_string, "LOG_BYTES"); Same comment as above. The default (unrecognized string) case should be "NORMAL" and not "LOG_BYTES". This is important since LOG_BYTES is pretty heavy-weight, and we wouldn't want to default to it if something went wrong. Moreover, if the string cannot be parsed let's add a NOT_REACHED(), since it implies a misunderstanding between this C++ code and the javascript frontend. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... File components/net_log/net_log_file_writer.h (right): https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:36: // NetLogFileWriter logs all the NetLog entries into a specified file. It does I suggest also mentioning that this is used exclusively as a support-class for net-export. * Shared by ios and non-ios implementations of net-export UI * Used as a singleton, so the net-export logging state can be shared between instances of the UI (In fact separately I will propose renaming this to something like NetExportFileWriter) https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:71: // |state_callback| will be executed at the end synchronously. Instead of having a synchronous case, can you make all paths call state_callback asynchronously? That way there are fewer special cases for testing. Synchronous callback invocation can be tricky and we avoid them wherever possible (it can introduce unexpected re-entrancy problems) https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:76: // Stops collecting NetLog data into the file. It is a no-op if we nit: can you modify the language to avoid "we" ? See also https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M for motivation. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:96: // asynchronously (whether initialization is successful or not). Otherwise, Same comment as earlier -- it is easier for consumers if this guarantees execution to only be async. This can be achieved by doing an extra post-task, and helps ensure that callers don't need to be prepared to handle synchronous completion. The error cases in particular tend to not be well exercised, so having it behave differently puts it at risk of misbehaving (re-entrancy bugs can be subtle especially as changes are made to the client code). https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:100: using FilePathCallback = base::Callback<void(const base::FilePath&)>; nit: move this to the top of the class declaration (beside the other using statement) See https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:102: // Gets the log filepath. |path_callback| will be used to notify the caller Excellent comments! https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:109: // In cases (1) and (2), |path_callback| will be executed synchronously. In Same comment -- can we make all the completions async? https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:111: void GetFilePath(const FilePathCallback& path_callback) const; [optional] I suggest changing this function's name (since it is a bit unexpected that it fails if logging is in progress). What about: * GetFilePathToCompletedLog() * GetFilePathIfDone() https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:113: // Sets the task runners used by NetLogFileWriter for running file-specific I suggest expanding this comment: Sets the task runners used by NetLogFileWriter for doing file I/O, and network I/O respectively. This must be called prior to using the NetLogFileWriter. The task runners must not be changed once set; however calling this function with the same parameters is OK. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:119: static std::string CaptureModeToString(net::NetLogCaptureMode capture_mode); Suggest adding this comment: // Converts to/from the string representation of a capture mode used by net_export.js https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:135: /*FRIEND_TEST_ALL_PREFIXES(NetLogFileWriterTest, EnsureInitFailure); NOTE: I assume this will be updated in a subsequent patchset https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:158: // (StartNetLog(), GetState()). This function attemps to initialize typo: attempts https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:160: void InitAndCallback(const base::Closure& callback); [optional] InitThenCallback() ? https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:164: struct DefaultLogPathResults { Move the struct declaration to be below the enum State (we group together declarations at the start of public/private/protected blocks) https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:175: // SetUpDefaultLogPath(), then executes |callback|. mention if the execution of |callback| is synchronous vs asynchronous https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:183: // Adds net info from net::GetNetInfo() to |polled_data|. Will run on the Will run on the --> must be run on the https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:185: static std::unique_ptr<base::DictionaryValue> AddNetInfo( Consider moving this declaration to the .cc (i.e. it doesn't need to be exposed in the header file, since it is a private static method). https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:201: static bool SetPosixFilePermissionsAll(const base::FilePath& path); This can also be moved to the .cc file (i.e. remove from header). https://codereview.chromium.org/2603523002/diff/60001/components/net_log/reso... File components/net_log/resources/net_export.js (right): https://codereview.chromium.org/2603523002/diff/60001/components/net_log/reso... components/net_log/resources/net_export.js:101: if (exportNetLogInfo.state == 'NOT_LOGGING') { For the record, there are now additional values for the |state| enum. I don't think it is necessary to handle the new states explicitly in here, but would like for you to confirm.
Description was changed from ========== Initial attempt at updating NetLogFileWriter to handle all the thread-hopping logic. In this implementation, the NetLogFileWriter will live on the UI thread instead of the IO thread as suggested here: https://docs.google.com/document/d/1nhA0PmkDYFGbmc3Ny98IEVQprZTyEcD457Jc4xoLp... The motivation for this is the following: The NetLogFileWriter has three possible states: STATE_UNINITIALIZED, STATE_LOGGING, and STATE_NOT_LOGGING. When the NetLogFileWriter receives a command (e.g. StartNetLog() or StopNetLog()), then depending on its current state and the command received, it will need to carry out a different sequence of thread-hops. For example, if StartNetLog() is called while NetLogFileWriter is in STATE_UNINITIALIZED, it will need to perform initialization steps on the FILE thread and then return to the main thread to create and attach a new observer. If StopNetLog() is called while NetLogFileWriter is in STATE_LOGGING, then it will need to grab the polled_data from the IO thread, and then return to the main thread to stop the observer. Because the sequence of threads required by NetLogFileWriter depends on what state it was in when a command is received, it makes sense to have that state live on the UI thread (where NetExportMessageHandler lives, which controls the NetLogFileWriter). If NetLogFileWriter is put on the IO thread, then every command from NetExportMessageHandler must first be posted to the IO thread in order to check the state of NetLogFileWriter before it can determine which threads it needs to visit to complete the command. This extra visit to the IO thread is unnecessary and will further complicate the code. ========== to ========== (1) Use net::FileNetLogObserver in place of net::WriteToFileNetLogObserver for chrome://net-export (which is a big part of crbug.com/679030). (2) Move NetLogFileWriter's public interface from the FILE_USER_BLOCKING to the UI thread (internalizing the post tasks to file thread/UI thread to simplify things) (3) Add plumbing for for passing polled data, which will be collected on both the UI thread and IO thread (crbug.com/438656). Some of the refactor done will also facilitate crbug.com/679021, which is just a few extra lines. In this CL, only the polled data from the IO thread will be retrieved; the UI thread polled data will be added in a follow-up CL. ==========
NetLogFileWriter has been updated with fixes for Eric's comments, but no unit test yet. Major changes: - All callbacks for NetLogFileWriter commands are now called asynchronously. - NetLogFileWriter::StopNetLog() param |context_getter| can now be null. In this case, posting net::GetNetInfo() to the IO thread is skipped. - Dependency has been injected into NetLogFileWriter for the unit test by adding a callback member |default_log_base_directory_getter_|. This callback is called by NetLogFileWriter during initialization to retrieve the default base directory used for the log file. A setter has been added for this member, which will only be used by the NetLogFileWriter unit-test to override the usual default-log-base-dir-getter. https://codereview.chromium.org/2603523002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/net_export_ui.cc (right): https://codereview.chromium.org/2603523002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:111: // Calls NetExportView.onExportNetLogInfoChanged JavsScript function in the On 2017/01/11 19:18:38, eroman (slow) wrote: > typo: javascript Done. https://codereview.chromium.org/2603523002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:231: std::unique_ptr<base::DictionaryValue> polled_data; On 2017/01/11 19:18:38, eroman (slow) wrote: > [optional] I suggest renaming this something like: > > * ui_polled_data > * browser_polled_data > * extra_polled_data > > (to emphasize it doesn't need to contain the NetInfo polled data) Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... File components/net_log/net_log_file_writer.cc (right): https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.cc:60: InitAndCallback(base::Bind(&NetLogFileWriter::StartLogging, On 2017/01/11 19:18:38, eroman (slow) wrote: > Side-comment: A common idiom that we use in //net code is a state-machine using > a single-parameter completion callback. For some examples of that search for > "DoLoop" > > To be clear, no action necessary here, the code as written is good; I just > wanted to introduce you to that other approach, which you will be seeing a lot > more of :) Acknowledged. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.cc:63: } else if (state_ == STATE_NOT_LOGGING) { On 2017/01/11 19:18:38, eroman (slow) wrote: > optional: I suggest using a switch statement rather than if/else. The advantage > of switch statements is when you omit a "default" we get compile-time errors > whenever a new value is introduced (which ensures that all code handling state_ > gets updated correctly). If you make this change, should be done everywhere > state_ is used for consistency. Acknowledged. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.cc:114: // Before we post that task to the net thread, change state to On 2017/01/11 19:18:38, eroman (slow) wrote: > Can you avoid using "we" pronoun throughout this comment block? Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.cc:276: results.default_log_path_success = false; On 2017/01/11 19:18:38, eroman (slow) wrote: > Can you also initialize log_exists = false ? (as there is a short-circuit which > may leave this uninitialized on return). > > (The downstream code (InitStateAndCallback) doesn't currently care as it only > accesses log_exists if default_log_path_success, but it is still good hygiene to > do so.) Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.cc:333: } else { On 2017/01/11 19:18:38, eroman (slow) wrote: > Please change the default case. > > If |capture_mode| has an unknown value we should return "NORMAL" rather than > "LOG_BYTES". > > Moreover, I suggest adding a NOT_REACHED() for that case (as it implies our > conversion code is incomplete). Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.cc:346: DCHECK_EQ(capture_mode_string, "LOG_BYTES"); On 2017/01/11 19:18:38, eroman (slow) wrote: > Same comment as above. > > The default (unrecognized string) case should be "NORMAL" and not "LOG_BYTES". > > This is important since LOG_BYTES is pretty heavy-weight, and we wouldn't want > to default to it if something went wrong. > > Moreover, if the string cannot be parsed let's add a NOT_REACHED(), since it > implies a misunderstanding between this C++ code and the javascript frontend. Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... File components/net_log/net_log_file_writer.h (right): https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:36: // NetLogFileWriter logs all the NetLog entries into a specified file. It does On 2017/01/11 19:18:38, eroman (slow) wrote: > I suggest also mentioning that this is used exclusively as a support-class for > net-export. > * Shared by ios and non-ios implementations of net-export UI > * Used as a singleton, so the net-export logging state can be shared between > instances of the UI > > (In fact separately I will propose renaming this to something like > NetExportFileWriter) Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:71: // |state_callback| will be executed at the end synchronously. On 2017/01/11 19:18:39, eroman (slow) wrote: > Instead of having a synchronous case, can you make all paths call state_callback > asynchronously? > > That way there are fewer special cases for testing. > > Synchronous callback invocation can be tricky and we avoid them wherever > possible (it can introduce unexpected re-entrancy problems) Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:76: // Stops collecting NetLog data into the file. It is a no-op if we On 2017/01/11 19:18:39, eroman (slow) wrote: > nit: can you modify the language to avoid "we" ? > > See also > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M > for motivation. Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:96: // asynchronously (whether initialization is successful or not). Otherwise, On 2017/01/11 19:18:39, eroman (slow) wrote: > Same comment as earlier -- it is easier for consumers if this guarantees > execution to only be async. > > This can be achieved by doing an extra post-task, and helps ensure that callers > don't need to be prepared to handle synchronous completion. The error cases in > particular tend to not be well exercised, so having it behave differently puts > it at risk of misbehaving (re-entrancy bugs can be subtle especially as changes > are made to the client code). Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:100: using FilePathCallback = base::Callback<void(const base::FilePath&)>; On 2017/01/11 19:18:39, eroman (slow) wrote: > nit: move this to the top of the class declaration (beside the other using > statement) > > See https://google.github.io/styleguide/cppguide.html#Declaration_Order Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:102: // Gets the log filepath. |path_callback| will be used to notify the caller On 2017/01/11 19:18:39, eroman (slow) wrote: > Excellent comments! Acknowledged. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:109: // In cases (1) and (2), |path_callback| will be executed synchronously. In On 2017/01/11 19:18:39, eroman (slow) wrote: > Same comment -- can we make all the completions async? Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:111: void GetFilePath(const FilePathCallback& path_callback) const; On 2017/01/11 19:18:39, eroman (slow) wrote: > [optional] I suggest changing this function's name (since it is a bit unexpected > that it fails if logging is in progress). What about: > > * GetFilePathToCompletedLog() > * GetFilePathIfDone() Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:113: // Sets the task runners used by NetLogFileWriter for running file-specific On 2017/01/11 19:18:38, eroman (slow) wrote: > I suggest expanding this comment: > > Sets the task runners used by NetLogFileWriter for doing file I/O, and network > I/O respectively. This must be called prior to using the NetLogFileWriter. The > task runners must not be changed once set; however calling this function with > the same parameters is OK. Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:119: static std::string CaptureModeToString(net::NetLogCaptureMode capture_mode); On 2017/01/11 19:18:38, eroman (slow) wrote: > Suggest adding this comment: > > // Converts to/from the string representation of a capture mode used by > net_export.js Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:135: /*FRIEND_TEST_ALL_PREFIXES(NetLogFileWriterTest, EnsureInitFailure); On 2017/01/11 19:18:39, eroman (slow) wrote: > NOTE: I assume this will be updated in a subsequent patchset Acknowledged. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:158: // (StartNetLog(), GetState()). This function attemps to initialize On 2017/01/11 19:18:39, eroman (slow) wrote: > typo: attempts Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:160: void InitAndCallback(const base::Closure& callback); On 2017/01/11 19:18:39, eroman (slow) wrote: > [optional] InitThenCallback() ? Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:164: struct DefaultLogPathResults { On 2017/01/11 19:18:39, eroman (slow) wrote: > Move the struct declaration to be below the enum State > (we group together declarations at the start of public/private/protected blocks) Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:175: // SetUpDefaultLogPath(), then executes |callback|. On 2017/01/11 19:18:39, eroman (slow) wrote: > mention if the execution of |callback| is synchronous vs asynchronous Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:183: // Adds net info from net::GetNetInfo() to |polled_data|. Will run on the On 2017/01/11 19:18:39, eroman (slow) wrote: > Will run on the --> must be run on the Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:185: static std::unique_ptr<base::DictionaryValue> AddNetInfo( On 2017/01/11 19:18:39, eroman (slow) wrote: > Consider moving this declaration to the .cc > (i.e. it doesn't need to be exposed in the header file, since it is a private > static method). Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/net_... components/net_log/net_log_file_writer.h:201: static bool SetPosixFilePermissionsAll(const base::FilePath& path); On 2017/01/11 19:18:38, eroman (slow) wrote: > This can also be moved to the .cc file (i.e. remove from header). Done. https://codereview.chromium.org/2603523002/diff/60001/components/net_log/reso... File components/net_log/resources/net_export.js (right): https://codereview.chromium.org/2603523002/diff/60001/components/net_log/reso... components/net_log/resources/net_export.js:101: if (exportNetLogInfo.state == 'NOT_LOGGING') { On 2017/01/11 19:18:39, eroman (slow) wrote: > For the record, there are now additional values for the |state| enum. > > I don't think it is necessary to handle the new states explicitly in here, but > would like for you to confirm. Yes this is correct. The two new possible values are 'STATE_INITIALIZING' and 'STATE_STOPPING_LOG'.
Going to go ahead and remove myself as a reviewer, and defer to eroman on this review.
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
xunjieli@chromium.org changed reviewers: - xunjieli@chromium.org
For the CL description, please copy the title to the CL description. Because when you check the commit box, I believe the CL title is discarded and the commit message will exclusively be the "description" field. And while editing the description: * typeo: "for for" * add: BUG=679030,438656,679021 https://codereview.chromium.org/2603523002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/net_export_ui.cc (right): https://codereview.chromium.org/2603523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:233: // TODO(crbug.com/438656): fill polled_data with browser-specific polled data. |ui_thread_polled_data| https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... File components/net_log/net_log_file_writer.cc (right): https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.cc:101: RunStateCallbackAsync(state_callback); If StartNetLog() is called multiple times the first call will post a task for initialization, and transition state to STATE_INITIALIZING. Until the initialization completes, each subsequent call will see STATE_INITIALIZING and go down this path, which fetches the current state and then calls back (which isn't quite right). Please see my suggested solution for Init(). https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.cc:131: // Only way to get here is if StartLogging() was called as a result of a Because of when StartNetLog() posts this task, this only holds if initialization completes before this task (otherwise this could be STATE_INITIALIZING too). We can rely on this, but let's keep that internal to Init(). https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.cc:249: std::unique_ptr<base::DictionaryValue> state = GetState(); There are disadvantages to running this immediately (notably when deferring because initialization is in progress). How about: PostTask(..., base::Bind(&RunStateCallback, state_callback)); https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.cc:283: void NetLogFileWriter::InitThenCallback(const base::Closure& callback) { Here is how I think the initialization can be handled and layered in this code. StartNetLog(state_callback) { Init(base::Bind(&StartNetLogAfterInitialized, state_callback)); } GetState(state_callback) { Init(base::Bind(&RunStateCallback, state_callback)); } In StartNetLog() and GetState(), now all we do is ask to run a callback after initialization has completed (without inspecting the state post-initialization yet). Init(callback) can then internalize the initialization state transitions, schedule any tasks for initialization, and then finally invoke callback() once initialization has run: void Init(callback) { if (state_ == STATE_UNINITIALIZED) { state_ = STATE_INITIALIZING; .... post task to finish initialization .... } else if (state_ == STATE_INITIALIZING) { RunStateCallbackAsync(callback); // HACK... but good enough } else { RunStateCallbackAsync(callback). } } This has the advantage of internalizing the state transitions to Init(), and also handling the STATE_INITIALIZING case. Also relies on a small change to RunStateCallbackAsync(). https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.cc:327: void NetLogFileWriter::InitStateThenCallback( These names are very difficult for me to follow because of how similar they are: * InitStateThenCallback vs InitThenCallback() * StartNetLog vs StartLogging * GetState(callback) vs GetState(void) Instead, we can pick names that are easier to see are different, and also explain how they are different. Here is how I seem them: * Init() * InitAfterInitialized() * StartNetLog() * StartNetLogAfterInitialized() * GetState(callback) The pattern here is the "AfterInitialized()" flavor, which run after initialization has completed (and are free to synchronously invoke the user-callback) For GetState(), I would argue we don't need two functions for this. Currently you are sharing that between RunStateCallback() and RunStateCallbackAsync(), however per my comment RunStateCallbackAsync() should not call GetState() directly. Assuming we did keep both functions, a common idiom in our code is to name things XXX() for the main entry point, and DoXXX() for the underlying implementation, barring a better name (so we would have GetState(callback) and DoGetState in this case). https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... File components/net_log/net_log_file_writer.h (right): https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.h:38: // which can tell it to start or stop logging in response to the net-export UIs' UIs' --> UI's https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.h:40: // between multiple instances of the net-export UI. Internally, it manages an Good comment https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.h:139: void SetDefaultLogBaseDirectoryGetter(const DirectoryGetter& getter); For functions that are exclusively exposed for unit-tests, please name them with a suffix of "ForTest()" or "ForTesting()". This hilights expectations to future code reviewers if someone tries to use the function in non-test code. There is also some automatic support for detecting this via our PRESUBMIT checks. See https://cs.chromium.org/chromium/src/PRESUBMIT.py?q=ForTest+f:.py&sq=package:... https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.h:180: static DefaultLogPathResults SetUpDefaultLogPath( Can this be part of the .cc file instead of a static method in the .h ?
Description was changed from ========== (1) Use net::FileNetLogObserver in place of net::WriteToFileNetLogObserver for chrome://net-export (which is a big part of crbug.com/679030). (2) Move NetLogFileWriter's public interface from the FILE_USER_BLOCKING to the UI thread (internalizing the post tasks to file thread/UI thread to simplify things) (3) Add plumbing for for passing polled data, which will be collected on both the UI thread and IO thread (crbug.com/438656). Some of the refactor done will also facilitate crbug.com/679021, which is just a few extra lines. In this CL, only the polled data from the IO thread will be retrieved; the UI thread polled data will be added in a follow-up CL. ========== to ========== Move net-export thread-hopping code into NetLogFileWriter and add IO polled data. (1) Use net::FileNetLogObserver in place of net::WriteToFileNetLogObserver for chrome://net-export (which is a big part of crbug.com/679030). (2) Move NetLogFileWriter's public interface from the FILE_USER_BLOCKING to the UI thread (internalizing the post tasks to file thread/UI thread to simplify things) (3) Add plumbing for passing polled data, which will be collected on both the UI thread and IO thread (crbug.com/438656). Some of the refactor done will also facilitate crbug.com/679021, which is just a few extra lines. In this CL, only the polled data from the IO thread will be retrieved; the UI thread polled data will be added in a follow-up CL. BUG=679030,438656,679021 ==========
The CQ bit was checked by wangyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset 6: added unit test for NetLogFileWriter, but haven't fixed comments from Patchset 5 yet. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Patchset 7: comments from patchset 5 addressed. Also, NetLogFileWriter unit-test BUILD file was updated so it could compile. https://codereview.chromium.org/2603523002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/net_export_ui.cc (right): https://codereview.chromium.org/2603523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:233: // TODO(crbug.com/438656): fill polled_data with browser-specific polled data. On 2017/01/18 21:32:43, eroman (slow) wrote: > |ui_thread_polled_data| Done. https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... File components/net_log/net_log_file_writer.cc (right): https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.cc:101: RunStateCallbackAsync(state_callback); On 2017/01/18 21:32:43, eroman (slow) wrote: > If StartNetLog() is called multiple times the first call will post a task for > initialization, and transition state to STATE_INITIALIZING. > > Until the initialization completes, each subsequent call will see > STATE_INITIALIZING and go down this path, which fetches the current state and > then calls back (which isn't quite right). > > Please see my suggested solution for Init(). Done. https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.cc:131: // Only way to get here is if StartLogging() was called as a result of a On 2017/01/18 21:32:43, eroman (slow) wrote: > Because of when StartNetLog() posts this task, this only holds if initialization > completes before this task (otherwise this could be STATE_INITIALIZING too). We > can rely on this, but let's keep that internal to Init(). Done. https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.cc:249: std::unique_ptr<base::DictionaryValue> state = GetState(); On 2017/01/18 21:32:43, eroman (slow) wrote: > There are disadvantages to running this immediately (notably when deferring > because initialization is in progress). > > How about: > > PostTask(..., base::Bind(&RunStateCallback, state_callback)); Done. https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.cc:283: void NetLogFileWriter::InitThenCallback(const base::Closure& callback) { On 2017/01/18 21:32:43, eroman (slow) wrote: > Here is how I think the initialization can be handled and layered in this code. > > StartNetLog(state_callback) { > Init(base::Bind(&StartNetLogAfterInitialized, state_callback)); > } > > GetState(state_callback) { > Init(base::Bind(&RunStateCallback, state_callback)); > } > > In StartNetLog() and GetState(), now all we do is ask to run a callback after > initialization has completed (without inspecting the state post-initialization > yet). > > Init(callback) can then internalize the initialization state transitions, > schedule any tasks for initialization, and then finally invoke callback() once > initialization has run: > > void Init(callback) { > if (state_ == STATE_UNINITIALIZED) { > state_ = STATE_INITIALIZING; > .... post task to finish initialization .... > } else if (state_ == STATE_INITIALIZING) { > RunStateCallbackAsync(callback); // HACK... but good enough > } else { > RunStateCallbackAsync(callback). > } > } > > This has the advantage of internalizing the state transitions to Init(), and > also handling the STATE_INITIALIZING case. Also relies on a small change to > RunStateCallbackAsync(). Done. https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.cc:327: void NetLogFileWriter::InitStateThenCallback( On 2017/01/18 21:32:43, eroman (slow) wrote: > These names are very difficult for me to follow because of how similar they are: > > * InitStateThenCallback vs InitThenCallback() > * StartNetLog vs StartLogging > * GetState(callback) vs GetState(void) > > Instead, we can pick names that are easier to see are different, and also > explain how they are different. Here is how I seem them: > > * Init() > * InitAfterInitialized() > * StartNetLog() > * StartNetLogAfterInitialized() > * GetState(callback) > > The pattern here is the "AfterInitialized()" flavor, which run after > initialization has completed (and are free to synchronously invoke the > user-callback) > > For GetState(), I would argue we don't need two functions for this. Currently > you are sharing that between RunStateCallback() and RunStateCallbackAsync(), > however per my comment RunStateCallbackAsync() should not call GetState() > directly. > > Assuming we did keep both functions, a common idiom in our code is to name > things XXX() for the main entry point, and DoXXX() for the underlying > implementation, barring a better name (so we would have GetState(callback) and > DoGetState in this case). Done. https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... File components/net_log/net_log_file_writer.h (right): https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.h:38: // which can tell it to start or stop logging in response to the net-export UIs' On 2017/01/18 21:32:43, eroman (slow) wrote: > UIs' --> UI's Done. https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.h:40: // between multiple instances of the net-export UI. Internally, it manages an On 2017/01/18 21:32:43, eroman (slow) wrote: > Good comment Acknowledged. https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.h:139: void SetDefaultLogBaseDirectoryGetter(const DirectoryGetter& getter); On 2017/01/18 21:32:43, eroman (slow) wrote: > For functions that are exclusively exposed for unit-tests, please name them with > a suffix of "ForTest()" or "ForTesting()". > > This hilights expectations to future code reviewers if someone tries to use the > function in non-test code. There is also some automatic support for detecting > this via our PRESUBMIT checks. See > https://cs.chromium.org/chromium/src/PRESUBMIT.py?q=ForTest+f:.py&sq=package:... Done. https://codereview.chromium.org/2603523002/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.h:180: static DefaultLogPathResults SetUpDefaultLogPath( On 2017/01/18 21:32:43, eroman (slow) wrote: > Can this be part of the .cc file instead of a static method in the .h ? This would require exposing the struct NetLogFileWriter::DefaultLogPathResults as public, which I don't think is appropriate. Also, I think the method makes sense as a static method in the class since it's a helper that's highly specific to NetLogFileWriter.
The CQ bit was checked by wangyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Description was changed from ========== Move net-export thread-hopping code into NetLogFileWriter and add IO polled data. (1) Use net::FileNetLogObserver in place of net::WriteToFileNetLogObserver for chrome://net-export (which is a big part of crbug.com/679030). (2) Move NetLogFileWriter's public interface from the FILE_USER_BLOCKING to the UI thread (internalizing the post tasks to file thread/UI thread to simplify things) (3) Add plumbing for passing polled data, which will be collected on both the UI thread and IO thread (crbug.com/438656). Some of the refactor done will also facilitate crbug.com/679021, which is just a few extra lines. In this CL, only the polled data from the IO thread will be retrieved; the UI thread polled data will be added in a follow-up CL. BUG=679030,438656,679021 ========== to ========== Move net-export thread-hopping code into NetLogFileWriter and add IO polled data. (1) Use net::FileNetLogObserver in place of net::WriteToFileNetLogObserver for chrome://net-export (which is a big part of crbug.com/679030). (2) Move NetLogFileWriter's public interface from the FILE_USER_BLOCKING to the UI thread (internalizing the post tasks to file thread/UI thread to simplify things) (3) Add plumbing for passing polled data, which will be collected on both the UI thread and IO thread (crbug.com/438656). Some of the refactor done will also facilitate crbug.com/679021, which is just a few extra lines. In this CL, only the polled data from the IO thread will be retrieved; the UI thread polled data will be added in a follow-up CL. BUG=679030,438656,679021 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
The CQ bit was checked by wangyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2603523002/diff/140001/components/net_log/net... File components/net_log/net_log_file_writer.cc (right): https://codereview.chromium.org/2603523002/diff/140001/components/net_log/net... components/net_log/net_log_file_writer.cc:93: void NetLogFileWriter::StartNetLogAfterInitialized( style: in chrome code we try to match the definition order in the .cc file to the declaration order in .h. I suggest doing likewise here. (see "Code Formatting" under https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md) https://codereview.chromium.org/2603523002/diff/140001/components/net_log/net... components/net_log/net_log_file_writer.cc:109: DCHECK(!log_path_.empty()); [optional] I suggest moving this up to line 103. https://codereview.chromium.org/2603523002/diff/140001/components/net_log/net... components/net_log/net_log_file_writer.cc:113: write_to_file_observer_.reset( [optional]: write_to_file_observer_ = base::MakeUnique<net::FileNetLogObserver>(file_task_runner_); https://codereview.chromium.org/2603523002/diff/140001/components/net_log/net... components/net_log/net_log_file_writer.cc:185: std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue); [optional]: auto dict = base::MakeUnique<base::DictionaryValue>(); https://codereview.chromium.org/2603523002/diff/140001/components/net_log/net... components/net_log/net_log_file_writer.cc:187: #ifndef NDEBUG Do you know why this is only enabled in release mode? Seems like it should be enabled everywhere. (Fine to leave as is if like me you don't understand this.) https://codereview.chromium.org/2603523002/diff/140001/components/net_log/net... components/net_log/net_log_file_writer.cc:228: base::PostTaskAndReplyWithResult( [optional] Another way to express this would be: PostTaskAndReplyWithResult( file_task_runner.get(), FROM_HERE, base::Bind(&GetPathWithAllPermissions, log_path), path_callback); FilePath GetPathWithAllPermissions(path) { if (... failed setting file permissions on path) return FilePath(); return path; } The advantage is not needing RunFilePathCallback() and the extra base::Bind() here. https://codereview.chromium.org/2603523002/diff/140001/components/net_log/net... components/net_log/net_log_file_writer.cc:315: if (state_callback_wait_if_already_initializing) { I would say that all cases, including GetState(), can set this to true (and hence don't need the variable). What is the scenario you are concerned with to not wait for STATE_INITIALIZING to complete?
Fixed Eric's comments from patchset 8. https://codereview.chromium.org/2603523002/diff/140001/components/net_log/net... File components/net_log/net_log_file_writer.cc (right): https://codereview.chromium.org/2603523002/diff/140001/components/net_log/net... components/net_log/net_log_file_writer.cc:93: void NetLogFileWriter::StartNetLogAfterInitialized( On 2017/01/21 02:17:40, eroman (slow) wrote: > style: in chrome code we try to match the definition order in the .cc file to > the declaration order in .h. I suggest doing likewise here. (see "Code > Formatting" under > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md) Done. https://codereview.chromium.org/2603523002/diff/140001/components/net_log/net... components/net_log/net_log_file_writer.cc:109: DCHECK(!log_path_.empty()); On 2017/01/21 02:17:40, eroman (slow) wrote: > [optional] I suggest moving this up to line 103. Done. https://codereview.chromium.org/2603523002/diff/140001/components/net_log/net... components/net_log/net_log_file_writer.cc:113: write_to_file_observer_.reset( On 2017/01/21 02:17:40, eroman (slow) wrote: > [optional]: write_to_file_observer_ = > base::MakeUnique<net::FileNetLogObserver>(file_task_runner_); Done. https://codereview.chromium.org/2603523002/diff/140001/components/net_log/net... components/net_log/net_log_file_writer.cc:185: std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue); On 2017/01/21 02:17:40, eroman (slow) wrote: > [optional]: auto dict = base::MakeUnique<base::DictionaryValue>(); Done. https://codereview.chromium.org/2603523002/diff/140001/components/net_log/net... components/net_log/net_log_file_writer.cc:187: #ifndef NDEBUG On 2017/01/21 02:17:40, eroman (slow) wrote: > Do you know why this is only enabled in release mode? > Seems like it should be enabled everywhere. > > (Fine to leave as is if like me you don't understand this.) I think this is only enabled in debug mode. Having a debug msg in the net-export ui showing the log path makes sense, so I'll leave this code as-is. https://codereview.chromium.org/2603523002/diff/140001/components/net_log/net... components/net_log/net_log_file_writer.cc:228: base::PostTaskAndReplyWithResult( On 2017/01/21 02:17:40, eroman (slow) wrote: > [optional] Another way to express this would be: > > PostTaskAndReplyWithResult( > file_task_runner.get(), FROM_HERE, > base::Bind(&GetPathWithAllPermissions, log_path), > path_callback); > > FilePath GetPathWithAllPermissions(path) { > if (... failed setting file permissions on path) > return FilePath(); > > return path; > } > > The advantage is not needing RunFilePathCallback() and the extra base::Bind() > here. Done. https://codereview.chromium.org/2603523002/diff/140001/components/net_log/net... components/net_log/net_log_file_writer.cc:315: if (state_callback_wait_if_already_initializing) { On 2017/01/21 02:17:40, eroman (slow) wrote: > I would say that all cases, including GetState(), can set this to true (and > hence don't need the variable). What is the scenario you are concerned with to > not wait for STATE_INITIALIZING to complete? It makes more sense to me from an API perspective: I feel that GetState() shouldn't have to wait for an initialization that it didn't trigger. Otherwise, no state_callback would ever be called with "STATE_INITIALIZING", which means that state could never be checked for by the user of NetLogFileWriter. So, if initialization takes a long time or gets stuck, no state_callback will be executed in the meantime, which makes it impossible for the caller to know what's going on.
https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... File components/net_log/net_log_file_writer_unittest.cc (right): https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:79: void RunTaskRunnersUntilIdle( I think a better approach is to use separate threads for the network task runner and file task runner. In other words instead of: scoped_refptr<base::TestSimpleTaskRunner> file_task_runner_; scoped_refptr<base::TestSimpleTaskRunner> net_task_runner_; You can do: base::Thread file_thread_; base::Thread net_thread_; The problem with using two TestSimpleTaskRunner is it linearizes execution to being single-threaded. This creates an awkward coupling that is exposed by this function (needing to drain tasks on both until things are quiescent). It also will mask any races that the code has. When using separate threads there are the benefits that: * The test includes coverage of possible race conditions (especially when being run under TSAN) * Can leverage TestCompletionCallback * More closely matches production runtime environment I don't think the helper threads need to be blocked for these tests, but if you do find the need you can use WaitableEvent to block their message loops. https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:298: // The log file should exist and be non-empty since StartnetLog() always StartNetLog https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:354: // initialization will fail since |net_log_file_writer_| will fail to retrive retrieve https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:388: const net::NetLogCaptureMode captureModes[3] = { either kCaptureModes, or capture_modes https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:393: const std::string captureModeStrings[3] = { same here https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:398: for (int i = 0; i < 3; ++i) { [optional] Could use a parameterized test via TEST_P() https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:593: // Use GetState() to retrive the state without running any of the tasks posted retrieve throughtout https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:640: // Use GetState() to retrive the state without running any of the tasks posted retrieve https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:653: // Use GetState() to retrive the state without running any of the tasks posted same typo
PTAL. https://codereview.chromium.org/2603523002/diff/140001/components/net_log/net... File components/net_log/net_log_file_writer.cc (right): https://codereview.chromium.org/2603523002/diff/140001/components/net_log/net... components/net_log/net_log_file_writer.cc:315: if (state_callback_wait_if_already_initializing) { On 2017/01/21 02:17:40, eroman (slow) wrote: > I would say that all cases, including GetState(), can set this to true (and > hence don't need the variable). What is the scenario you are concerned with to > not wait for STATE_INITIALIZING to complete? Done. Changed my mind https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... File components/net_log/net_log_file_writer_unittest.cc (right): https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:79: void RunTaskRunnersUntilIdle( On 2017/01/24 20:01:36, eroman (slow) wrote: > I think a better approach is to use separate threads for the network task runner > and file task runner. > > In other words instead of: > scoped_refptr<base::TestSimpleTaskRunner> file_task_runner_; > scoped_refptr<base::TestSimpleTaskRunner> net_task_runner_; > > You can do: > base::Thread file_thread_; > base::Thread net_thread_; > > The problem with using two TestSimpleTaskRunner is it linearizes execution to > being single-threaded. > This creates an awkward coupling that is exposed by this function (needing to > drain tasks on both until things are quiescent). It also will mask any races > that the code has. > > When using separate threads there are the benefits that: > > * The test includes coverage of possible race conditions (especially when being > run under TSAN) > * Can leverage TestCompletionCallback > * More closely matches production runtime environment > > I don't think the helper threads need to be blocked for these tests, but if you > do find the need you can use WaitableEvent to block their message loops. Done. Many tests have been modified to become whitebox tests. Specifically, StartThenVerifyFileAndState() has been modified to no longer check the file after StartNetLog(), since that requires knowledge about when NetLogFileWriter actually writes the constants. This change eliminates the need to explicitly wait until the file thread is idle. https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:298: // The log file should exist and be non-empty since StartnetLog() always On 2017/01/24 20:01:36, eroman (slow) wrote: > StartNetLog Comment no longer exists. https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:354: // initialization will fail since |net_log_file_writer_| will fail to retrive On 2017/01/24 20:01:36, eroman (slow) wrote: > retrieve Done. https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:388: const net::NetLogCaptureMode captureModes[3] = { On 2017/01/24 20:01:36, eroman (slow) wrote: > either kCaptureModes, or capture_modes Done. https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:393: const std::string captureModeStrings[3] = { On 2017/01/24 20:01:36, eroman (slow) wrote: > same here Done. https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:398: for (int i = 0; i < 3; ++i) { On 2017/01/24 20:01:36, eroman (slow) wrote: > [optional] Could use a parameterized test via TEST_P() I'm not sure there's a clean way to do this. Each generated test would need to know the NetLogCaptureMode being tested, its string representation, and the other NetLogCaptureModes that aren't being tested. The only way I can think of is to store all NetLogCaptureModes and their string representations in two arrays somewhere, and then the test class would accept an int parameter to be used as an index to select which NetLogCaptureMode to test. But it seems weird for this test's parameter to be an index. https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:593: // Use GetState() to retrive the state without running any of the tasks posted On 2017/01/24 20:01:36, eroman (slow) wrote: > retrieve throughtout Comment no longer exists. https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:640: // Use GetState() to retrive the state without running any of the tasks posted On 2017/01/24 20:01:36, eroman (slow) wrote: > retrieve Comment no longer exists. https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:653: // Use GetState() to retrive the state without running any of the tasks posted On 2017/01/24 20:01:36, eroman (slow) wrote: > same typo Comment no longer exists.
The CQ bit was checked by wangyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by wangyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... File components/net_log/net_log_file_writer_unittest.cc (right): https://codereview.chromium.org/2603523002/diff/160001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:398: for (int i = 0; i < 3; ++i) { On 2017/01/25 22:48:27, wangyix1 wrote: > On 2017/01/24 20:01:36, eroman (slow) wrote: > > [optional] Could use a parameterized test via TEST_P() > > I'm not sure there's a clean way to do this. Each generated test would need to > know the NetLogCaptureMode being tested, its string representation, and the > other NetLogCaptureModes that aren't being tested. The only way I can think of > is to store all NetLogCaptureModes and their string representations in two > arrays somewhere, and then the test class would accept an int parameter to be > used as an index to select which NetLogCaptureMode to test. But it seems weird > for this test's parameter to be an index. Had we gone this route, I would envision having the parameter be a struct that binds the capture mode with the capture mode string: struct CaptureModeParam { net::NetLogCaptureMode capture_mode; const char* capture_mode_string; }; So inside of the TEST_P() you would use |GetParam().capture_mode| in place of captureModes[i], and |GetParam().capture_mode_string| in place of captureModeStrings[i], and do those tests outside of a loop. For the two redundant calls to StartThenVerifyFileAndState(), rather than referencing captureModes[(i+1)%3] and captureModes[(i+2)%3] you could use a loop: const net::NetLogCaptureMode kModes[] = { net::NetLogCaptureMode::Default(), net::NetLogCaptureMode::IncludeCookiesAndCredentials(), net::NetLogCaptureMode::IncludeSocketBytes() }; // Starting with other capture modes should also be no-ops. for (const auto& mode : kModes) { if (GetParam().capture_mode != mode) { StartThenVerifyFileAndState(base::FilePath(), mode, GetParam().capture_mode_string); } } The only duplication would be kModes which renames modes of interest to call StartThenVerifyFileAndState() with. https://codereview.chromium.org/2603523002/diff/220001/components/net_log/net... File components/net_log/net_log_file_writer_unittest.cc (right): https://codereview.chromium.org/2603523002/diff/220001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:132: base::WaitableEvent* done_event) { See comment later on (I think this can be accomplished using PostTaskAndReply() instead of WaitableEvent. https://codereview.chromium.org/2603523002/diff/220001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:229: file_thread_.Start(); Can you put an ASSERT_TRUE() around the calls to Start() ? https://codereview.chromium.org/2603523002/diff/220001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:231: ASSERT_TRUE(file_thread_.WaitUntilThreadStarted()); I don't think these two lines are needed for correctness of the tests. https://codereview.chromium.org/2603523002/diff/220001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:259: VerifyState(test_callback.WaitForResult(), kStateLoggingString, true, true, optional: For readability I suggest extracting test_callback.WatitForResult() to its own line: std::unique_ptr<base::DictionaryValue> state = test_callback.WaitForResult(); https://codereview.chromium.org/2603523002/diff/220001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:277: VerifyState(test_callback.WaitForResult(), kStateNotLoggingString, true, same optional suggestion here. https://codereview.chromium.org/2603523002/diff/220001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:487: // Create test context getter on |net_thread_| and wait for it to finish. wording: the "context getter" isn't created on the |net_thread|, only the context is. https://codereview.chromium.org/2603523002/diff/220001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:491: base::WaitableEvent create_context_done_event( Another less verbose way to accomplish this is using PostTaskAndReply(). I haven't verified but I think this would work: base::TestClosure init_done; ...->PostTaskAndReply(FROM_HERE, base::Bind(...), init_done.closure()); init_done.WaitForResult(); The advantage of this way is you don't need to initialize a WaitableEvent, or pass one into the setup function. Also if initialization needed to run tasks on the main thread that would work (since the main thread isn't blocked). https://codereview.chromium.org/2603523002/diff/220001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:500: scoped_refptr<net::TestURLRequestContextGetter> context_getter( I would say this can move into SetUpTestContextWithQuicTimeout. Currently it populates |context|, however that is transferred tot eh context_getter -- so might as well have it just fill in the TestURLRequestContextGetter. (It can obtain the current task runner since it will be running on network thread). https://codereview.chromium.org/2603523002/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/webui/net_export/net_export_ui.cc (right): https://codereview.chromium.org/2603523002/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/webui/net_export/net_export_ui.cc:79: // Cache of GetApplicationContex()->GetNetLog()->net_log_file_writer(). This Update this comment (to match changes you made in non-ios file)
The CQ bit was checked by wangyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2603523002/diff/220001/components/net_log/net... File components/net_log/net_log_file_writer_unittest.cc (right): https://codereview.chromium.org/2603523002/diff/220001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:132: base::WaitableEvent* done_event) { On 2017/01/27 02:23:23, eroman (slow) wrote: > See comment later on (I think this can be accomplished using PostTaskAndReply() > instead of WaitableEvent. Done. https://codereview.chromium.org/2603523002/diff/220001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:229: file_thread_.Start(); On 2017/01/27 02:23:23, eroman (slow) wrote: > Can you put an ASSERT_TRUE() around the calls to Start() ? Done. https://codereview.chromium.org/2603523002/diff/220001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:231: ASSERT_TRUE(file_thread_.WaitUntilThreadStarted()); On 2017/01/27 02:23:24, eroman (slow) wrote: > I don't think these two lines are needed for correctness of the tests. Done. https://codereview.chromium.org/2603523002/diff/220001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:259: VerifyState(test_callback.WaitForResult(), kStateLoggingString, true, true, On 2017/01/27 02:23:23, eroman (slow) wrote: > optional: For readability I suggest extracting test_callback.WatitForResult() to > its own line: > > std::unique_ptr<base::DictionaryValue> state = test_callback.WaitForResult(); Done. https://codereview.chromium.org/2603523002/diff/220001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:277: VerifyState(test_callback.WaitForResult(), kStateNotLoggingString, true, On 2017/01/27 02:23:24, eroman (slow) wrote: > same optional suggestion here. Done. https://codereview.chromium.org/2603523002/diff/220001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:487: // Create test context getter on |net_thread_| and wait for it to finish. On 2017/01/27 02:23:24, eroman (slow) wrote: > wording: the "context getter" isn't created on the |net_thread|, only the > context is. Done. https://codereview.chromium.org/2603523002/diff/220001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:491: base::WaitableEvent create_context_done_event( On 2017/01/27 02:23:24, eroman (slow) wrote: > Another less verbose way to accomplish this is using PostTaskAndReply(). > I haven't verified but I think this would work: > > base::TestClosure init_done; > > ...->PostTaskAndReply(FROM_HERE, base::Bind(...), init_done.closure()); > > init_done.WaitForResult(); > > The advantage of this way is you don't need to initialize a WaitableEvent, or > pass one into the setup function. Also if initialization needed to run tasks on > the main thread that would work (since the main thread isn't blocked). Done. https://codereview.chromium.org/2603523002/diff/220001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:500: scoped_refptr<net::TestURLRequestContextGetter> context_getter( On 2017/01/27 02:23:24, eroman (slow) wrote: > I would say this can move into SetUpTestContextWithQuicTimeout. Currently it > populates |context|, however that is transferred tot eh context_getter -- so > might as well have it just fill in the TestURLRequestContextGetter. > > (It can obtain the current task runner since it will be running on network > thread). Done. https://codereview.chromium.org/2603523002/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/webui/net_export/net_export_ui.cc (right): https://codereview.chromium.org/2603523002/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/webui/net_export/net_export_ui.cc:79: // Cache of GetApplicationContex()->GetNetLog()->net_log_file_writer(). This On 2017/01/27 02:23:24, eroman (slow) wrote: > Update this comment (to match changes you made in non-ios file) Done.
wangyix@chromium.org changed reviewers: + mmenke@chromium.org, tommycli@chromium.org
Adding mmenke@ and tommycli@ for OWNER status
On 2017/01/27 18:32:31, wangyix1 wrote: > Adding mmenke@ and tommycli@ for OWNER status Which files do you need me to sign off on? Generally best to make this clear when you add people for reviews.
On 2017/01/27 18:40:33, mmenke (Out Feb 4 to March 5) wrote: > On 2017/01/27 18:32:31, wangyix1 wrote: > > Adding mmenke@ and tommycli@ for OWNER status > > Which files do you need me to sign off on? Generally best to make this clear > when you add people for reviews. mmenke@: components/cronet/android/cronet_url_request_context_adapter.cc tommycli@: chrome/browser/ui/webui/net_export_ui.cc
On 2017/01/27 18:54:14, wangyix1 wrote: > On 2017/01/27 18:40:33, mmenke (Out Feb 4 to March 5) wrote: > > On 2017/01/27 18:32:31, wangyix1 wrote: > > > Adding mmenke@ and tommycli@ for OWNER status > > > > Which files do you need me to sign off on? Generally best to make this clear > > when you add people for reviews. > > mmenke@: components/cronet/android/cronet_url_request_context_adapter.cc > tommycli@: chrome/browser/ui/webui/net_export_ui.cc Thanks! LGTM.
lgtm https://codereview.chromium.org/2603523002/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/net_export_ui.cc (right): https://codereview.chromium.org/2603523002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/net_export_ui.cc:93: net::NetLogCaptureMode capture_mode); indent is off. did you run git cl format on this patch? https://codereview.chromium.org/2603523002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/net_export_ui.cc:127: // Its value is only valid when the save dialog is open on the desktop UI. *only valid while the ...
https://codereview.chromium.org/2603523002/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/net_export_ui.cc (right): https://codereview.chromium.org/2603523002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/net_export_ui.cc:93: net::NetLogCaptureMode capture_mode); On 2017/01/27 19:11:24, tommycli wrote: > indent is off. did you run git cl format on this patch? Done. https://codereview.chromium.org/2603523002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/net_export_ui.cc:127: // Its value is only valid when the save dialog is open on the desktop UI. On 2017/01/27 19:11:24, tommycli wrote: > *only valid while the ... Done.
The CQ bit was checked by wangyix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org, tommycli@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2603523002/#ps260001 (title: "Fixed tommycli's nits from patchset 13")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by wangyix@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1485565178695300,
"parent_rev": "b18b3b209f5fd75613706fbfc7d6182e23250572", "commit_rev":
"8ae679d977a7eb3c784d7bc6483cdf74400b52bd"}
Message was sent while issue was closed.
Description was changed from ========== Move net-export thread-hopping code into NetLogFileWriter and add IO polled data. (1) Use net::FileNetLogObserver in place of net::WriteToFileNetLogObserver for chrome://net-export (which is a big part of crbug.com/679030). (2) Move NetLogFileWriter's public interface from the FILE_USER_BLOCKING to the UI thread (internalizing the post tasks to file thread/UI thread to simplify things) (3) Add plumbing for passing polled data, which will be collected on both the UI thread and IO thread (crbug.com/438656). Some of the refactor done will also facilitate crbug.com/679021, which is just a few extra lines. In this CL, only the polled data from the IO thread will be retrieved; the UI thread polled data will be added in a follow-up CL. BUG=679030,438656,679021 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Move net-export thread-hopping code into NetLogFileWriter and add IO polled data. (1) Use net::FileNetLogObserver in place of net::WriteToFileNetLogObserver for chrome://net-export (which is a big part of crbug.com/679030). (2) Move NetLogFileWriter's public interface from the FILE_USER_BLOCKING to the UI thread (internalizing the post tasks to file thread/UI thread to simplify things) (3) Add plumbing for passing polled data, which will be collected on both the UI thread and IO thread (crbug.com/438656). Some of the refactor done will also facilitate crbug.com/679021, which is just a few extra lines. In this CL, only the polled data from the IO thread will be retrieved; the UI thread polled data will be added in a follow-up CL. BUG=679030,438656,679021 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2603523002 Cr-Commit-Position: refs/heads/master@{#446875} Committed: https://chromium.googlesource.com/chromium/src/+/8ae679d977a7eb3c784d7bc6483c... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/8ae679d977a7eb3c784d7bc6483c... |
