Search for history items that was imported from IE is not working.

chrome/utility/importer/ie_importer_win.cc

Index: chrome/utility/importer/ie_importer_win.cc
diff --git a/chrome/utility/importer/ie_importer_win.cc b/chrome/utility/importer/ie_importer_win.cc
index 4865397260acaac1ecee67f5f757ed012bcac32a..36973c3026e7f4d93c45ec91196bb872dabd3d5a 100644
--- a/chrome/utility/importer/ie_importer_win.cc
+++ b/chrome/utility/importer/ie_importer_win.cc
@@ -477,18 +477,25 @@ void IEImporter::ImportHistory() {
   int total_schemes = arraysize(kSchemes);
 
   base::win::ScopedComPtr<IUrlHistoryStg2> url_history_stg2;
-  HRESULT result;
-  result = url_history_stg2.CreateInstance(CLSID_CUrlHistory, NULL,
-                                           CLSCTX_INPROC_SERVER);
-  if (FAILED(result))
+  if (FAILED(url_history_stg2.CreateInstance(CLSID_CUrlHistory, NULL,
+                                             CLSCTX_INPROC_SERVER))) {
     return;
+  }
   base::win::ScopedComPtr<IEnumSTATURL> enum_url;
-  if (SUCCEEDED(result = url_history_stg2->EnumUrls(enum_url.Receive()))) {
+  if (SUCCEEDED(url_history_stg2->EnumUrls(enum_url.Receive()))) {
     std::vector<ImporterURLRow> rows;
     STATURL stat_url;
-    ULONG fetched;
+
+    // IEnumSTATURL::Next() doesn't fill STATURL::dwFlags by default. Need to
+    // call IEnumSTATURL::SetFilter() with STATURL_QUERYFLAG_TOPLEVEL |
+    // STATURL_QUERYFLAG_ISCACHED flags to specify how STATURL will be filled.
+    // First argument of IEnumSTATURL::SetFilter() specifies the URL prefix
+    // that is used by IEnumSTATURL::Next() for filtering items by URL.
+    // So need to pass an empty string here to get all history items.
+    enum_url->SetFilter(L"", STATURL_QUERYFLAG_TOPLEVEL |
+                             STATURL_QUERYFLAG_ISCACHED);

[gab, 2014/12/17 18:55:30]

One last question, IIUC STATURL_QUERYFLAG_ISCACHED is always set alongside
STATURL_QUERYFLAG_TOPLEVEL, but we can also only have
STATURL_QUERYFLAG_ISCACHED.

But can we ever not have STATURL_QUERYFLAG_ISCACHED? (If so, the DCHECK below is
wrong; and if not this API is kind of silly :-)!).

[Alexey Seren, 2014/12/18 12:08:15]

Yes, we can miss STATURL_QUERYFLAG_ISCACHED during SetFilter() call. In this
case only STATURLFLAG_ISTOPLEVEL flag will be set and DCHECK below becomes
wrong.

So should I remove DCHECK below or not? 
If yes, I will change second argument and comment for SetFilter() call.

[gab, 2014/12/18 15:48:01]

Right, but given we have STATURL_QUERYFLAG_ISCACHED in SetFilter(), are we
guaranteed that it will always be set? (i.e. are there any URLs for which it is
not set... if not, this part of the API seems kind of redundant...).
Feels like STATURL_QUERYFLAG_ISCACHED is redundant (i.e. doesn't provide extra
information since it's always set).

So I think we should just filter for STATURL_QUERYFLAG_TOPLEVEL and change the
check below for:

if (stat_url.dwFlags == STATURLFLAG_ISTOPLEVEL) {
  ...
} else {
  // dwFlags should only contain the STATURLFLAG_ISTOPLEVEL bit per
  // the filter set above.
  DCHECK(!stat_url.dwFlags);
  ...
}


WDYT?

[Alexey Seren, 2014/12/22 13:01:40]

You are right. It seems like STATURL_QUERYFLAG_ISCACHED flag is redundant
because all history items are visited, so they are all cached.
Thank you for the code. I will use it for the next patch.

     while (!cancelled() &&
-           (result = enum_url->Next(1, &stat_url, &fetched)) == S_OK) {
+           enum_url->Next(1, &stat_url, NULL) == S_OK) {
       base::string16 url_string;
       if (stat_url.pwcsUrl) {
         url_string = stat_url.pwcsUrl;
@@ -510,10 +517,11 @@ void IEImporter::ImportHistory() {
       ImporterURLRow row(url);
       row.title = title_string;
       row.last_visit = base::Time::FromFileTime(stat_url.ftLastVisited);
-      if (stat_url.dwFlags == STATURL_QUERYFLAG_TOPLEVEL) {
+      if (stat_url.dwFlags & STATURLFLAG_ISTOPLEVEL) {
         row.visit_count = 1;
         row.hidden = false;
       } else {
+        DCHECK(stat_url.dwFlags == STATURLFLAG_ISCACHED);

[gab, 2014/12/17 18:55:30]

DCHECK_EQ(stat_url.dwFlags, STATURLFLAG_ISCACHED);

(Provides nicer error log message on mismatch)

[Alexey Seren, 2014/12/18 12:08:15]

Ok, I will change it if it will not be removed (due to comments above)

         row.hidden = true;
       }