Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
784523002:
Populating browser list in WebRTCDeviceProvider. (Closed)
|
Created:
6 years ago by SeRya Modified:
6 years ago Reviewers:
dgozman CC:
chromium-reviews, vsevik, yurys, paulirish+reviews_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org, pfeldman Base URL:
https://chromium.googlesource.com/chromium/src.git@new-web-client-2 Project:
chromium Visibility:
Public. |
DescriptionPopulating browser list in WebRTCDeviceProvider.
To test this code:
1. register DevToolsBridge Test App instance in GCD (https://docs.google.com/a/chromium.org/document/d/188V6TWV8ivbjAPIp6aqwffWrY78xN2an5REajZHpunk/edit#heading=h.6gpqqy16q0ga).
2. Enable #enable-devtools-experiments flag.
3. Observe this app as a browser in chrome://inspect/#devices
BUG=383418
TBR=vitalybuka@chromium.org
TEST=See instructions above
Committed: https://crrev.com/55f1da3788e45125ae2d6c55646d71523c9651b4
Cr-Commit-Position: refs/heads/master@{#307291}
Patch Set 1 #Patch Set 2 : Merged #
Total comments: 21
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : Fixed compilation on win. #Messages
Total messages: 16 (4 generated)
serya@chromium.org changed reviewers: + dgozman@chromium.org
PTAL. Based on uncommited CL https://codereview.chromium.org/762903004/
Can we use local_discovery testing to test this code? https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... File chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc (right): https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:64: static Serials GetDevices(base::WeakPtr<DevToolsBridgeClient> weak_ptr); Why not use an instance method with callback instead of static? https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:176: return; nit: indentation https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:251: BrowserInfoList().swap(browsers_); This looks awkward to me... https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:258: // TODO(serya): Not all devices are browsers. Filter them out. Let's implement this right away. We should also group browsers from the same device. https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:319: const DeviceInfoCallback& callback) { nit: indentation
Regarding testring. I don't see how local_discovery code could help with testing. Meaningful test should include a mock for GCDApiFlow that is't exists. I think we may add one to chrome/browser/local_discovery in a separate CL and make use of it. I did't upload new patch set since beside indention nits there is nothing to change. I will fix it with more meaningful changes or before commitintg. https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... File chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc (right): https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:64: static Serials GetDevices(base::WeakPtr<DevToolsBridgeClient> weak_ptr); On 2014/12/06 13:16:33, dgozman wrote: > Why not use an instance method with callback instead of static? Non-void method can't be bound to a weak reference. And it makes sense: the call may not be swallowed if the object died. It must return something. https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:251: BrowserInfoList().swap(browsers_); On 2014/12/06 13:16:33, dgozman wrote: > This looks awkward to me... This is well-known pattern. Standard clear method remains memory reserverd by the vector. https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:258: // TODO(serya): Not all devices are browsers. Filter them out. On 2014/12/06 13:16:33, dgozman wrote: > Let's implement this right away. We should also group browsers from the same > device. There is not enough information provided by CloudDeviceList right away.
https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... File chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc (right): https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:29: using Serials = std::vector<std::string>; Let's be consistent: SerialsList. https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:47: const char kSerial[] = "clouddevices"; nit: add empty line after this please. https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:61: void UpdateBrowserList(); this one is private https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:62: BrowserInfoList const& GetRemoteBrowsers() const { return browsers_; } nit: this is not used. https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:202: result.model = "Remote browsers"; nit: make a named constant https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:258: // TODO(serya): Not all devices are browsers. Filter them out. On 2014/12/07 13:40:46, SeRya wrote: > On 2014/12/06 13:16:33, dgozman wrote: > > Let's implement this right away. We should also group browsers from the same > > device. > > There is not enough information provided by CloudDeviceList right away. We should check for the command we use to be present on device. Let's follow up with a patch immediately.
https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... File chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc (right): https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:29: using Serials = std::vector<std::string>; On 2014/12/08 14:27:21, dgozman wrote: > Let's be consistent: SerialsList. Done. https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:47: const char kSerial[] = "clouddevices"; On 2014/12/08 14:27:21, dgozman wrote: > nit: add empty line after this please. Done. https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:61: void UpdateBrowserList(); On 2014/12/08 14:27:22, dgozman wrote: > this one is private Done. https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:62: BrowserInfoList const& GetRemoteBrowsers() const { return browsers_; } On 2014/12/08 14:27:22, dgozman wrote: > nit: this is not used. Done. https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:176: return; On 2014/12/06 13:16:33, dgozman wrote: > nit: indentation Done. https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:202: result.model = "Remote browsers"; On 2014/12/08 14:27:21, dgozman wrote: > nit: make a named constant Done. https://codereview.chromium.org/784523002/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:319: const DeviceInfoCallback& callback) { On 2014/12/06 13:16:33, dgozman wrote: > nit: indentation Done.
lgtm https://codereview.chromium.org/784523002/diff/40001/chrome/browser/devtools/... File chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc (right): https://codereview.chromium.org/784523002/diff/40001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:207: if (auto* ptr = weak_ptr.get()) { What about renaming to |self| and |weak_self|? |ptr| is too ambiguous.
https://codereview.chromium.org/784523002/diff/40001/chrome/browser/devtools/... File chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc (right): https://codereview.chromium.org/784523002/diff/40001/chrome/browser/devtools/... chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:207: if (auto* ptr = weak_ptr.get()) { On 2014/12/08 16:08:16, dgozman wrote: > What about renaming to |self| and |weak_self|? |ptr| is too ambiguous. Done.
The CQ bit was checked by serya@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/784523002/60001
The CQ bit was unchecked by serya@chromium.org
The CQ bit was checked by serya@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/784523002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/55f1da3788e45125ae2d6c55646d71523c9651b4 Cr-Commit-Position: refs/heads/master@{#307291} |
