Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
447763002:
Implement CSVReader and CSVWriter to be used for password import and export. (Closed)
|
Created:
6 years, 4 months ago by engedy Modified:
5 years, 11 months ago CC:
chromium-reviews, tfarina, gcasto+watchlist_chromium.org, mkwst+watchlist_chromium.org, Peter Kasting Base URL:
svn://svn.chromium.org/chrome/trunk/src Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement CSVReader and CSVWriter.
These will be used for password import and export. We opted for reinventing the wheel because it seemed to be still less work than reviewing and adding an existing (third party) library into the Chromium code base.
The code can be moved into "base" once there is at least one other user for it than the password manager.
BUG=341477
Committed: https://crrev.com/972fd742e9c996e5300728b3ea797c66a05782e8
Cr-Commit-Position: refs/heads/master@{#311271}
Patch Set 1 #
Total comments: 38
Patch Set 2 : Address comments, fixed a bug. #
Total comments: 10
Patch Set 3 : Rebase. #Patch Set 4 : Address comments from vasilii@. #
Total comments: 4
Patch Set 5 : Addressed outstanding comments. #Patch Set 6 : Rebase. #Patch Set 7 : Nit. #
Total comments: 54
Patch Set 8 : Addressed comments from vabr@. #
Total comments: 4
Patch Set 9 : Addressed most comments from vabr@. #Patch Set 10 : Rename macro. #Patch Set 11 : Changed API interface to avoid stateless classes. #
Total comments: 4
Patch Set 12 : Address last 2 comments. #Patch Set 13 : Fix whitespace in DEPS. #Patch Set 14 : Rebase. #Patch Set 15 : Add build config header. #Messages
Total messages: 42 (9 generated)
@Vasilii, could you please take a look?
https://codereview.chromium.org/447763002/diff/1/components/password_manager/... File components/password_manager/core/browser/export/csv_writer.h (right): https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/export/csv_writer.h:12: #include "base/basictypes.h" macros.h https://codereview.chromium.org/447763002/diff/1/components/password_manager/... File components/password_manager/core/browser/import/csv_reader.h (right): https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.h:12: #include "base/basictypes.h" this can be, in fact, it should be macros.h https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.h:13: #include "base/strings/string_piece.h" I bet you can forward declare this: namespace base { class StringPiece; }
https://codereview.chromium.org/447763002/diff/1/components/password_manager/... File components/password_manager/core/browser/export/csv_writer.cc (right): https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/export/csv_writer.cc:60: typedef std::vector<std::string>::const_iterator ConstFieldIterator; ConstColumnIterator? https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/export/csv_writer.cc:78: AppendValue(it_row->at(*it_column_name), csv); It's C++11 feature. https://codereview.chromium.org/447763002/diff/1/components/password_manager/... File components/password_manager/core/browser/export/csv_writer.h (right): https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/export/csv_writer.h:19: // * Lines will be separated by the platform-specific EOL terminator. Shouldn't we use CRLF as the RFC suggest? Otherwise Chrome generates platform dependent files. https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/export/csv_writer.h:20: // * Values will be enclosed in double-quotes iff they contain either of the s/iff/if https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/export/csv_writer.h:28: // there are elements in |rows|. Each element in |rows| should be a dictionary There is no |row|. https://codereview.chromium.org/447763002/diff/1/components/password_manager/... File components/password_manager/core/browser/import/csv_reader.cc (right): https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:19: // as two double-quotes, they do not need special treatment. Though the comment matches the regexp, it's not clear to me why it matches the first row. What's the algorithm? What is the sense of '*?' in the end? https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:40: static RE2 first_field_regex(kFirstFieldRE); This is thread unsafe. I'm also pretty sure that we have compilation error for this case (-Wexit-time-destructors) https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:82: ReplaceSubstringsAfterOffset(&normalized_csv, 0, "\r\n", "\n"); What if the field contain \r\n. It's valid. https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:93: return false; indent https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:97: std::string& target = records->back()[(*column_names)[i]]; column_names->at(i) looks nicer IMO https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:99: target.swap(fields[i]); What if there are more fields than columns? https://codereview.chromium.org/447763002/diff/1/components/password_manager/... File components/password_manager/core/browser/import/csv_reader.h (right): https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.h:36: static bool Read(const base::StringPiece& csv, By value.
https://codereview.chromium.org/447763002/diff/1/components/password_manager/... File components/password_manager/core/browser/import/csv_reader.cc (right): https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:97: std::string& target = records->back()[(*column_names)[i]]; On 2014/08/06 17:42:16, vasilii wrote: > column_names->at(i) looks nicer IMO might be nicer, but it throws an exception. I'd say to keep using operator[]
https://codereview.chromium.org/447763002/diff/1/components/password_manager/... File components/password_manager/core/browser/import/csv_reader.cc (right): https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:97: std::string& target = records->back()[(*column_names)[i]]; On 2014/08/06 18:00:52, tfarina wrote: > On 2014/08/06 17:42:16, vasilii wrote: > > column_names->at(i) looks nicer IMO > > might be nicer, but it throws an exception. I'd say to keep using operator[] Well, it doesn't actually throw, since we've disabled exceptions. But yes, in general it would probably be better if people never used at(), since it doesn't mean the same thing as operator[](), so when it's used in code it's never clear if the author actually intended the difference or not.
https://codereview.chromium.org/447763002/diff/1/components/password_manager/... File components/password_manager/core/browser/export/csv_writer.cc (right): https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/export/csv_writer.cc:60: typedef std::vector<std::string>::const_iterator ConstFieldIterator; On 2014/08/06 17:42:16, vasilii wrote: > ConstColumnIterator? Done. https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/export/csv_writer.cc:78: AppendValue(it_row->at(*it_column_name), csv); On 2014/08/06 17:42:16, vasilii wrote: > It's C++11 feature. Done. Replaced with find()-based solution. https://codereview.chromium.org/447763002/diff/1/components/password_manager/... File components/password_manager/core/browser/export/csv_writer.h (right): https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/export/csv_writer.h:12: #include "base/basictypes.h" On 2014/08/06 16:45:12, tfarina wrote: > macros.h Done. https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/export/csv_writer.h:19: // * Lines will be separated by the platform-specific EOL terminator. On 2014/08/06 17:42:16, vasilii wrote: > Shouldn't we use CRLF as the RFC suggest? Otherwise Chrome generates platform > dependent files. Given that CSV files are text files after all, I thought that it is better to be platform-dependent than to generate files that simply have incorrect EOL sequences on that platform -- which might cause problems with other editors on the platform. I think this behavior is consistent with the C standard library (reading/writing files opened text mode does this conversion between \n and platform specific EOL sequences) and also our JSONWriter/JSONReader implementation. I would stick to this. https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/export/csv_writer.h:20: // * Values will be enclosed in double-quotes iff they contain either of the On 2014/08/06 17:42:16, vasilii wrote: > s/iff/if That was intentional, but I have replaced with the more readable "if and only if" version. https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/export/csv_writer.h:28: // there are elements in |rows|. Each element in |rows| should be a dictionary On 2014/08/06 17:42:16, vasilii wrote: > There is no |row|. Renamed to |records|. https://codereview.chromium.org/447763002/diff/1/components/password_manager/... File components/password_manager/core/browser/import/csv_reader.cc (right): https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:19: // as two double-quotes, they do not need special treatment. On 2014/08/06 17:42:17, vasilii wrote: > Though the comment matches the regexp, it's not clear to me why it matches the > first row. What's the algorithm? > What is the sense of '*?' in the end? PTAL. https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:40: static RE2 first_field_regex(kFirstFieldRE); On 2014/08/06 17:42:17, vasilii wrote: > This is thread unsafe. > I'm also pretty sure that we have compilation error for this case > (-Wexit-time-destructors) I have moved this as non-static local variables to CSVReader::Read for now. Do you have a better idea where these could live in order to avoid recompiling the RE for all files? https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:82: ReplaceSubstringsAfterOffset(&normalized_csv, 0, "\r\n", "\n"); On 2014/08/06 17:42:17, vasilii wrote: > What if the field contain \r\n. It's valid. Hmm, do you have a particular case in mind where this might be a problem? https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:93: return false; On 2014/08/06 17:42:17, vasilii wrote: > indent Done. https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:97: std::string& target = records->back()[(*column_names)[i]]; On 2014/08/07 03:59:50, Peter Kasting wrote: > On 2014/08/06 18:00:52, tfarina wrote: > > On 2014/08/06 17:42:16, vasilii wrote: > > > column_names->at(i) looks nicer IMO > > > > might be nicer, but it throws an exception. I'd say to keep using operator[] > > Well, it doesn't actually throw, since we've disabled exceptions. But yes, in > general it would probably be better if people never used at(), since it doesn't > mean the same thing as operator[](), so when it's used in code it's never clear > if the author actually intended the difference or not. Acknowledged. https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:99: target.swap(fields[i]); On 2014/08/06 17:42:17, vasilii wrote: > What if there are more fields than columns? I have changed to behavior, PTAL: Extra fields are ignored. Missing fields will have no corresponding key-value pair in the record's ColumnNameToValueMap. https://codereview.chromium.org/447763002/diff/1/components/password_manager/... File components/password_manager/core/browser/import/csv_reader.h (right): https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.h:12: #include "base/basictypes.h" On 2014/08/06 16:45:12, tfarina wrote: > this can be, in fact, it should be macros.h Done. https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.h:13: #include "base/strings/string_piece.h" On 2014/08/06 16:45:12, tfarina wrote: > I bet you can forward declare this: > > namespace base { > class StringPiece; > } > > Unfortunately, I think this will not work, as base::StringPiece is a typedef. I would rather not forward-declare the template it is a specialization of. https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.h:36: static bool Read(const base::StringPiece& csv, On 2014/08/06 17:42:17, vasilii wrote: > By value. Done.
https://codereview.chromium.org/447763002/diff/1/components/password_manager/... File components/password_manager/core/browser/import/csv_reader.cc (right): https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:40: static RE2 first_field_regex(kFirstFieldRE); On 2014/08/08 17:06:13, engedy wrote: > On 2014/08/06 17:42:17, vasilii wrote: > > This is thread unsafe. > > I'm also pretty sure that we have compilation error for this case > > (-Wexit-time-destructors) > > I have moved this as non-static local variables to CSVReader::Read for now. Do > you have a better idea where these could live in order to avoid recompiling the > RE for all files? Do you really need it? If yes, there is LazyInstance<>. https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:82: ReplaceSubstringsAfterOffset(&normalized_csv, 0, "\r\n", "\n"); On 2014/08/08 17:06:13, engedy wrote: > On 2014/08/06 17:42:17, vasilii wrote: > > What if the field contain \r\n. It's valid. > > Hmm, do you have a particular case in mind where this might be a problem? Not really. Do you want to support the case when we have \n inside the field? https://codereview.chromium.org/447763002/diff/40001/components/password_mana... File components/password_manager/core/browser/export/csv_writer_unittest.cc (right): https://codereview.chromium.org/447763002/diff/40001/components/password_mana... components/password_manager/core/browser/export/csv_writer_unittest.cc:30: DISALLOW_COPY_AND_ASSIGN(CSVWriterTest); This should be in private. https://codereview.chromium.org/447763002/diff/40001/components/password_mana... File components/password_manager/core/browser/import/csv_reader.cc (right): https://codereview.chromium.org/447763002/diff/40001/components/password_mana... components/password_manager/core/browser/import/csv_reader.cc:38: // |fields|, and sets |had_trailing_eol| to whether or not the consumed row was had_trailing_eol? https://codereview.chromium.org/447763002/diff/40001/components/password_mana... components/password_manager/core/browser/import/csv_reader.cc:50: re2::StringPiece field; You can move this inside the block. https://codereview.chromium.org/447763002/diff/40001/components/password_mana... File components/password_manager/core/browser/import/csv_reader_unittest.cc (right): https://codereview.chromium.org/447763002/diff/40001/components/password_mana... components/password_manager/core/browser/import/csv_reader_unittest.cc:190: std::make_pair("left", "A"))); Do you want to check that there is no "right" value?
https://codereview.chromium.org/447763002/diff/1/components/password_manager/... File components/password_manager/core/browser/import/csv_reader.cc (right): https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:40: static RE2 first_field_regex(kFirstFieldRE); On 2014/08/11 11:56:38, vasilii wrote: > On 2014/08/08 17:06:13, engedy wrote: > > On 2014/08/06 17:42:17, vasilii wrote: > > > This is thread unsafe. > > > I'm also pretty sure that we have compilation error for this case > > > (-Wexit-time-destructors) > > > > I have moved this as non-static local variables to CSVReader::Read for now. Do > > you have a better idea where these could live in order to avoid recompiling > the > > RE for all files? > > Do you really need it? If yes, there is LazyInstance<>. I think LazyInstance<> cannot be used with classes whose constructors take arguments. Therefore I have introduced a CSVParser class here to hold instances of the compiled RegExes, so at least we can re-use them for all rows of the same file. I would prefer reusing them across files, ideally, but I do not see how that would be possible :-(. https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:82: ReplaceSubstringsAfterOffset(&normalized_csv, 0, "\r\n", "\n"); On 2014/08/11 11:56:38, vasilii wrote: > On 2014/08/08 17:06:13, engedy wrote: > > On 2014/08/06 17:42:17, vasilii wrote: > > > What if the field contain \r\n. It's valid. > > > > Hmm, do you have a particular case in mind where this might be a problem? > > Not really. Do you want to support the case when we have \n inside the field? I do not think that this would be a particularly frequent use-case for our purposes, but using the same reasoning as before (with how the C standard library does it) I think normalizing line endings uniformly everywhere makes sense. https://codereview.chromium.org/447763002/diff/40001/components/password_mana... File components/password_manager/core/browser/export/csv_writer_unittest.cc (right): https://codereview.chromium.org/447763002/diff/40001/components/password_mana... components/password_manager/core/browser/export/csv_writer_unittest.cc:30: DISALLOW_COPY_AND_ASSIGN(CSVWriterTest); On 2014/08/11 11:56:38, vasilii wrote: > This should be in private. Done. https://codereview.chromium.org/447763002/diff/40001/components/password_mana... File components/password_manager/core/browser/import/csv_reader.cc (right): https://codereview.chromium.org/447763002/diff/40001/components/password_mana... components/password_manager/core/browser/import/csv_reader.cc:38: // |fields|, and sets |had_trailing_eol| to whether or not the consumed row was On 2014/08/11 11:56:38, vasilii wrote: > had_trailing_eol? Sorry, that was garbage left behind. https://codereview.chromium.org/447763002/diff/40001/components/password_mana... components/password_manager/core/browser/import/csv_reader.cc:50: re2::StringPiece field; On 2014/08/11 11:56:38, vasilii wrote: > You can move this inside the block. Done. https://codereview.chromium.org/447763002/diff/40001/components/password_mana... File components/password_manager/core/browser/import/csv_reader_unittest.cc (right): https://codereview.chromium.org/447763002/diff/40001/components/password_mana... components/password_manager/core/browser/import/csv_reader_unittest.cc:190: std::make_pair("left", "A"))); On 2014/08/11 11:56:38, vasilii wrote: > Do you want to check that there is no "right" value? Yes, testing::ElementsAre() is strict in this sense that it does that already.
https://codereview.chromium.org/447763002/diff/1/components/password_manager/... File components/password_manager/core/browser/import/csv_reader.cc (right): https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:40: static RE2 first_field_regex(kFirstFieldRE); On 2014/08/12 11:27:11, engedy wrote: > On 2014/08/11 11:56:38, vasilii wrote: > > On 2014/08/08 17:06:13, engedy wrote: > > > On 2014/08/06 17:42:17, vasilii wrote: > > > > This is thread unsafe. > > > > I'm also pretty sure that we have compilation error for this case > > > > (-Wexit-time-destructors) > > > > > > I have moved this as non-static local variables to CSVReader::Read for now. > Do > > > you have a better idea where these could live in order to avoid recompiling > > the > > > RE for all files? > > > > Do you really need it? If yes, there is LazyInstance<>. > > I think LazyInstance<> cannot be used with classes whose constructors take > arguments. > > Therefore I have introduced a CSVParser class here to hold instances of the > compiled RegExes, so at least we can re-use them for all rows of the same file. > I would prefer reusing them across files, ideally, but I do not see how that > would be possible :-(. You could hardcode the const char* constant into the type using templates but don't do it. 99% users will never import passwords. The remaining 1% will import it once during the session. https://codereview.chromium.org/447763002/diff/40001/components/password_mana... File components/password_manager/core/browser/import/csv_reader.cc (right): https://codereview.chromium.org/447763002/diff/40001/components/password_mana... components/password_manager/core/browser/import/csv_reader.cc:50: re2::StringPiece field; On 2014/08/12 11:27:12, engedy wrote: > On 2014/08/11 11:56:38, vasilii wrote: > > You can move this inside the block. > > Done. |field| is still outside the loop. https://codereview.chromium.org/447763002/diff/80001/components/password_mana... File components/password_manager/core/browser/import/csv_reader.cc (right): https://codereview.chromium.org/447763002/diff/80001/components/password_mana... components/password_manager/core/browser/import/csv_reader.cc:41: CSVParser(const std::string& csv) I prefer to pass "const std::string*" because you store the pointer in fact. Currently "CSVParser pasrser("die Chrome")" compiles fine. https://codereview.chromium.org/447763002/diff/80001/components/password_mana... components/password_manager/core/browser/import/csv_reader.cc:75: bool HasMoreRows() { const
https://codereview.chromium.org/447763002/diff/1/components/password_manager/... File components/password_manager/core/browser/import/csv_reader.cc (right): https://codereview.chromium.org/447763002/diff/1/components/password_manager/... components/password_manager/core/browser/import/csv_reader.cc:40: static RE2 first_field_regex(kFirstFieldRE); On 2014/08/12 12:20:01, vasilii wrote: > On 2014/08/12 11:27:11, engedy wrote: > > On 2014/08/11 11:56:38, vasilii wrote: > > > On 2014/08/08 17:06:13, engedy wrote: > > > > On 2014/08/06 17:42:17, vasilii wrote: > > > > > This is thread unsafe. > > > > > I'm also pretty sure that we have compilation error for this case > > > > > (-Wexit-time-destructors) > > > > > > > > I have moved this as non-static local variables to CSVReader::Read for > now. > > Do > > > > you have a better idea where these could live in order to avoid > recompiling > > > the > > > > RE for all files? > > > > > > Do you really need it? If yes, there is LazyInstance<>. > > > > I think LazyInstance<> cannot be used with classes whose constructors take > > arguments. > > > > Therefore I have introduced a CSVParser class here to hold instances of the > > compiled RegExes, so at least we can re-use them for all rows of the same > file. > > I would prefer reusing them across files, ideally, but I do not see how that > > would be possible :-(. > > You could hardcode the const char* constant into the type using templates but > don't do it. 99% users will never import passwords. The remaining 1% will import > it once during the session. Agreed. The current solution with CSVParse seems fine, then. https://codereview.chromium.org/447763002/diff/40001/components/password_mana... File components/password_manager/core/browser/import/csv_reader.cc (right): https://codereview.chromium.org/447763002/diff/40001/components/password_mana... components/password_manager/core/browser/import/csv_reader.cc:50: re2::StringPiece field; On 2014/08/12 12:20:01, vasilii wrote: > On 2014/08/12 11:27:12, engedy wrote: > > On 2014/08/11 11:56:38, vasilii wrote: > > > You can move this inside the block. > > > > Done. > > |field| is still outside the loop. Argh, I have moved it... again! :-) https://codereview.chromium.org/447763002/diff/80001/components/password_mana... File components/password_manager/core/browser/import/csv_reader.cc (right): https://codereview.chromium.org/447763002/diff/80001/components/password_mana... components/password_manager/core/browser/import/csv_reader.cc:41: CSVParser(const std::string& csv) On 2014/08/12 12:20:01, vasilii wrote: > I prefer to pass "const std::string*" because you store the pointer in fact. > Currently "CSVParser pasrser("die Chrome")" compiles fine. I have changed it to take a base::StringPiece. https://codereview.chromium.org/447763002/diff/80001/components/password_mana... components/password_manager/core/browser/import/csv_reader.cc:75: bool HasMoreRows() { On 2014/08/12 12:20:01, vasilii wrote: > const Done.
lgtm
engedy@chromium.org changed reviewers: - pkasting@chromium.org
The CQ bit was checked by engedy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/447763002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
engedy@chromium.org changed reviewers: + vabr@chromium.org
Vaclav, I am planning to resume work on this. There is still the Win compile error to figure out, but in the meantime, could you please review for Owner's approval?
On 2014/09/25 12:38:59, engedy wrote: > Vaclav, I am planning to resume work on this. There is still the Win compile > error to figure out, but in the meantime, could you please review for Owner's > approval? And, by "figuring out", I mean, to "trivially fix".
LGTM with comments. Happy to discuss more. Cheers, Vaclav https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/export/csv_writer.cc (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer.cc:22: *out += raw_value; optional: I see 2 advantages of using out->append(raw_value) instead. 1. I suppose out->append(raw_value) has a potential to be less wasteful than +=, as += is likely to generate intermediate strings. 2. I subjectively find "append(raw_value)" much easier to read than *out += raw_value, which looks very similar to *out = raw_value. I was lazy to back the above with data, hence this is optional. If you decide to switch, then please also switch on other places where += is used in this file. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer.cc:31: if (raw_value[i] == '\"') Should you use ReplaceSubstringsAfterOffset also here, instead of the for-loop? https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer.cc:62: it_column_name != column_names.end(); ++it_column_name) { Please make sure that this is how clang-format would break the line. If you can, prefer clang-format's way of formatting. If you cannot, please consider disabling clang-format on this line (http://b/14285621#comment14) and explaining why. Also in the for-loops below. https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/export/csv_writer.h (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer.h:22: class CSVWriter { Why is this a class and not just a helper method? https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer.h:26: // Generates the CSV representation of the supplied data into |csv|. The first nit: Please specify if the generated data gets appended to |csv|, or if |csv| is overwritten. https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/export/csv_writer_unittest.cc (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer_unittest.cc:82: // We also initialize the output argument here to be non-empty so as to verify I'd suggest making this a separate test. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer_unittest.cc:103: TEST_F(CSVWriterTest, CharactersOutsideASCIIPrintableArePreservedAsIs) { nit: I am not a native speaker, but "are preserved as is" sounds weird to me. My suggestion is to drop "AsIs", because the meaning should remain the same, and the name gets shorter. If you keep AsIs, please make sure to verify it's grammatically correct. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer_unittest.cc:112: // cent sign, euro sign, CJK unified ideograph for [ce`i] in Han script. And where is the Unicode Snowman ☃? :) https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer_unittest.cc:190: } // namespace password_manager Please add #undef EOL_SEQUENCE It's sort of pointless from the view of the preprocessor at the end of the file, but let's make the guide happy: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Preprocessor_... https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/import/csv_reader.cc (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.cc:15: // Will throw away the potential trailing EOL character. Please explain that this RE does only understand \n as EOL, and that \r is normalised away for its input. One way to do the former would be just speaking about \n instead of "EOL character". https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.cc:46: bool ParseNextCSVRow(std::vector<std::string>* fields) { Please try not to inline this non-trivial method here. It's fine to do so in tests, but this is production code. Lawyer-style: the style guide prohibits inlining for functions of more than 10 lines http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inline_Functions. If this was a header, I could also cite http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... :) On the technical side, inlining should have a justification. One could be performance, supported by an actual benchmark to prove the need for inlining. Another readability (when the code is short and readable, it might be better to inline than to comment on the effect). But neither of those seems to apply here. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.cc:46: bool ParseNextCSVRow(std::vector<std::string>* fields) { Please comment on the return value and the argument. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.cc:58: if (field.starts_with("\"") && field.ends_with("\"")) { Should you rather do if (field.starts_with("\"")) { CHECK(field.ends_with("\"")); CHECK_GE(field.size(), 2); ... } ? It is an error to have a " at just one end, because one " present means the whole string needs to be in quotes, right? https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.cc:66: fields->back().swap(field_copy); IMO, the importance of performance benefits of this is questionable (how often will this be executed during a single browsing session?), and the code is obscured by the swap(). If you really want to avoid the string copy, what about passing &fields->back() to ReplaceSubstringsAfterOffset instead? https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.cc:80: re2::StringPiece remaining_csv_piece_; Please comment on the data members. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.cc:82: RE2 first_row_regex_; Could those be const? https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/import/csv_reader.h (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.h:26: class CSVReader { Why a class and not just a helper function? https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.h:34: // |records|. nit: I suggest explicitly stating that both vectors get overwritten, not appended to. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.h:35: static bool Read(base::StringPiece csv, Please comment on the return value. https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/import/csv_reader_unittest.cc (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader_unittest.cc:260: } // namespace password_manager Further cases worth testing: * data rows containing multiple commas, in particular ,, * header row containing (only/some) empty values * header row containing repetition of the same value: a,a or ,, etc. -> this is probably syntactically valid (unless the RFC says otherwise), but we should document the effect (some values get overwritten - which ones?) * more invalid inputs (all the tests so far seem to have valid inputs)
Thanks for the thorough review. Given the amount of changes, PTAL. https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/export/csv_writer.cc (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer.cc:22: *out += raw_value; On 2014/09/25 14:44:27, vabr (Chromium) wrote: > optional: > I see 2 advantages of using out->append(raw_value) instead. > 1. I suppose out->append(raw_value) has a potential to be less wasteful than +=, > as += is likely to generate intermediate strings. > 2. I subjectively find "append(raw_value)" much easier to read than *out += > raw_value, which looks very similar to *out = raw_value. > > I was lazy to back the above with data, hence this is optional. > > If you decide to switch, then please also switch on other places where += is > used in this file. Hmm, why do you expect += to generate temporary strings? I would have assumed there would not be a performance difference. On the other hand, I can accept that you find append() more readable. Done. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer.cc:31: if (raw_value[i] == '\"') On 2014/09/25 14:44:27, vabr (Chromium) wrote: > Should you use ReplaceSubstringsAfterOffset also here, instead of the for-loop? Done. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer.cc:62: it_column_name != column_names.end(); ++it_column_name) { On 2014/09/25 14:44:27, vabr (Chromium) wrote: > Please make sure that this is how clang-format would break the line. If you can, > prefer clang-format's way of formatting. If you cannot, please consider > disabling clang-format on this line (http://b/14285621#comment14) and explaining > why. > > Also in the for-loops below. Reformatted with clang-format. https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/export/csv_writer.h (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer.h:22: class CSVWriter { On 2014/09/25 14:44:27, vabr (Chromium) wrote: > Why is this a class and not just a helper method? For consistency with JSONWriter. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer.h:26: // Generates the CSV representation of the supplied data into |csv|. The first On 2014/09/25 14:44:27, vabr (Chromium) wrote: > nit: Please specify if the generated data gets appended to |csv|, or if |csv| is > overwritten. Done. https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/export/csv_writer_unittest.cc (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer_unittest.cc:82: // We also initialize the output argument here to be non-empty so as to verify On 2014/09/25 14:44:27, vabr (Chromium) wrote: > I'd suggest making this a separate test. Done. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer_unittest.cc:103: TEST_F(CSVWriterTest, CharactersOutsideASCIIPrintableArePreservedAsIs) { On 2014/09/25 14:44:27, vabr (Chromium) wrote: > nit: I am not a native speaker, but "are preserved as is" sounds weird to me. My > suggestion is to drop "AsIs", because the meaning should remain the same, and > the name gets shorter. If you keep AsIs, please make sure to verify it's > grammatically correct. I have changed it to "Verbatim". https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer_unittest.cc:112: // cent sign, euro sign, CJK unified ideograph for [ce`i] in Han script. On 2014/09/25 14:44:27, vabr (Chromium) wrote: > And where is the Unicode Snowman ☃? :) Agreed. € is boring. Replaced it with ☃. I have also switched from \u to \x, because \u is C++11. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer_unittest.cc:190: } // namespace password_manager On 2014/09/25 14:44:27, vabr (Chromium) wrote: > Please add > #undef EOL_SEQUENCE > It's sort of pointless from the view of the preprocessor at the end of the file, > but let's make the guide happy: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Preprocessor_... Hmm, nice catch. Done. https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/import/csv_reader.cc (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.cc:15: // Will throw away the potential trailing EOL character. On 2014/09/25 14:44:27, vabr (Chromium) wrote: > Please explain that this RE does only understand \n as EOL, and that \r is > normalised away for its input. > One way to do the former would be just speaking about \n instead of "EOL > character". Done. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.cc:46: bool ParseNextCSVRow(std::vector<std::string>* fields) { On 2014/09/25 14:44:27, vabr (Chromium) wrote: > Please comment on the return value and the argument. Done. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.cc:46: bool ParseNextCSVRow(std::vector<std::string>* fields) { On 2014/09/25 14:44:27, vabr (Chromium) wrote: > Please try not to inline this non-trivial method here. It's fine to do so in > tests, but this is production code. > > Lawyer-style: the style guide prohibits inlining for functions of more than 10 > lines > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inline_Functions. > If this was a header, I could also cite > http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... > :) > > On the technical side, inlining should have a justification. One could be > performance, supported by an actual benchmark to prove the need for inlining. > Another readability (when the code is short and readable, it might be better to > inline than to comment on the effect). But neither of those seems to apply here. My reasoning was the following: * We are in an anonymous namespace in a CC file, so no danger of much bloating. * The class has 2 methods, so readability should be fine either way. * Not performance code. So I went and I optimized for less lines. :-) Can you think of a concrete reason why we should not inline here? https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.cc:58: if (field.starts_with("\"") && field.ends_with("\"")) { On 2014/09/25 14:44:27, vabr (Chromium) wrote: > Should you rather do > if (field.starts_with("\"")) { > CHECK(field.ends_with("\"")); > CHECK_GE(field.size(), 2); > ... > } > ? > It is an error to have a " at just one end, because one " present means the > whole string needs to be in quotes, right? Done. Yes, furthermore, the regex will not fit in case of a mismatched double quotes, so we will not crash the browser in case of an erroneous input. I will add tests too. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.cc:66: fields->back().swap(field_copy); On 2014/09/25 14:44:27, vabr (Chromium) wrote: > IMO, the importance of performance benefits of this is questionable (how often > will this be executed during a single browsing session?), and the code is > obscured by the swap(). > If you really want to avoid the string copy, what about passing &fields->back() > to ReplaceSubstringsAfterOffset instead? You are right. I have removed the premature optimization. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.cc:80: re2::StringPiece remaining_csv_piece_; On 2014/09/25 14:44:27, vabr (Chromium) wrote: > Please comment on the data members. Can you think of anything that would add value? I could not come up with anything sensible. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.cc:82: RE2 first_row_regex_; On 2014/09/25 14:44:27, vabr (Chromium) wrote: > Could those be const? Yeah, done. https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/import/csv_reader.h (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.h:26: class CSVReader { On 2014/09/25 14:44:27, vabr (Chromium) wrote: > Why a class and not just a helper function? Same reason as for CSVWriter: for consistency with JSONReader. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.h:34: // |records|. On 2014/09/25 14:44:28, vabr (Chromium) wrote: > nit: I suggest explicitly stating that both vectors get overwritten, not > appended to. Done. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.h:35: static bool Read(base::StringPiece csv, On 2014/09/25 14:44:27, vabr (Chromium) wrote: > Please comment on the return value. Done. https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/import/csv_reader_unittest.cc (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader_unittest.cc:260: } // namespace password_manager On 2014/09/25 14:44:28, vabr (Chromium) wrote: > Further cases worth testing: > * data rows containing multiple commas, in particular ,, > * header row containing (only/some) empty values > * header row containing repetition of the same value: a,a or ,, etc. > -> this is probably syntactically valid (unless the RFC says otherwise), but > we should document the effect (some values get overwritten - which ones?) > * more invalid inputs (all the tests so far seem to have valid inputs) Done.
Thanks, Balázs! I liked the new tests you added. The only major pushback from me is against presenting the Reader and Writer as a class, when they really are just single functions (more below). Cheers, Vaclav https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/export/csv_writer.cc (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer.cc:22: *out += raw_value; On 2014/11/06 14:55:38, engedy wrote: > On 2014/09/25 14:44:27, vabr (Chromium) wrote: > > optional: > > I see 2 advantages of using out->append(raw_value) instead. > > 1. I suppose out->append(raw_value) has a potential to be less wasteful than > +=, > > as += is likely to generate intermediate strings. > > 2. I subjectively find "append(raw_value)" much easier to read than *out += > > raw_value, which looks very similar to *out = raw_value. > > > > I was lazy to back the above with data, hence this is optional. > > > > If you decide to switch, then please also switch on other places where += is > > used in this file. > > Hmm, why do you expect += to generate temporary strings? I would have assumed > there would not be a performance difference. > > On the other hand, I can accept that you find append() more readable. Done. The reason why I expect += to generate temporary strings is in case it is implemented through + and = (as X += Y -> X = X + Y). Having said that, I believe this is not the case in any sensible implementation of STL, and stupid implementations can also have an inefficient append(), so my reason 1. is not really a good reason. Thanks for considering reason 2. and switching, nevertheless! :) https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/export/csv_writer.h (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer.h:22: class CSVWriter { On 2014/11/06 14:55:39, engedy wrote: > On 2014/09/25 14:44:27, vabr (Chromium) wrote: > > Why is this a class and not just a helper method? > > For consistency with JSONWriter. But the situation is different for JSONWriter -- JSONWriter is a class, because although the interface is just a static method, every call to it is realised by constructing an instance of JSONWriter, which does a non-trivial thing. Here you only have static methods, therefore I don't think this should be a class. https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/export/csv_writer_unittest.cc (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer_unittest.cc:103: TEST_F(CSVWriterTest, CharactersOutsideASCIIPrintableArePreservedAsIs) { On 2014/11/06 14:55:39, engedy wrote: > On 2014/09/25 14:44:27, vabr (Chromium) wrote: > > nit: I am not a native speaker, but "are preserved as is" sounds weird to me. > My > > suggestion is to drop "AsIs", because the meaning should remain the same, and > > the name gets shorter. If you keep AsIs, please make sure to verify it's > > grammatically correct. > > I have changed it to "Verbatim". Acknowledged. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer_unittest.cc:112: // cent sign, euro sign, CJK unified ideograph for [ce`i] in Han script. On 2014/11/06 14:55:39, engedy wrote: > On 2014/09/25 14:44:27, vabr (Chromium) wrote: > > And where is the Unicode Snowman ☃? :) > > Agreed. € is boring. Replaced it with ☃. Thanks! :) > > I have also switched from \u to \x, because \u is C++11. SGTM. https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/import/csv_reader.cc (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.cc:46: bool ParseNextCSVRow(std::vector<std::string>* fields) { On 2014/11/06 14:55:39, engedy wrote: > On 2014/09/25 14:44:27, vabr (Chromium) wrote: > > Please try not to inline this non-trivial method here. It's fine to do so in > > tests, but this is production code. > > > > Lawyer-style: the style guide prohibits inlining for functions of more than 10 > > lines > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inline_Functions. > > If this was a header, I could also cite > > > http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... > > :) > > > > On the technical side, inlining should have a justification. One could be > > performance, supported by an actual benchmark to prove the need for inlining. > > Another readability (when the code is short and readable, it might be better > to > > inline than to comment on the effect). But neither of those seems to apply > here. > > My reasoning was the following: > * We are in an anonymous namespace in a CC file, so no danger of much bloating. > * The class has 2 methods, so readability should be fine either way. > * Not performance code. > So I went and I optimized for less lines. :-) > > Can you think of a concrete reason why we should not inline here? My concrete reason is boring: The style guide, permissive in general, is quite decisive in this particular point, so we should stick to it. The readability argument is always subjective. Personally, I have troubles to even count the number of methods of a class with inlined methods like this, so having just 2 of them is not making a lot of change for me. I guess that's what the style guide for: choosing one option if more can be subjectively argued for. https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.cc:80: re2::StringPiece remaining_csv_piece_; On 2014/11/06 14:55:39, engedy wrote: > On 2014/09/25 14:44:27, vabr (Chromium) wrote: > > Please comment on the data members. > > Can you think of anything that would add value? I could not come up with > anything sensible. You are right, the names make it clear enough. No comments needed. :) https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/import/csv_reader.h (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.h:26: class CSVReader { On 2014/11/06 14:55:39, engedy wrote: > On 2014/09/25 14:44:27, vabr (Chromium) wrote: > > Why a class and not just a helper function? > > Same reason as for CSVWriter: for consistency with JSONReader. Again, JSONReader actually uses the non-static parts of itself, whereas CSVWriter does not. I vote against making it a static-only class. https://codereview.chromium.org/447763002/diff/160001/components/password_man... File components/password_manager/core/browser/export/csv_writer.cc (right): https://codereview.chromium.org/447763002/diff/160001/components/password_man... components/password_manager/core/browser/export/csv_writer.cc:68: typedef std::vector<ColumnNameToValueMap>::const_iterator ConstRecordIterator; You can remove this typedef and use auto instead. (Also on line 58.) http://chromium-cpp.appspot.com/ https://codereview.chromium.org/447763002/diff/160001/components/password_man... File components/password_manager/core/browser/export/csv_writer_unittest.cc (right): https://codereview.chromium.org/447763002/diff/160001/components/password_man... components/password_manager/core/browser/export/csv_writer_unittest.cc:21: virtual void SetUp() override { Drop the "virtual", it's cleaner. :) https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/VTNZzizN0zo https://codereview.chromium.org/447763002/diff/160001/components/password_man... File components/password_manager/core/browser/import/csv_reader.cc (right): https://codereview.chromium.org/447763002/diff/160001/components/password_man... components/password_manager/core/browser/import/csv_reader.cc:35: // to reject inputs like: "a"b"c". Good catch!
Please see my responses -- other than that, I have C++11-ized it, and refactored a bit. https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/export/csv_writer.h (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer.h:22: class CSVWriter { On 2014/11/06 16:16:02, vabr (OOO until 9 Nov) wrote: > On 2014/11/06 14:55:39, engedy wrote: > > On 2014/09/25 14:44:27, vabr (Chromium) wrote: > > > Why is this a class and not just a helper method? > > > > For consistency with JSONWriter. > > But the situation is different for JSONWriter -- JSONWriter is a class, because > although the interface is just a static method, every call to it is realised by > constructing an instance of JSONWriter, which does a non-trivial thing. > > Here you only have static methods, therefore I don't think this should be a > class. Yes, this is what I mean by consistency: there is no dire need to make it a class, but this way it is similar to JSONWriter. My arguments for making it a class+static are: * Having global functions looks weird. * Having so much comment for a single method looks weird. * I kinda like that it acts as a namespace, so that the typedef is not hanging out there as the global. I can be convinced to change it to a global if you feel strongly about it. :-) https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/import/csv_reader.cc (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.cc:46: bool ParseNextCSVRow(std::vector<std::string>* fields) { On 2014/11/06 16:16:02, vabr (OOO until 9 Nov) wrote: > On 2014/11/06 14:55:39, engedy wrote: > > On 2014/09/25 14:44:27, vabr (Chromium) wrote: > > > Please try not to inline this non-trivial method here. It's fine to do so in > > > tests, but this is production code. > > > > > > Lawyer-style: the style guide prohibits inlining for functions of more than > 10 > > > lines > > > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inline_Functions. > > > If this was a header, I could also cite > > > > > > http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... > > > :) > > > > > > On the technical side, inlining should have a justification. One could be > > > performance, supported by an actual benchmark to prove the need for > inlining. > > > Another readability (when the code is short and readable, it might be better > > to > > > inline than to comment on the effect). But neither of those seems to apply > > here. > > > > My reasoning was the following: > > * We are in an anonymous namespace in a CC file, so no danger of much > bloating. > > * The class has 2 methods, so readability should be fine either way. > > * Not performance code. > > So I went and I optimized for less lines. :-) > > > > Can you think of a concrete reason why we should not inline here? > > My concrete reason is boring: The style guide, permissive in general, is quite > decisive in this particular point, so we should stick to it. > > The readability argument is always subjective. Personally, I have troubles to > even count the number of methods of a class with inlined methods like this, so > having just 2 of them is not making a lot of change for me. I guess that's what > the style guide for: choosing one option if more can be subjectively argued for. Roger that. I have refactored it. https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/import/csv_reader.h (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/import/csv_reader.h:26: class CSVReader { On 2014/11/06 16:16:02, vabr (OOO until 9 Nov) wrote: > On 2014/11/06 14:55:39, engedy wrote: > > On 2014/09/25 14:44:27, vabr (Chromium) wrote: > > > Why a class and not just a helper function? > > > > Same reason as for CSVWriter: for consistency with JSONReader. > > Again, JSONReader actually uses the non-static parts of itself, whereas > CSVWriter does not. I vote against making it a static-only class. Please see my response at CSVWriter. https://codereview.chromium.org/447763002/diff/160001/components/password_man... File components/password_manager/core/browser/export/csv_writer.cc (right): https://codereview.chromium.org/447763002/diff/160001/components/password_man... components/password_manager/core/browser/export/csv_writer.cc:68: typedef std::vector<ColumnNameToValueMap>::const_iterator ConstRecordIterator; On 2014/11/06 16:16:02, vabr (OOO until 9 Nov) wrote: > You can remove this typedef and use auto instead. > (Also on line 58.) > http://chromium-cpp.appspot.com/ Woo-hoo, C++11! It seems I have missed the memo! \o/
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
Since I'm CCed on this and you guys seem at a difference, here's a random opinion, for what it's worth. https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/export/csv_writer.h (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer.h:22: class CSVWriter { On 2014/11/07 16:26:11, engedy wrote: > On 2014/11/06 16:16:02, vabr (OOO until 9 Nov) wrote: > > On 2014/11/06 14:55:39, engedy wrote: > > > On 2014/09/25 14:44:27, vabr (Chromium) wrote: > > > > Why is this a class and not just a helper method? > > > > > > For consistency with JSONWriter. > > > > But the situation is different for JSONWriter -- JSONWriter is a class, > because > > although the interface is just a static method, every call to it is realised > by > > constructing an instance of JSONWriter, which does a non-trivial thing. > > > > Here you only have static methods, therefore I don't think this should be a > > class. > > Yes, this is what I mean by consistency: there is no dire need to make it a > class, but this way it is similar to JSONWriter. > > My arguments for making it a class+static are: > * Having global functions looks weird. > * Having so much comment for a single method looks weird. > * I kinda like that it acts as a namespace, so that the typedef is not hanging > out there as the global. > > I can be convinced to change it to a global if you feel strongly about it. :-) I think it would be better as a global*, assuming you name it WriteCSV() instead of just Write(). *It's not really global, it's namespace-scoped to password_manager, which also makes this more palatable. Also I thought you were going to get rid of the typedef due to being able to use auto?
https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/export/csv_writer.h (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer.h:22: class CSVWriter { On 2014/11/07 18:38:29, Peter Kasting wrote: > On 2014/11/07 16:26:11, engedy wrote: > > On 2014/11/06 16:16:02, vabr (OOO until 9 Nov) wrote: > > > On 2014/11/06 14:55:39, engedy wrote: > > > > On 2014/09/25 14:44:27, vabr (Chromium) wrote: > > > > > Why is this a class and not just a helper method? > > > > > > > > For consistency with JSONWriter. > > > > > > But the situation is different for JSONWriter -- JSONWriter is a class, > > because > > > although the interface is just a static method, every call to it is realised > > by > > > constructing an instance of JSONWriter, which does a non-trivial thing. > > > > > > Here you only have static methods, therefore I don't think this should be a > > > class. > > > > Yes, this is what I mean by consistency: there is no dire need to make it a > > class, but this way it is similar to JSONWriter. > > > > My arguments for making it a class+static are: > > * Having global functions looks weird. > > * Having so much comment for a single method looks weird. > > * I kinda like that it acts as a namespace, so that the typedef is not > hanging > > out there as the global. > > > > I can be convinced to change it to a global if you feel strongly about it. :-) > > I think it would be better as a global*, assuming you name it WriteCSV() instead > of just Write(). > > *It's not really global, it's namespace-scoped to password_manager, which also > makes this more palatable. Also I thought you were going to get rid of the > typedef due to being able to use auto? OK, I have to concede. The typedef cannot be removed, unfortunately, that is needed for the function declaration. :(
https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/export/csv_writer.h (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer.h:22: class CSVWriter { On 2014/11/07 19:39:43, engedy wrote: > On 2014/11/07 18:38:29, Peter Kasting wrote: > > On 2014/11/07 16:26:11, engedy wrote: > > > On 2014/11/06 16:16:02, vabr (OOO until 9 Nov) wrote: > > > > On 2014/11/06 14:55:39, engedy wrote: > > > > > On 2014/09/25 14:44:27, vabr (Chromium) wrote: > > > > > > Why is this a class and not just a helper method? > > > > > > > > > > For consistency with JSONWriter. > > > > > > > > But the situation is different for JSONWriter -- JSONWriter is a class, > > > because > > > > although the interface is just a static method, every call to it is > realised > > > by > > > > constructing an instance of JSONWriter, which does a non-trivial thing. > > > > > > > > Here you only have static methods, therefore I don't think this should be > a > > > > class. > > > > > > Yes, this is what I mean by consistency: there is no dire need to make it a > > > class, but this way it is similar to JSONWriter. > > > > > > My arguments for making it a class+static are: > > > * Having global functions looks weird. > > > * Having so much comment for a single method looks weird. > > > * I kinda like that it acts as a namespace, so that the typedef is not > > hanging > > > out there as the global. > > > > > > I can be convinced to change it to a global if you feel strongly about it. > :-) > > > > I think it would be better as a global*, assuming you name it WriteCSV() > instead > > of just Write(). > > > > *It's not really global, it's namespace-scoped to password_manager, which also > > makes this more palatable. Also I thought you were going to get rid of the > > typedef due to being able to use auto? > > OK, I have to concede. > > The typedef cannot be removed, unfortunately, that is needed for the function > declaration. :( Oh, you can't just write "const std::vector<std::map<std::string, std::string>>&"? It may be ugly enough that the typedef is better, though.
https://codereview.chromium.org/447763002/diff/140001/components/password_man... File components/password_manager/core/browser/export/csv_writer.h (right): https://codereview.chromium.org/447763002/diff/140001/components/password_man... components/password_manager/core/browser/export/csv_writer.h:22: class CSVWriter { On 2014/11/07 21:33:44, Peter Kasting wrote: > On 2014/11/07 19:39:43, engedy wrote: > > On 2014/11/07 18:38:29, Peter Kasting wrote: > > > On 2014/11/07 16:26:11, engedy wrote: > > > > On 2014/11/06 16:16:02, vabr (OOO until 9 Nov) wrote: > > > > > On 2014/11/06 14:55:39, engedy wrote: > > > > > > On 2014/09/25 14:44:27, vabr (Chromium) wrote: > > > > > > > Why is this a class and not just a helper method? > > > > > > > > > > > > For consistency with JSONWriter. > > > > > > > > > > But the situation is different for JSONWriter -- JSONWriter is a class, > > > > because > > > > > although the interface is just a static method, every call to it is > > realised > > > > by > > > > > constructing an instance of JSONWriter, which does a non-trivial thing. > > > > > > > > > > Here you only have static methods, therefore I don't think this should > be > > a > > > > > class. > > > > > > > > Yes, this is what I mean by consistency: there is no dire need to make it > a > > > > class, but this way it is similar to JSONWriter. > > > > > > > > My arguments for making it a class+static are: > > > > * Having global functions looks weird. > > > > * Having so much comment for a single method looks weird. > > > > * I kinda like that it acts as a namespace, so that the typedef is not > > > hanging > > > > out there as the global. > > > > > > > > I can be convinced to change it to a global if you feel strongly about it. > > :-) > > > > > > I think it would be better as a global*, assuming you name it WriteCSV() > > instead > > > of just Write(). > > > > > > *It's not really global, it's namespace-scoped to password_manager, which > also > > > makes this more palatable. Also I thought you were going to get rid of the > > > typedef due to being able to use auto? > > > > OK, I have to concede. > > > > The typedef cannot be removed, unfortunately, that is needed for the function > > declaration. :( > > Oh, you can't just write "const std::vector<std::map<std::string, > std::string>>&"? > > It may be ugly enough that the typedef is better, though. Done. It turned out not so ugly after all.
LGTM with two easy comments. Thanks for the changes, Balázs. I also like encapsulating the CSV formatter in a class, the code is much more readable now. Cheers, Vaclav https://codereview.chromium.org/447763002/diff/220001/components/password_man... File components/password_manager/core/browser/export/csv_writer.cc (right): https://codereview.chromium.org/447763002/diff/220001/components/password_man... components/password_manager/core/browser/export/csv_writer.cc:16: CSVFormatter(std::string* output) explicit https://codereview.chromium.org/447763002/diff/220001/components/password_man... File components/password_manager/core/browser/export/csv_writer_unittest.cc (right): https://codereview.chromium.org/447763002/diff/220001/components/password_man... components/password_manager/core/browser/export/csv_writer_unittest.cc:21: virtual void SetUp() override { Bumping my comment about dropping "virtual": https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/VTNZzizN0zo
https://codereview.chromium.org/447763002/diff/220001/components/password_man... File components/password_manager/core/browser/export/csv_writer.cc (right): https://codereview.chromium.org/447763002/diff/220001/components/password_man... components/password_manager/core/browser/export/csv_writer.cc:16: CSVFormatter(std::string* output) On 2014/11/11 08:59:49, vabr (Chromium) wrote: > explicit After all this time... Done. https://codereview.chromium.org/447763002/diff/220001/components/password_man... File components/password_manager/core/browser/export/csv_writer_unittest.cc (right): https://codereview.chromium.org/447763002/diff/220001/components/password_man... components/password_manager/core/browser/export/csv_writer_unittest.cc:21: virtual void SetUp() override { On 2014/11/11 08:59:49, vabr (Chromium) wrote: > Bumping my comment about dropping "virtual": > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/VTNZzizN0zo Oh yes, I started to read the discussion the last time too, and then forgot to actually fix the code. Done now.
engedy@chromium.org changed reviewers: + cpu@chromium.org
@Carlos: Could you please review usage of third_party/re2/ for components/password_manager/DEPS?
Still LGTM, thanks! Vaclav
lgtm
The CQ bit was checked by engedy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/447763002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by engedy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/447763002/300001
Message was sent while issue was closed.
Committed patchset #15 (id:300001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/972fd742e9c996e5300728b3ea797c66a05782e8 Cr-Commit-Position: refs/heads/master@{#311271} |
