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..f538bcafc1f9c83537c6a0029007210e5865c75f 100644
--- a/chrome/utility/importer/ie_importer_win.cc
+++ b/chrome/utility/importer/ie_importer_win.cc
@@ -487,6 +487,9 @@ void IEImporter::ImportHistory() {
     std::vector<ImporterURLRow> rows;
     STATURL stat_url;
     ULONG fetched;
+
+    // Fill STATURL::dwFlags attribute for top-level items.
+    enum_url->SetFilter(NULL, STATURL_QUERYFLAG_TOPLEVEL);

[gab, 2014/12/11 16:29:49]

Gosh this API is confusing..!

So you need to set a NULL filter for STATURL_QUERYFLAG_TOPLEVEL on
IEnumURLSTATURL in order for IEnumSTATURL::Next() to fill this field in the
STATURL it returns??

Seems like a null filter should do nothing (a non-null filter would filter so
that only matches make it through Next and a null filter would clear that filter
and have Next return all URLs one-by-one).

Have you found more documentation on how SetFilter() and Next() work together?

Could you please expand this comment (ideally with supporting links)?

[Alexey Seren, 2014/12/12 10:07:08]

By passing NULL here I've missed an OPTIONAL poszFilter parameter to indicate
that history will not be filtered by URL matching. poszFilter parameter is
OPTIONAL due to last line in documentation for SetFilter()
http://msdn.microsoft.com/en-us/library/ie/aa767728(v=vs.85).aspx:
<<If a filter is specified in the poszFilter parameter, call IEnumSTATURL::Reset
after the enumerator is no longer required.>>
In fact that NULL in WinAPI frequently indicates that optional passing parameter
is ignored. For example optional parameters hMenu, lpParam in CreateWindow()
function
http://msdn.microsoft.com/en-us/library/windows/desktop/ms632679(v=vs.85).aspx

From another point of view poszFilter means prefix match. So we can pass here an
empty string OLESTR("") to get all history items.
Here are some successful stories of using poszFilter for prefix match:
http://www.eternalwindows.jp/browser/webbrowser/webbrowser13.html
http://blog.csdn.net/lbird/article/details/724862
http://www.programdevelop.com/1112484/
http://answer.techwikihow.com/902414/failure-enumrating-ie-history.html
http://www.codeproject.com/script/Content/ViewAssociatedFile.aspx?rzp=%2FKB%2...
http://hpcgi1.nifty.com/MADIA/VBBBS/wwwlng.cgi?print+200405/04050105.txt
http://mrxray.on.coocan.jp/Delphi/plSamples/760_InternetCacheHistroy.htm

So we can do two things:
- pass NULL to indicate ignoring of optional parameter
- pass OLESTR("") to indicate that all URLs match this prefix

What do you think about it? Which solution is more appropriate?

On 2014/12/11 16:29:49, gab wrote:

[gab, 2014/12/12 14:58:31]

Right, but in this case poszFilter is documented as [in] not [in, optional]...
but I guess it's another documentation fluke...
Hmmm, but most of these all use a non-empty filter (1 uses "" and none use
NULL). What I don't understand is why do we even need to ask for a NULL filter?
Isn't that the same as not even calling SetFilter? Or is SetFilter required to
ask Next() to fill dwFlags? Where is that documented?
I think I'd prefer L"" as it's more explicit (and more inline with the
documentation which is [in], not [in, optional]), but please expand the code
comment to explain why SetFilter() is even required (i.e. why can't we just not
call SetFilter() and expect Next() to return all elements one-by-one with
dwFlags fully filled?

[Alexey Seren, 2014/12/15 14:00:23]

Basing on documentation SetFilter() can be used to control how STATURL is
filled. And it is the only way to do it (there is no other method with such
functionality).
http://msdn.microsoft.com/en-us/library/ie/aa767728(v=vs.85).aspx

Here are some samples from social.msdn of how IUrlHistory flags are specified:
https://social.msdn.microsoft.com/Forums/ie/en-US/3563b93f-d9c2-4c2c-a317-f4d...
https://social.msdn.microsoft.com/Forums/ie/en-US/5a0d0aab-eb3f-4d7f-ba91-1d0...

So IUrlHistory::STATURL_QUERYFLAG_ISCACHED and
IUrlHistory::STATURL_QUERYFLAG_TOPLEVEL flags are used to configure some members
of STATURL. And I think it's obvious that its configure STATURL::dwFlags
parameter that can indicates is item cached or toplevel.

Also presence of such flags indicates that filling of STATURL::dwFlags is turned
off by default for some reasons, may be for performance issues.
Ok, I will use OLESTR("") here and will expand the comment.

[gab, 2014/12/15 15:50:01]

I see, hadn't realized filling those was off by default (and thus that the only
way to turn it on is to explicitly ask for them through SetFilter()), please
specify this in comments above the SetFilter() call. 
L"" should do I think, I've never seen usage of OLESTR() in our codebase.

[Alexey Seren, 2014/12/17 11:45:45]

Acknowledged.

     while (!cancelled() &&
            (result = enum_url->Next(1, &stat_url, &fetched)) == S_OK) {

[gab, 2014/12/11 16:29:49]

Looks like |fetched| is an optional parameter [1] and looks like we're not using
its result... please consider getting rid of it.

[1] http://msdn.microsoft.com/en-us/library/ie/ms774958%28v=vs.85%29.aspx

[Alexey Seren, 2014/12/12 10:07:08]

Acknowledged.

       base::string16 url_string;
@@ -510,7 +513,7 @@ 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) {

[gab, 2014/12/11 16:29:49]

As you mentioned in the CL description, the definition for dwFlags is:

dwFlags: DWORD that can be either STATURL_QUERYFLAG_ISCACHED or
STATURL_QUERYFLAG_TOPLEVEL.

So shouldn't this be comparing for equality rather than doing bitwise
comparison?

[Alexey Seren, 2014/12/12 10:07:08]

No we cannot do equality comparing here because flags STATURL_QUERYFLAG_ISCACHED
and STATURL_QUERYFLAG_TOPLEVEL can be set together if we call SetFilter() in
such way: enum_url->SetFilter(NULL, STATURL_QUERYFLAG_TOPLEVEL |
STATURL_QUERYFLAG_ISCACHED);

This is cleared in documentation for ADDURL_FLAG
(http://msdn.microsoft.com/ru-ru/aa767730): if we use
ADDURL_ADDTOHISTORYANDCACHE then URL will be added to both the visited links and
the dated containers.

On 2014/12/11 16:29:49, gab wrote:

[gab, 2014/12/12 14:58:31]

I see, so the "either or" documentation for dwFlags is incorrect...

         row.visit_count = 1;
         row.hidden = false;
       } else {