Extract BookmarksFileImporter from Firefox2Importer.

chrome/browser/importer/bookmarks_file_importer.h

+// Copyright (c) 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 CHROME_BROWSER_IMPORTER_BOOKMARKS_FILE_IMPORTER_H_
+#define CHROME_BROWSER_IMPORTER_BOOKMARKS_FILE_IMPORTER_H_
+
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/callback_forward.h"
+#include "base/compiler_specific.h"
+#include "base/gtest_prod_util.h"
+#include "chrome/browser/importer/importer.h"
+#include "chrome/browser/importer/profile_writer.h"

[gab, 2013/05/07 17:47:54]

This include is only for ProfileWriter::BookmarkEntry right? You could forward
declare it like history::ImportedFaviconUsage?

I don't feel strongly though, this would also imply you have to explicitly
include this in the cc file (like history_types.h is being explicitly included
there now), do we have a rule for when to forward declare and when to simply
include in the header? Strictly speaking mostly everything (i.e. even string16)
can be forward declared in headers, but that feels like it adds a lot of boiler
plate to the header...

[Avi (use Gerrit), 2013/05/07 18:29:39]

We forward-declare whenever possible.

history:: is a namespace; ProfileWriter is a class. You can't forward-declare a
nested class.

+
+class GURL;
+
+namespace base {
+class FilePath;
+}
+
+namespace history {
+struct ImportedFaviconUsage;
+}
+
+// Importer for bookmarks files.
+class BookmarksFileImporter : public Importer {
+ public:
+  BookmarksFileImporter();
+
+  // Importer:
+  virtual void StartImport(const importer::SourceProfile& source_profile,

[gab, 2013/05/07 17:47:54]

#include "chrome/browser/importer/importer_data_types.h"
for this

+                           uint16 items,
+                           ImporterBridge* bridge) OVERRIDE;

[gab, 2013/05/07 17:47:54]

#include "chrome/browser/importer/importer_bridge.h"
for this

+
+  // Imports the bookmarks from the specified file. |favicons| may be null, in
+  // which case favicons are not parsed. |cancellation_callback| is polled to
+  // query if the import should be cancelled; if it returns |true| at any time
+  // the import will be cancelled. If |cancellation_callback| is NULL the
+  // import will run to completion.
+  static void ImportBookmarksFile(

[gab, 2013/05/07 17:47:54]

Seems the only reason this is static is to be used directly in tests.

I think this should not be static and be moved to the private section to only be
invoked by StartImport (and would then also no longer need to take a
|cancellation_callback|).

The tests could then test the private method directly (they are already
friends...) or they could be made into browser_tests and use the regular import
flow which is used to test all other importers (except for some unit_tests
directly testing the parsing methods) -- e.g., ie_importer_browsertest_win.cc
and firefox_importer_browsertest.cc.

[Avi (use Gerrit), 2013/05/07 18:29:39]

A future CL that makes the FF3 importer not depend on the FF2 importer also will
end up calling this. https://codereview.chromium.org/15039004
I'd rather keep tests as unit tests rather than browser tests. The question that
I have is how the FF3 importer should end up looking.

[gab, 2013/05/08 16:24:34]

Sounds like a case for a common virtual parent class.

i.e.

class Foo : public Importer {
  ... (keep StartImport() virtual)
 protected:
  void ImportBookmarksFromFile(
      const base::FilePath& file_path,
      std::vector<ProfileWriter::BookmarkEntry>* bookmarks,
      std::vector<history::ImportedFaviconUsage>* favicons);
};

And then both Firefox3Importer and BookmarksFileImporter can subclass Foo.

This would allow ImportBookmarksFromFile() to directly check cancelled().

I'm not sure I like adding this complexity just to avoid the callback though...


I'm just generally not a fan of having a static method in a class which is used
by other classes (other than factory methods to create instances of the same
class, etc.), in particular in this case since it involves a method of the class
itself passing a callback to one of its static methods to check something the
static method could just check itself was it not static...

Perhaps simply keep it as is, but pull the whole method out of this class into
bookmarks_importer_utils.h or something and put it in the importer namespace?

[Avi (use Gerrit), 2013/05/08 18:30:27]

That points to something that tfarina asked, where there's a dependency here
that he wanted to eliminate, one where bookmarks code depended on this code in
the importer.

If I pull this into a function in the utils class, then it might be easier to
move it to the bookmarks code and make the importer code depend on the bookmarks
code. I was thinking about doing that now, but that's more rewriting than I'd
like for one CL. This has dependencies on Profiles and history, etc, so I'd like
an intermediate stage.

+      const base::FilePath& file_path,
+      base::Callback<bool(void)>* cancellation_callback,
+      std::vector<ProfileWriter::BookmarkEntry>* bookmarks,
+      std::vector<history::ImportedFaviconUsage>* favicons);
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(BookmarksFileImporterTest, ParseTests);
+
+  virtual ~BookmarksFileImporter();
+
+  // Helper methods exposed for testing.
+
+  // The file format that BookmarksFileImporter parses starts with a heading
+  // tag, which contains its title. All bookmarks and sub-folders follow,
+  // bracketed by a <DL> tag:
+  //   <DT><H3 PERSONAL_TOOLBAR_FOLDER="true" ...>title</H3>
+  //   <DL><p>
+  //      ... container ...
+  //   </DL><p>
+  // And a bookmark is presented by a <A> tag:
+  //   <DT><A HREF="url" SHORTCUTURL="shortcut" ADD_DATE="11213014"...>name</A>
+  // Reference: http://kb.mozillazine.org/Bookmarks.html
+
+  static bool ParseCharsetFromLine(const std::string& line,
+                                   std::string* charset);

[gab, 2013/05/07 17:47:54]

#include <string>
for this

+  static bool ParseFolderNameFromLine(const std::string& line,
+                                      const std::string& charset,
+                                      string16* folder_name,

[tfarina, 2013/05/07 16:08:15]

base::string16

[Avi (use Gerrit), 2013/05/07 16:46:13]

Good point; will update those in the next patch.

[gab, 2013/05/07 17:47:54]

#include "base/string16.h"
for this

+                                      bool* is_toolbar_folder,
+                                      base::Time* add_date);
+  // See above, this will also put the data: URL of the favicon into |*favicon|
+  // if there is a favicon given. |post_data| is set for POST base keywords to
+  // the contents of the actual POST (with %s for the search term).
+  static bool ParseBookmarkFromLine(const std::string& line,
+                                    const std::string& charset,
+                                    string16* title,
+                                    GURL* url,
+                                    GURL* favicon,
+                                    string16* shortcut,
+                                    base::Time* add_date,

[gab, 2013/05/07 17:47:54]

#include "base/time.h"
for this

+                                    string16* post_data);
+  // Save bookmarks imported from browsers with Firefox 2 compatible bookmark
+  // systems such as Epiphany. This bookmark format is the same as that of the
+  // basic Firefox 2 bookmark, but it misses additional properties and uses
+  // lower-case tag:
+  //   ...<h1>Bookmarks</h1><dl>
+  //   <dt><a href="url">name</a></dt>
+  //   <dt><a href="url">name</a></dt>
+  //   </dl>
+  static bool ParseMinimumBookmarkFromLine(const std::string& line,
+                                           const std::string& charset,
+                                           string16* title,
+                                           GURL* url);
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarksFileImporter);
+};
+
+#endif  // CHROME_BROWSER_IMPORTER_BOOKMARKS_FILE_IMPORTER_H_