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 465 matching lines @@
                                   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))
     return;
   base::win::ScopedComPtr<IEnumSTATURL> enum_url;
   if (SUCCEEDED(result = url_history_stg2->EnumUrls(enum_url.Receive()))) {

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

Small cleanup while you're here, I'm seeing all these (result = ...) inside
conditionals.

1) This is bad style (but this is also very very old code, so not blaming anyone
-- in fact git blaming for the hell of it returns initial.commit from 2008 (i.e.
old Firefox code IIRC)... =S!).
2) |result| is actually never even read so this is not even slightly useful...

Could you please remove |result| (and inline the only place where it's read on
line 483)?

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

Acknowledged.

     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;
       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) {

[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 {
         row.hidden = true;

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

In this scenario are we guaranteed that dwFlags is STATURLFLAG_ISCACHED? Or can
it be neither..? The "either or" documentation makes me think is *has to be* one
or the other...

If so, let's 
DCHECK_EQ(stat_url.dwFlags, STATURLFLAG_ISCACHED);
to make sure we're getting dwFlags to be filled properly this time around (and
also to augment documentation of this weird API).

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

Yes, it is a good idea!

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

[Alexey Seren, 2014/12/12 10:38:40]

I'm sorry, I've fogotten to mention that to do this we need to change SetFilter
call: enum_url->SetFilter(NULL, STATURL_QUERYFLAG_TOPLEVEL |
STATURL_QUERYFLAG_ISCACHED);

So is it ok?

On 2014/12/12 10:07:08, aseren wrote:

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

If we need to call SetFilter() to have a non-zero dwFlags sure.

But if we can avoid calling SetFilter() and just have a fully filled dwFlags I'd
prefer that (and here we would DCHECK(stat_url.dwFlags & STATURLFLAG_ISCACHED);)

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

Ok, requesting the flag STATURLFLAG_ISCACHED will be very useful for clarifying
and augumenting the documentation.

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

SG.

       }
 
       rows.push_back(row);
     }
 
     if (!rows.empty() && !cancelled()) {
       bridge_->SetHistoryItems(rows, importer::VISIT_SOURCE_IE_IMPORTED);
     }
   }
 }
@@ skipping 346 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;
 }