Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9147060:
Rewrite the gdata_parser by using JSONValueConverter. (Closed)
|
Created:
8 years, 11 months ago by Jun Mukai Modified:
8 years, 11 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org, awong Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionRewrite the gdata_parser by using JSONValueConverter.
By using JSONValueConverter, we can eliminate ~200 lines of code.
This change depends on http://codereview.chromium.org/9184002 and http://codereview.chromium.org/9187047/
BUG=109375
TEST=passed
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=118068
Patch Set 1 #Patch Set 2 : Fix some comments in the header file #
Total comments: 41
Patch Set 3 : Fix code based on reviewer comments #Patch Set 4 : Modify std::vector back to ScopedVector #
Total comments: 6
Patch Set 5 : Add comment to FillRemainingFields and more #Patch Set 6 : Put names for internal structs because arraysize may not work with anonymous types #
Messages
Total messages: 12 (0 generated)
I'll take a closer look tomorrow, but per the stats numbers, gdata_parser.s is reduced by 200 lines? that sounds awesome.
200-line reduction is awesome. parsing rules are now declarative, which is also nice. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_parser.cc (right): http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:110: bool GetGURLFromString(const base::StringPiece& url_string, GURL* result) { function comment is missing. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:169: for (size_t i = 0; kFeedLinkTypeMap[i].rel; i++) { rather than relying on NULL, can you do the following? i < arraysize(kFeedLinkTypeMap[i]) http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:198: for (size_t i = 0; kCategoryTypeMap[i].scheme; i++) { arraysize() would be a bit cleaner. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:218: for (std::vector<Link>::const_iterator iter = links_.begin(); for (int i = 0; i < links_.size(); ++i) is more concise. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:353: for (std::vector<Category>::const_iterator iter = categories_.begin(); for (int i = 0; ...) ? http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:431: for (std::vector<Link>::iterator iter = links_.begin(); for (int i = 0; ...) ? http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_parser.h (right): http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:13: #include "base/memory/scoped_vector.h" remove this. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:26: class RepeatedMessageConverter; Rather than trying to forward declare, wouldn't it be simpler to include "base/json/json_value_converter.h"? http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:57: static void RegisterJSONConverter(base::JSONValueConverter<Link>* converter); Please add some function comment. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:72: // Converts value of link.rel into LinkType. please document the return value. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:84: }; do we allow copying? otherwise, DISALLOW_COPY_ASSIGN() is missing. please check other classes too. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:97: static void RegisterJSONConverter( function comment is missing. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:107: // Converts value of gd$feedLink.rel into FeedLinkType enum. document the return value? http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:173: class Content { Class comment is missing. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:208: const std::vector<Author>& authors() const { return authors_; } zel chose to use ScopedVector, instead of std::vector, to avoid extra copying, but I think the extra copy wouldn't be a performance issue for the gdata business. Besides, I found std::vector is cleaner for the return value here. It'd be interesting to benchmark the performance of the parser and compare the before-and-after numbers. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:304: // Fix the remaining field calculation where JSONValueConverter Fix -> Fixes. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:306: void PostProcess(); This looks like a bad name. Could you give it a more descriptive name, based on what the function does? http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_parser_unittest.cc (right): http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser_unittest.cc:92: EXPECT_STREQ("\"HhMOFgcNHSt7ImBr\"", folder_entry.etag().c_str()); please replace STREQ with EQ. somehow this didn't work for zel , but it should work! EXPECT_EQ("\"HhMOFgcNHSt7ImBr\"", folder_entry.etag());
yes, 200 less lines on gdata parsing, but 400+ on the JSON converter side does not really shortens the overall codesize :) nevertheless, we will end shaving more code in the future in other places.. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_parser.cc (right): http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:184: converter->RegisterCustomField<GURL>( GURL fileds are so common, so I sugest you create RegisterGURLField() method http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_parser.h (right): http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:208: const std::vector<Author>& authors() const { return authors_; } On 2012/01/11 17:38:40, satorux1 wrote: > zel chose to use ScopedVector, instead of std::vector, to avoid extra copying, > but I think the extra copy wouldn't be a performance issue for the gdata > business. Besides, I found std::vector is cleaner for the return value here. > > It'd be interesting to benchmark the performance of the parser and compare the > before-and-after numbers. Yes, Zel just hates reallocating memory for the same thing over and over again. I suspect the difference here to be negligible, but when we do such things all over the place (i.e. look at std::string allocations in Value class) it does end up slowing/bloating the browser.
The patchset 3 does not work but let me reply at this time to share my thoughts, mainly for ScopedVector and DISALLOW_COPY_AND_ASSIGN. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_parser.cc (right): http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:110: bool GetGURLFromString(const base::StringPiece& url_string, GURL* result) { On 2012/01/11 17:38:40, satorux1 wrote: > function comment is missing. Done. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:169: for (size_t i = 0; kFeedLinkTypeMap[i].rel; i++) { On 2012/01/11 17:38:40, satorux1 wrote: > rather than relying on NULL, can you do the following? > > i < arraysize(kFeedLinkTypeMap[i]) Done. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:184: converter->RegisterCustomField<GURL>( On 2012/01/11 18:03:02, zel wrote: > GURL fileds are so common, so I sugest you create RegisterGURLField() method Do you mean to add to json_value_converter.h? I am not sure how common it is. Can I do it in another change? http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:198: for (size_t i = 0; kCategoryTypeMap[i].scheme; i++) { On 2012/01/11 17:38:40, satorux1 wrote: > arraysize() would be a bit cleaner. Done. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:218: for (std::vector<Link>::const_iterator iter = links_.begin(); On 2012/01/11 17:38:40, satorux1 wrote: > for (int i = 0; i < links_.size(); ++i) is more concise. Done. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:353: for (std::vector<Category>::const_iterator iter = categories_.begin(); On 2012/01/11 17:38:40, satorux1 wrote: > for (int i = 0; ...) ? Done. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:431: for (std::vector<Link>::iterator iter = links_.begin(); On 2012/01/11 17:38:40, satorux1 wrote: > for (int i = 0; ...) ? Done. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_parser.h (right): http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:13: #include "base/memory/scoped_vector.h" On 2012/01/11 17:38:40, satorux1 wrote: > remove this. Done. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:26: class RepeatedMessageConverter; On 2012/01/11 17:38:40, satorux1 wrote: > Rather than trying to forward declare, wouldn't it be simpler to include > "base/json/json_value_converter.h"? json_value_converter is >400-lines size, so I'm afraid that including it here will slow down the compilation of the user of this parser. Am I too conservative? http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:57: static void RegisterJSONConverter(base::JSONValueConverter<Link>* converter); On 2012/01/11 17:38:40, satorux1 wrote: > Please add some function comment. Done. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:72: // Converts value of link.rel into LinkType. On 2012/01/11 17:38:40, satorux1 wrote: > please document the return value. Done. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:84: }; On 2012/01/11 17:38:40, satorux1 wrote: > do we allow copying? otherwise, DISALLOW_COPY_ASSIGN() is missing. please check > other classes too. Ughhh, my JSONValueConverter implementation is using a copy constructor (see http://codesearch.google.com/#OAMlx_jo-ck/src/base/json/json_value_converter....) so cannot put DISALLOW_COPY_AND_ASSIGN here with the current code. Let me modify the JSONValueConverter though. I prefer to be with DISALLOW_COPY_AND_ASSIGN. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:97: static void RegisterJSONConverter( On 2012/01/11 17:38:40, satorux1 wrote: > function comment is missing. Done. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:107: // Converts value of gd$feedLink.rel into FeedLinkType enum. On 2012/01/11 17:38:40, satorux1 wrote: > document the return value? Done. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:173: class Content { On 2012/01/11 17:38:40, satorux1 wrote: > Class comment is missing. Added. Thanks for catching this. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:208: const std::vector<Author>& authors() const { return authors_; } On 2012/01/11 18:03:02, zel wrote: > On 2012/01/11 17:38:40, satorux1 wrote: > > zel chose to use ScopedVector, instead of std::vector, to avoid extra copying, > > but I think the extra copy wouldn't be a performance issue for the gdata > > business. Besides, I found std::vector is cleaner for the return value here. > > > > It'd be interesting to benchmark the performance of the parser and compare the > > before-and-after numbers. > > Yes, Zel just hates reallocating memory for the same thing over and over again. > I suspect the difference here to be negligible, but when we do such things all > over the place (i.e. look at std::string allocations in Value class) it does end > up slowing/bloating the browser. In addition to the performance reason, I've met difficulty to use std::vector because there're no handy way to avoid copy constructors with it (see my response to DISALLOW_COPY_AND_ASSIGN), so I'm now tempted to be back to ScopedVector. Please wait for another change for JSONValueConverter to support it. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:304: // Fix the remaining field calculation where JSONValueConverter On 2012/01/11 17:38:40, satorux1 wrote: > Fix -> Fixes. Done. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:306: void PostProcess(); On 2012/01/11 17:38:40, satorux1 wrote: > This looks like a bad name. Could you give it a more descriptive name, based on > what the function does? Renamed to FillRemainingFields() http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_parser_unittest.cc (right): http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser_unittest.cc:92: EXPECT_STREQ("\"HhMOFgcNHSt7ImBr\"", folder_entry.etag().c_str()); On 2012/01/11 17:38:40, satorux1 wrote: > please replace STREQ with EQ. somehow this didn't work for zel , but it should > work! > > EXPECT_EQ("\"HhMOFgcNHSt7ImBr\"", folder_entry.etag()); Done.
Uploaded patchset 4, which I modified the std::vector back to ScopedVector. Please take another look at this.
LGTM http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_parser.h (right): http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:26: class RepeatedMessageConverter; On 2012/01/12 07:20:03, mukai wrote: > On 2012/01/11 17:38:40, satorux1 wrote: > > Rather than trying to forward declare, wouldn't it be simpler to include > > "base/json/json_value_converter.h"? > > json_value_converter is >400-lines size, so I'm afraid that including it here > will slow down the compilation of the user of this parser. Am I too > conservative? It's up to you, but I'd #include the header file for conciseness. http://codereview.chromium.org/9147060/diff/2001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.h:28: } } // namespace internal } // namespace base http://codereview.chromium.org/9147060/diff/9001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_parser.cc (right): http://codereview.chromium.org/9147060/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:46: { DocumentEntry::PDF, "pdf"} keeping , would make it easier to add new items. http://codereview.chromium.org/9147060/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:103: // Convert |url_string| to |result|. Always returns true to be used Convert -> Converts http://codereview.chromium.org/9147060/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:348: void DocumentEntry::FillRemainingFields() { it may be nice to document why JSONValueConverter cannot do this.
http://codereview.chromium.org/9147060/diff/9001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_parser.cc (right): http://codereview.chromium.org/9147060/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:46: { DocumentEntry::PDF, "pdf"} On 2012/01/13 17:20:33, satorux1 wrote: > keeping , would make it easier to add new items. Done. http://codereview.chromium.org/9147060/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:103: // Convert |url_string| to |result|. Always returns true to be used On 2012/01/13 17:20:33, satorux1 wrote: > Convert -> Converts Done. http://codereview.chromium.org/9147060/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_parser.cc:348: void DocumentEntry::FillRemainingFields() { On 2012/01/13 17:20:33, satorux1 wrote: > it may be nice to document why JSONValueConverter cannot do this. Done.
Modified the .cc file a bit. arraysize does not work well with anonymous types (see comments for http://codesearch.google.com/#wZuuyuB8jKQ/chromium/src/base/basictypes.h&q=pa...), so I put names on the mapping data and use it.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/9147060/17007
Change committed as 118068 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
