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

chrome/utility/importer/ie_importer_win.cc

 // Copyright (c) 2012 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/ie_importer_win.h"
 
 #include <ole2.h>
 #include <intshcut.h>
 #include <shlobj.h>
 #include <urlhist.h>
@@ skipping 459 matching lines @@
 }
 
 void IEImporter::ImportHistory() {
   const std::string kSchemes[] = {url::kHttpScheme,
                                   url::kHttpsScheme,
                                   url::kFtpScheme,
                                   url::kFileScheme};
   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;
         CoTaskMemFree(stat_url.pwcsUrl);
       }
       base::string16 title_string;
       if (stat_url.pwcsTitle) {
         title_string = stat_url.pwcsTitle;
         CoTaskMemFree(stat_url.pwcsTitle);
       }
 
       GURL url(url_string);
       // Skips the URLs that are invalid or have other schemes.
       if (!url.is_valid() ||
           (std::find(kSchemes, kSchemes + total_schemes, url.scheme()) ==
            kSchemes + total_schemes))
         continue;
 
       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;
       }
 
       rows.push_back(row);
     }
 
     if (!rows.empty() && !cancelled()) {
       bridge_->SetHistoryItems(rows, importer::VISIT_SOURCE_IE_IMPORTED);
     }
   }
@@ skipping 347 matching lines @@
   static int version = -1;
   if (version < 0) {
     wchar_t buffer[128];
     DWORD buffer_length = sizeof(buffer);
     base::win::RegKey reg_key(HKEY_LOCAL_MACHINE, kIEVersionKey, KEY_READ);
     LONG result = reg_key.ReadValue(L"Version", buffer, &buffer_length, NULL);
     version = ((result == ERROR_SUCCESS)? _wtoi(buffer) : 0);
   }
   return version;
 }