Move Firefox importer's INI parser to c/browser/common.

base/ini_parser.h

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_INI_PARSER_H_
+#define BASE_INI_PARSER_H_
+
+#include <string>
+
+#include "base/base_export.h"
+#include "base/values.h"
+
+namespace base {
+
+// A simple class to parse INI files in a string. Users should inherit from this

[Mark Mentovai, 2013/06/17 18:52:37]

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.

[tommycli, 2013/06/17 19:49:24]

Done.

[erikwright (departed), 2013/06/17 20:30:47]

Use the descriptive in the opening sentence.

// Parses INI files in a string.

[tommycli, 2013/06/17 21:23:25]

Done.

+// class and override the HandlePair method.
+class BASE_EXPORT INIParser {
+ public:
+  INIParser();
+  virtual ~INIParser();
+
+  void Parse(const std::string& content);

[erikwright (departed), 2013/06/17 20:30:47]

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.

[tommycli, 2013/06/17 21:23:25]

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?

[erikwright (departed), 2013/06/18 00:56:14]

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.

[tommycli, 2013/06/18 16:29:47]

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?

+ private:

[Mark Mentovai, 2013/06/17 18:52:37]

Blank line before this one to break up the sections. Same on line 36.

[tommycli, 2013/06/17 19:49:24]

Done.

[erikwright (departed), 2013/06/17 20:30:47]

NL before private: here and below.

[tommycli, 2013/06/17 21:23:25]

Done.

+  virtual void HandlePair(const std::string& section, const std::string& key,

[Mark Mentovai, 2013/06/17 18:52:37]

This method is misnamed. It’s not really handling a pair, it’s handling a
triplet.

[tommycli, 2013/06/17 19:49:24]

Done.

[erikwright (departed), 2013/06/17 20:30:47]

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).

[tommycli, 2013/06/17 21:23:25]

Done.

+                          const std::string& value) = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(INIParser);

[Mark Mentovai, 2013/06/17 18:52:37]

Include What You Use: #include "base/basictypes.h" to use this macro.

[tommycli, 2013/06/17 19:49:24]

Done.

[erikwright (departed), 2013/06/17 20:30:47]

Not required for this abstract class.

[tommycli, 2013/06/17 21:23:25]

Done.

+};
+
+class BASE_EXPORT DictionaryValueINIParser : public INIParser {

[erikwright (departed), 2013/06/17 20:30:47]

Add a class comment. It should, at a minimum, explain the mapping from
section/key to dictionary keys.

[erikwright (departed), 2013/06/17 20:30:47]

Will your new client of this code use INIParser directly, or only
DictionaryValueINIParser?

In the latter case, don't introduce the unnecessary indirection.

[tommycli, 2013/06/17 21:23:25]

Done.

[tommycli, 2013/06/17 21:23:25]

Will use INIParser directly.

+ public:
+  DictionaryValueINIParser();
+  virtual ~DictionaryValueINIParser();
+
+  const DictionaryValue& root() const { return root_; }
+ private:
+  virtual void HandlePair(const std::string& section, const std::string& key,

[erikwright (departed), 2013/06/17 20:30:47]

Add a comment:

// INIParser implementation.

[tommycli, 2013/06/17 21:23:25]

Done.

+                          const std::string& value) OVERRIDE;
+
+  DictionaryValue root_;
+
+  DISALLOW_COPY_AND_ASSIGN(DictionaryValueINIParser);
+};
+
+}  // namespace base
+
+#endif  // BASE_INI_PARSER_H_