Split ValueSerializer into separate Serializer and Deserializer classes.

Split ValueSerializer into separate Serializer and Deserializer classes.

This CL removes the Deserializer functionality from ValueSerializer,
and puts into separate class ValueDeserializer, so that class responsibility
is maintained.

BUG=455453
TBR=brettw@chromium.org

Committed: https://crrev.com/54a99450c633d64842217b10fa6841b2a3e7eb77
Cr-Commit-Position: refs/heads/master@{#319239}

[prashant.hiremath, 2015-02-13 03:50:39]

prashhir@cisco.com changed reviewers:
+ mgiuca@chromium.org

[prashant.hiremath, 2015-02-13 03:53:08]

This is initial patch, showing how it looks. Based on your review comment, next
patch I'll change all clients using Deserialize() and upload patch. Please
Review this change.

[Matt Giuca, 2015-02-16 00:18:28]

Hi Prashant,

You clearly haven't compiled this code. There is an error in
json_string_value_serializer.cc (definition without declaration), and then after
that is resolved, you will need to update every piece of code in Chromium that
uses JSONStringValueSerializer or JSONFileValueSerializer for deserialization,
and have them use the Deserializer version instead.

You need to compile code before sending it out for review.

You will also need to update json_value_serializer_unittest.cc to use the new
interface, and ensure that the 'base_unittests' target compiles.

You should then make sure that the JSONValueSerializerTest tests pass. You can
do this by running:

out/Debug/base_unittests --gtest_filter='JSONValueSerializerTest.*'

https://codereview.chromium.org/925783002/diff/1/base/json/json_string_value_...
File base/json/json_string_value_serializer.cc (right):

https://codereview.chromium.org/925783002/diff/1/base/json/json_string_value_...
base/json/json_string_value_serializer.cc:18:
JSONStringValueSerializer::JSONStringValueSerializer(
You deleted this constructor in the header file, so you need to delete it here.

https://codereview.chromium.org/925783002/diff/1/base/json/json_string_value_...
File base/json/json_string_value_serializer.h (right):

https://codereview.chromium.org/925783002/diff/1/base/json/json_string_value_...
base/json/json_string_value_serializer.h:40: // String for writing. Owned by the
caller of the constructor. Will be null if
Rewrite this comment. "String for writing" no longer necessary because we only
write. It cannot be null, because there is no const string constructor.

https://codereview.chromium.org/925783002/diff/1/base/json/json_string_value_...
base/json/json_string_value_serializer.h:41: // the serializer was initialized
with a const string.
Delete the last sentence; it is no longer possible to initialize with a const
string.

https://codereview.chromium.org/925783002/diff/1/base/json/json_string_value_...
base/json/json_string_value_serializer.h:53: explicit
JSONStringValueDeserializer(std::string* json_string);
This constructor should be deleted (the whole point of this CL is to fix the
awkwardness of having a read-write and a read-only constructor).

https://codereview.chromium.org/925783002/diff/1/base/json/json_string_value_...
base/json/json_string_value_serializer.h:55: // This version allows
initialization with a StringPiece for deserialization
You will need to update this comment ("This version ..." no longer makes sense
if you delete the other constructor).

https://codereview.chromium.org/925783002/diff/1/base/json/json_string_value_...
base/json/json_string_value_serializer.h:77: // String for reading. Data is
owned by the caller of the constructor. If
As above, rewrite this comment. "String for reading" is not necessary because we
only do reading in this class. There is no |json_string_| so get rid of the last
sentence.

https://codereview.chromium.org/925783002/diff/1/base/json/json_string_value_...
base/json/json_string_value_serializer.h:79: base::StringPiece
json_string_readonly_;
Rename this to |json_string_|. There is no need for the "readonly" any more.

[Matt Giuca, 2015-02-16 00:19:59]

Oh, I see your comment "Based on your review comment, next patch I'll change all
clients using Deserialize() and upload patch." Sorry, I didn't see that before.

That's fair enough (getting a reviewer to look over it before updating all the
clients), but you should at least make sure the code you are updating and its
own test case passes before sending out a review.

[prashant.hiremath, 2015-02-18 08:34:35]

Hi, I have uploaded all the changes, Please review.
Please let me know, if I need to add comments.

[Matt Giuca, 2015-02-19 03:57:15]

Hi Prashant,

Thanks for updating the CL. I am pretty busy today and tomorrow, so I might not
get onto this until Monday. Hope this is OK (there is about to be a Chromium
branch point anyway, and it is probably best to hold off until after branch
point for this internal change).

[prashant.hiremath, 2015-02-19 06:49:42]

On 2015/02/19 03:57:15, Matt Giuca wrote:
> Hi Prashant,
> 
> Thanks for updating the CL. I am pretty busy today and tomorrow, so I might
not
> get onto this until Monday. Hope this is OK (there is about to be a Chromium
> branch point anyway, and it is probably best to hold off until after branch
> point for this internal change).

Hi Matt,
Thanks for the reply, sure no problem, please review when you are free.

[Matt Giuca, 2015-02-23 02:41:25]

Thanks, Prashant. This is looking really good now.

Just have a couple of things for you to fix up.

Also extensions/browser/api/printer_provider/printer_provider_api.cc is failing
the build (just another thing that needs to be converted to use Deserializer).
It was added to the code base about 2 weeks ago, so I'm guessing you're building
against an old version of the code. You should pull to the current tip of tree,
merge, and try compiling again.

https://codereview.chromium.org/925783002/diff/20001/base/json/json_file_valu...
File base/json/json_file_value_serializer.h (right):

https://codereview.chromium.org/925783002/diff/20001/base/json/json_file_valu...
base/json/json_file_value_serializer.h:49: // deserialization. When
deserializing, the file should exist.
Remove "When deserializing" (because this is a Deserializer, we are always
deserializing).

https://codereview.chromium.org/925783002/diff/20001/base/json/json_string_va...
File base/json/json_string_value_serializer.h (right):

https://codereview.chromium.org/925783002/diff/20001/base/json/json_string_va...
base/json/json_string_value_serializer.h:49: // This allows initialization with
a StringPiece for deserialization.
Drop this first sentence altogether (it's implied by it being the constructor of
a class called "Deserializer").

https://codereview.chromium.org/925783002/diff/20001/base/json/json_value_ser...
File base/json/json_value_serializer_unittest.cc (right):

https://codereview.chromium.org/925783002/diff/20001/base/json/json_value_ser...
base/json/json_value_serializer_unittest.cc:106: TEST(JSONValueSerializerTest,
ReadProperJSONFromStringPointer) {
Just delete this test. It exists solely to test the std::string* constructor
which doesn't exist in JSONStringValueDeserializer.

(By deleting the &, you have made this test identical to
ReadProperJSONFromStringPiece.)

[Matt Giuca, 2015-02-23 02:42:53]

Oh another thing: your CL description is worded funny ("ValueSerializer should
be split be into separate Serializer and Deserializer.").

You should describe *what you changed*, not *what the bug was*. So reword this
as "Split ValueSerializer into separate Serializer and Deserializer classes."

[prashant.hiremath, 2015-02-24 12:03:18]

Please have a look.

https://codereview.chromium.org/925783002/diff/20001/base/json/json_value_ser...
File base/json/json_value_serializer_unittest.cc (right):

https://codereview.chromium.org/925783002/diff/20001/base/json/json_value_ser...
base/json/json_value_serializer_unittest.cc:106: TEST(JSONValueSerializerTest,
ReadProperJSONFromStringPointer) {
On 2015/02/23 02:41:24, Matt Giuca wrote:
> Just delete this test. It exists solely to test the std::string* constructor
> which doesn't exist in JSONStringValueDeserializer.
> 
> (By deleting the &, you have made this test identical to
> ReadProperJSONFromStringPiece.)

This was edited blindly, so one good learning for me from now on. Thanks for
pointing this out.

[Matt Giuca, 2015-02-24 23:19:38]

mgiuca@chromium.org changed reviewers:
+ rvargas@chromium.org

[Matt Giuca, 2015-02-24 23:19:39]

+rvargas to review changes in base. Prashant volunteered to do a follow-up to my
JSON StringPiece change from a few weeks ago
(https://codereview.chromium.org/895913004); you suggested splitting up the
class. I have done an initial code review and it meets my standards.

Hi Prashant,

Some minor fixes for the CL description:

- There are two periods at the end of the first sentence.
- "responsibility" is misspelled ("responibilty").

Please fix those.

LGTM from me, but I'm not actually an owner. You'll need Ricardo's approval on
base, and then you'll also need another reviewer to approve all of the
mechanical changes to the rest of the code base.

Finally, some future advice. When you merge or rebase to bring your local branch
up to tip-of-tree, if you then make changes and upload them in the same patch
set, your reviewer has no choice but to read all of your new changes alongside
the rebased changes that came from upstream (for example, try looking at the
diff between Patch Set 2 and 3). In some cases, like this, that means reading a
whole bunch of completely unrelated code changes. In the future, please do the
rebase/merge, upload a patch set for just that (called "Rebase" or similar), and
then do your changes, and upload those as a separate patch set. Then the
reviewer can ignore the "Rebase" patch sets. Thanks!

[rvargas (doing something else), 2015-02-25 03:38:25]

Base looks fine, just some nits.

https://codereview.chromium.org/925783002/diff/40001/base/json/json_file_valu...
File base/json/json_file_value_serializer.cc (right):

https://codereview.chromium.org/925783002/diff/40001/base/json/json_file_valu...
base/json/json_file_value_serializer.cc:104: std::string* error_str) {
nit: indentation

https://codereview.chromium.org/925783002/diff/40001/base/json/json_file_valu...
File base/json/json_file_value_serializer.h (right):

https://codereview.chromium.org/925783002/diff/40001/base/json/json_file_valu...
base/json/json_file_value_serializer.h:49: // deserialization. The file should
exist in specified location.
nit: Remove the last sentence (doesn't add anything)

https://codereview.chromium.org/925783002/diff/40001/base/json/json_string_va...
File base/json/json_string_value_serializer.cc (right):

https://codereview.chromium.org/925783002/diff/40001/base/json/json_string_va...
base/json/json_string_value_serializer.cc:52: std::string* error_str) {
nit: Indentation is off.

https://codereview.chromium.org/925783002/diff/40001/base/json/json_value_ser...
File base/json/json_value_serializer_unittest.cc (left):

https://codereview.chromium.org/925783002/diff/40001/base/json/json_value_ser...
base/json/json_value_serializer_unittest.cc:89: // Test proper JSON
[de]serialization from string is working.
nit: deserialization

https://codereview.chromium.org/925783002/diff/40001/base/json/json_value_ser...
base/json/json_value_serializer_unittest.cc:90: TEST(JSONValueSerializerTest,
ReadProperJSONFromString) {
nit: Deserializer. (& others)

[prashant.hiremath, 2015-02-25 06:01:37]

https://codereview.chromium.org/925783002/diff/40001/base/json/json_value_ser...
File base/json/json_value_serializer_unittest.cc (left):

https://codereview.chromium.org/925783002/diff/40001/base/json/json_value_ser...
base/json/json_value_serializer_unittest.cc:90: TEST(JSONValueSerializerTest,
ReadProperJSONFromString) {
On 2015/02/25 03:38:24, rvargas wrote:
> nit: Deserializer. (& others)

Hi Ricardo, 
Thanks for the review.
I'll change the test category name to JSONValueDeserializerTest, which is
explicitly testing deserialization.
But there are some tests which are testing both serialization and
deserialization like "Roundtrip". what should we name for those test categories
?

[Matt Giuca, 2015-02-25 06:11:40]

https://codereview.chromium.org/925783002/diff/40001/base/json/json_value_ser...
File base/json/json_value_serializer_unittest.cc (left):

https://codereview.chromium.org/925783002/diff/40001/base/json/json_value_ser...
base/json/json_value_serializer_unittest.cc:90: TEST(JSONValueSerializerTest,
ReadProperJSONFromString) {
I think the test name should probably match the filename. So if we were to
rename half of these tests to JSONValueDeserializerTest, we probably should move
them to json_value_deserializer_unittest.cc. That seems like too much churn,
especially for this already-big CL. I would just leave it as-is (which solves
the dilemma of which test class to put Roundtrip in).

[rvargas (doing something else), 2015-02-25 19:00:16]

https://codereview.chromium.org/925783002/diff/40001/base/json/json_value_ser...
File base/json/json_value_serializer_unittest.cc (left):

https://codereview.chromium.org/925783002/diff/40001/base/json/json_value_ser...
base/json/json_value_serializer_unittest.cc:90: TEST(JSONValueSerializerTest,
ReadProperJSONFromString) {
On 2015/02/25 06:11:40, Matt Giuca wrote:
> I think the test name should probably match the filename. So if we were to
> rename half of these tests to JSONValueDeserializerTest, we probably should
move
> them to json_value_deserializer_unittest.cc. That seems like too much churn,
> especially for this already-big CL. I would just leave it as-is (which solves
> the dilemma of which test class to put Roundtrip in).

While I agree that in general the test name matches the name of the file, I see
that as a consequence of the guideline of having a class Foo live on foo.h.

On the other hand, I see no big issue in having both classes live in the same
header (as they are very closely related)... which also means that tests live
together in the same file.

I think it is more important to match the (main) class that is under test. (see
time_unittest.cc for an example). As for Roundtrip, leaving it under
serialization sounds fine to me.

[Matt Giuca, 2015-02-25 23:20:49]

https://codereview.chromium.org/925783002/diff/40001/base/json/json_value_ser...
File base/json/json_value_serializer_unittest.cc (left):

https://codereview.chromium.org/925783002/diff/40001/base/json/json_value_ser...
base/json/json_value_serializer_unittest.cc:90: TEST(JSONValueSerializerTest,
ReadProperJSONFromString) {
On 2015/02/25 19:00:15, rvargas wrote:
> On 2015/02/25 06:11:40, Matt Giuca wrote:
> > I think the test name should probably match the filename. So if we were to
> > rename half of these tests to JSONValueDeserializerTest, we probably should
> move
> > them to json_value_deserializer_unittest.cc. That seems like too much churn,
> > especially for this already-big CL. I would just leave it as-is (which
solves
> > the dilemma of which test class to put Roundtrip in).
> 
> While I agree that in general the test name matches the name of the file, I
see
> that as a consequence of the guideline of having a class Foo live on foo.h.
> 
> On the other hand, I see no big issue in having both classes live in the same
> header (as they are very closely related)... which also means that tests live
> together in the same file.
> 
> I think it is more important to match the (main) class that is under test.
(see
> time_unittest.cc for an example). As for Roundtrip, leaving it under
> serialization sounds fine to me.

Acknowledged.

[prashant.hiremath, 2015-03-02 10:44:41]

Please Review.
Sorry for the delay, was on PTO :) .

[rvargas (doing something else), 2015-03-02 23:45:58]

base lgtm

[Matt Giuca, 2015-03-03 22:59:19]

mgiuca@chromium.org changed reviewers:
+ brettw@chromium.org

[Matt Giuca, 2015-03-03 22:59:20]

OK, this is looking good now.

I am adding brettw@chromium.org for top-level owners (technically, his approval
is required for changing all of these other files), but because of the
mechanical nature of the change, I added TBR to the CL description (which means
you do not need to wait for his approval before committing).

(Note that you should not normally use TBR on code reviews; I am only using it
here because of the widespread mechanical changes.)

Feel free to check the Commit box when you are ready. (You may run into trouble
if someone has added new dependencies on JSONValueSerializer since the last
patch set, in which case you should merge and try again.)

[prashant.hiremath, 2015-03-04 02:46:39]

The CQ bit was checked by prashhir@cisco.com

[prashant.hiremath, 2015-03-04 02:46:41]

The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org
Link to the patchset: https://codereview.chromium.org/925783002/#ps80001 (title:
"Changed Test Category name as per review comment.")

[commit-bot: I haz the power, 2015-03-04 02:47:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/925783002/80001

[commit-bot: I haz the power, 2015-03-04 03:00:40]

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

[commit-bot: I haz the power, 2015-03-04 03:00:42]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[prashant.hiremath, 2015-03-05 06:58:18]

The CQ bit was checked by prashhir@cisco.com

[prashant.hiremath, 2015-03-05 06:58:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
rvargas@chromium.org, mgiuca@chromium.org
Link to the patchset: https://codereview.chromium.org/925783002/#ps200001
(title: "Fix build failure due to ctor change.")

[commit-bot: I haz the power, 2015-03-05 06:58:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/925783002/200001

[prashant.hiremath, 2015-03-05 06:59:46]

Fixed the build failures in windows/chromeos/modules which are not enabled by
default in chrome and browser_tests build.

[commit-bot: I haz the power, 2015-03-05 07:20:37]

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

[commit-bot: I haz the power, 2015-03-05 07:20:38]

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

[prashant.hiremath, 2015-03-05 08:12:59]

Fixed cpplint warnings. Will try committing now. Hope this passes :)

[prashant.hiremath, 2015-03-05 08:13:20]

The CQ bit was checked by prashhir@cisco.com

[prashant.hiremath, 2015-03-05 08:13:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
rvargas@chromium.org, mgiuca@chromium.org
Link to the patchset: https://codereview.chromium.org/925783002/#ps220001
(title: "Fixed cpplint warnings.")

[commit-bot: I haz the power, 2015-03-05 08:14:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/925783002/220001

[commit-bot: I haz the power, 2015-03-05 09:31:08]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-03-05 09:31:54]

Patchset 12 (id:??) landed as
https://crrev.com/54a99450c633d64842217b10fa6841b2a3e7eb77
Cr-Commit-Position: refs/heads/master@{#319239}

[Matt Giuca, 2015-03-06 01:52:11]

Thanks, Prashant!