Inline StringValue into base::Value

Inline StringValue into base::Value

Similarly as done with FundamentalValue we inline StringValue into base::Value.
There will be a follow up CL updating all call sites to use Value instead of
StringValue, again similarly to how the removal of FundamentalValue was done.

BUG=646113
R=brettw@chromium.org
CC=vabr@chromium.org

Review-Url: https://codereview.chromium.org/2516363005
Cr-Commit-Position: refs/heads/master@{#447508}
Committed: https://chromium.googlesource.com/chromium/src/+/f38f37b34fda511b74b32569f9f2fb23f69c8cea

[jdoerrie, 2016-11-22 01:54:57]

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

[commit-bot: I haz the power, 2016-11-22 01:55:38]

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-11-22 01:59:02]

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

[commit-bot: I haz the power, 2016-11-22 01:59:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jdoerrie, 2016-11-23 00:38:12]

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

[commit-bot: I haz the power, 2016-11-23 00:38:52]

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-11-23 01:06:30]

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

[commit-bot: I haz the power, 2016-11-23 01:06:31]

Dry run: 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...)

[jdoerrie, 2016-11-23 01:34:30]

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

[commit-bot: I haz the power, 2016-11-23 01:34:52]

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-11-23 01:44:03]

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

[commit-bot: I haz the power, 2016-11-23 01:44:04]

Dry run: 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...)

[jdoerrie, 2016-11-23 04:25:27]

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

[commit-bot: I haz the power, 2016-11-23 04:25:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdoerrie, 2016-11-23 04:30:37]

The CQ bit was unchecked by jdoerrie@chromium.org

[jdoerrie, 2016-11-28 18:32:53]

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

[commit-bot: I haz the power, 2016-11-28 18:33:57]

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-11-28 20:46:39]

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

[commit-bot: I haz the power, 2016-11-28 20:46:40]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[jdoerrie, 2016-12-06 14:19:26]

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

[commit-bot: I haz the power, 2016-12-06 14:19:35]

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-12-06 14:26:06]

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

[commit-bot: I haz the power, 2016-12-06 14:26:07]

Dry run: 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...)

[vabr (Chromium), 2016-12-07 10:17:26]

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

[vabr (Chromium), 2016-12-07 10:17:27]

Hi Jan,

One question and one nit so far.

Thanks!
Vaclav

https://codereview.chromium.org/2516363005/diff/100001/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2516363005/diff/100001/base/json/json_parser....
base/json/json_parser.cc:156: // TODO(jdoerrie): Clean this up when StringValue
will be removed.
nit: It is a good practice to also mention the bug number, e.g., as
TODO(crbug.com/646113), because it provides more context if needed. One can
still derive it from git blame, but having the number in the comment itself is
much easier to follow.

(Also elsewhere in the CL.)

https://codereview.chromium.org/2516363005/diff/100001/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/2516363005/diff/100001/base/values.cc#newcode317
base/values.cc:317: }
Should this (or ~Value) run string_value_.Destroy()?

[jdoerrie, 2016-12-08 09:59:34]

https://codereview.chromium.org/2516363005/diff/100001/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2516363005/diff/100001/base/json/json_parser....
base/json/json_parser.cc:156: // TODO(jdoerrie): Clean this up when StringValue
will be removed.
On 2016/12/07 10:17:27, vabr (Chromium) wrote:
> nit: It is a good practice to also mention the bug number, e.g., as
> TODO(crbug.com/646113), because it provides more context if needed. One can
> still derive it from git blame, but having the number in the comment itself is
> much easier to follow.
> 
> (Also elsewhere in the CL.)

Done.

https://codereview.chromium.org/2516363005/diff/100001/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/2516363005/diff/100001/base/values.cc#newcode317
base/values.cc:317: }
On 2016/12/07 10:17:27, vabr (Chromium) wrote:
> Should this (or ~Value) run string_value_.Destroy()?

Done. Implemented it as part of InternalCleanup. Currently we have to hack
around JSONStringValue unfortunately.

[jdoerrie, 2016-12-08 13:43:23]

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

[commit-bot: I haz the power, 2016-12-08 13:43:37]

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-12-08 14:01:13]

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

[commit-bot: I haz the power, 2016-12-08 14:01:14]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[jdoerrie, 2016-12-08 14:17:41]

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

[commit-bot: I haz the power, 2016-12-08 14:18: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-12-08 16:19:46]

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

[commit-bot: I haz the power, 2016-12-08 16:19:47]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jdoerrie, 2016-12-16 09:41:59]

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

[commit-bot: I haz the power, 2016-12-16 09:42:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdoerrie, 2016-12-16 09:54:37]

Dear all, PTAL.

https://codereview.chromium.org/2516363005/diff/180001/base/values.h
File base/values.h (right):

https://codereview.chromium.org/2516363005/diff/180001/base/values.h#newcode97
base/values.h:97: std::string& GetString();
For now I added the non-const ref version for simplicity. vabr@ and I discussed
offline how SetString should work, and we decided that we probably need 4
versions:

- SetString(const char*)
- SetString(const std::string&)
- SetString(std::string&&)
- SetString(const base::string16&)

Do you agree with this, brettw@? A couple of questions remain, however:
- Do we plan on supporting SetString for Values of type() != Type::STRING?
- If we add SetString, should we also add SetInt, SetDouble, etc?

https://codereview.chromium.org/2516363005/diff/180001/base/values.h#newcode149
base/values.h:149: explicit Value(StringPiece in_string);
Given that we need an explicit Value::Value(const char*), this should only be
called for std::string arguments. Should we replace this in favor of
Value::Value(const std::string&) and Value::Value(std::string&&)?

https://codereview.chromium.org/2516363005/diff/180001/chromeos/network/netwo...
File chromeos/network/network_state_unittest.cc (right):

https://codereview.chromium.org/2516363005/diff/180001/chromeos/network/netwo...
chromeos/network/network_state_unittest.cc:25: class TestStringValue : public
base::Value {
This class and these tests will break once we remove the virtual keyword from
base::Value. Should we consider removing the DCHECK in Value(StringPiece) to
make this derived class unnecessary?

[commit-bot: I haz the power, 2016-12-16 12:26:31]

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

[commit-bot: I haz the power, 2016-12-16 12:26:31]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[vabr (Chromium), 2016-12-16 17:00:56]

LGTM with some suggestions.

Thanks!
Vaclav

https://codereview.chromium.org/2516363005/diff/180001/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/2516363005/diff/180001/base/values.cc#newcode379
base/values.cc:379: InternalCleanup();
Having the InternalCleanup here, and using Init inside InternalCopyFrom would
enforce heap allocation of strings even if not needed. Consider:

(1) |a| holds the string "12345"
(2) |b| holds the string "123"
(3) a = b

If you do this with raw std::strings instead of StringValues, the operator= will
make a copy of "123", but it will not need to allocate heap space for that;
instead, it will overwrite the buffer for "12345", which is long enough.

But with the current StringValue:
1. InternalCleanup destroys the existing string and its heap space.
2. InternalCopyFrom initialises the (now empty) a.string_value_ with
b.string_value_, resulting in a heap allocation.

Dropping just InternalCleanup does not work, because then you are calling Init
(=constructor) without Destroy(=destructor) on the original string in |a|. To me
this seems like we should not delegate to a common method and just do the
hand-optimised thing here. That means, switch on the type, and for STRING do:

this.string_value_ = that.string_value_;

https://codereview.chromium.org/2516363005/diff/180001/base/values.cc#newcode389
base/values.cc:389: InternalMoveFrom(std::move(that));
My comment from the const-ref operator= applies here as well.

https://codereview.chromium.org/2516363005/diff/180001/base/values.h
File base/values.h (right):

https://codereview.chromium.org/2516363005/diff/180001/base/values.h#newcode97
base/values.h:97: std::string& GetString();
On 2016/12/16 09:54:37, jdoerrie wrote:
> For now I added the non-const ref version for simplicity. vabr@ and I
discussed
> offline how SetString should work, and we decided that we probably need 4
> versions:
> 
> - SetString(const char*)
> - SetString(const std::string&)
> - SetString(std::string&&)
> - SetString(const base::string16&)

Just for context: part of the discussion was also about replacing the middle two
with just SetString(std::string) with std::move inside. For cases when a copy is
being made, the latter always does heap allocation, whereas SetString(const
base::string&) skips the allocation if the currently held string is long enough.
To choose the right approach, we would need to know what is the typical use and
perhaps measure that.

> 
> Do you agree with this, brettw@? A couple of questions remain, however:
> - Do we plan on supporting SetString for Values of type() != Type::STRING?
> - If we add SetString, should we also add SetInt, SetDouble, etc?

I left a comment in the design doc about this [1]. In short, to keep the API
simple, we should not offer multiple ways to do things. And resetting the
StringValue is already possible by creating a new one and assigning it to the
current one. We should just ensure that that operation is efficient.

[1]
https://docs.google.com/document/d/1uDLu5uTRlCWePxQUEHc8yNQdEoE1BDISYdpggWEAB...

https://codereview.chromium.org/2516363005/diff/180001/base/values.h#newcode149
base/values.h:149: explicit Value(StringPiece in_string);
On 2016/12/16 09:54:37, jdoerrie wrote:
> Given that we need an explicit Value::Value(const char*), this should only be
> called for std::string arguments. Should we replace this in favor of
> Value::Value(const std::string&) and Value::Value(std::string&&)?

Definitely in favour of replacing by string-based constructors (see my comment
above). In addition, not relying on implicit conversions will make the behaviour
of the API (esp. wrt. copies) clearer.

https://codereview.chromium.org/2516363005/diff/180001/base/values.h#newcode154
base/values.h:154: explicit Value(const char* in_string);
As a TODO to be done in a separate CL, could we transform the callsites to pass
StringPieces and mark this constructor as =delete (or private) to avoid the
fall-back to Value(bool)? The goal being that the API would stay smaller and
there would only be one way to pass a pointer to a C-string -- by upgrading it
to a StringPiece.

https://codereview.chromium.org/2516363005/diff/180001/base/values.h#newcode183
base/values.h:183: class BASE_EXPORT StringValue : public Value {
Could we actually make this a typedef instead? (For that we would need to make
the string-related Value() constructors public:

using StringValue = Value;

Doing so would make it clearer that StringValue is no more different, and also
would avoid explicit down-casts via static_cast.

https://codereview.chromium.org/2516363005/diff/180001/base/values_unittest.cc
File base/values_unittest.cc (right):

https://codereview.chromium.org/2516363005/diff/180001/base/values_unittest.c...
base/values_unittest.cc:47: EXPECT_EQ(str.as_string(), value.GetString());
To avoid testing as_string, please use
EXPECT_EQ("foobar", value.GetString());

What looks like an error-prone constant-duplication is actually a good tactic
for tests: if somebody breaks both as_string and GetString in compatible ways,
the current test is fooled. Also, if somebody just breaks as_string(), the
resulting failure here will be confusing. With a hard-coded expectation, the
test is better scoped and clearer. See also go/tott-338.

Also below for the Construct* cases. Note that Copy* cases are good as they are,
there the goal is to compare the result of both constructions.

[jdoerrie, 2016-12-19 11:54:09]

Thank you Vaclav!

https://codereview.chromium.org/2516363005/diff/180001/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/2516363005/diff/180001/base/values.cc#newcode156
base/values.cc:156: that.type_ = Type::NONE;
I decided against setting the type to NONE here, since this will prevent calling
that.string_value_.Destoy() once it gets out of scope. |that.string_value_| will
be in an unspecified state, but this is in accordance with the standard:

17.6.5.15 Moved-from state of library types                                     
   [lib.types.movedfrom]

Objects of types defined in the C++ standard library may be moved from (12.8).
Move operations may be explicitly specified or implicitly generated. Unless
otherwise specified, such moved-from objects shall be placed in a valid but
unspecified state.

https://codereview.chromium.org/2516363005/diff/180001/base/values.cc#newcode379
base/values.cc:379: InternalCleanup();
On 2016/12/16 17:00:55, vabr (Chromium) wrote:
> Having the InternalCleanup here, and using Init inside InternalCopyFrom would
> enforce heap allocation of strings even if not needed. Consider:
> 
> (1) |a| holds the string "12345"
> (2) |b| holds the string "123"
> (3) a = b
> 
> If you do this with raw std::strings instead of StringValues, the operator=
will
> make a copy of "123", but it will not need to allocate heap space for that;
> instead, it will overwrite the buffer for "12345", which is long enough.
> 
> But with the current StringValue:
> 1. InternalCleanup destroys the existing string and its heap space.
> 2. InternalCopyFrom initialises the (now empty) a.string_value_ with
> b.string_value_, resulting in a heap allocation.
> 
> Dropping just InternalCleanup does not work, because then you are calling Init
> (=constructor) without Destroy(=destructor) on the original string in |a|. To
me
> this seems like we should not delegate to a common method and just do the
> hand-optimised thing here. That means, switch on the type, and for STRING do:
> 
> this.string_value_ = that.string_value_;

I agree. Most likely we should act similarly with lists and dicts when they get
implemented.

https://codereview.chromium.org/2516363005/diff/180001/base/values.cc#newcode389
base/values.cc:389: InternalMoveFrom(std::move(that));
On 2016/12/16 17:00:55, vabr (Chromium) wrote:
> My comment from the const-ref operator= applies here as well.

Acknowledged.

https://codereview.chromium.org/2516363005/diff/180001/base/values.h
File base/values.h (right):

https://codereview.chromium.org/2516363005/diff/180001/base/values.h#newcode154
base/values.h:154: explicit Value(const char* in_string);
On 2016/12/16 17:00:55, vabr (Chromium) wrote:
> As a TODO to be done in a separate CL, could we transform the callsites to
pass
> StringPieces and mark this constructor as =delete (or private) to avoid the
> fall-back to Value(bool)? The goal being that the API would stay smaller and
> there would only be one way to pass a pointer to a C-string -- by upgrading it
> to a StringPiece.

If it is your goal to stop being able to do |Value val("foobar")|, we actually
don't need to declare a const char* constructor at all, the implicit conversion
to bool already raises compilation errors:

error: implicit conversion turns string literal into bool: 'const char [7]' to
'bool' [-Werror,-Wstring-conversion]

However, making it private or marking it deleted definitely makes our intent
clearer.

Just out of curiosity, what is the value of only allowing StringPieces? Both
StringPieces and const char*s would need to be copied to the internal
std::string, correct?

https://codereview.chromium.org/2516363005/diff/180001/base/values.h#newcode183
base/values.h:183: class BASE_EXPORT StringValue : public Value {
On 2016/12/16 17:00:56, vabr (Chromium) wrote:
> Could we actually make this a typedef instead? (For that we would need to make
> the string-related Value() constructors public:
> 
> using StringValue = Value;
> 
> Doing so would make it clearer that StringValue is no more different, and also
> would avoid explicit down-casts via static_cast.

This is true, but it probably also would require making the copy and move
constructors and assignment operators public. Currently there's a comment that
making the constructors public would not be safe, so I am not quite sure what to
do here.

https://codereview.chromium.org/2516363005/diff/180001/base/values_unittest.cc
File base/values_unittest.cc (right):

https://codereview.chromium.org/2516363005/diff/180001/base/values_unittest.c...
base/values_unittest.cc:47: EXPECT_EQ(str.as_string(), value.GetString());
On 2016/12/16 17:00:56, vabr (Chromium) wrote:
> To avoid testing as_string, please use
> EXPECT_EQ("foobar", value.GetString());
> 
> What looks like an error-prone constant-duplication is actually a good tactic
> for tests: if somebody breaks both as_string and GetString in compatible ways,
> the current test is fooled. Also, if somebody just breaks as_string(), the
> resulting failure here will be confusing. With a hard-coded expectation, the
> test is better scoped and clearer. See also go/tott-338.
> 
> Also below for the Construct* cases. Note that Copy* cases are good as they
are,
> there the goal is to compare the result of both constructions.

Done. For some reason I thought we needed the exact same types on both sides (ie
two std::strings), but it's enough to have a defined operator== between the
types.

[vabr (Chromium), 2016-12-19 13:00:56]

https://codereview.chromium.org/2516363005/diff/180001/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/2516363005/diff/180001/base/values.cc#newcode156
base/values.cc:156: that.type_ = Type::NONE;
On 2016/12/19 11:54:09, jdoerrie wrote:
> I decided against setting the type to NONE here, since this will prevent
calling
> that.string_value_.Destoy() once it gets out of scope. |that.string_value_|
will
> be in an unspecified state, but this is in accordance with the standard:
> 
> 17.6.5.15 Moved-from state of library types                                   
 
>    [lib.types.movedfrom]
> 
> Objects of types defined in the C++ standard library may be moved from (12.8).
> Move operations may be explicitly specified or implicitly generated. Unless
> otherwise specified, such moved-from objects shall be placed in a valid but
> unspecified state.

Acknowledged.

https://codereview.chromium.org/2516363005/diff/180001/base/values.h
File base/values.h (right):

https://codereview.chromium.org/2516363005/diff/180001/base/values.h#newcode154
base/values.h:154: explicit Value(const char* in_string);
On 2016/12/19 11:54:09, jdoerrie wrote:
> On 2016/12/16 17:00:55, vabr (Chromium) wrote:
> > As a TODO to be done in a separate CL, could we transform the callsites to
> pass
> > StringPieces and mark this constructor as =delete (or private) to avoid the
> > fall-back to Value(bool)? The goal being that the API would stay smaller and
> > there would only be one way to pass a pointer to a C-string -- by upgrading
it
> > to a StringPiece.
> 
> If it is your goal to stop being able to do |Value val("foobar")|, we actually
> don't need to declare a const char* constructor at all, the implicit
conversion
> to bool already raises compilation errors:
> 
> error: implicit conversion turns string literal into bool: 'const char [7]' to
> 'bool' [-Werror,-Wstring-conversion]
> 
> However, making it private or marking it deleted definitely makes our intent
> clearer.

Good to know about the error. However, because it is not self-evident what is
the benefit of forbidding char*-based constructor, we might need to document it
well to keep it that way. The explicit =delete with some explanation in a
comment would be good for that.

> 
> Just out of curiosity, what is the value of only allowing StringPieces? Both
> StringPieces and const char*s would need to be copied to the internal
> std::string, correct?

That's correct. My slight concern with char* is that it is too easy to be
surprised with tricks like converting it to a bool. char* is a C-language way to
work with strings, whereas StringPiece is a C++ way of being explicit of what
that data is and what actions on it are expected/allowed.

https://codereview.chromium.org/2516363005/diff/180001/base/values.h#newcode183
base/values.h:183: class BASE_EXPORT StringValue : public Value {
On 2016/12/19 11:54:09, jdoerrie wrote:
> On 2016/12/16 17:00:56, vabr (Chromium) wrote:
> > Could we actually make this a typedef instead? (For that we would need to
make
> > the string-related Value() constructors public:
> > 
> > using StringValue = Value;
> > 
> > Doing so would make it clearer that StringValue is no more different, and
also
> > would avoid explicit down-casts via static_cast.
> 
> This is true, but it probably also would require making the copy and move
> constructors and assignment operators public. Currently there's a comment that
> making the constructors public would not be safe, so I am not quite sure what
to
> do here.

I'm not sure the comment about safety applies to all constructors. IMO that's
just about the ones allowing to set the type and nothing else. Perhaps Brett can
confirm or refute this?

https://codereview.chromium.org/2516363005/diff/180001/base/values_unittest.cc
File base/values_unittest.cc (right):

https://codereview.chromium.org/2516363005/diff/180001/base/values_unittest.c...
base/values_unittest.cc:47: EXPECT_EQ(str.as_string(), value.GetString());
On 2016/12/19 11:54:09, jdoerrie wrote:
> On 2016/12/16 17:00:56, vabr (Chromium) wrote:
> > To avoid testing as_string, please use
> > EXPECT_EQ("foobar", value.GetString());
> > 
> > What looks like an error-prone constant-duplication is actually a good
tactic
> > for tests: if somebody breaks both as_string and GetString in compatible
ways,
> > the current test is fooled. Also, if somebody just breaks as_string(), the
> > resulting failure here will be confusing. With a hard-coded expectation, the
> > test is better scoped and clearer. See also go/tott-338.
> > 
> > Also below for the Construct* cases. Note that Copy* cases are good as they
> are,
> > there the goal is to compare the result of both constructions.
> 
> Done. For some reason I thought we needed the exact same types on both sides
(ie
> two std::strings), but it's enough to have a defined operator== between the
> types.

Ah, actually, I missed that it was taking the expected value from |str|, and not
from the value somehow. So even your current code looks good to me (although
putting in the literal directly looks a little bit more readable to me).
(Not sure how to explain my previous confusion, it was probably too late or
something. :))

https://codereview.chromium.org/2516363005/diff/180001/chromeos/network/netwo...
File chromeos/network/network_state_unittest.cc (right):

https://codereview.chromium.org/2516363005/diff/180001/chromeos/network/netwo...
chromeos/network/network_state_unittest.cc:25: class TestStringValue : public
base::Value {
On 2016/12/16 09:54:37, jdoerrie wrote:
> This class and these tests will break once we remove the virtual keyword from
> base::Value. Should we consider removing the DCHECK in Value(StringPiece) to
> make this derived class unnecessary?

Not sure yet. The written contract on the current StringValue is that the input
should be UTF8. The networking code is breaking it if it makes it possible to be
otherwise. I'm not sure if it is possible, but I guess when they put effort into
it to validate the string, then it is.
Ideally, they would validate the string before putting it into the Value. May be
worth filing a bug for them.

Or perhaps Brett knows that Value getting unsanitised input from the
environment/users is a wide-spread practice in Chromium, and hence the contract
about UTF8 should be dropped?

[jdoerrie, 2016-12-19 13:56:30]

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

[commit-bot: I haz the power, 2016-12-19 13:56: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-12-19 16:15:28]

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

[commit-bot: I haz the power, 2016-12-19 16:15:29]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[jdoerrie, 2016-12-22 12:52:40]

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

[commit-bot: I haz the power, 2016-12-22 12:52: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, 2016-12-22 16:44:53]

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

[commit-bot: I haz the power, 2016-12-22 16:44:55]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-01-09 09:59:31]

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

[commit-bot: I haz the power, 2017-01-09 09:59:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdoerrie, 2017-01-09 11:05:49]

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

[jdoerrie, 2017-01-09 11:05:49]

stevenjb@chromium.org: Please review changes in
chromeos/network/network_state_unittest.cc

That code will eventually break when type_ is turned into a private member of
Value. Could you comment on how common non-UTF8 strings are for you? Would it be
possible to validate the string before putting it into the Value?

[commit-bot: I haz the power, 2017-01-09 11:40:38]

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

[commit-bot: I haz the power, 2017-01-09 11:40:39]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-01-09 16:37:18]

lgtm

[jdoerrie, 2017-01-20 10:13:39]

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

[commit-bot: I haz the power, 2017-01-20 10:13: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-01-20 10:48:53]

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

[commit-bot: I haz the power, 2017-01-20 10:48:54]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[jdoerrie, 2017-01-21 14:10:11]

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

[commit-bot: I haz the power, 2017-01-21 14:10:17]

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-01-21 16:10:22]

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

[commit-bot: I haz the power, 2017-01-21 16:10:23]

Dry run: This issue passed the CQ dry run.

[brettw, 2017-01-31 22:22:24]

lgtm

https://codereview.chromium.org/2516363005/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/api/streams_private/streams_private_api.cc
(right):

https://codereview.chromium.org/2516363005/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/streams_private/streams_private_api.cc:43:
existing_string_value->GetString() + ", " + std::move(header_value));
This use of std::move seems not to be helpful to me. I think you should just be
able to do "+ header_value" since it will be passed by const ref into operator+
either way.

[stevenjb, 2017-01-31 22:25:05]

network_state_unittest.cc lgtm

[jdoerrie, 2017-02-01 12:36:26]

https://codereview.chromium.org/2516363005/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/api/streams_private/streams_private_api.cc
(right):

https://codereview.chromium.org/2516363005/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/streams_private/streams_private_api.cc:43:
existing_string_value->GetString() + ", " + std::move(header_value));
On 2017/01/31 22:22:24, brettw (ping after 24h) wrote:
> This use of std::move seems not to be helpful to me. I think you should just
be
> able to do "+ header_value" since it will be passed by const ref into
operator+
> either way.

C++11 introduced overloads for operator+ that take rvalues:
http://en.cppreference.com/w/cpp/string/basic_string/operator%2B

There might be some use for std::move here, but it does feel like a
micro-optimization, so I removed it.

I inlined |existing_string_value|, since the cast to |StringValue| should be a
no-op now and is not needed anymore.

[jdoerrie, 2017-02-01 12:36:38]

The CQ bit was checked by jdoerrie@chromium.org

[jdoerrie, 2017-02-01 12:36:39]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
stevenjb@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2516363005/#ps300001
(title: "Rebase and Nits.")

[commit-bot: I haz the power, 2017-02-01 12:36:52]

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-02-01 14:38:38]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1485952598856450,
"parent_rev": "bd716dcb8575e046af147dd1b69f56090c77b4e4", "commit_rev":
"f38f37b34fda511b74b32569f9f2fb23f69c8cea"}

[commit-bot: I haz the power, 2017-02-01 14:39:12]

Description was changed from

==========
Inline StringValue into base::Value

Similarly as done with FundamentalValue we inline StringValue into base::Value.
There will be a follow up CL updating all call sites to use Value instead of
StringValue, again similarly to how the removal of FundamentalValue was done.

BUG=646113
R=brettw@chromium.org
CC=vabr@chromium.org
==========

to

==========
Inline StringValue into base::Value

Similarly as done with FundamentalValue we inline StringValue into base::Value.
There will be a follow up CL updating all call sites to use Value instead of
StringValue, again similarly to how the removal of FundamentalValue was done.

BUG=646113
R=brettw@chromium.org
CC=vabr@chromium.org

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

[commit-bot: I haz the power, 2017-02-01 14:39:15]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/f38f37b34fda511b74b32569f9f2...