Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(132)

Issue 2698143004: Add ongoing events to net-export log when logging starts (Closed)

Created:
3 years, 10 months ago by wangyix
Modified:
3 years, 10 months ago
Reviewers:
wangyix1, eroman, xunjieli
CC:
chromium-reviews, cbentzel+watch_chromium.org, bnc+watch_chromium.org, eroman, mmenke, net-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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
Unified diffs Side-by-side diffs Delta from patch set Stats (+400 lines, -247 lines) Patch
M chrome/browser/ui/webui/net_export_ui.cc View 1 2 3 4 5 6 7 8 9 10 11 9 chunks +60 lines, -2 lines 0 comments Download
M components/cronet/android/cronet_url_request_context_adapter.cc View 1 2 2 chunks +9 lines, -6 lines 4 comments Download
M components/net_log/net_log_file_writer.h View 1 2 3 4 5 chunks +24 lines, -5 lines 0 comments Download
M components/net_log/net_log_file_writer.cc View 1 2 3 4 5 9 chunks +55 lines, -13 lines 0 comments Download
M components/net_log/net_log_file_writer_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 18 chunks +120 lines, -34 lines 0 comments Download
M ios/chrome/browser/ui/webui/net_export/net_export_ui.cc View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -1 line 0 comments Download
M net/log/file_net_log_observer.h View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +40 lines, -51 lines 0 comments Download
M net/log/file_net_log_observer.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +60 lines, -67 lines 0 comments Download
M net/log/file_net_log_observer_unittest.cc View 1 2 3 4 5 6 7 16 chunks +29 lines, -68 lines 0 comments Download

Messages

Total messages: 45 (29 generated)
wangyix1
PTAL NetLogFileWriter unit-test has been updated to work with new StartNetLog() signature, but doesn't have ...
3 years, 10 months ago (2017-02-16 23:58:38 UTC) #5
eroman
https://codereview.chromium.org/2698143004/diff/40001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2698143004/diff/40001/chrome/browser/io_thread.cc#newcode293 chrome/browser/io_thread.cc:293: // net::URLRequestContextGetter implementation. Was this a manual change or ...
3 years, 10 months ago (2017-02-21 22:14:23 UTC) #10
wangyix1
Fixed comments from PS2. Still need to update FileNetLogObserver and NetLogFileWriter unit-tests. https://codereview.chromium.org/2698143004/diff/40001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc ...
3 years, 10 months ago (2017-02-22 01:28:18 UTC) #11
eroman
https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/io_thread.cc#newcode293 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 ...
3 years, 10 months ago (2017-02-22 02:03:29 UTC) #12
wangyix1
PTAL https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2698143004/diff/80001/chrome/browser/io_thread.cc#newcode293 chrome/browser/io_thread.cc:293: // net::URLRequestContextGetter implementation. On 2017/02/22 02:03:29, eroman wrote: ...
3 years, 10 months ago (2017-02-23 02:14:57 UTC) #13
eroman
lgtm! Comments for the CL description below: > from which to retrive ongoing events Typo. ...
3 years, 10 months ago (2017-02-24 00:17:39 UTC) #22
wangyix1
Did not switch over to MakeUnique in the two suggested spots since constructor is private. ...
3 years, 10 months ago (2017-02-24 00:58:08 UTC) #28
eroman
lgtm https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_observer.cc File net/log/file_net_log_observer.cc (right): https://codereview.chromium.org/2698143004/diff/200001/net/log/file_net_log_observer.cc#newcode244 net/log/file_net_log_observer.cc:244: return std::unique_ptr<FileNetLogObserver>( On 2017/02/24 00:58:08, wangyix1 wrote: > ...
3 years, 10 months ago (2017-02-24 01:05:46 UTC) #29
wangyix1
Adding xunjieli@ for OWNER status for file: components/cronet/android/cronet_url_request_context_adapter.cc
3 years, 10 months ago (2017-02-24 18:25:50 UTC) #35
xunjieli
components/cronet/android/cronet_url_request_context_adapter.cc lgtm mod one nit below https://codereview.chromium.org/2698143004/diff/240001/components/cronet/android/cronet_url_request_context_adapter.cc File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2698143004/diff/240001/components/cronet/android/cronet_url_request_context_adapter.cc#newcode13 components/cronet/android/cronet_url_request_context_adapter.cc:13: #include <set> why ...
3 years, 10 months ago (2017-02-24 18:49:03 UTC) #36
wangyix1
https://codereview.chromium.org/2698143004/diff/240001/components/cronet/android/cronet_url_request_context_adapter.cc File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2698143004/diff/240001/components/cronet/android/cronet_url_request_context_adapter.cc#newcode13 components/cronet/android/cronet_url_request_context_adapter.cc:13: #include <set> On 2017/02/24 18:49:03, xunjieli wrote: > why ...
3 years, 10 months ago (2017-02-24 22:07:20 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698143004/240001
3 years, 10 months ago (2017-02-24 22:08:27 UTC) #39
xunjieli
https://codereview.chromium.org/2698143004/diff/240001/components/cronet/android/cronet_url_request_context_adapter.cc File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2698143004/diff/240001/components/cronet/android/cronet_url_request_context_adapter.cc#newcode13 components/cronet/android/cronet_url_request_context_adapter.cc:13: #include <set> On 2017/02/24 22:07:19, wangyix1 wrote: > On ...
3 years, 10 months ago (2017-02-24 22:10:09 UTC) #40
commit-bot: I haz the power
Committed patchset #12 (id:240001) as https://chromium.googlesource.com/chromium/src/+/179c480dc64ee7b8a7b8927a8ff4cc15e04071cf
3 years, 10 months ago (2017-02-24 23:30:11 UTC) #43
eroman
https://codereview.chromium.org/2698143004/diff/240001/components/cronet/android/cronet_url_request_context_adapter.cc File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2698143004/diff/240001/components/cronet/android/cronet_url_request_context_adapter.cc#newcode13 components/cronet/android/cronet_url_request_context_adapter.cc:13: #include <set> On 2017/02/24 22:10:08, xunjieli wrote: > On ...
3 years, 10 months ago (2017-02-24 23:43:45 UTC) #44
wangyix1
3 years, 10 months ago (2017-02-25 02:54:24 UTC) #45
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.

Powered by Google App Engine
This is Rietveld 408576698