New issue
Advanced search Search tips

Issue 873187 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 17
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

Migrate WebRtcEventLogUploaderImpl to SimpleURLLoader

Project Member Reported by mmenke@chromium.org, Aug 10

Issue description

WebRtcEventLogUploaderImpl 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.
 
Status: tonikitoochromium.org (was: Untriaged)
I 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?
Owner: toniki...@chromium.org
Status: (was: tonikitoochromium.org)
I defer to the WebRTC folks on that, as I've never had to debug a WebRTC issue.
Status: Assigned
Cc: jam@chromium.org
jam@, can you comment?
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?
Status: Started (was: Assigned)
CL https://chromium-review.googlesource.com/c/chromium/src/+/1171622 (gathering bots results).
yes! (re: comment #7)
Project Member

Comment 10 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Labels: M-70

Sign in to add a comment