SerializeHostDescriptions ignores hosts with duplicated names

SerializeHostDescriptions ignores hosts with duplicated names

|HostDescriptionNode.name| identifies the node and should not be shared by two
different nodes. If multiple nodes are passed to SerializeHostDescriptions with
the same name, currently SerializeHostDescriptions ends up using
DictionaryValue after performing std::move on it, because of mixing nodes with
the same name.

This CL replaces some internal use of std::vector with std::unordered_set to
discard duplicities.

The CL also improves the unit test to be less dependent on the resulting order
of the serialized descriptions.

The CL also fixes the test name to match its filename and the name of the tested
function.

BUG=714368
Review-Url: https://codereview.chromium.org/2835823002
Cr-Commit-Position: refs/heads/master@{#466913}
Committed: https://chromium.googlesource.com/chromium/src/+/b95f5bc3e4433d6e28d44850506f47efcf8e4fff

[vabr (Chromium), 2017-04-22 20:21:40]

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

[commit-bot: I haz the power, 2017-04-22 20:21:50]

Dry run: CQ is trying da patch.

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

[vabr (Chromium), 2017-04-22 20:26:28]

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

[commit-bot: I haz the power, 2017-04-22 20:26:30]

Dry run: CQ is trying da patch.

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

[vabr (Chromium), 2017-04-22 20:27:51]

Description was changed from

==========
SerializeHostDescriptions ignores hosts with duplicated names

|HostDescriptionNode.name| identifies the node and should not be shared by two
different nodes. If multiple nodes are passed to SerializeHostDescriptions with
the same name, currently SerializeHostDescriptions ends up using
DictionaryValue after performing std::move on it, because of mixing nodes with
the same name.

This CL replaces some internal use of std::vector with std::unordered_set to
discard duplicities.

The CL also improves the unit test to be less dependent on the resulting order
of the serialized descriptions.

The CL does not fix the naming of the unittest, which will be done in a
follow-up CL. The reason is to keep this one small, because it will need to be
merged.

BUG=714368
==========

to

==========
SerializeHostDescriptions ignores hosts with duplicated names

|HostDescriptionNode.name| identifies the node and should not be shared by two
different nodes. If multiple nodes are passed to SerializeHostDescriptions with
the same name, currently SerializeHostDescriptions ends up using
DictionaryValue after performing std::move on it, because of mixing nodes with
the same name.

This CL replaces some internal use of std::vector with std::unordered_set to
discard duplicities.

The CL also improves the unit test to be less dependent on the resulting order
of the serialized descriptions.

The CL also fixes the test name to match its filename and the name of the tested
function.

BUG=714368
==========

[commit-bot: I haz the power, 2017-04-22 21:00:28]

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

[commit-bot: I haz the power, 2017-04-22 21:00:28]

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

[vabr (Chromium), 2017-04-23 10:24:00]

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

[commit-bot: I haz the power, 2017-04-23 10:24:06]

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-23 11:09:34]

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

[commit-bot: I haz the power, 2017-04-23 11:09:34]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2017-04-23 11:47:31]

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

[vabr (Chromium), 2017-04-23 11:47:32]

Hi dgozman@,

Could you please review this?

Thanks!
Vaclav

https://codereview.chromium.org/2835823002/diff/60001/chrome/browser/devtools...
File chrome/browser/devtools/serialize_host_descriptions.cc (right):

https://codereview.chromium.org/2835823002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/serialize_host_descriptions.cc:51:
std::unordered_set<base::DictionaryValue*>* roots,
I chose unordered_set, because I don't care about the order, just uniqueness and
efficient inserting. I did not chose base::flat_set, because IIUC, that might
harm efficient inserting (O(N^2) uniqueness check on insert).

https://codereview.chromium.org/2835823002/diff/60001/chrome/browser/devtools...
File chrome/browser/devtools/serialize_host_descriptions_unittest.cc (right):

https://codereview.chromium.org/2835823002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/serialize_host_descriptions_unittest.cc:29: const
base::Value* children = nullptr;
I keep children a Value as opposed to ListValue, because it is sufficient and
will spare code changes once ListValue is folded into Value. It also allows me
to drop base::Value:: prefix for GetList() on some places.

https://codereview.chromium.org/2835823002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/serialize_host_descriptions_unittest.cc:46: // Matches
every |arg| with label |label| and checks that it has no children.
One can argue that the GMock matchers are a bit heavy for this task, but I used
them to be able to use UnorderedElementsAre. The latter is crucial to make the
tests both succinct and robust against changes in order in serialization,
because the order is not guaranteed by the tested code.

https://codereview.chromium.org/2835823002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/serialize_host_descriptions_unittest.cc:59:
EXPECT_THAT(result.base::Value::GetList(), ::testing::IsEmpty());
Here I use EXPECT_THAT and IsEmpty instead of EXPECT_TRUE+empty() because of the
better error message (the former tells you more about the actual content of the
tested container).

https://codereview.chromium.org/2835823002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/serialize_host_descriptions_unittest.cc:59:
EXPECT_THAT(result.base::Value::GetList(), ::testing::IsEmpty());
Also, I am using GetList() to get a vector instead of ListValue, because the
latter is not recognised as container by GMock (strange, because it exposes
begin() and end(), but I did not investigate that).

[jdoerrie, 2017-04-23 18:16:10]

jdoerrie@chromium.org changed reviewers:
+ jdoerrie@chromium.org

[jdoerrie, 2017-04-23 18:16:11]

Thanks for addressing this so promptly, vabr@!

https://codereview.chromium.org/2835823002/diff/60001/chrome/browser/devtools...
File chrome/browser/devtools/serialize_host_descriptions.cc (right):

https://codereview.chromium.org/2835823002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/serialize_host_descriptions.cc:73:
std::make_pair(base::StringPiece(node.name), &representations->back()));
Recently the Linux sysroot got upgraded (crbug.com/701894), which updated GCC
4.8. This results in more support of C++11 features, in particular
std::map::emplace. This means you should be able to replace this by
|name_to_representation.emplace(node.name, &representations-back)|, since the
arguments will be perfectly forwarded (ie no temporaries) and the StringPiece in
the map will point to the correct string.

[vabr (Chromium), 2017-04-23 18:52:14]

https://codereview.chromium.org/2835823002/diff/60001/chrome/browser/devtools...
File chrome/browser/devtools/serialize_host_descriptions.cc (right):

https://codereview.chromium.org/2835823002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/serialize_host_descriptions.cc:73:
std::make_pair(base::StringPiece(node.name), &representations->back()));
On 2017/04/23 18:16:11, jdoerrie wrote:
> Recently the Linux sysroot got upgraded (crbug.com/701894), which updated GCC
> 4.8. This results in more support of C++11 features, in particular
> std::map::emplace. This means you should be able to replace this by
> |name_to_representation.emplace(node.name, &representations-back)|, since the
> arguments will be perfectly forwarded (ie no temporaries) and the StringPiece
in
> the map will point to the correct string.

Thanks for the information!

However, looking at https://chromium-cpp.appspot.com/, it still warns that map's
emplace might not be supported. It also points out that emplace might not always
be a readability win. I'm not sure in this case, so my feeling would be to keep
insert().

[jdoerrie, 2017-04-24 09:09:54]

https://codereview.chromium.org/2835823002/diff/60001/chrome/browser/devtools...
File chrome/browser/devtools/serialize_host_descriptions.cc (right):

https://codereview.chromium.org/2835823002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/serialize_host_descriptions.cc:73:
std::make_pair(base::StringPiece(node.name), &representations->back()));
On 2017/04/23 18:52:13, vabr (Chromium) wrote:
> On 2017/04/23 18:16:11, jdoerrie wrote:
> > Recently the Linux sysroot got upgraded (crbug.com/701894), which updated
GCC
> > 4.8. This results in more support of C++11 features, in particular
> > std::map::emplace. This means you should be able to replace this by
> > |name_to_representation.emplace(node.name, &representations-back)|, since
the
> > arguments will be perfectly forwarded (ie no temporaries) and the
StringPiece
> in
> > the map will point to the correct string.
> 
> Thanks for the information!
> 
> However, looking at https://chromium-cpp.appspot.com/, it still warns that
map's
> emplace might not be supported. It also points out that emplace might not
always
> be a readability win. I'm not sure in this case, so my feeling would be to
keep
> insert().

Fair point, that sentence probably should be updated. I'll make a new comment in
the corresponding chromium-dev thread.

Regarding readability: Given that |std::map::emplace| was not supported until
recently, I suspect the concern was mostly regarding choosing
|std::vector::emplace_back| over |std::vector::push_back|. There it is certainly
a valid point. Further background can be found here: http://go/totw/112.

For the std::map case here I think it's both a clarity and performance win, but
that is only my personal opinion. While http://go/totw/67 tends to agree with
me, this is only an internal guideline and I'll understand if you want to stay
with |insert| here.

[vabr (Chromium), 2017-04-24 10:17:51]

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

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

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 12:39:42]

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

[commit-bot: I haz the power, 2017-04-24 12:39:43]

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

[vabr (Chromium), 2017-04-24 14:51:54]

jdoerrie@: Thanks -- addressed in patch set 5.

dgozman@: When reviewing, you might want to check out my comments in patch set
4, which try to explain some things you might ask about. The patch set 5 only
differs from 4 in a single place, and there is a discussion about that in
comments as well.

Cheers,
Vaclav

https://codereview.chromium.org/2835823002/diff/60001/chrome/browser/devtools...
File chrome/browser/devtools/serialize_host_descriptions.cc (right):

https://codereview.chromium.org/2835823002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/serialize_host_descriptions.cc:73:
std::make_pair(base::StringPiece(node.name), &representations->back()));
On 2017/04/24 09:09:54, jdoerrie wrote:
> On 2017/04/23 18:52:13, vabr (Chromium) wrote:
> > On 2017/04/23 18:16:11, jdoerrie wrote:
> > > Recently the Linux sysroot got upgraded (crbug.com/701894), which updated
> GCC
> > > 4.8. This results in more support of C++11 features, in particular
> > > std::map::emplace. This means you should be able to replace this by
> > > |name_to_representation.emplace(node.name, &representations-back)|, since
> the
> > > arguments will be perfectly forwarded (ie no temporaries) and the
> StringPiece
> > in
> > > the map will point to the correct string.
> > 
> > Thanks for the information!
> > 
> > However, looking at https://chromium-cpp.appspot.com/, it still warns that
> map's
> > emplace might not be supported. It also points out that emplace might not
> always
> > be a readability win. I'm not sure in this case, so my feeling would be to
> keep
> > insert().
> 
> Fair point, that sentence probably should be updated. I'll make a new comment
in
> the corresponding chromium-dev thread.
> 
> Regarding readability: Given that |std::map::emplace| was not supported until
> recently, I suspect the concern was mostly regarding choosing
> |std::vector::emplace_back| over |std::vector::push_back|. There it is
certainly
> a valid point. Further background can be found here: http://go/totw/112.
> 
> For the std::map case here I think it's both a clarity and performance win,
but
> that is only my personal opinion. While http://go/totw/67 tends to agree with
> me, this is only an internal guideline and I'll understand if you want to stay
> with |insert| here.

Thanks! I actually like how using emplace simplified the code and removed the
comment.

[dgozman, 2017-04-24 23:39:34]

Thanks for the fix! Note this does not happen in practice since host id is
always unique, but defending against that sounds reasonable.

lgtm

[vabr (Chromium), 2017-04-25 06:51:41]

On 2017/04/24 23:39:34, dgozman wrote:
> Thanks for the fix! Note this does not happen in practice since host id is
> always unique, but defending against that sounds reasonable.
> 
> lgtm

It's good to hear that this does not happen in practice. Thanks for review, I'm
landing now.

Cheers,
Vaclav

[vabr (Chromium), 2017-04-25 06:51:50]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2017-04-25 06:52:21]

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-25 07:17:06]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493103110202700,
"parent_rev": "72254d5183b29735f1cfe999cc7926f55c4dedb0", "commit_rev":
"b95f5bc3e4433d6e28d44850506f47efcf8e4fff"}

[commit-bot: I haz the power, 2017-04-25 07:17:18]

Description was changed from

==========
SerializeHostDescriptions ignores hosts with duplicated names

|HostDescriptionNode.name| identifies the node and should not be shared by two
different nodes. If multiple nodes are passed to SerializeHostDescriptions with
the same name, currently SerializeHostDescriptions ends up using
DictionaryValue after performing std::move on it, because of mixing nodes with
the same name.

This CL replaces some internal use of std::vector with std::unordered_set to
discard duplicities.

The CL also improves the unit test to be less dependent on the resulting order
of the serialized descriptions.

The CL also fixes the test name to match its filename and the name of the tested
function.

BUG=714368
==========

to

==========
SerializeHostDescriptions ignores hosts with duplicated names

|HostDescriptionNode.name| identifies the node and should not be shared by two
different nodes. If multiple nodes are passed to SerializeHostDescriptions with
the same name, currently SerializeHostDescriptions ends up using
DictionaryValue after performing std::move on it, because of mixing nodes with
the same name.

This CL replaces some internal use of std::vector with std::unordered_set to
discard duplicities.

The CL also improves the unit test to be less dependent on the resulting order
of the serialized descriptions.

The CL also fixes the test name to match its filename and the name of the tested
function.

BUG=714368

Review-Url: https://codereview.chromium.org/2835823002
Cr-Commit-Position: refs/heads/master@{#466913}
Committed:
https://chromium.googlesource.com/chromium/src/+/b95f5bc3e4433d6e28d44850506f...
==========

[commit-bot: I haz the power, 2017-04-25 07:17:19]

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