Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16982004:
Move Firefox importer's INI parser to c/browser/common. (Closed)
|
Created:
7 years, 6 months ago by tommycli Modified:
7 years, 6 months ago CC:
vandebo (ex-Chrome), chromium-reviews, tfarina, browser-components-watch_chromium.org, brettw Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMove Firefox importer's INI parser to c/browser/common.
Media Galleries API Picasa Import will require reading some INI files. Rather than re-invent the wheel and duplicate code, we are moving the INI parser used by Firefox import into chrome/browser/common.
BUG=151701
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207111
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 2
Patch Set 3 : Added a unit test. #Patch Set 4 : Add unit test missing file. #
Total comments: 6
Patch Set 5 : Move to base/, namespace base, and exported. #
Total comments: 38
Patch Set 6 : #Patch Set 7 : #
Total comments: 4
Patch Set 8 : #Patch Set 9 : Add multiple calls to Parse unit test. #
Total comments: 2
Patch Set 10 : #Patch Set 11 : #Patch Set 12 : Forbid multiple invocations. #Patch Set 13 : #Patch Set 14 : Fix build on compilers that don't support uniform initializers. #Patch Set 15 : Change test case to POD #Patch Set 16 : Remove extended initializer list for linux_chromeos build. #
Messages
Total messages: 38 (0 generated)
thestig: Need OWNER stamp. gab: Need review on chrome/browser/importer changes. erikwright: Need review on chrome/browser/common changes.
https://codereview.chromium.org/16982004/diff/1/chrome/browser/importer/firef... File chrome/browser/importer/firefox_importer_utils.cc (left): https://codereview.chromium.org/16982004/diff/1/chrome/browser/importer/firef... chrome/browser/importer/firefox_importer_utils.cc:133: void ParseProfileINI(const base::FilePath& file, DictionaryValue* root) { Don't you also need to delete this code since it's being moved?
Could you add unit-test coverage for this code? I realize it's not new, but one of the advantages of extracting it is increased testability. Also, since you are now going to use it on new inputs there's a chance that it covers a subset of INI files which happens to be everything used by FF but not by the files you are parsing. Adding unit tests now will be a good foundation in case you need to extend/adapt the functionality.
On 2013/06/13 22:36:08, erikwright wrote: > Could you add unit-test coverage for this code? I realize it's not new, but one > of the advantages of extracting it is increased testability. Also, since you are > now going to use it on new inputs there's a chance that it covers a subset of > INI files which happens to be everything used by FF but not by the files you are > parsing. Adding unit tests now will be a good foundation in case you need to > extend/adapt the functionality. Sounds good. Will do.
https://codereview.chromium.org/16982004/diff/1/chrome/browser/importer/firef... File chrome/browser/importer/firefox_importer_utils.cc (left): https://codereview.chromium.org/16982004/diff/1/chrome/browser/importer/firef... chrome/browser/importer/firefox_importer_utils.cc:133: void ParseProfileINI(const base::FilePath& file, DictionaryValue* root) { On 2013/06/13 21:04:30, Lei Zhang wrote: > Don't you also need to delete this code since it's being moved? That's correct. I thought I deleted it, but I must have forgotten or pressed Ctrl-Z. Done.
This looks good sans the unit test. https://codereview.chromium.org/16982004/diff/8001/chrome/browser/common/ini_... File chrome/browser/common/ini_parser.h (right): https://codereview.chromium.org/16982004/diff/8001/chrome/browser/common/ini_... chrome/browser/common/ini_parser.h:38: DictionaryValue root_; base::
I checked into the story with this directory. It was part of an early vision for browser components but has since been abandoned. The only remaining files here were supposed to move but the person responsible stopped contributing to Chrome. So you will need to find a different location. At the moment my best suggestion is to put it in base/. I don't know how brettw@ will feel about that though. We kind of lack a good place for utility code used by more than one component/feature.
On 2013/06/14 01:12:29, erikwright wrote: > I checked into the story with this directory. It was part of an early vision for > browser components but has since been abandoned. The only remaining files here > were supposed to move but the person responsible stopped contributing to Chrome. Huh, ya, sometimes people leave and you don't even notice. Presumably this is someone you have worked with. If they have stopped contributing altogether then maybe they shouldn't be in OWNERS files. > So you will need to find a different location. > > At the moment my best suggestion is to put it in base/. I don't know how brettw@ > will feel about that though. We kind of lack a good place for utility code used > by more than one component/feature. We have a JSON parser in base. I'll CC him and see what he thinks.
+brettw ^^^
Hi guys, I added the unit test. I'm cool with it being in base/ - though I agree with you that it would be nice to have a place to put utility code just used by browser components and not bloat base/ too much. Do you know the reason why c/browser/common was abandoned? We could just revive it...
On 2013/06/14 16:03:16, tommycli wrote: > Hi guys, I added the unit test. > > I'm cool with it being in base/ - though I agree with you that it would be nice > to have a place to put utility code just used by browser components and not > bloat base/ too much. > > Do you know the reason why c/browser/common was abandoned? We could just revive > it... Components now live in src/components/<xxx> and can't depend on chrome/.. or content/.. Code of this size, I think it's just as well for it to be in base/ for now. It would be interesting to have a src/utilities or something that would have lots of very small targets for standalone utilities used by two or more clients, but unless there is pain in base/ there's no real good reason to do that.
LGTM minus the location. https://codereview.chromium.org/16982004/diff/19001/chrome/browser/common/ini... File chrome/browser/common/ini_parser.cc (right): https://codereview.chromium.org/16982004/diff/19001/chrome/browser/common/ini... chrome/browser/common/ini_parser.cc:19: std::string current_section; This code probably predates StringPiece support in StringTokenizer. You could avoid a fair bit of the string copying by using StringPiece instead of String up until the call to HandlePair. But that's not required. Only if you enjoy that sort of thing. https://codereview.chromium.org/16982004/diff/19001/chrome/browser/importer/f... File chrome/browser/importer/firefox_importer_utils.cc (right): https://codereview.chromium.org/16982004/diff/19001/chrome/browser/importer/f... chrome/browser/importer/firefox_importer_utils.cc:15: #include "base/strings/string_util.h" Just verify that you still need string_util.h and string_split.h here.
https://codereview.chromium.org/16982004/diff/19001/chrome/browser/common/ini... File chrome/browser/common/ini_parser.h (right): https://codereview.chromium.org/16982004/diff/19001/chrome/browser/common/ini... chrome/browser/common/ini_parser.h:19: // Pass by value because we will be sanitizing the string before parsing. This comment doesn't match the code. If you want to modify a string argument, just make a in the function instead of making that part of the interface.
Hi guys - I moved this to base/. Who would be my best bet as a reviewer for a speedy trial? https://codereview.chromium.org/16982004/diff/19001/chrome/browser/common/ini... File chrome/browser/common/ini_parser.cc (right): https://codereview.chromium.org/16982004/diff/19001/chrome/browser/common/ini... chrome/browser/common/ini_parser.cc:19: std::string current_section; On 2013/06/14 19:26:46, erikwright wrote: > This code probably predates StringPiece support in StringTokenizer. > > You could avoid a fair bit of the string copying by using StringPiece instead of > String up until the call to HandlePair. > > But that's not required. Only if you enjoy that sort of thing. That's a good suggestion. I will try to get this committed as a simple move first, then optimize/modify after. https://codereview.chromium.org/16982004/diff/19001/chrome/browser/common/ini... File chrome/browser/common/ini_parser.h (right): https://codereview.chromium.org/16982004/diff/19001/chrome/browser/common/ini... chrome/browser/common/ini_parser.h:19: // Pass by value because we will be sanitizing the string before parsing. On 2013/06/17 15:00:48, vandebo wrote: > This comment doesn't match the code. If you want to modify a string argument, > just make a in the function instead of making that part of the interface. Done. Out of date comment. https://codereview.chromium.org/16982004/diff/19001/chrome/browser/importer/f... File chrome/browser/importer/firefox_importer_utils.cc (right): https://codereview.chromium.org/16982004/diff/19001/chrome/browser/importer/f... chrome/browser/importer/firefox_importer_utils.cc:15: #include "base/strings/string_util.h" On 2013/06/14 19:26:46, erikwright wrote: > Just verify that you still need string_util.h and string_split.h here. Done.
mark: Need OWNER review on new inclusion into base/. Thanks!
Ugh, Picasa really uses this format? https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.cc File base/ini_parser.cc (right): https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.cc#newcode11 base/ini_parser.cc:11: INIParser::INIParser() {} Why {} here but on separate lines for the destructor and for the other class? Be consistent. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.cc#newcode37 base/ini_parser.cc:37: current_section.erase(end); This just silently throws away any data after the ']'? https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.cc#newcode61 base/ini_parser.cc:61: // Those sections and keys break DictionaryValue's path format, I don’t believe this comment. There’s a whole suite of *WithoutPathExpansion functions that don’t treat dots specially. See base/values.h. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h File base/ini_parser.h (right): https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode15 base/ini_parser.h:15: // A simple class to parse INI files in a string. Users should inherit from this Is there a textual reference to the ini format somewhere? A link or comment explaining the format should be considered a requirement for any parser. There may be ambiguities in the implementation or in a file that a textual specification may help resolve. Since ini is not a generally well-defined format for which everyone agrees upon a single specification, it would be useful to know which syntax this class implements. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode23 base/ini_parser.h:23: private: Blank line before this one to break up the sections. Same on line 36. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode24 base/ini_parser.h:24: virtual void HandlePair(const std::string& section, const std::string& key, This method is misnamed. It’s not really handling a pair, it’s handling a triplet. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode27 base/ini_parser.h:27: DISALLOW_COPY_AND_ASSIGN(INIParser); Include What You Use: #include "base/basictypes.h" to use this macro. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser_unittest.cc File base/ini_parser_unittest.cc (right): https://codereview.chromium.org/16982004/diff/29001/base/ini_parser_unittest.... base/ini_parser_unittest.cc:56: "key3=value3\n" Also test that the right thing happens when there is a \r but no \n. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser_unittest.... base/ini_parser_unittest.cc:60: "key6=value=with=equals\n"); Also test that the right thing happens when the last line doesn’t have an \r or \n at the end of it. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser_unittest.... base/ini_parser_unittest.cc:86: ";Comment2\n" You supported # as a comment character, test that too.
Thanks for the review mark. I've tried to address your comments. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.cc File base/ini_parser.cc (right): https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.cc#newcode11 base/ini_parser.cc:11: INIParser::INIParser() {} On 2013/06/17 18:52:37, Mark Mentovai wrote: > Why {} here but on separate lines for the destructor and for the other class? Be > consistent. Done. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.cc#newcode37 base/ini_parser.cc:37: current_section.erase(end); On 2013/06/17 18:52:37, Mark Mentovai wrote: > This just silently throws away any data after the ']'? Yes. Noted in comment in header. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.cc#newcode61 base/ini_parser.cc:61: // Those sections and keys break DictionaryValue's path format, On 2013/06/17 18:52:37, Mark Mentovai wrote: > I don’t believe this comment. There’s a whole suite of *WithoutPathExpansion > functions that don’t treat dots specially. See base/values.h. I'm not certain why it was implemented this way in the Firefox importer. I believe they wanted a "section.key" path for the DictionaryValue, in which case calling the *WithoutPathExpansion methods wouldn't work anymore. I added an explanatory comment in the header file. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h File base/ini_parser.h (right): https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode15 base/ini_parser.h:15: // A simple class to parse INI files in a string. Users should inherit from this On 2013/06/17 18:52:37, Mark Mentovai wrote: > Is there a textual reference to the ini format somewhere? A link or comment > explaining the format should be considered a requirement for any parser. There > may be ambiguities in the implementation or in a file that a textual > specification may help resolve. > > Since ini is not a generally well-defined format for which everyone agrees upon > a single specification, it would be useful to know which syntax this class > implements. Done. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode23 base/ini_parser.h:23: private: On 2013/06/17 18:52:37, Mark Mentovai wrote: > Blank line before this one to break up the sections. Same on line 36. Done. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode24 base/ini_parser.h:24: virtual void HandlePair(const std::string& section, const std::string& key, On 2013/06/17 18:52:37, Mark Mentovai wrote: > This method is misnamed. It’s not really handling a pair, it’s handling a > triplet. Done. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode27 base/ini_parser.h:27: DISALLOW_COPY_AND_ASSIGN(INIParser); On 2013/06/17 18:52:37, Mark Mentovai wrote: > Include What You Use: #include "base/basictypes.h" to use this macro. Done. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser_unittest.cc File base/ini_parser_unittest.cc (right): https://codereview.chromium.org/16982004/diff/29001/base/ini_parser_unittest.... base/ini_parser_unittest.cc:56: "key3=value3\n" On 2013/06/17 18:52:37, Mark Mentovai wrote: > Also test that the right thing happens when there is a \r but no \n. Done. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser_unittest.... base/ini_parser_unittest.cc:60: "key6=value=with=equals\n"); On 2013/06/17 18:52:37, Mark Mentovai wrote: > Also test that the right thing happens when the last line doesn’t have an \r or > \n at the end of it. Done. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser_unittest.... base/ini_parser_unittest.cc:86: ";Comment2\n" On 2013/06/17 18:52:37, Mark Mentovai wrote: > You supported # as a comment character, test that too. Done.
LGTM in that this is mostly just a move of existing code into its own class in base. I wouldn’t let this fly if it was brand-new code, but for compatibility with the existing use that’s moving, we can take this. https://codereview.chromium.org/16982004/diff/42001/base/ini_parser.cc File base/ini_parser.cc (right): https://codereview.chromium.org/16982004/diff/42001/base/ini_parser.cc#newcode57 base/ini_parser.cc:57: // Checks whether the section and key contain a '.' character. This comment is not entirely correct—say that it breaks the path format when not using the *WithoutPathExpansion methods. https://codereview.chromium.org/16982004/diff/42001/base/ini_parser_unittest.cc File base/ini_parser_unittest.cc (right): https://codereview.chromium.org/16982004/diff/42001/base/ini_parser_unittest.... base/ini_parser_unittest.cc:58: "key4=value4\r" // Testing Mac "\r" line endings. \r is an obsolete (pre-Mac OS X) line ending. It’s confusing to refer to this as a Mac line ending in 2013. Just remove the word Mac.
A few more comments. Sorry, forgot to submit them. Feel free to ignore in light of Mark's LGTM. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h File base/ini_parser.h (right): https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode15 base/ini_parser.h:15: // A simple class to parse INI files in a string. Users should inherit from this Use the descriptive in the opening sentence. // Parses INI files in a string. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode22 base/ini_parser.h:22: void Parse(const std::string& content); Is it correct to call this method more than once? Add a comment that answers this question. If the answer is No, or "it depends on the subclass", I find that there's a bad code smell. I would suggest, instead, a free function 'void ParseIniFile(const std::string& content, IniParser* parser)'. The IniParser class would be pure-virtual with a single method, HandlePair. It could even be a callback instead of an actual class. Even the dictionary version could then be a free function 'DictionaryValue ParseIniFileAsDictionary(const std::string& content)' implemented in terms of ParseIniFile. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode23 base/ini_parser.h:23: private: NL before private: here and below. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode24 base/ini_parser.h:24: virtual void HandlePair(const std::string& section, const std::string& key, nit, styleguide would have each on its own line if at least one needs to be wrapped. (i.e., NL after 'section,' here and below). https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode27 base/ini_parser.h:27: DISALLOW_COPY_AND_ASSIGN(INIParser); Not required for this abstract class. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode30 base/ini_parser.h:30: class BASE_EXPORT DictionaryValueINIParser : public INIParser { Add a class comment. It should, at a minimum, explain the mapping from section/key to dictionary keys. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode30 base/ini_parser.h:30: class BASE_EXPORT DictionaryValueINIParser : public INIParser { Will your new client of this code use INIParser directly, or only DictionaryValueINIParser? In the latter case, don't introduce the unnecessary indirection. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode37 base/ini_parser.h:37: virtual void HandlePair(const std::string& section, const std::string& key, Add a comment: // INIParser implementation.
Please incorporate Erik’s late feedback before checking in.
erik: One question below. https://codereview.chromium.org/16982004/diff/8001/chrome/browser/common/ini_... File chrome/browser/common/ini_parser.h (right): https://codereview.chromium.org/16982004/diff/8001/chrome/browser/common/ini_... chrome/browser/common/ini_parser.h:38: DictionaryValue root_; On 2013/06/13 23:21:17, Lei Zhang wrote: > base:: Done. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h File base/ini_parser.h (right): https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode15 base/ini_parser.h:15: // A simple class to parse INI files in a string. Users should inherit from this On 2013/06/17 20:30:47, erikwright wrote: > Use the descriptive in the opening sentence. > > // Parses INI files in a string. Done. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode22 base/ini_parser.h:22: void Parse(const std::string& content); On 2013/06/17 20:30:47, erikwright wrote: > Is it correct to call this method more than once? Add a comment that answers > this question. > > If the answer is No, or "it depends on the subclass", I find that there's a bad > code smell. > > I would suggest, instead, a free function 'void ParseIniFile(const std::string& > content, IniParser* parser)'. The IniParser class would be pure-virtual with a > single method, HandlePair. It could even be a callback instead of an actual > class. > > Even the dictionary version could then be a free function 'DictionaryValue > ParseIniFileAsDictionary(const std::string& content)' implemented in terms of > ParseIniFile. Yes, it's correct to call multiple times. I added a comment to the effect. Making it a free function would work. What's the benefit you're seeing? https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode23 base/ini_parser.h:23: private: On 2013/06/17 20:30:47, erikwright wrote: > NL before private: here and below. Done. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode24 base/ini_parser.h:24: virtual void HandlePair(const std::string& section, const std::string& key, On 2013/06/17 20:30:47, erikwright wrote: > nit, styleguide would have each on its own line if at least one needs to be > wrapped. (i.e., NL after 'section,' here and below). Done. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode27 base/ini_parser.h:27: DISALLOW_COPY_AND_ASSIGN(INIParser); On 2013/06/17 20:30:47, erikwright wrote: > Not required for this abstract class. Done. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode30 base/ini_parser.h:30: class BASE_EXPORT DictionaryValueINIParser : public INIParser { On 2013/06/17 20:30:47, erikwright wrote: > Add a class comment. It should, at a minimum, explain the mapping from > section/key to dictionary keys. Done. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode30 base/ini_parser.h:30: class BASE_EXPORT DictionaryValueINIParser : public INIParser { On 2013/06/17 20:30:47, erikwright wrote: > Will your new client of this code use INIParser directly, or only > DictionaryValueINIParser? > > In the latter case, don't introduce the unnecessary indirection. Will use INIParser directly. https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode37 base/ini_parser.h:37: virtual void HandlePair(const std::string& section, const std::string& key, On 2013/06/17 20:30:47, erikwright wrote: > Add a comment: > > // INIParser implementation. Done. https://codereview.chromium.org/16982004/diff/42001/base/ini_parser.cc File base/ini_parser.cc (right): https://codereview.chromium.org/16982004/diff/42001/base/ini_parser.cc#newcode57 base/ini_parser.cc:57: // Checks whether the section and key contain a '.' character. On 2013/06/17 19:59:48, Mark Mentovai wrote: > This comment is not entirely correct—say that it breaks the path format when not > using the *WithoutPathExpansion methods. Done. https://codereview.chromium.org/16982004/diff/42001/base/ini_parser_unittest.cc File base/ini_parser_unittest.cc (right): https://codereview.chromium.org/16982004/diff/42001/base/ini_parser_unittest.... base/ini_parser_unittest.cc:58: "key4=value4\r" // Testing Mac "\r" line endings. On 2013/06/17 19:59:48, Mark Mentovai wrote: > \r is an obsolete (pre-Mac OS X) line ending. It’s confusing to refer to this as > a Mac line ending in 2013. Just remove the word Mac. Done.
https://codereview.chromium.org/16982004/diff/46002/base/ini_parser.h File base/ini_parser.h (right): https://codereview.chromium.org/16982004/diff/46002/base/ini_parser.h#newcode36 base/ini_parser.h:36: // May be called multiple times. Sections cannot span calls. In your implementation, root_ isn’t cleared between Parse() calls, but the caller may expecting it to be cleared. Having Parse() result in a root() that has data from a previous Parse() may be unexpected. At the very least, you need to document this. “Sections cannot span calls” does not make this obvious or address this point precisely, and I’m not sure why you’ve stated this because it doesn’t seem to be a restriction that shows up in the implementation. I’d caution you that the more expected and more useful behavior would be to clear root_ and lift the questionable “sections cannot span” limitation while placing a new “existing data is dumped” limitation, OR punt on this edge case by prohibiting Parse() from being called more than once.
Back from vacation, seems this review is quite far along, lgtm for extracting this code from chrome/browser/importer, but I didn't look at where it landed or anything else (I trust erikwright and mark on that). Cheers, Gab
https://codereview.chromium.org/16982004/diff/46002/base/ini_parser.h File base/ini_parser.h (right): https://codereview.chromium.org/16982004/diff/46002/base/ini_parser.h#newcode36 base/ini_parser.h:36: // May be called multiple times. Sections cannot span calls. On 2013/06/17 22:28:24, Mark Mentovai wrote: > In your implementation, root_ isn’t cleared between Parse() calls, but the > caller may expecting it to be cleared. Having Parse() result in a root() that > has data from a previous Parse() may be unexpected. At the very least, you need > to document this. “Sections cannot span calls” does not make this obvious or > address this point precisely, and I’m not sure why you’ve stated this because it > doesn’t seem to be a restriction that shows up in the implementation. > > I’d caution you that the more expected and more useful behavior would be to > clear root_ and lift the questionable “sections cannot span” limitation while > placing a new “existing data is dumped” limitation, OR punt on this edge case by > prohibiting Parse() from being called more than once. I originally hoped to retain state between multiple calls. I can see that it could lead to counter-intuitive behavior. I could force subclasses to reset their state between each call and add a hook, but I think it'd be cleaner to just forbid multiple invocations. It's enforced with a bool flag and a DCHECK.
https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h File base/ini_parser.h (right): https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode22 base/ini_parser.h:22: void Parse(const std::string& content); On 2013/06/17 21:23:25, tommycli wrote: > On 2013/06/17 20:30:47, erikwright wrote: > > Is it correct to call this method more than once? Add a comment that answers > > this question. > > > > If the answer is No, or "it depends on the subclass", I find that there's a > bad > > code smell. > > > > I would suggest, instead, a free function 'void ParseIniFile(const > std::string& > > content, IniParser* parser)'. The IniParser class would be pure-virtual with a > > single method, HandlePair. It could even be a callback instead of an actual > > class. > > > > Even the dictionary version could then be a free function 'DictionaryValue > > ParseIniFileAsDictionary(const std::string& content)' implemented in terms of > > ParseIniFile. > > Yes, it's correct to call multiple times. I added a comment to the effect. > > Making it a free function would work. What's the benefit you're seeing? The benefit is that it's a stateless class. It doesn't really make any sense to be a class. The only purpose of being a class is to allow customization via instance inheritance, which in and of itself is frowned upon in general. Furthermore, by being a class it's confusing because you can only invoke this method once. So you have this DCHECK, extra state, and extra comment just to prevent something that would never be possible with the free-function version.
https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h File base/ini_parser.h (right): https://codereview.chromium.org/16982004/diff/29001/base/ini_parser.h#newcode22 base/ini_parser.h:22: void Parse(const std::string& content); On 2013/06/18 00:56:14, erikwright wrote: > On 2013/06/17 21:23:25, tommycli wrote: > > On 2013/06/17 20:30:47, erikwright wrote: > > > Is it correct to call this method more than once? Add a comment that answers > > > this question. > > > > > > If the answer is No, or "it depends on the subclass", I find that there's a > > bad > > > code smell. > > > > > > I would suggest, instead, a free function 'void ParseIniFile(const > > std::string& > > > content, IniParser* parser)'. The IniParser class would be pure-virtual with > a > > > single method, HandlePair. It could even be a callback instead of an actual > > > class. > > > > > > Even the dictionary version could then be a free function 'DictionaryValue > > > ParseIniFileAsDictionary(const std::string& content)' implemented in terms > of > > > ParseIniFile. > > > > Yes, it's correct to call multiple times. I added a comment to the effect. > > > > Making it a free function would work. What's the benefit you're seeing? > > The benefit is that it's a stateless class. It doesn't really make any sense to > be a class. The only purpose of being a class is to allow customization via > instance inheritance, which in and of itself is frowned upon in general. > > Furthermore, by being a class it's confusing because you can only invoke this > method once. So you have this DCHECK, extra state, and extra comment just to > prevent something that would never be possible with the free-function version. Well in some cases, such as the DictionaryValue builder, the parser needs to store state somewhere to build up the DictionaryValue. If we used a free function in those cases, the caller has to ensure that the same INIParser isn't re-used in multiple calls to the free function Parse. A free function could wrap the process and create a one-time-use parser, but then every parser would require a wrapper function. Or use a template. Am I missing something?
You could wind up with:
typedef void(*HandleTripletFunctionType)(const std::string&,
const std::string&,
const std::string&
void*);
void ParseINIContent(const std::string& content,
HandleTripletFunctionType* handle_triplet,
void* context);
and then
namespace {
void ParseINIContentToDictionaryValue_HandleTriplet(const std::string& section,
const std::string& key,
const std::string& value,
void* context) {
DictionaryValue* root = static_cast<DictionaryValue*>(context);
root.SetString(…);
}
} // namespace
void ParseINIContentToDictionaryValue(const std::string& content,
DictionaryValue* root) {
ParseINIContent(content,
ParseINIContentToDictionaryValue_HandleTriplet,
root);
}
I’m not sure that this is better because the context parameter you’re passing
around is basically the same as a “this” pointer but with the drawbacks of
having to write it by hand and cast it to the correct type.
On 2013/06/18 16:42:16, Mark Mentovai wrote:
> You could wind up with:
>
> typedef void(*HandleTripletFunctionType)(const std::string&,
> const std::string&,
> const std::string&
> void*);
> void ParseINIContent(const std::string& content,
> HandleTripletFunctionType* handle_triplet,
> void* context);
>
> and then
>
> namespace {
>
> void ParseINIContentToDictionaryValue_HandleTriplet(const std::string&
section,
> const std::string& key,
> const std::string& value,
> void* context) {
> DictionaryValue* root = static_cast<DictionaryValue*>(context);
> root.SetString(…);
> }
>
> } // namespace
>
> void ParseINIContentToDictionaryValue(const std::string& content,
> DictionaryValue* root) {
> ParseINIContent(content,
> ParseINIContentToDictionaryValue_HandleTriplet,
> root);
> }
>
> I’m not sure that this is better because the context parameter you’re passing
> around is basically the same as a “this” pointer but with the drawbacks of
> having to write it by hand and cast it to the correct type.
I was thinking more along the lines of:
--- in .h --
class INIParser {
public:
virtual void HandleTriplet(x, y, z) = 0;
};
void ParseINIContent(const std::string& content, INIParser*);
void ParseIniContentToDictionary(const std::string& content, DictionaryValue*
value);
--- in .cc --
namespace {
class DictionaryINIParser : public INIParser {
public:
explicit DictionaryINIParser(DictionaryValue* value) : value_(value) {}
virtual void HandleTriplet(x, y, z) {
dict.do_something...
}
private:
DictionaryValue* value_;
};
} // namespace
void ParseIniContentToDictionary(const std::string& content, DictionaryValue*
value) {
ParseIniValue(content, &DictionaryINIParser(value))
}
On 2013/06/18 17:25:13, erikwright wrote:
> On 2013/06/18 16:42:16, Mark Mentovai wrote:
> > You could wind up with:
> >
> > typedef void(*HandleTripletFunctionType)(const std::string&,
> > const std::string&,
> > const std::string&
> > void*);
> > void ParseINIContent(const std::string& content,
> > HandleTripletFunctionType* handle_triplet,
> > void* context);
> >
> > and then
> >
> > namespace {
> >
> > void ParseINIContentToDictionaryValue_HandleTriplet(const std::string&
> section,
> > const std::string& key,
> > const std::string&
value,
> > void* context) {
> > DictionaryValue* root = static_cast<DictionaryValue*>(context);
> > root.SetString(…);
> > }
> >
> > } // namespace
> >
> > void ParseINIContentToDictionaryValue(const std::string& content,
> > DictionaryValue* root) {
> > ParseINIContent(content,
> > ParseINIContentToDictionaryValue_HandleTriplet,
> > root);
> > }
> >
> > I’m not sure that this is better because the context parameter you’re
passing
> > around is basically the same as a “this” pointer but with the drawbacks of
> > having to write it by hand and cast it to the correct type.
>
> I was thinking more along the lines of:
>
> --- in .h --
>
> class INIParser {
> public:
> virtual void HandleTriplet(x, y, z) = 0;
> };
>
> void ParseINIContent(const std::string& content, INIParser*);
> void ParseIniContentToDictionary(const std::string& content, DictionaryValue*
> value);
>
> --- in .cc --
>
> namespace {
> class DictionaryINIParser : public INIParser {
> public:
> explicit DictionaryINIParser(DictionaryValue* value) : value_(value) {}
>
> virtual void HandleTriplet(x, y, z) {
> dict.do_something...
> }
> private:
> DictionaryValue* value_;
> };
> } // namespace
>
> void ParseIniContentToDictionary(const std::string& content, DictionaryValue*
> value) {
> ParseIniValue(content, &DictionaryINIParser(value))
> }
I'm going to leave as is. Although erikwright's idea is clearer for the
DictionaryValue case, it means the other parsers need to define both a free
function and an anonymous class.
Thanks!
On 2013/06/18 17:33:56, tommycli wrote:
> On 2013/06/18 17:25:13, erikwright wrote:
> > On 2013/06/18 16:42:16, Mark Mentovai wrote:
> > > You could wind up with:
> > >
> > > typedef void(*HandleTripletFunctionType)(const std::string&,
> > > const std::string&,
> > > const std::string&
> > > void*);
> > > void ParseINIContent(const std::string& content,
> > > HandleTripletFunctionType* handle_triplet,
> > > void* context);
> > >
> > > and then
> > >
> > > namespace {
> > >
> > > void ParseINIContentToDictionaryValue_HandleTriplet(const std::string&
> > section,
> > > const std::string&
key,
> > > const std::string&
> value,
> > > void* context) {
> > > DictionaryValue* root = static_cast<DictionaryValue*>(context);
> > > root.SetString(…);
> > > }
> > >
> > > } // namespace
> > >
> > > void ParseINIContentToDictionaryValue(const std::string& content,
> > > DictionaryValue* root) {
> > > ParseINIContent(content,
> > > ParseINIContentToDictionaryValue_HandleTriplet,
> > > root);
> > > }
> > >
> > > I’m not sure that this is better because the context parameter you’re
> passing
> > > around is basically the same as a “this” pointer but with the drawbacks of
> > > having to write it by hand and cast it to the correct type.
> >
> > I was thinking more along the lines of:
> >
> > --- in .h --
> >
> > class INIParser {
> > public:
> > virtual void HandleTriplet(x, y, z) = 0;
> > };
> >
> > void ParseINIContent(const std::string& content, INIParser*);
> > void ParseIniContentToDictionary(const std::string& content,
DictionaryValue*
> > value);
> >
> > --- in .cc --
> >
> > namespace {
> > class DictionaryINIParser : public INIParser {
> > public:
> > explicit DictionaryINIParser(DictionaryValue* value) : value_(value) {}
> >
> > virtual void HandleTriplet(x, y, z) {
> > dict.do_something...
> > }
> > private:
> > DictionaryValue* value_;
> > };
> > } // namespace
> >
> > void ParseIniContentToDictionary(const std::string& content,
DictionaryValue*
> > value) {
> > ParseIniValue(content, &DictionaryINIParser(value))
> > }
>
> I'm going to leave as is. Although erikwright's idea is clearer for the
> DictionaryValue case, it means the other parsers need to define both a free
> function and an anonymous class.
>
> Thanks!
*anonymous namespace class.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommycli@chromium.org/16982004/61001
On 2013/06/18 17:34:35, tommycli wrote:
> On 2013/06/18 17:33:56, tommycli wrote:
> > On 2013/06/18 17:25:13, erikwright wrote:
> > > On 2013/06/18 16:42:16, Mark Mentovai wrote:
> > > > You could wind up with:
> > > >
> > > > typedef void(*HandleTripletFunctionType)(const std::string&,
> > > > const std::string&,
> > > > const std::string&
> > > > void*);
> > > > void ParseINIContent(const std::string& content,
> > > > HandleTripletFunctionType* handle_triplet,
> > > > void* context);
> > > >
> > > > and then
> > > >
> > > > namespace {
> > > >
> > > > void ParseINIContentToDictionaryValue_HandleTriplet(const std::string&
> > > section,
> > > > const std::string&
> key,
> > > > const std::string&
> > value,
> > > > void* context) {
> > > > DictionaryValue* root = static_cast<DictionaryValue*>(context);
> > > > root.SetString(…);
> > > > }
> > > >
> > > > } // namespace
> > > >
> > > > void ParseINIContentToDictionaryValue(const std::string& content,
> > > > DictionaryValue* root) {
> > > > ParseINIContent(content,
> > > > ParseINIContentToDictionaryValue_HandleTriplet,
> > > > root);
> > > > }
> > > >
> > > > I’m not sure that this is better because the context parameter you’re
> > passing
> > > > around is basically the same as a “this” pointer but with the drawbacks
of
> > > > having to write it by hand and cast it to the correct type.
> > >
> > > I was thinking more along the lines of:
> > >
> > > --- in .h --
> > >
> > > class INIParser {
> > > public:
> > > virtual void HandleTriplet(x, y, z) = 0;
> > > };
> > >
> > > void ParseINIContent(const std::string& content, INIParser*);
> > > void ParseIniContentToDictionary(const std::string& content,
> DictionaryValue*
> > > value);
> > >
> > > --- in .cc --
> > >
> > > namespace {
> > > class DictionaryINIParser : public INIParser {
> > > public:
> > > explicit DictionaryINIParser(DictionaryValue* value) : value_(value) {}
> > >
> > > virtual void HandleTriplet(x, y, z) {
> > > dict.do_something...
> > > }
> > > private:
> > > DictionaryValue* value_;
> > > };
> > > } // namespace
> > >
> > > void ParseIniContentToDictionary(const std::string& content,
> DictionaryValue*
> > > value) {
> > > ParseIniValue(content, &DictionaryINIParser(value))
> > > }
> >
> > I'm going to leave as is. Although erikwright's idea is clearer for the
> > DictionaryValue case, it means the other parsers need to define both a free
> > function and an anonymous class.
> >
> > Thanks!
>
> *anonymous namespace class.
SG, up to you and Mark.
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
LGTM. Like I said, I wouldn’t really approve this as-is if it were a new contribution of code, because I think the implementation is a little flaky and now because of the concerns about the API itself. But as a “move,” if this satisfies the needs of the consumers, it’s fine now, and at least the tricky bits have been commented and guarded with DCHECKs.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommycli@chromium.org/16982004/69003
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommycli@chromium.org/16982004/101002
Message was sent while issue was closed.
Change committed as 207111 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
