Implement support for importing favorites from Edge on Windows 10.

chrome/utility/importer/edge_importer_win.cc

Index: chrome/utility/importer/edge_importer_win.cc
diff --git a/chrome/utility/importer/edge_importer_win.cc b/chrome/utility/importer/edge_importer_win.cc
new file mode 100644
index 0000000000000000000000000000000000000000..8497035ff9d1a5c1ccda1ea30ffd4586717cf905
--- /dev/null
+++ b/chrome/utility/importer/edge_importer_win.cc
@@ -0,0 +1,557 @@
+// Copyright 2015 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.
+
+#include "chrome/utility/importer/edge_importer_win.h"
+
+#define JET_UNICODE
+#include <esent.h>
+#undef JET_UNICODE
+#include <Shlobj.h>
+
+#include <algorithm>
+#include <map>
+#include <string>
+#include <vector>
+
+#include "base/files/file_enumerator.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/strings/string16.h"
+#include "base/strings/string_util.h"
+#include "base/time/time.h"
+#include "base/win/windows_version.h"
+

[Ilya Sherman, 2015/11/26 02:04:45]

nit: This line break is typically omitted in Chromium code.  (The other line
breaks separating includes are typically kept, though.)

[forshaw, 2015/11/30 12:57:58]

Acknowledged.

+#include "chrome/common/importer/edge_importer_utils_win.h"
+#include "chrome/common/importer/imported_bookmark_entry.h"
+#include "chrome/common/importer/importer_bridge.h"
+#include "chrome/grit/generated_resources.h"
+#include "chrome/utility/importer/favicon_reencode.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "url/gurl.h"
+
+namespace {
+
+// Toolbar favorites are placed under this special folder name.
+const base::char16 kFavoritesBarTitle[] = L"_Favorites_Bar_";
+const base::char16 kSpartanDatabaseFile[] = L"spartan.edb";
+
+class EdgeErrorObject {
+ public:
+  EdgeErrorObject() : last_error_(JET_errSuccess) {}
+
+  base::string16 GetErrorMessage() const {
+    WCHAR error_message[1024] = {};

[Ilya Sherman, 2015/11/26 02:04:45]

nit: Why 1024?

[forshaw, 2015/11/30 12:57:58]

Just an arbitrary size, the API provides no way of determining the error message
size and there's no constant or define which describes a maximum size for this
parameter. 

+    JET_API_PTR err = last_error_;
+    JET_ERR result = JetGetSystemParameter(
+        JET_instanceNil, JET_sesidNil, JET_paramErrorToString, &err,
+        error_message, sizeof(error_message));
+    if (result != JET_errSuccess)
+      return L"";
+
+    return error_message;
+  }
+
+ protected:
+  JET_ERR last_error() const { return last_error_; }
+
+  bool set_last_error(JET_ERR error) {

[Ilya Sherman, 2015/11/26 02:04:44]

Please document what the return value means.  Also, since this is not just a
trivial setter, it's probably appropriate to name it SetLastError.

[forshaw, 2015/11/30 12:57:58]

Acknowledged.

+    last_error_ = error;
+    if (error == JET_errSuccess)
+      return true;
+    return false;

[Ilya Sherman, 2015/11/26 02:04:44]

nit: "return error == JET_errSuccess;"

[forshaw, 2015/11/30 12:57:59]

Acknowledged.

+  }
+
+ private:
+  JET_ERR last_error_;
+
+  DISALLOW_COPY_AND_ASSIGN(EdgeErrorObject);
+};
+
+class EdgeDatabaseTableEnumerator : public EdgeErrorObject {
+ public:
+  EdgeDatabaseTableEnumerator(const base::string16& table_name,
+                              JET_SESID session_id,
+                              JET_TABLEID table_id)
+      : table_id_(table_id), table_name_(table_name), session_id_(session_id) {}
+
+  ~EdgeDatabaseTableEnumerator() {
+    if (table_id_ != JET_tableidNil) {
+      JetCloseTable(session_id_, table_id_);
+      table_id_ = JET_tableidNil;

[Ilya Sherman, 2015/11/26 02:04:44]

Why is this line needed, given that it's in the destructor?

[forshaw, 2015/11/30 12:57:58]

I tend to do this to reduce risk of breakage if it gets moved around at a later
date and accidentally gets released twice. But will remove it.

+    }
+  }
+
+  const base::string16& table_name() { return table_name_; }
+
+  bool Reset() {
+    return set_last_error(JetMove(session_id_, table_id_, JET_MoveFirst, 0));
+  }
+
+  bool Next() {
+    return set_last_error(JetMove(session_id_, table_id_, JET_MoveNext, 0));
+  }
+
+  template <typename T>
+  bool RetrieveColumn(const base::string16& column_name,
+                      T& value,
+                      bool& is_null) {

[Ilya Sherman, 2015/11/26 02:04:44]

Please pass these by pointer rather than by reference.  Passing by non-const
reference is pretty much disallowed by the Chromium style guide.  (Please fix
this throughout.)

[forshaw, 2015/11/30 12:57:58]

Acknowledged.

+    const JET_COLUMNBASE& column_base = GetColumnByName(column_name);
+    if (column_base.cbMax == 0)
+      return false;
+    std::vector<uint8_t> column_data(column_base.cbMax);
+    unsigned long actual_size = 0;
+    JET_ERR err = JetRetrieveColumn(
+        session_id_, table_id_, column_base.columnid, &column_data[0],
+        column_data.size(), &actual_size, 0, nullptr);
+    if (err != JET_errSuccess && err != JET_wrnColumnNull) {
+      set_last_error(err);

[Ilya Sherman, 2015/11/26 02:04:44]

Is it important whether or not set_last_error is ever called for the "success"
cases?  In Reset() and Next() it is; whereas here it isn't.

[forshaw, 2015/11/30 12:57:59]

Not really important, just in most cases there's no need to distinguish between
two "success" codes so a straight call to set_last_error seems reasonable. In
this case as I need to distinguish between the success and the Null column I had
to add a check anyway so didn't seem to make much of a difference. But I'll move
it for consistency anyway.

+      return false;
+    }
+
+    if (err == JET_errSuccess) {
+      column_data.resize(actual_size);
+      if (!ValidateAndConvertValue(column_base.coltyp, column_data, value))
+        return false;
+    } else {
+      value = T();
+    }
+
+    is_null = err == JET_wrnColumnNull;
+    return true;
+  }
+
+  template <typename T>
+  bool RetrieveColumn(const base::string16& column_name, T& value) {

[Ilya Sherman, 2015/11/26 02:04:45]

Chromium style generally speaking frowns upon default parameter values, which is
essentially what this function overload is providing.

[forshaw, 2015/11/30 12:57:58]

Well I'd argue that it isn't providing default parameter values, it's hiding
information being returned that we don't actually care about. This was used when
I thought I'd need to distinguish between Null columns and not Null columns. I
could just remove that functionality entirely now.

+    bool is_null;
+    return RetrieveColumn(column_name, value, is_null);
+  }
+
+ private:
+  template <typename T>
+  bool ValidateAndConvertValueGeneric(const JET_COLTYP match_column_type,
+                                      const JET_COLTYP column_type,
+                                      const std::vector<uint8_t>& column_data,
+                                      T& value) {
+    if ((column_type == match_column_type) &&
+        (column_data.size() >= sizeof(value))) {

[Ilya Sherman, 2015/11/26 02:04:44]

Why is this >= rather than simply =?  Truncating data seems bad.

[forshaw, 2015/11/30 12:57:58]

The database engine is one of the ones where the type enforcement is done by the
calling application not the engine. There's no technical reason that a 4 byte
'int' field couldn't actually be considerably larger. I did this just from a
resilience perspective if Edge devs ever messed up and added too much data to
the column or they overloaded the data to add additional information. Maybe
unlikely but I'd expect that any truncated data is only ever going to be data
which wouldn't matter to the importer. I can change if you feel strongly enough.

+      memcpy(&value, &column_data[0], sizeof(value));
+      return true;
+    }
+    set_last_error(JET_errInvalidColumnType);
+    return false;
+  }
+
+  bool ValidateAndConvertValue(const JET_COLTYP column_type,
+                               const std::vector<uint8_t>& column_data,
+                               bool& value) {
+    if ((column_type == JET_coltypBit) && (column_data.size() >= 1)) {
+      value = (column_data[0] & 1) == 1;
+      return true;
+    }
+    set_last_error(JET_errInvalidColumnType);
+    return false;
+  }
+
+  bool ValidateAndConvertValue(const JET_COLTYP column_type,
+                               const std::vector<uint8_t>& column_data,
+                               base::string16& value) {
+    if ((column_type == JET_coltypLongText) &&
+        ((column_data.size() % sizeof(base::char16)) == 0)) {
+      const base::char16* ptr =
+          reinterpret_cast<const base::char16*>(&column_data[0]);

[Ilya Sherman, 2015/11/26 02:04:44]

I may be wrong, I don't think that this is a safe use of reinterpret_cast.  I
think that you want to memcpy instead.

[forshaw, 2015/11/30 12:57:59]

Reworked

+      size_t length = column_data.size() / sizeof(base::char16);
+      // Remove any trailing NUL characters.
+      while (length > 0) {
+        if (ptr[length - 1])
+          break;
+        length--;
+      }
+      value = base::string16(ptr, length);
+      return true;
+    }
+    set_last_error(JET_errInvalidColumnType);
+    return false;
+  }
+
+  bool ValidateAndConvertValue(const JET_COLTYP column_type,
+                               const std::vector<uint8_t>& column_data,
+                               GUID& value) {
+    return ValidateAndConvertValueGeneric(JET_coltypGUID, column_type,
+                                          column_data, value);
+  }
+
+  bool ValidateAndConvertValue(const JET_COLTYP column_type,
+                               const std::vector<uint8_t>& column_data,
+                               int32_t& value) {
+    return ValidateAndConvertValueGeneric(JET_coltypLong, column_type,
+                                          column_data, value);
+  }
+
+  bool ValidateAndConvertValue(const JET_COLTYP column_type,
+                               const std::vector<uint8_t>& column_data,
+                               int64_t& value) {
+    return ValidateAndConvertValueGeneric(JET_coltypLongLong, column_type,
+                                          column_data, value);
+  }
+
+  bool ValidateAndConvertValue(const JET_COLTYP column_type,
+                               const std::vector<uint8_t>& column_data,
+                               FILETIME& value) {
+    return ValidateAndConvertValueGeneric(JET_coltypLongLong, column_type,
+                                          column_data, value);
+  }
+
+  bool ValidateAndConvertValue(const JET_COLTYP column_type,
+                               const std::vector<uint8_t>& column_data,
+                               uint32_t& value) {
+    return ValidateAndConvertValueGeneric(JET_coltypUnsignedLong, column_type,
+                                          column_data, value);
+  }
+
+  const JET_COLUMNBASE& GetColumnByName(const base::string16& column_name) {
+    auto found_col = columns_by_name_.find(column_name);
+    if (found_col == columns_by_name_.end()) {
+      JET_COLUMNBASE column_base = {};
+      column_base.cbStruct = sizeof(JET_COLUMNBASE);
+      if (!set_last_error(JetGetTableColumnInfo(
+              session_id_, table_id_, column_name.c_str(), &column_base,
+              sizeof(column_base), JET_ColInfoBase))) {
+        // 0 means we'll fail to extract the column data.
+        column_base.cbMax = 0;
+      }
+      columns_by_name_[column_name] = column_base;
+      found_col = columns_by_name_.find(column_name);
+    }
+    return found_col->second;
+  }
+
+  std::map<const base::string16, JET_COLUMNBASE> columns_by_name_;
+  JET_TABLEID table_id_;
+  base::string16 table_name_;
+  JET_SESID session_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(EdgeDatabaseTableEnumerator);
+};
+
+class EdgeDatabaseSession : public EdgeErrorObject {
+ public:
+  EdgeDatabaseSession()
+      : db_id_(JET_dbidNil),
+        instance_id_(JET_instanceNil),
+        session_id_(JET_sesidNil) {}
+
+  ~EdgeDatabaseSession() {
+    // We don't need to collect other ID handles, terminating instance
+    // is enough to shut the entire session down.
+    if (instance_id_ != JET_instanceNil) {
+      JetTerm(instance_id_);
+      instance_id_ = JET_instanceNil;

[Ilya Sherman, 2015/11/26 02:04:44]

nit: Why is this line needed?

[forshaw, 2015/11/30 12:57:59]

As above but removed.

+    }
+  }
+
+  bool OpenDatabase(const base::string16& database_file) {
+    if (IsOpen()) {
+      set_last_error(JET_errOneDatabasePerSession);
+      return false;
+    }
+    if (!set_last_error(JetSetSystemParameter(
+            nullptr, JET_sesidNil, JET_paramDatabasePageSize, 8192, nullptr)))

[Ilya Sherman, 2015/11/26 02:04:44]

Why 8192?  Is that a guaranteed constant, or might it change with new Edge
releases?

[forshaw, 2015/11/30 12:57:58]

In theory it could change but it's unlikely to do so. But I'll push it out as a
constant anyway.

+      return false;
+    if (!set_last_error(JetCreateInstance(&instance_id_, L"EdgeDataImporter")))
+      return false;
+    if (!set_last_error(JetSetSystemParameter(&instance_id_, JET_sesidNil,
+                                              JET_paramRecovery, 0, L"Off")))
+      return false;
+    if (!set_last_error(JetInit(&instance_id_)))
+      return false;
+    if (!set_last_error(
+            JetBeginSession(instance_id_, &session_id_, nullptr, nullptr)))
+      return false;
+    if (!set_last_error(JetAttachDatabase2(session_id_, database_file.c_str(),
+                                           0, JET_bitDbReadOnly)))
+      return false;
+    if (!set_last_error(JetOpenDatabase(session_id_, database_file.c_str(),
+                                        nullptr, &db_id_, JET_bitDbReadOnly)))
+      return false;
+    return true;
+  }
+
+  scoped_ptr<EdgeDatabaseTableEnumerator> OpenTableEnumerator(
+      const base::string16& table_name) {
+    JET_TABLEID table_id;
+
+    if (!IsOpen()) {
+      set_last_error(JET_errDatabaseNotFound);
+      return nullptr;
+    }
+
+    if (!set_last_error(JetOpenTable(session_id_, db_id_, table_name.c_str(),
+                                     nullptr, 0, JET_bitTableReadOnly,
+                                     &table_id)))
+      return nullptr;
+
+    return scoped_ptr<EdgeDatabaseTableEnumerator>(

[Ilya Sherman, 2015/11/26 02:04:44]

nit: You can use make_scoped_ptr here.

[forshaw, 2015/11/30 12:57:58]

Acknowledged.

+        new EdgeDatabaseTableEnumerator(table_name, session_id_, table_id));
+  }
+
+ private:
+  bool IsOpen() { return instance_id_ != JET_instanceNil; }
+
+  JET_DBID db_id_;
+  JET_INSTANCE instance_id_;
+  JET_SESID session_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(EdgeDatabaseSession);
+};
+
+struct EdgeFavoriteEntry {
+  EdgeFavoriteEntry()
+      : is_folder(false),
+        order_number(0),
+        item_id(GUID_NULL),
+        parent_id(GUID_NULL) {}
+
+  base::string16 title;
+  GURL url;
+  base::FilePath favicon_file;
+  bool is_folder;
+  int64_t order_number;
+  base::Time date_updated;
+  GUID item_id;
+  GUID parent_id;
+
+  std::vector<EdgeFavoriteEntry*> children;

[Ilya Sherman, 2015/11/26 02:04:44]

nit: Can the items be const?

[forshaw, 2015/11/30 12:57:58]

I've moved the sorting of the children out of the recursive loop so that this
can now be const.

+
+  ImportedBookmarkEntry ToBookmarkEntry(
+      bool in_toolbar,
+      const std::vector<base::string16>& path) {
+    ImportedBookmarkEntry entry;
+    entry.in_toolbar = in_toolbar;
+    entry.is_folder = is_folder;
+    entry.url = url;
+    entry.path = path;
+    entry.title = title;
+    entry.creation_time = date_updated;
+    return entry;
+  }
+};
+
+struct EdgeFavoriteEntryComparator {
+  bool operator()(const EdgeFavoriteEntry* lhs,
+                  const EdgeFavoriteEntry* rhs) const {
+    if (lhs->order_number == rhs->order_number)
+      return lhs->title < rhs->title;
+    else
+      return lhs->order_number < rhs->order_number;

[Ilya Sherman, 2015/11/26 02:04:44]

nit: Please implement this using std::tie: https://chromium-cpp.appspot.com/

[forshaw, 2015/11/30 12:57:59]

Acknowledged.

+  }
+};
+
+// The name of the database file is spartan.edb, however it isn't clear how
+// the intermediate path between the DataStore and the database is generated.
+// Therefore we just do a simple recursive search until we find a matching name.
+base::FilePath FindSpartanDatabase(base::FilePath profile_path) {

[Ilya Sherman, 2015/11/26 02:04:44]

nit: Pass by const-ref?

[forshaw, 2015/11/30 12:57:58]

Acknowledged.

+  base::FilePath data_path =
+      profile_path.empty() ? importer::GetEdgeDataFilePath() : profile_path;

[Ilya Sherman, 2015/11/26 02:04:44]

Are both of these cases possible?  I'd kind of expect that profile_path is
either always empty or never empty -- no?

[forshaw, 2015/11/30 12:57:58]

GetEdgeDataFilePath could also return an empty path so the second check guards
again that function failing. The logic is supposed to be if the passed profile
path has overridden the location use that. If not try and get the "real" profile
path but verify if that fails.

+  if (data_path.empty())
+    return base::FilePath();
+
+  base::FileEnumerator enumerator(data_path.Append(L"DataStore\\Data"), true,
+                                  base::FileEnumerator::FILES);
+  base::FilePath path = enumerator.Next();
+  while (!path.empty()) {
+    if (base::EqualsCaseInsensitiveASCII(path.BaseName().value(),
+                                         kSpartanDatabaseFile))
+      return path;
+    path = enumerator.Next();
+  }
+  return base::FilePath();
+}
+
+struct GuidComparator {
+  bool operator()(const GUID& a, const GUID& b) const {
+    return memcmp(&a, &b, sizeof(a)) < 0;
+  }
+};
+
+bool ReadFaviconData(const base::FilePath& file,
+                     std::vector<unsigned char>* data) {
+  std::string image_data;
+  if (!base::ReadFileToString(file, &image_data))
+    return false;
+
+  const unsigned char* ptr =
+      reinterpret_cast<const unsigned char*>(image_data.c_str());
+  return importer::ReencodeFavicon(ptr, image_data.size(), data);
+}
+
+void BuildBookmarkEntries(EdgeFavoriteEntry* current_entry,
+                          std::vector<ImportedBookmarkEntry>* bookmarks,
+                          favicon_base::FaviconUsageDataList* favicons,

[Ilya Sherman, 2015/11/26 02:04:44]

nit: Outparams are usually written after the in-params.

[forshaw, 2015/11/30 12:57:58]

Acknowledged.

+                          bool is_toolbar,
+                          std::vector<base::string16>& path) {
+  // Sort the children first by order number and then by title.
+  std::sort(current_entry->children.begin(), current_entry->children.end(),
+            EdgeFavoriteEntryComparator());
+  for (EdgeFavoriteEntry* entry : current_entry->children) {
+    if (entry->is_folder) {
+      // If the favorites bar then load all children as toolbar items.
+      if (base::EqualsCaseInsensitiveASCII(entry->title, kFavoritesBarTitle)) {
+        // Replace name with Links similar to IE.
+        path.push_back(L"Links");
+        BuildBookmarkEntries(entry, bookmarks, favicons, true, path);
+        path.pop_back();
+      } else {
+        path.push_back(entry->title);
+        BuildBookmarkEntries(entry, bookmarks, favicons, is_toolbar, path);
+        path.pop_back();
+      }
+    } else {
+      bookmarks->push_back(entry->ToBookmarkEntry(is_toolbar, path));
+      favicon_base::FaviconUsageData favicon;
+      if (entry->url.is_valid() && !entry->favicon_file.empty() &&
+          ReadFaviconData(entry->favicon_file, &favicon.png_data)) {
+        // As the database doesn't provide us a favicon URL we'll fake one.
+        GURL::Replacements path_replace;
+        path_replace.SetPathStr("/favicon.ico");
+        favicon.favicon_url =
+            entry->url.GetWithEmptyPath().ReplaceComponents(path_replace);
+        favicon.urls.insert(entry->url);
+        favicons->push_back(favicon);
+      }
+    }
+  }
+}
+
+}  // namespace
+
+EdgeImporter::EdgeImporter() {}
+
+void EdgeImporter::StartImport(const importer::SourceProfile& source_profile,
+                               uint16 items,
+                               ImporterBridge* bridge) {
+  bridge_ = bridge;
+  bridge_->NotifyStarted();
+  source_path_ = source_profile.source_path;
+
+  if ((items & importer::FAVORITES) && !cancelled()) {
+    bridge_->NotifyItemStarted(importer::FAVORITES);
+    ImportFavorites();
+    bridge_->NotifyItemEnded(importer::FAVORITES);
+  }
+  bridge_->NotifyEnded();
+}
+
+EdgeImporter::~EdgeImporter() {}
+
+void EdgeImporter::ImportFavorites() {
+  BookmarkVector bookmarks;
+  favicon_base::FaviconUsageDataList favicons;
+  ParseFavoritesDatabase(&bookmarks, &favicons);
+
+  if (!bookmarks.empty() && !cancelled()) {
+    const base::string16& first_folder_name =
+        l10n_util::GetStringUTF16(IDS_BOOKMARK_GROUP_FROM_EDGE);
+    bridge_->AddBookmarks(bookmarks, first_folder_name);
+  }
+  if (!favicons.empty() && !cancelled())
+    bridge_->SetFavicons(favicons);
+}
+
+// From Edge 13 (released with Windows 10 TH2) Favorites are stored in a JET

[Ilya Sherman, 2015/11/26 02:04:44]

nit: Please add a comma after the closing paren.

[forshaw, 2015/11/30 12:57:58]

Acknowledged.

+// database within the Edge local storage. The import uses the ESE library to
+// open and read the data file. The data is stored in a Favorites table with
+// the following schema.
+// Column Name          Column Type
+// ------------------------------------------
+// DateUpdated          LongLong - FILETIME
+// FaviconFile          LongText - Relative path
+// HashedUrl            ULong
+// IsDeleted            Bit
+// IsFolder             Bit
+// ItemId               Guid
+// OrderNumber          LongLong
+// ParentId             Guid
+// RoamDisabled         Bit
+// RowId                Long
+// Title                LongText
+// URL                  LongText
+void EdgeImporter::ParseFavoritesDatabase(
+    BookmarkVector* bookmarks,
+    favicon_base::FaviconUsageDataList* favicons) {
+  base::FilePath database_path = FindSpartanDatabase(source_path_);
+  if (database_path.empty())
+    return;
+
+  EdgeDatabaseSession database;
+  if (!database.OpenDatabase(database_path.value())) {
+    DLOG(ERROR) << "Error opening database " << database.GetErrorMessage();

[Ilya Sherman, 2015/11/26 02:04:44]

Optional nit: You probably want DVLOG or just LOG.

[forshaw, 2015/11/30 12:57:58]

Acknowledged.

+    return;
+  }
+
+  scoped_ptr<EdgeDatabaseTableEnumerator> enumerator =
+      database.OpenTableEnumerator(L"Favorites");
+  if (!enumerator) {
+    DLOG(ERROR) << "Error opening database table "
+                << database.GetErrorMessage();
+    return;
+  }
+
+  std::map<GUID, EdgeFavoriteEntry, GuidComparator> database_entries;
+  base::FilePath favicon_base =
+      source_path_.empty()
+          ? importer::GetEdgeDataFilePath().Append(L"DataStore")
+          : source_path_.Append(L"DataStore");

[Ilya Sherman, 2015/11/26 02:04:44]

nit: I'd move the .Append(L"DataStore") out of the ternary operator.

[forshaw, 2015/11/30 12:57:59]

Acknowledged.

+
+  enumerator->Reset();
+  do {

[Ilya Sherman, 2015/11/26 02:04:45]

Is it possible for the enumerator to be empty?

[forshaw, 2015/11/30 12:57:58]

Yes it could be, this would be caught by the loop failing. But I'll add a check
for the return code from Reset just in case.

+    EdgeFavoriteEntry entry;
+    bool is_deleted = false;
+    if (!enumerator->RetrieveColumn(L"IsDeleted", is_deleted))
+      continue;
+    if (is_deleted)
+      continue;
+    if (!enumerator->RetrieveColumn(L"IsFolder", entry.is_folder))
+      continue;
+    base::string16 url;
+    if (!enumerator->RetrieveColumn(L"URL", url))
+      continue;
+    entry.url = GURL(url);
+    if (!entry.is_folder && !entry.url.is_valid())
+      continue;
+    if (!enumerator->RetrieveColumn(L"Title", entry.title))
+      continue;
+    base::string16 favicon_file;
+    if (!enumerator->RetrieveColumn(L"FaviconFile", favicon_file))
+      continue;
+    if (!favicon_file.empty())
+      entry.favicon_file = favicon_base.Append(favicon_file);
+    if (!enumerator->RetrieveColumn(L"ParentId", entry.parent_id))
+      continue;
+    if (!enumerator->RetrieveColumn(L"ItemId", entry.item_id))
+      continue;
+    if (!enumerator->RetrieveColumn(L"OrderNumber", entry.order_number))
+      continue;
+    FILETIME ft;
+    if (!enumerator->RetrieveColumn(L"DateUpdated", ft))
+      continue;
+    entry.date_updated = base::Time::FromFileTime(ft);
+    database_entries[entry.item_id] = entry;
+  } while (enumerator->Next() && !cancelled());
+
+  // Build simple tree.
+  EdgeFavoriteEntry root_entry;
+  for (auto& e : database_entries) {

[Ilya Sherman, 2015/11/26 02:04:44]

nit: Can the entry be const?

[Ilya Sherman, 2015/11/26 02:04:44]

nit: s/e/entry

[forshaw, 2015/11/30 12:57:58]

Acknowledged.

[forshaw, 2015/11/30 12:57:58]

In the loop? Don't think so unless I'm missing something as we're changing the
children member. Or do you mean adding into the children member? I've changed
that as above.

[forshaw, 2015/11/30 12:57:58]

Acknowledged.

[forshaw, 2015/11/30 12:57:59]

Acknowledged.

+    auto found_parent = database_entries.find(e.second.parent_id);
+    if (found_parent == database_entries.end() ||
+        !found_parent->second.is_folder) {
+      root_entry.children.push_back(&e.second);
+    } else {
+      found_parent->second.children.push_back(&e.second);
+    }
+  }
+  std::vector<base::string16> path;
+  BuildBookmarkEntries(&root_entry, bookmarks, favicons, false, path);
+}