Migrate WebRtcEventLogUploaderImpl to SimpleURLLoader |
|||||||
Issue descriptionWebRtcEventLogUploaderImpl is still using the legacy URLFetcher + URLRequestContextGetter APIs to upload data. We need to switch it to using SimpleURLLoader. This should block any experimentation with the NetworkService on Canary.
,
Aug 10
,
Aug 10
I defer to the WebRTC folks on that, as I've never had to debug a WebRTC issue.
,
Aug 13
,
Aug 13
,
Aug 13
jam@, can you comment?
,
Aug 13
Sergio just added upload progress a few days ago in https://chromium-review.googlesource.com/c/chromium/src/+/1154971, so it seems that can be used?
,
Aug 13
CL https://chromium-review.googlesource.com/c/chromium/src/+/1171622 (gathering bots results).
,
Aug 13
yes! (re: comment #7)
,
Aug 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/75cdb263f3b7d0949d6a5dc19894c172a600ea69 commit 75cdb263f3b7d0949d6a5dc19894c172a600ea69 Author: Antonio Gomes <tonikitoo@igalia.com> Date: Fri Aug 17 13:51:23 2018 Migrate WebRtcEventLogUploaderImpl to SimpleURLLoader URLFetcher will stop working with advent of Network Service, and SimpleURLLoader is the replacement API for most clients. This CL migrates WebRtcEventLogUploaderImpl et al away from URLFetcher. Note that this CL slightly changes the flow of the WebRtcEventLogUploader code. Previously, an URLRequestContextGetter instance was acquired on UI thread, passed to the IO-capable task runner, used there, and ultimately deleted either on the UI thread or on the IO-capable task runner thread, depending on the termination circumstance (eg BrowserProcessImpl shutdown or upload completion, respectively). However, URLLoaderFactory and SimpleURLLoader have stricter threading restrictions than URLFetcher and URLRequestContextGetter. For instance, a SharedURLLoaderFactory instance needs to be created, used and deleted on the same thread. Given this scenario, a natural approach to tackle this migration would be the use of CrossThreadSharedURLLoaderFactory{Info} classes. However, as mentioned earlier, the fact that WebRtcEventLogUploader instances are constructed the aforementioned IO-capable task runner thread and the deletion thread varies, make the use of CrossThreadSharedURLLoaderFactory{Info} less straightforward. This CL makes use of the thread agnostic network::mojom::URLLoaderFactoryPtrInfo class instead: from the IO-capable task runner it posts a message to the UI thread to bind a network::mojom::URLLoaderFactory object. As per the mojo documentation [1]: "Once the LoggerPtr is bound we can immediately begin calling Logger interface methods on it, which will immediately write messages into the pipe. These messages will stay queued on the receiving end of the pipe until someone binds to it and starts reading them." [1] https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindings/README.md#Binding-an-Interface-Request BUG=773295, 873187 Change-Id: Ic29222ea63e90dcb26f3124bdea1c627570dc04d Reviewed-on: https://chromium-review.googlesource.com/1171622 Commit-Queue: Antonio Gomes <tonikitoo@igalia.com> Reviewed-by: Elad Alon <eladalon@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Cr-Commit-Position: refs/heads/master@{#584048} [modify] https://crrev.com/75cdb263f3b7d0949d6a5dc19894c172a600ea69/chrome/browser/media/webrtc/webrtc_event_log_manager.cc [modify] https://crrev.com/75cdb263f3b7d0949d6a5dc19894c172a600ea69/chrome/browser/media/webrtc/webrtc_event_log_manager.h [modify] https://crrev.com/75cdb263f3b7d0949d6a5dc19894c172a600ea69/chrome/browser/media/webrtc/webrtc_event_log_manager_remote.cc [modify] https://crrev.com/75cdb263f3b7d0949d6a5dc19894c172a600ea69/chrome/browser/media/webrtc/webrtc_event_log_manager_remote.h [modify] https://crrev.com/75cdb263f3b7d0949d6a5dc19894c172a600ea69/chrome/browser/media/webrtc/webrtc_event_log_manager_unittest.cc [modify] https://crrev.com/75cdb263f3b7d0949d6a5dc19894c172a600ea69/chrome/browser/media/webrtc/webrtc_event_log_uploader.cc [modify] https://crrev.com/75cdb263f3b7d0949d6a5dc19894c172a600ea69/chrome/browser/media/webrtc/webrtc_event_log_uploader.h [modify] https://crrev.com/75cdb263f3b7d0949d6a5dc19894c172a600ea69/chrome/browser/media/webrtc/webrtc_event_log_uploader_impl_unittest.cc
,
Aug 17
,
Aug 27
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by toniki...@chromium.org
, Aug 10I can help here. Quick question: #if DCHECK_IS_ON() void WebRtcEventLogUploaderImpl::Delegate::OnURLFetchUploadProgress( const net::URLFetcher* source, int64_t current, int64_t total) { std::string unit; if (total <= 1000) { unit = "bytes"; } else if (total <= 1000 * 1000) { unit = "KBs"; current /= 1000; total /= 1000; } else { unit = "MBs"; current /= 1000 * 1000; total /= 1000 * 1000; } VLOG(1) << "WebRTC event log upload progress: " << current << " / " << total << " " << unit << "."; } #endif Code hooks to the progress update callback for debugging purposes. Would it make sense to get the class without this support right away, and file a follow up issue to track adding this code back, once hook is available?