Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2698143004:
Add ongoing events to net-export log when logging starts (Closed)
DescriptionAdd ongoing events to net-export log when logging starts
Update NetLogFileWriter::StartNetLog() to take a list of URLRequestContextGetters from which to retrieve ongoing events to add to the log.
Refactor FileNetLogObserver so log entries (specifically, entries for ongoing events) can be manually added from outside the class before it's attached to ChromeNetLog.
Add helper function to NetExportMessageHandler that retrieves a list of URLRequestContextGetters from which ongoing events are retrieved.
BUG=679021
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Review-Url: https://codereview.chromium.org/2698143004
Cr-Commit-Position: refs/heads/master@{#452993}
Committed: https://chromium.googlesource.com/chromium/src/+/179c480dc64ee7b8a7b8927a8ff4cc15e04071cf
Patch Set 1 #Patch Set 2 : Added/improved some comments #
Total comments: 19
Patch Set 3 : Fixed Eric's comments from ps2 #Patch Set 4 : Forgot to update comment for StopNetLog() #
Total comments: 20
Patch Set 5 : Forgot to add something to StartNetLog() comment #Patch Set 6 : Fixed Eric's comments from ps4 #Patch Set 7 : Forgot a fix #Patch Set 8 : Updated NetLogFileWriter and FileNetLogObserver unit-tests #Patch Set 9 : Fixed EXPECT statement in NetLogFileWriterTest.StartWithContextGetters #Patch Set 10 : Updated ios NetExportMessageHandler #
Total comments: 23
Patch Set 11 : Fix "chosen constructor is explicit in copy-initialization" compile error #Patch Set 12 : Fixed Eric's comments from ps10 #
Total comments: 4
Messages
Total messages: 45 (29 generated)
Description was changed from ========== git cl format Added class ProxyScriptFetcherContextGetter and member proxy_script_fetcher_context_getter_ to IOThread. Added proxy_script_fetcher_context_getter_ and system_url_request_context_getter_ to NetExportMessageHandler::context_getters_. Added member context_getters_ to NetExportMessageHandler. Currently includes 3 context getters: profile->GetRequestContext() content::BrowserContext::GetDefaultStoragePartition(profile)->GetMediaURLRequestContext() profile->GetRequestContextForExtensions() context_getters_ is passed to all StartNetLog() calls in NetExportMessageHandler. Compiles, does't seem to crash Chrome at least. Changed NetLogFileWriter to add events from a list of context getters passed in StartNetLog(). Added state STATE_STARTING_LOG. Updated unit-test, passing in empty list everywhere for now. Updated FileNetLogObserver interface to move StartObserving() code into constructor. Updated callers in NetLogFileWriter and android cronet. Compiles BUG= ========== to ========== Add ongoing events to net-export log when logging starts Update NetLogFileWriter::StartNetLog() to take a list of URLRequestContextGetters from which to retrive ongoing events to add to the log. Refactor FileNetLogObserver so log entries (specifically, entries for ongoing events) can be manually added from outside the class before it's attached to ChromeNetLog. Update NetExportMessageHandler to cache a list of URLRequestContextGetters from which log entries for ongoing events are retrieved. Update IOThread class to expose a context getter for the proxy_script_fetcher_context. BUG=679021 ==========
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Add ongoing events to net-export log when logging starts Update NetLogFileWriter::StartNetLog() to take a list of URLRequestContextGetters from which to retrive ongoing events to add to the log. Refactor FileNetLogObserver so log entries (specifically, entries for ongoing events) can be manually added from outside the class before it's attached to ChromeNetLog. Update NetExportMessageHandler to cache a list of URLRequestContextGetters from which log entries for ongoing events are retrieved. Update IOThread class to expose a context getter for the proxy_script_fetcher_context. BUG=679021 ========== to ========== Add ongoing events to net-export log when logging starts Update NetLogFileWriter::StartNetLog() to take a list of URLRequestContextGetters from which to retrive ongoing events to add to the log. Refactor FileNetLogObserver so log entries (specifically, entries for ongoing events) can be manually added from outside the class before it's attached to ChromeNetLog. Update NetExportMessageHandler to cache a list of URLRequestContextGetters from which log entries for ongoing events are retrieved. Update IOThread class to expose a context getter for the proxy_script_fetcher_context. BUG=679021 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
wangyix@chromium.org changed reviewers: + eroman@chromium.org, wangyix@chromium.org
PTAL NetLogFileWriter unit-test has been updated to work with new StartNetLog() signature, but doesn't have any tests that actually passes in a context getter.
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
https://codereview.chromium.org/2698143004/diff/40001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2698143004/diff/40001/chrome/browser/io_threa... chrome/browser/io_thread.cc:293: // net::URLRequestContextGetter implementation. Was this a manual change or something the linter did? https://codereview.chromium.org/2698143004/diff/40001/chrome/browser/io_threa... chrome/browser/io_thread.cc:341: IOThread* const io_thread_; // Weak pointer, owned by BrowserProcess. Can remove "weak pointer" (which has specific meaning in our code, and does not apply here). Saying it is owned by BrowserProcess is sufficient. https://codereview.chromium.org/2698143004/diff/40001/chrome/browser/io_threa... chrome/browser/io_thread.cc:342: scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_; Instead of this member variable how about just returning BrowserThread::GetTaskRunnerForThread(BrowserThread::IO) from GetNetworkTaskRunner() ? https://codereview.chromium.org/2698143004/diff/40001/chrome/browser/io_thread.h File chrome/browser/io_thread.h (right): https://codereview.chromium.org/2698143004/diff/40001/chrome/browser/io_threa... chrome/browser/io_thread.h:233: net::URLRequestContextGetter* proxy_script_fetcher_context_getter(); This is only used by NetExportUI. Instead of exposing it as API in IOThread can you move the corresponding implementation into net_export_ui.cc? io_thread.{h,cc} is already a big ball of spaghetti, so I would like to avoid adding more stuff into it which isn't core. In this case the need for a URLRequestContextGetter is only a constraint for the NetExportUI so we can isolate the ugliness to that client. https://codereview.chromium.org/2698143004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/net_export_ui.cc (right): https://codereview.chromium.org/2698143004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:187: context_getters_.push_back( Rather than adding a member variable initialized in RegisterMessages(), can you extract this block of code to a method? i.e.: URLRequestContextGetterList GetURLRequestContexts() const; This can be called directly from the two locations that need it (both of which are UI thread), and avoids adding a member variable for it. https://codereview.chromium.org/2698143004/diff/40001/components/net_log/net_... File components/net_log/net_log_file_writer.cc (right): https://codereview.chromium.org/2698143004/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:183: state_ = STATE_STARTING_LOG; Can you explain the role of this new state? It does seem useful for understanding the interactions with StopNetLog(). Are there any use-cases where communicating this to the Javascript is necessary https://codereview.chromium.org/2698143004/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:270: default: Why add the default case? We try to avoid specifying default cases, as it gives better static checks for future-proofing code. https://codereview.chromium.org/2698143004/diff/40001/components/net_log/net_... File components/net_log/net_log_file_writer.h (right): https://codereview.chromium.org/2698143004/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.h:103: const URLRequestContextGetterList& context_getters); Please update the comments to mention what this does. In particular can you clarify the following: * These contexts are optional * They are ONLY used for logging the outstanding requests when logging was first started * They are not used for getting the polled data * Why there is an asymmetry in the API (starting takes a list, whereas stopping takes a single one) * All the contexts must be bound to the same thread. https://codereview.chromium.org/2698143004/diff/40001/net/log/file_net_log_ob... File net/log/file_net_log_observer.h (right): https://codereview.chromium.org/2698143004/diff/40001/net/log/file_net_log_ob... net/log/file_net_log_observer.h:51: // Constructs a FileNetLogObserver in bounded mode. I generally like the split of file allocation vs start observing, however I am not enthusiastic about the constructor overloading. Instead of using overloaded constructors how about either of these patterns (just a small adjustment from current code): (1) Factory method (my preferred approach) (2) Named initialization methods The difficulty with constructor overloading is it is unclear when reading the callers whether they are using bounded vs unbounded modes as the parameter all kind of look the same. Having different symbol names clarifies this as part of the API contract. ------- Approach 1 details: ------- Using the factory method idiom you would split things up into 2 static methods, and one instance method: static std::unique_ptr<FileNetLogObserver> CreateBounded(... params for bounded mode...); static std::unique_ptr<FileNetLogObserver> CreateUnbounded(... params for bounded mode...); void StartObserving(NetLog*, CaptureMode); Additionally you would make all the constructors (probably just 1 total) private. This is conceptually the same as what you have now, except the constructors are essentially given names (and allocation is handled internally rather than by the caller). -------- Approach 2 -------- With this approach instead of factory methods you would have two initialization methods: InitBounded(...); InitUnbounded(...); This makes selection of the mode explicit as they have a different name, however has issues that the constructor method version does not (there are more illegal states to consider, such as if the object is never initialized, or initialized twice). https://codereview.chromium.org/2698143004/diff/40001/net/log/file_net_log_ob... net/log/file_net_log_observer.h:94: // Stops observing net_log(). Must be called after StartObservingBounded/ (a) StartObservingBounded/ StartObservingUnbounded no longer exist (b) Can you also mention in the comment that this has the effect of closing the output file(s) ?
Fixed comments from PS2. Still need to update FileNetLogObserver and NetLogFileWriter unit-tests. https://codereview.chromium.org/2698143004/diff/40001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2698143004/diff/40001/chrome/browser/io_threa... chrome/browser/io_thread.cc:293: // net::URLRequestContextGetter implementation. On 2017/02/21 22:14:23, eroman wrote: > Was this a manual change or something the linter did? I manually changed this. I don't really know why. https://codereview.chromium.org/2698143004/diff/40001/chrome/browser/io_threa... chrome/browser/io_thread.cc:341: IOThread* const io_thread_; // Weak pointer, owned by BrowserProcess. On 2017/02/21 22:14:23, eroman wrote: > Can remove "weak pointer" (which has specific meaning in our code, and does not > apply here). > Saying it is owned by BrowserProcess is sufficient. Done. https://codereview.chromium.org/2698143004/diff/40001/chrome/browser/io_threa... chrome/browser/io_thread.cc:342: scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_; On 2017/02/21 22:14:23, eroman wrote: > Instead of this member variable how about just returning > BrowserThread::GetTaskRunnerForThread(BrowserThread::IO) from > GetNetworkTaskRunner() ? Done. https://codereview.chromium.org/2698143004/diff/40001/chrome/browser/io_thread.h File chrome/browser/io_thread.h (right): https://codereview.chromium.org/2698143004/diff/40001/chrome/browser/io_threa... chrome/browser/io_thread.h:233: net::URLRequestContextGetter* proxy_script_fetcher_context_getter(); On 2017/02/21 22:14:23, eroman wrote: > This is only used by NetExportUI. > > Instead of exposing it as API in IOThread can you move the corresponding > implementation into net_export_ui.cc? > > io_thread.{h,cc} is already a big ball of spaghetti, so I would like to avoid > adding more stuff into it which isn't core. > > In this case the need for a URLRequestContextGetter is only a constraint for the > NetExportUI so we can isolate the ugliness to that client. Done. https://codereview.chromium.org/2698143004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/net_export_ui.cc (right): https://codereview.chromium.org/2698143004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:187: context_getters_.push_back( On 2017/02/21 22:14:23, eroman wrote: > Rather than adding a member variable initialized in RegisterMessages(), can you > extract this block of code to a method? > > i.e.: > URLRequestContextGetterList GetURLRequestContexts() const; > > This can be called directly from the two locations that need it (both of which > are UI thread), and avoids adding a member variable for it. Done. https://codereview.chromium.org/2698143004/diff/40001/components/net_log/net_... File components/net_log/net_log_file_writer.cc (right): https://codereview.chromium.org/2698143004/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:183: state_ = STATE_STARTING_LOG; On 2017/02/21 22:14:23, eroman wrote: > Can you explain the role of this new state? > > It does seem useful for understanding the interactions with StopNetLog(). Are > there any use-cases where communicating this to the Javascript is necessary This state is needed so that while pending entries are added on the net thread, commands received on the main thread can be no-oped. For example, suppose we do "state_ = STATE_LOGGING" here instead. If StopNetLog() is called while CreateNetLogEntriesForActiveObjects() is running on the net thread, it would not be no-oped, which is incorrect. https://codereview.chromium.org/2698143004/diff/40001/components/net_log/net_... components/net_log/net_log_file_writer.cc:270: default: On 2017/02/21 22:14:23, eroman wrote: > Why add the default case? We try to avoid specifying default cases, as it gives > better static checks for future-proofing code. Done. https://codereview.chromium.org/2698143004/diff/40001/net/log/file_net_log_ob... File net/log/file_net_log_observer.h (right): https://codereview.chromium.org/2698143004/diff/40001/net/log/file_net_log_ob... net/log/file_net_log_observer.h:51: // Constructs a FileNetLogObserver in bounded mode. On 2017/02/21 22:14:23, eroman wrote: > I generally like the split of file allocation vs start observing, however I am > not enthusiastic about the constructor overloading. > > Instead of using overloaded constructors how about either of these patterns > (just a small adjustment from current code): > > (1) Factory method (my preferred approach) > (2) Named initialization methods > > The difficulty with constructor overloading is it is unclear when reading the > callers whether they are using bounded vs unbounded modes as the parameter all > kind of look the same. Having different symbol names clarifies this as part of > the API contract. > > ------- > Approach 1 details: > ------- > > Using the factory method idiom you would split things up into 2 static methods, > and one instance method: > > static std::unique_ptr<FileNetLogObserver> CreateBounded(... params for bounded > mode...); > static std::unique_ptr<FileNetLogObserver> CreateUnbounded(... params for > bounded mode...); > void StartObserving(NetLog*, CaptureMode); > > Additionally you would make all the constructors (probably just 1 total) > private. > > This is conceptually the same as what you have now, except the constructors are > essentially given names (and allocation is handled internally rather than by the > caller). > > -------- > Approach 2 > -------- > > With this approach instead of factory methods you would have two initialization > methods: > > InitBounded(...); > InitUnbounded(...); > > This makes selection of the mode explicit as they have a different name, however > has issues that the constructor method version does not (there are more illegal > states to consider, such as if the object is never initialized, or initialized > twice). Done, I went with factory methods. https://codereview.chromium.org/2698143004/diff/40001/net/log/file_net_log_ob... net/log/file_net_log_observer.h:94: // Stops observing net_log(). Must be called after StartObservingBounded/ On 2017/02/21 22:14:23, eroman wrote: > (a) StartObservingBounded/ StartObservingUnbounded no longer exist (b) Can you > also mention in the comment that this has the effect of closing the output > file(s) ? Done.
https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/io_threa... chrome/browser/io_thread.cc:293: // net::URLRequestContextGetter implementation. same comment here. https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/io_thread.h File chrome/browser/io_thread.h (right): https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/io_threa... chrome/browser/io_thread.h:227: // Returns a getter for the system URLRequestContext. Only called on the UI Please propose refactors io_thread as a separate CL. Also note as a practical matter that I am not an OWNER for io_thread.{h,cc} so you would need a different reviewer for those changes anyway. https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/net_export_ui.cc (right): https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:68: ProxyScriptFetcherContextGetter::ProxyScriptFetcherContextGetter( [optional] My style suggestion here would be to inline all the definitions in the class declaration above (given how small these all are). https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:187: proxy_script_fetcher_context_getter_; Please remove this variable (can be constructed directly as part of GetURLRequestContextGetterList()). https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:379: NetExportMessageHandler::GetURLRequestContexts() const { Pelase add: DCHECK_CURRENTLY_ON(BrowserThread::UI); https://codereview.chromium.org/2698143004/diff/80001/components/net_log/net_... File components/net_log/net_log_file_writer.cc (right): https://codereview.chromium.org/2698143004/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.cc:77: contexts.insert(getter->GetURLRequestContext()); Can you add: DCHECK(getter->GetNetworkTaskRunner()->BelongsToCurrentThread()); (The assumption is they all were for the same thread). https://codereview.chromium.org/2698143004/diff/80001/net/log/file_net_log_ob... File net/log/file_net_log_observer.cc (right): https://codereview.chromium.org/2698143004/diff/80001/net/log/file_net_log_ob... net/log/file_net_log_observer.cc:238: FileWriter* file_writer = see suggestion in header file -- we try to keep pointers in std::unique_ptr<> especially when passing ownership across function boundaries. https://codereview.chromium.org/2698143004/diff/80001/net/log/file_net_log_ob... File net/log/file_net_log_observer.h (right): https://codereview.chromium.org/2698143004/diff/80001/net/log/file_net_log_ob... net/log/file_net_log_observer.h:91: // Attach this observer to |net_log| and begin observing events. Attaches this observer to |net_log| and begins observing events. https://codereview.chromium.org/2698143004/diff/80001/net/log/file_net_log_ob... net/log/file_net_log_observer.h:120: FileWriter* file_writer, Can you change this parameter to std::unique_ptr<FileWriter> file_writer, Then you can also remove the comment about its lifetime, since it is clear that ownership is being passed. (the implementation could then call .release() to transfer to raw pointer). (Wherever possible we avoid naked pointers) https://codereview.chromium.org/2698143004/diff/80001/net/log/file_net_log_ob... net/log/file_net_log_observer.h:121: size_t write_queue_memory_max, [optional] I suggest changing this parameter to be a scoped_refptr<WriteQueue>. Then any constructor details of WriteQueue are internalized to the bounded vs unbounded factory. This also more closely matches how the functions were split before, where each was responsible for assigning write_queue_ and file_writer_.
PTAL https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/io_threa... chrome/browser/io_thread.cc:293: // net::URLRequestContextGetter implementation. On 2017/02/22 02:03:29, eroman wrote: > same comment here. Change reverted. https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/io_thread.h File chrome/browser/io_thread.h (right): https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/io_threa... chrome/browser/io_thread.h:227: // Returns a getter for the system URLRequestContext. Only called on the UI On 2017/02/22 02:03:29, eroman wrote: > Please propose refactors io_thread as a separate CL. > > Also note as a practical matter that I am not an OWNER for io_thread.{h,cc} so > you would need a different reviewer for those changes anyway. Change reverted. https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/net_export_ui.cc (right): https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:68: ProxyScriptFetcherContextGetter::ProxyScriptFetcherContextGetter( On 2017/02/22 02:03:29, eroman wrote: > [optional] My style suggestion here would be to inline all the definitions in > the class declaration above (given how small these all are). Done. https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:187: proxy_script_fetcher_context_getter_; On 2017/02/22 02:03:29, eroman wrote: > Please remove this variable (can be constructed directly as part of > GetURLRequestContextGetterList()). Done. https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:379: NetExportMessageHandler::GetURLRequestContexts() const { On 2017/02/22 02:03:29, eroman wrote: > Pelase add: DCHECK_CURRENTLY_ON(BrowserThread::UI); Done. https://codereview.chromium.org/2698143004/diff/80001/components/net_log/net_... File components/net_log/net_log_file_writer.cc (right): https://codereview.chromium.org/2698143004/diff/80001/components/net_log/net_... components/net_log/net_log_file_writer.cc:77: contexts.insert(getter->GetURLRequestContext()); On 2017/02/22 02:03:29, eroman wrote: > Can you add: > > DCHECK(getter->GetNetworkTaskRunner()->BelongsToCurrentThread()); > > (The assumption is they all were for the same thread). Done. https://codereview.chromium.org/2698143004/diff/80001/net/log/file_net_log_ob... File net/log/file_net_log_observer.cc (right): https://codereview.chromium.org/2698143004/diff/80001/net/log/file_net_log_ob... net/log/file_net_log_observer.cc:238: FileWriter* file_writer = On 2017/02/22 02:03:29, eroman wrote: > see suggestion in header file -- we try to keep pointers in std::unique_ptr<> > especially when passing ownership across function boundaries. Done. https://codereview.chromium.org/2698143004/diff/80001/net/log/file_net_log_ob... File net/log/file_net_log_observer.h (right): https://codereview.chromium.org/2698143004/diff/80001/net/log/file_net_log_ob... net/log/file_net_log_observer.h:91: // Attach this observer to |net_log| and begin observing events. On 2017/02/22 02:03:29, eroman wrote: > Attaches this observer to |net_log| and begins observing events. Done. https://codereview.chromium.org/2698143004/diff/80001/net/log/file_net_log_ob... net/log/file_net_log_observer.h:120: FileWriter* file_writer, On 2017/02/22 02:03:29, eroman wrote: > Can you change this parameter to > std::unique_ptr<FileWriter> file_writer, > > Then you can also remove the comment about its lifetime, since it is clear that > ownership is being passed. (the implementation could then call .release() to > transfer to raw pointer). > > (Wherever possible we avoid naked pointers) Done. https://codereview.chromium.org/2698143004/diff/80001/net/log/file_net_log_ob... net/log/file_net_log_observer.h:121: size_t write_queue_memory_max, On 2017/02/22 02:03:29, eroman wrote: > [optional] I suggest changing this parameter to be a scoped_refptr<WriteQueue>. > Then any constructor details of WriteQueue are internalized to the bounded vs > unbounded factory. > > This also more closely matches how the functions were split before, where each > was responsible for assigning write_queue_ and file_writer_. Done.
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm! Comments for the CL description below: > from which to retrive ongoing events Typo. > Update NetExportMessageHandler to cache a list of No longer applicable. > Update IOThread class to expose a context getter for the proxy_script_fetcher_context. No longer applicable. https://codereview.chromium.org/2698143004/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/net_export_ui.cc (right): https://codereview.chromium.org/2698143004/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/net_export_ui.cc:57: // net::URLRequestContextGetter implementation. [optional] I suggest removing this comment and the same one below (class is so simple this feels like clutter) https://codereview.chromium.org/2698143004/diff/200001/components/net_log/net... File components/net_log/net_log_file_writer_unittest.cc (right): https://codereview.chromium.org/2698143004/diff/200001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:190: std::unique_ptr<net::TestURLRequestContext> context = [optional] My preference is to use "auto" with MakeUnique https://codereview.chromium.org/2698143004/diff/200001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:195: *request = context->CreateRequest(GURL(request_url), net::IDLE, delegate); [optional] I would suggest making |request_url| be of type |const GURL&| instead of StringPiece, especially given that it is used with base::Bind(). (It is possible with StringPiece to end up with invalid pointers by the time the task runs, since it doesn't own the memory). https://codereview.chromium.org/2698143004/diff/200001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:688: TEST_F(NetLogFileWriterTest, StartWithContextGetters) { Thanks for adding the test! https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... File net/log/file_net_log_observer.cc (right): https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... net/log/file_net_log_observer.cc:244: return std::unique_ptr<FileNetLogObserver>( [optional] base::MakeUnique. This has the advantage of (a) Don't have to type the class name twice (b) Once we get C++14 support they will get bulk translated to std::make_unique<> which is the modern C++ way https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... net/log/file_net_log_observer.cc:246: write_queue, std::move(constants))); std::move(write_queue) --- saves a refcount increment/decrement. https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... net/log/file_net_log_observer.cc:259: return std::unique_ptr<FileNetLogObserver>( [optional] same thing with base::MakeUnique https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... net/log/file_net_log_observer.cc:261: write_queue, std::move(constants))); std::move(write_queue) https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... net/log/file_net_log_observer.cc:316: : file_task_runner_(file_task_runner), std::move() https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... net/log/file_net_log_observer.cc:317: write_queue_(write_queue), std::move() https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... File net/log/file_net_log_observer.h (right): https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... net/log/file_net_log_observer.h:44: // The consumer must call StartObservingBounded/StartObservingUnbounded before Please update these comments.
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...
Description was changed from ========== Add ongoing events to net-export log when logging starts Update NetLogFileWriter::StartNetLog() to take a list of URLRequestContextGetters from which to retrive ongoing events to add to the log. Refactor FileNetLogObserver so log entries (specifically, entries for ongoing events) can be manually added from outside the class before it's attached to ChromeNetLog. Update NetExportMessageHandler to cache a list of URLRequestContextGetters from which log entries for ongoing events are retrieved. Update IOThread class to expose a context getter for the proxy_script_fetcher_context. BUG=679021 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Add ongoing events to net-export log when logging starts Update NetLogFileWriter::StartNetLog() to take a list of URLRequestContextGetters from which to retrieve ongoing events to add to the log. Refactor FileNetLogObserver so log entries (specifically, entries for ongoing events) can be manually added from outside the class before it's attached to ChromeNetLog. Add helper function to NetExportMessageHandler that retrieves a list of URLRequestContextGetters from which ongoing events are retrieved. BUG=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...
Did not switch over to MakeUnique in the two suggested spots since constructor is private. Is there a workaround for not being able to use MakeUnique<T> when the constructor for T is private? https://codereview.chromium.org/2698143004/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/net_export_ui.cc (right): https://codereview.chromium.org/2698143004/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/net_export_ui.cc:57: // net::URLRequestContextGetter implementation. On 2017/02/24 00:17:38, eroman wrote: > [optional] I suggest removing this comment and the same one below (class is so > simple this feels like clutter) Done. https://codereview.chromium.org/2698143004/diff/200001/components/net_log/net... File components/net_log/net_log_file_writer_unittest.cc (right): https://codereview.chromium.org/2698143004/diff/200001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:190: std::unique_ptr<net::TestURLRequestContext> context = On 2017/02/24 00:17:38, eroman wrote: > [optional] My preference is to use "auto" with MakeUnique Done. https://codereview.chromium.org/2698143004/diff/200001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:195: *request = context->CreateRequest(GURL(request_url), net::IDLE, delegate); On 2017/02/24 00:17:38, eroman wrote: > [optional] I would suggest making |request_url| be of type |const GURL&| instead > of StringPiece, especially given that it is used with base::Bind(). > > (It is possible with StringPiece to end up with invalid pointers by the time the > task runs, since it doesn't own the memory). Done. https://codereview.chromium.org/2698143004/diff/200001/components/net_log/net... components/net_log/net_log_file_writer_unittest.cc:688: TEST_F(NetLogFileWriterTest, StartWithContextGetters) { On 2017/02/24 00:17:38, eroman wrote: > Thanks for adding the test! Acknowledged. https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... File net/log/file_net_log_observer.cc (right): https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... net/log/file_net_log_observer.cc:244: return std::unique_ptr<FileNetLogObserver>( On 2017/02/24 00:17:39, eroman wrote: > [optional] base::MakeUnique. > > This has the advantage of > (a) Don't have to type the class name twice > (b) Once we get C++14 support they will get bulk translated to > std::make_unique<> which is the modern C++ way Using MakeUnique failed to compile since the FileNetLogObserver constructor is private. https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... net/log/file_net_log_observer.cc:246: write_queue, std::move(constants))); On 2017/02/24 00:17:39, eroman wrote: > std::move(write_queue) --- saves a refcount increment/decrement. Done. https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... net/log/file_net_log_observer.cc:259: return std::unique_ptr<FileNetLogObserver>( On 2017/02/24 00:17:39, eroman wrote: > [optional] same thing with base::MakeUnique Using MakeUnique failed to compile since the FileNetLogObserver constructor is private. https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... net/log/file_net_log_observer.cc:261: write_queue, std::move(constants))); On 2017/02/24 00:17:38, eroman wrote: > std::move(write_queue) Done. https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... net/log/file_net_log_observer.cc:316: : file_task_runner_(file_task_runner), On 2017/02/24 00:17:39, eroman wrote: > std::move() Done. https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... net/log/file_net_log_observer.cc:317: write_queue_(write_queue), On 2017/02/24 00:17:39, eroman wrote: > std::move() Done. https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... File net/log/file_net_log_observer.h (right): https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... net/log/file_net_log_observer.h:44: // The consumer must call StartObservingBounded/StartObservingUnbounded before On 2017/02/24 00:17:39, eroman wrote: > Please update these comments. Done.
lgtm https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... File net/log/file_net_log_observer.cc (right): https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_o... net/log/file_net_log_observer.cc:244: return std::unique_ptr<FileNetLogObserver>( On 2017/02/24 00:58:08, wangyix1 wrote: > On 2017/02/24 00:17:39, eroman wrote: > > [optional] base::MakeUnique. > > > > This has the advantage of > > (a) Don't have to type the class name twice > > (b) Once we get C++14 support they will get bulk translated to > > std::make_unique<> which is the modern C++ way > > Using MakeUnique failed to compile since the FileNetLogObserver constructor is > private. Oh right, good point!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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
The CQ bit was unchecked by wangyix@chromium.org
wangyix@chromium.org changed reviewers: + xunjieli@chromium.org
Adding xunjieli@ for OWNER status for file: components/cronet/android/cronet_url_request_context_adapter.cc
components/cronet/android/cronet_url_request_context_adapter.cc lgtm mod one nit below https://codereview.chromium.org/2698143004/diff/240001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2698143004/diff/240001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:13: #include <set> why is this needed?
https://codereview.chromium.org/2698143004/diff/240001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2698143004/diff/240001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:13: #include <set> On 2017/02/24 18:49:03, xunjieli wrote: > why is this needed? The first param to CreateNetLogEntriesForActiveObjects() at line 1030 is const std::set<URLRequestContext*>& contexts; I just used an initializer list for that param.
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...
https://codereview.chromium.org/2698143004/diff/240001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2698143004/diff/240001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:13: #include <set> On 2017/02/24 22:07:19, wangyix1 wrote: > On 2017/02/24 18:49:03, xunjieli wrote: > > why is this needed? > > The first param to CreateNetLogEntriesForActiveObjects() at line 1030 is const > std::set<URLRequestContext*>& contexts; I just used an initializer list for that > param. Acknowledged.
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1487974049605180,
"parent_rev": "e6007d0b43c4a3ee34598349aa3246086f451feb", "commit_rev":
"179c480dc64ee7b8a7b8927a8ff4cc15e04071cf"}
Message was sent while issue was closed.
Description was changed from ========== Add ongoing events to net-export log when logging starts Update NetLogFileWriter::StartNetLog() to take a list of URLRequestContextGetters from which to retrieve ongoing events to add to the log. Refactor FileNetLogObserver so log entries (specifically, entries for ongoing events) can be manually added from outside the class before it's attached to ChromeNetLog. Add helper function to NetExportMessageHandler that retrieves a list of URLRequestContextGetters from which ongoing events are retrieved. BUG=679021 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Add ongoing events to net-export log when logging starts Update NetLogFileWriter::StartNetLog() to take a list of URLRequestContextGetters from which to retrieve ongoing events to add to the log. Refactor FileNetLogObserver so log entries (specifically, entries for ongoing events) can be manually added from outside the class before it's attached to ChromeNetLog. Add helper function to NetExportMessageHandler that retrieves a list of URLRequestContextGetters from which ongoing events are retrieved. BUG=679021 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2698143004 Cr-Commit-Position: refs/heads/master@{#452993} Committed: https://chromium.googlesource.com/chromium/src/+/179c480dc64ee7b8a7b8927a8ff4... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001) as https://chromium.googlesource.com/chromium/src/+/179c480dc64ee7b8a7b8927a8ff4...
Message was sent while issue was closed.
https://codereview.chromium.org/2698143004/diff/240001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2698143004/diff/240001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:13: #include <set> On 2017/02/24 22:10:08, xunjieli wrote: > On 2017/02/24 22:07:19, wangyix1 wrote: > > On 2017/02/24 18:49:03, xunjieli wrote: > > > why is this needed? > > > > The first param to CreateNetLogEntriesForActiveObjects() at line 1030 is const > > std::set<URLRequestContext*>& contexts; I just used an initializer list for > that > > param. > > Acknowledged. I don't think you quite answered Helen's question. cronet_url_request_context_adapter.cc was already including <set> transitively by way of "net/log/net_log_util.h". So in that sense, the addition of <set> should not have been needed for compilation sake (if you remove the line it still compiles right?). In general APIs that take a std::set<> will end up polluting <set> since it can't be forward declared. Including <set> in this file explicitly isn't necessarily a bad thing. Adherents of the include-what-you-use would encourage doing so. However we don't enforce IWYU in Chrome thanks to lack of tooling support, so I would say this include is entirely optional, and Helen's question still stands. I don't really care whether we include <set> here or not. But would like to see the motivation explained one way or another. Thanks!
Message was sent while issue was closed.
Oh, interesting. I added it because I assumed IWYU, that's all. I haven't tried compiling without <set>, but I assume it would compile successfully for the reason you stated. |
