DevTools: retain DTAH in all the targets to match their life time.

DevTools: retain DTAH in all the targets to match their life time.

BUG=704503
TBR= changes in cast/ reflect changes to the API

Review-Url: https://codereview.chromium.org/2833213002
Cr-Commit-Position: refs/heads/master@{#466759}
Committed: https://chromium.googlesource.com/chromium/src/+/82f29400ec0c56897e7e464cf91af7a1ce86e145

[pfeldman, 2017-04-21 23:32:48]

pfeldman@chromium.org changed reviewers:
+ dgozman@chromium.org

[pfeldman, 2017-04-21 23:32:53]

The CQ bit was checked by pfeldman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 23:33:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 23:49:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 23:49:37]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[dgozman, 2017-04-22 00:00:45]

https://codereview.chromium.org/2833213002/diff/1/content/browser/devtools/de...
File content/browser/devtools/devtools_agent_host_impl.cc (right):

https://codereview.chromium.org/2833213002/diff/1/content/browser/devtools/de...
content/browser/devtools/devtools_agent_host_impl.cc:86: DevToolsAgentHost::List
DevToolsAgentHost::DiscoverAllHosts() {
Inline this method to http handler.

https://codereview.chromium.org/2833213002/diff/1/content/browser/devtools/de...
content/browser/devtools/devtools_agent_host_impl.cc:90: return
manager->delegate()->DiscoverTargets();
DiscoverTargetsAvailableRemotely

https://codereview.chromium.org/2833213002/diff/1/content/public/browser/devt...
File content/public/browser/devtools_agent_host.h (right):

https://codereview.chromium.org/2833213002/diff/1/content/public/browser/devt...
content/public/browser/devtools_agent_host.h:108: using DiscoveryCallback =
base::Callback<void(List)>;
Remove?

https://codereview.chromium.org/2833213002/diff/1/content/public/browser/devt...
File content/public/browser/devtools_manager_delegate.h (right):

https://codereview.chromium.org/2833213002/diff/1/content/public/browser/devt...
content/public/browser/devtools_manager_delegate.h:38: // Returns whether
embedder wants to be responsible for targets.
nit: extra dot at the end..

[pfeldman, 2017-04-22 00:19:28]

All done.

[dgozman, 2017-04-22 00:30:26]

lgtm

https://codereview.chromium.org/2833213002/diff/20001/content/browser/devtool...
File content/browser/devtools/devtools_http_handler.cc (right):

https://codereview.chromium.org/2833213002/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_http_handler.cc:542:
RespondToJsonList(connection_id, info.headers["host"], list);
std::move(list)

[pfeldman, 2017-04-22 00:35:11]

The CQ bit was checked by pfeldman@chromium.org to run a CQ dry run

[pfeldman, 2017-04-22 00:35:18]

The CQ bit was unchecked by pfeldman@chromium.org

[pfeldman, 2017-04-22 00:41:00]

pfeldman@chromium.org changed reviewers:
+ bauerb@chromium.org

[pfeldman, 2017-04-22 00:41:01]

+bauerb as owner for chrome/browser/android/tab_android.*

We need to retain DevToolsAgentHost as long as the actual target it represents,
TabAndroid, exists. We do that for RFAH and everything else, would like to do it
for tabs in Android. It is essential that it is created lazily and is retained
by the target until it is destroyed. I did not want to put lazy construction
into TabAndroid to keep devtools business aside, hence the getter and setter.

[Bernhard Bauer, 2017-04-24 12:16:57]

https://codereview.chromium.org/2833213002/diff/40001/chrome/browser/android/...
File chrome/browser/android/devtools_manager_delegate_android.cc (right):

https://codereview.chromium.org/2833213002/diff/40001/chrome/browser/android/...
chrome/browser/android/devtools_manager_delegate_android.cc:169:
base::WrapUnique(new TabProxyDelegate(tab)));
Use base::MakeUnique<>?

https://codereview.chromium.org/2833213002/diff/40001/chrome/browser/android/...
chrome/browser/android/devtools_manager_delegate_android.cc:248: return tab ?
DevToolsAgentHostForTab(tab) : nullptr;
Mini-nit: I kind of liked the old `if (tab) return nullptr`, as it fits with the
other early returns.

https://codereview.chromium.org/2833213002/diff/40001/chrome/browser/android/...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2833213002/diff/40001/chrome/browser/android/...
chrome/browser/android/tab_android.cc:807: devtools_host_ = host;
std::move(host)?

[Bernhard Bauer, 2017-04-24 14:38:34]

LGTM assuming you'll fix the nits before submitting :)

[pfeldman, 2017-04-24 14:40:30]

The CQ bit was checked by pfeldman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 14:41:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 14:58:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-24 14:58:41]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[pfeldman, 2017-04-24 17:57:10]

The CQ bit was checked by pfeldman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 17:57:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 18:14:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-24 18:14:31]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[pfeldman, 2017-04-24 18:49:05]

The CQ bit was checked by pfeldman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 18:50:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 20:00:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-24 20:00:12]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2017-04-24 20:29:18]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2017-04-24 20:29:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2833213002/#ps80001
(title: "cast test")

[commit-bot: I haz the power, 2017-04-24 20:30:14]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 20:42:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-24 20:42:08]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pfeldman, 2017-04-24 20:51:32]

Description was changed from

==========
DevTools: retain DTAH in all the targets to match their life time.

BUG=704503
==========

to

==========
DevTools: retain DTAH in all the targets to match their life time.

BUG=704503
TBR= for changes of the API
==========

[pfeldman, 2017-04-24 20:51:51]

Description was changed from

==========
DevTools: retain DTAH in all the targets to match their life time.

BUG=704503
TBR= for changes of the API
==========

to

==========
DevTools: retain DTAH in all the targets to match their life time.

BUG=704503
TBR= changes in cast/ reflect changes to the API
==========

[pfeldman, 2017-04-24 20:51:57]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2017-04-24 20:52:30]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 20:58:07]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493067117027830,
"parent_rev": "9fa61b8eb03ca17eb5e9bffabd01f338e8fd1420", "commit_rev":
"82f29400ec0c56897e7e464cf91af7a1ce86e145"}

[commit-bot: I haz the power, 2017-04-24 20:58:17]

Description was changed from

==========
DevTools: retain DTAH in all the targets to match their life time.

BUG=704503
TBR= changes in cast/ reflect changes to the API
==========

to

==========
DevTools: retain DTAH in all the targets to match their life time.

BUG=704503
TBR= changes in cast/ reflect changes to the API

Review-Url: https://codereview.chromium.org/2833213002
Cr-Commit-Position: refs/heads/master@{#466759}
Committed:
https://chromium.googlesource.com/chromium/src/+/82f29400ec0c56897e7e464cf91a...
==========

[commit-bot: I haz the power, 2017-04-24 20:58:18]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/82f29400ec0c56897e7e464cf91a...