Remove deprecated ListValue::Append(Value*) overload on Linux.

Remove deprecated ListValue::Append(Value*) overload on Linux.

Less comments, more type safety.

BUG=581865
R=danakj@chromium.org
TBR=sky@chromium.org

Committed: https://crrev.com/71bb8453e8df8e6b5b596a3d4b038b42d03529d2
Cr-Commit-Position: refs/heads/master@{#419360}

[dcheng, 2016-05-25 23:45:26]

Description was changed from

==========
Moo


Moo

BUG=
==========

to

==========
Moo


Moo

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[dcheng, 2016-05-25 23:45:27]

Description was changed from

==========
Moo


Moo

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Moo


Moo

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[dcheng, 2016-09-14 20:36:52]

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

[commit-bot: I haz the power, 2016-09-14 20:38:01]

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, 2016-09-14 20:38:04]

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

[commit-bot: I haz the power, 2016-09-14 20:38:05]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[dcheng, 2016-09-14 21:50:31]

Description was changed from

==========
Moo


Moo

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Moo

BUG=
==========

[dcheng, 2016-09-15 00:42:38]

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

[dcheng, 2016-09-15 00:42:49]

Description was changed from

==========
Moo

BUG=
==========

to

==========
Remove deprecated ListValue::Append(Value*) overload on Linux.

Less comments, more type safety.

BUG=581865
==========

[dcheng, 2016-09-15 00:42:49]

dcheng@chromium.org changed reviewers:
+ danakj@chromium.org

[commit-bot: I haz the power, 2016-09-15 00:43:42]

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, 2016-09-15 02:47:32]

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

[commit-bot: I haz the power, 2016-09-15 02:47:33]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-09-15 21:07:34]

https://codereview.chromium.org/2014103002/diff/140001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_targets_ui.cc (right):

https://codereview.chromium.org/2014103002/diff/140001/chrome/browser/devtool...
chrome/browser/devtools/devtools_targets_ui.cc:256: base::DictionaryValue*
parent = id_to_descriptor[parent_id].get();
Um, hm. We're moving things out of this map now, which means we leave behind
null. Is it true that we'll always still have parent_id's uniqueptr in the set
here? Cuz L249 removes them..

[dcheng, 2016-09-15 21:10:21]

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

[dcheng, 2016-09-15 21:10:22]

+pfeldman for the inspector question

https://codereview.chromium.org/2014103002/diff/140001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_targets_ui.cc (right):

https://codereview.chromium.org/2014103002/diff/140001/chrome/browser/devtool...
chrome/browser/devtools/devtools_targets_ui.cc:256: base::DictionaryValue*
parent = id_to_descriptor[parent_id].get();
On 2016/09/15 21:07:34, danakj wrote:
> Um, hm. We're moving things out of this map now, which means we leave behind
> null. Is it true that we'll always still have parent_id's uniqueptr in the set
> here? Cuz L249 removes them..

Well. The tests pass ^_^

But I agree that this is very confusing, and frankly, I'm not sure it's right.

[pfeldman, 2016-09-16 21:39:03]

https://codereview.chromium.org/2014103002/diff/140001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_targets_ui.cc (right):

https://codereview.chromium.org/2014103002/diff/140001/chrome/browser/devtool...
chrome/browser/devtools/devtools_targets_ui.cc:256: base::DictionaryValue*
parent = id_to_descriptor[parent_id].get();
Haven't looked at this code for a while...

So we have a list and we want to end up with the tree based on the parentId
relationship. It means that you need to:
- maintain the map with ownership in one id_to_descriptor.
- keep rawpointer-based mapping in another map next to it for parent lookup.

Otherwise line 258 fails with nullptr access. And there are obviously no tests
for this branch :(

[dcheng, 2016-09-16 23:15:01]

PTAL =( =( =(

https://codereview.chromium.org/2014103002/diff/140001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_targets_ui.cc (right):

https://codereview.chromium.org/2014103002/diff/140001/chrome/browser/devtool...
chrome/browser/devtools/devtools_targets_ui.cc:256: base::DictionaryValue*
parent = id_to_descriptor[parent_id].get();
On 2016/09/16 21:39:02, pfeldman wrote:
> Haven't looked at this code for a while...
> 
> So we have a list and we want to end up with the tree based on the parentId
> relationship. It means that you need to:
> - maintain the map with ownership in one id_to_descriptor.
> - keep rawpointer-based mapping in another map next to it for parent lookup.
> 
> Otherwise line 258 fails with nullptr access. And there are obviously no tests
> for this branch :(

Keeping two trees seems strictly worse... so I just turned it back into raw
pointers.

[danakj, 2016-09-16 23:29:05]

LGTM :|

https://codereview.chromium.org/2014103002/diff/160001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_targets_ui.cc (right):

https://codereview.chromium.org/2014103002/diff/160001/chrome/browser/devtool...
chrome/browser/devtools/devtools_targets_ui.cc:252:
list_value.Append(WrapUnique(descriptor));
base::

https://codereview.chromium.org/2014103002/diff/160001/chrome/browser/devtool...
chrome/browser/devtools/devtools_targets_ui.cc:260:
guests->Append(WrapUnique(descriptor));
base::

[dcheng, 2016-09-16 23:33:47]

dcheng@chromium.org changed reviewers:
+ sky@chromium.org

[dcheng, 2016-09-16 23:33:48]

TBRing sky@ for //chrome changes.

[dcheng, 2016-09-16 23:34:03]

Description was changed from

==========
Remove deprecated ListValue::Append(Value*) overload on Linux.

Less comments, more type safety.

BUG=581865
==========

to

==========
Remove deprecated ListValue::Append(Value*) overload on Linux.

Less comments, more type safety.

BUG=581865
R=danakj@chromium.org
TBR=sky@chromium.org
==========

[dcheng, 2016-09-16 23:34:08]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-09-16 23:34:08]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2014103002/#ps180001
(title: "ADL fail")

[commit-bot: I haz the power, 2016-09-16 23:34:42]

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, 2016-09-17 01:30:34]

Description was changed from

==========
Remove deprecated ListValue::Append(Value*) overload on Linux.

Less comments, more type safety.

BUG=581865
R=danakj@chromium.org
TBR=sky@chromium.org
==========

to

==========
Remove deprecated ListValue::Append(Value*) overload on Linux.

Less comments, more type safety.

BUG=581865
R=danakj@chromium.org
TBR=sky@chromium.org
==========

[commit-bot: I haz the power, 2016-09-17 01:30:36]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-17 01:32:11]

Description was changed from

==========
Remove deprecated ListValue::Append(Value*) overload on Linux.

Less comments, more type safety.

BUG=581865
R=danakj@chromium.org
TBR=sky@chromium.org
==========

to

==========
Remove deprecated ListValue::Append(Value*) overload on Linux.

Less comments, more type safety.

BUG=581865
R=danakj@chromium.org
TBR=sky@chromium.org

Committed: https://crrev.com/71bb8453e8df8e6b5b596a3d4b038b42d03529d2
Cr-Commit-Position: refs/heads/master@{#419360}
==========

[commit-bot: I haz the power, 2016-09-17 01:32:13]

Patchset 10 (id:??) landed as
https://crrev.com/71bb8453e8df8e6b5b596a3d4b038b42d03529d2
Cr-Commit-Position: refs/heads/master@{#419360}