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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 596253003: DevTools: RemoteDevice and RemoteBrowser are now value types (Closed)

Created:
6 years, 3 months ago by vkuzkokov
Modified:
6 years, 2 months ago
Reviewers:
dgozman, pfeldman
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@pfc3
Project:
chromium
Visibility:
Public.

Description

DevTools: RemoteDevice and RemoteBrowser are now value types This removes unnecessary references that could prevent Android Device from being freed. BUG=387067 Committed: https://crrev.com/f16940a62fe2782ddc8e90d3c36a25b746ef4587 Cr-Commit-Position: refs/heads/master@{#299504}

Patch Set 1 #

Patch Set 2 : Removed unnecessary refptrs to Device #

Patch Set 3 : Fixed test #

Total comments: 12

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : Used field instead of global for AgentHostDelegate map #

Patch Set 7 : Removed setters from RemoteBrowser #

Total comments: 4

Patch Set 8 : Rebased. Addressed comments #

Total comments: 15

Patch Set 9 : #

Total comments: 2

Patch Set 10 : #

Patch Set 11 : #

Total comments: 4

Patch Set 12 : #

Patch Set 13 : #

Patch Set 14 : #

Patch Set 15 : Rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+349 lines, -299 lines) Patch
M chrome/browser/devtools/device/adb/adb_client_socket_browsertest.cc View 1 2 3 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/browser/devtools/device/devtools_android_bridge.h View 1 2 3 4 5 6 7 8 9 5 chunks +85 lines, -70 lines 0 comments Download
M chrome/browser/devtools/device/devtools_android_bridge.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 25 chunks +217 lines, -193 lines 0 comments Download
M chrome/browser/devtools/device/port_forwarding_controller.h View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/devtools/device/port_forwarding_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +18 lines, -16 lines 0 comments Download
M chrome/browser/devtools/devtools_targets_ui.cc View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +20 lines, -14 lines 0 comments Download
M chrome/browser/ui/webui/devtools_ui.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +5 lines, -2 lines 0 comments Download

Messages

Total messages: 23 (6 generated)
vkuzkokov
6 years, 2 months ago (2014-09-25 11:18:35 UTC) #2
dgozman
https://codereview.chromium.org/596253003/diff/40001/chrome/browser/devtools/device/devtools_android_bridge.h File chrome/browser/devtools/device/devtools_android_bridge.h (right): https://codereview.chromium.org/596253003/diff/40001/chrome/browser/devtools/device/devtools_android_bridge.h#newcode85 chrome/browser/devtools/device/devtools_android_bridge.h:85: RemoteBrowser(const std::string& serial, Can we make constructor private? https://codereview.chromium.org/596253003/diff/40001/chrome/browser/devtools/device/devtools_android_bridge.h#newcode124 ...
6 years, 2 months ago (2014-09-25 14:23:17 UTC) #3
vkuzkokov
Fixed prerequisite CL in description.
6 years, 2 months ago (2014-09-25 14:50:49 UTC) #4
vkuzkokov
https://codereview.chromium.org/596253003/diff/40001/chrome/browser/devtools/device/devtools_android_bridge.h File chrome/browser/devtools/device/devtools_android_bridge.h (right): https://codereview.chromium.org/596253003/diff/40001/chrome/browser/devtools/device/devtools_android_bridge.h#newcode85 chrome/browser/devtools/device/devtools_android_bridge.h:85: RemoteBrowser(const std::string& serial, On 2014/09/25 14:23:17, dgozman wrote: > ...
6 years, 2 months ago (2014-09-25 16:48:47 UTC) #5
dgozman
https://codereview.chromium.org/596253003/diff/120001/chrome/browser/devtools/device/devtools_android_bridge.h File chrome/browser/devtools/device/devtools_android_bridge.h (right): https://codereview.chromium.org/596253003/diff/120001/chrome/browser/devtools/device/devtools_android_bridge.h#newcode211 chrome/browser/devtools/device/devtools_android_bridge.h:211: friend class PortForwardingController; Why we need this friend? https://codereview.chromium.org/596253003/diff/120001/chrome/browser/devtools/device/devtools_android_bridge.h#newcode223 ...
6 years, 2 months ago (2014-10-01 14:27:28 UTC) #7
vkuzkokov
https://codereview.chromium.org/596253003/diff/120001/chrome/browser/devtools/device/devtools_android_bridge.h File chrome/browser/devtools/device/devtools_android_bridge.h (right): https://codereview.chromium.org/596253003/diff/120001/chrome/browser/devtools/device/devtools_android_bridge.h#newcode211 chrome/browser/devtools/device/devtools_android_bridge.h:211: friend class PortForwardingController; On 2014/10/01 14:27:28, dgozman wrote: > ...
6 years, 2 months ago (2014-10-02 11:35:01 UTC) #8
vkuzkokov
On 2014/10/02 11:35:01, vkuzkokov wrote: > https://codereview.chromium.org/596253003/diff/120001/chrome/browser/devtools/device/devtools_android_bridge.h > File chrome/browser/devtools/device/devtools_android_bridge.h (right): > > https://codereview.chromium.org/596253003/diff/120001/chrome/browser/devtools/device/devtools_android_bridge.h#newcode211 > ...
6 years, 2 months ago (2014-10-02 12:07:52 UTC) #9
dgozman
lgtm with nits https://codereview.chromium.org/596253003/diff/160001/chrome/browser/devtools/device/devtools_android_bridge.cc File chrome/browser/devtools/device/devtools_android_bridge.cc (right): https://codereview.chromium.org/596253003/diff/160001/chrome/browser/devtools/device/devtools_android_bridge.cc#newcode108 chrome/browser/devtools/device/devtools_android_bridge.cc:108: for (AndroidDeviceManager::Devices::const_iterator it = devices.begin(); for ...
6 years, 2 months ago (2014-10-02 12:19:34 UTC) #10
vkuzkokov
https://codereview.chromium.org/596253003/diff/160001/chrome/browser/devtools/device/devtools_android_bridge.cc File chrome/browser/devtools/device/devtools_android_bridge.cc (right): https://codereview.chromium.org/596253003/diff/160001/chrome/browser/devtools/device/devtools_android_bridge.cc#newcode108 chrome/browser/devtools/device/devtools_android_bridge.cc:108: for (AndroidDeviceManager::Devices::const_iterator it = devices.begin(); On 2014/10/02 12:19:34, dgozman ...
6 years, 2 months ago (2014-10-02 13:07:47 UTC) #11
vkuzkokov
Pavel, PTAL. Needs owners approval for: chrome/browser/ui/webui/devtools_ui.cc
6 years, 2 months ago (2014-10-02 13:21:47 UTC) #13
dgozman
https://codereview.chromium.org/596253003/diff/220001/chrome/browser/devtools/device/devtools_android_bridge.cc File chrome/browser/devtools/device/devtools_android_bridge.cc (right): https://codereview.chromium.org/596253003/diff/220001/chrome/browser/devtools/device/devtools_android_bridge.cc#newcode576 chrome/browser/devtools/device/devtools_android_bridge.cc:576: if (value && value->GetAsDictionary(&dict)) I think, |if (value)| is ...
6 years, 2 months ago (2014-10-02 13:21:49 UTC) #15
vkuzkokov
https://codereview.chromium.org/596253003/diff/220001/chrome/browser/devtools/device/devtools_android_bridge.cc File chrome/browser/devtools/device/devtools_android_bridge.cc (right): https://codereview.chromium.org/596253003/diff/220001/chrome/browser/devtools/device/devtools_android_bridge.cc#newcode576 chrome/browser/devtools/device/devtools_android_bridge.cc:576: if (value && value->GetAsDictionary(&dict)) On 2014/10/02 13:21:48, dgozman wrote: ...
6 years, 2 months ago (2014-10-03 10:53:32 UTC) #16
vkuzkokov
Pavel, PTAL. Needs owners approval for: chrome/browser/ui/webui/devtools_ui.cc
6 years, 2 months ago (2014-10-07 10:16:37 UTC) #18
pfeldman
lgtm
6 years, 2 months ago (2014-10-14 14:15:37 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/596253003/300001
6 years, 2 months ago (2014-10-14 16:54:42 UTC) #21
commit-bot: I haz the power
Committed patchset #15 (id:300001)
6 years, 2 months ago (2014-10-14 17:49:20 UTC) #22
commit-bot: I haz the power
6 years, 2 months ago (2014-10-14 17:50:27 UTC) #23
Message was sent while issue was closed. 
Patchset 15 (id:??) landed as
https://crrev.com/f16940a62fe2782ddc8e90d3c36a25b746ef4587
Cr-Commit-Position: refs/heads/master@{#299504}

Powered by Google App Engine
This is Rietveld 408576698