Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
800433002:
Search for history items that was imported from IE is not working. (Closed)
|
Created:
6 years ago by Alexey Seren Modified:
5 years, 11 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSearch for history items that was imported from IE is not working.
The problem is that top level history items were recognized
as cache history items and were added with hidden attribute on.
This bug was caused by unclear Microsoft documentation:
- here http://msdn.microsoft.com/en-us/library/ie/ms774962(v=vs.85).aspx
document states that STATURLFLAG_ISTOPLEVEL is <<Flag on the dwFlags parameter
of the STATURL structure indicating that the item is a top-level item.>>
- here http://msdn.microsoft.com/en-us/library/ie/ms774942(v=vs.85).aspx
document states that dwFlags is <<DWORD that can be either
STATURL_QUERYFLAG_ISCACHED or STATURL_QUERYFLAG_TOPLEVEL.>>
R=gab@chromium.org
BUG=441654
TEST=
A) IEImporterBrowserTest.IEImporter regression test fails without corresponding
product changes.
B) Import history from IE and confirm that imported items can be searched for.
Committed: https://crrev.com/a32f1fb46916344f3975c3b1eddef6e34a0cd9da
Cr-Commit-Position: refs/heads/master@{#310023}
Patch Set 1 #Patch Set 2 : Refactored testing code. #
Total comments: 25
Patch Set 3 : Fixed some issues found by Gab@ #
Total comments: 2
Patch Set 4 : Added/expanded comments for IE history API. #
Total comments: 8
Patch Set 5 : Removed redundant flag from SetFilter call. #
Total comments: 6
Patch Set 6 : Fixed some issues/comments found by Gab@ #
Messages
Total messages: 23 (4 generated)
aseren@yandex-team.ru changed reviewers: + gab@chromium.org
aseren@yandex-team.ru changed required reviewers: + gab@chromium.org, isherman@chromium.org
Search for history items that was imported from IE is not working. The problem is that top level history items were recognized as cache history items and were added with hidden attribute on. This bug was caused by unclear Microsoft documentation: - here http://msdn.microsoft.com/en-us/library/ie/ms774962(v=vs.85).aspx document states that STATURLFLAG_ISTOPLEVEL is <<Flag on the dwFlags parameter of the STATURL structure indicating that the item is a top-level item.>> - here http://msdn.microsoft.com/en-us/library/ie/ms774942(v=vs.85).aspx document states that dwFlags is <<DWORD that can be either STATURL_QUERYFLAG_ISCACHED or STATURL_QUERYFLAG_TOPLEVEL.>> TEST=Import history from IE and search imported items.
Thanks, some comments/suggestions below. please also file a bug on crbug.com and link to it from a BUG= entry in this CL's description. Please also confirm that your new tests fail without the matching product change (to confirm that this is a good regression test) and add a statement stating this fact in the TEST= line, e.g.: TEST= A) IEImporterBrowserTest.IEImporter regression test fails without corresponding product changes. B) Import history from IE and confirm that imported items can be searched for. Cheers! Gab https://codereview.chromium.org/800433002/diff/20001/chrome/browser/importer/... File chrome/browser/importer/ie_importer_browsertest_win.cc (right): https://codereview.chromium.org/800433002/diff/20001/chrome/browser/importer/... chrome/browser/importer/ie_importer_browsertest_win.cc:501: ADDURL_ADDTOHISTORYANDCACHE); Any documentation for ADDURL_ADDTOHISTORYANDCACHE? All documentation I can find for IUrlHistoryStg(2) reports that the last param is "Not Implemented"... (and I also can't find documentation for ADDURL_ADDTOHISTORYANDCACHE and ADDURL_ADDTOCACHE themselves -- besides seeing that they are indeed part of an undocumented enum in the SDK in UrlList.h ...) Please at least add a comment (ideally with supporting links) to describe what this does. https://codereview.chromium.org/800433002/diff/20001/chrome/browser/importer/... chrome/browser/importer/ie_importer_browsertest_win.cc:528: url_history_stg2->DeleteUrl(kIEIdentifyUrl, 0); nit: flip these two since kIECacheItemUrl always comes second everywhere else in this file https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/ie_importer_win.cc (right): https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:486: if (SUCCEEDED(result = url_history_stg2->EnumUrls(enum_url.Receive()))) { 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)? https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:492: enum_url->SetFilter(NULL, STATURL_QUERYFLAG_TOPLEVEL); 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)? https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:494: (result = enum_url->Next(1, &stat_url, &fetched)) == S_OK) { 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 https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:516: if (stat_url.dwFlags & STATURLFLAG_ISTOPLEVEL) { 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? https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:520: row.hidden = true; 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).
https://codereview.chromium.org/800433002/diff/20001/chrome/browser/importer/... File chrome/browser/importer/ie_importer_browsertest_win.cc (right): https://codereview.chromium.org/800433002/diff/20001/chrome/browser/importer/... chrome/browser/importer/ie_importer_browsertest_win.cc:501: ADDURL_ADDTOHISTORYANDCACHE); Here is documentation: http://msdn.microsoft.com/ru-ru/aa767730 On 2014/12/11 16:29:49, gab wrote: > Any documentation for ADDURL_ADDTOHISTORYANDCACHE? All documentation I can find > for IUrlHistoryStg(2) reports that the last param is "Not Implemented"... > > (and I also can't find documentation for ADDURL_ADDTOHISTORYANDCACHE and > ADDURL_ADDTOCACHE themselves -- besides seeing that they are indeed part of an > undocumented enum in the SDK in UrlList.h ...) > > Please at least add a comment (ideally with supporting links) to describe what > this does. https://codereview.chromium.org/800433002/diff/20001/chrome/browser/importer/... chrome/browser/importer/ie_importer_browsertest_win.cc:528: url_history_stg2->DeleteUrl(kIEIdentifyUrl, 0); On 2014/12/11 16:29:49, gab wrote: > nit: flip these two since kIECacheItemUrl always comes second everywhere else in > this file Acknowledged. https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/ie_importer_win.cc (right): https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:486: if (SUCCEEDED(result = url_history_stg2->EnumUrls(enum_url.Receive()))) { On 2014/12/11 16:29:49, gab wrote: > 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)? Acknowledged. https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:492: enum_url->SetFilter(NULL, STATURL_QUERYFLAG_TOPLEVEL); 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: > 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)? https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:494: (result = enum_url->Next(1, &stat_url, &fetched)) == S_OK) { On 2014/12/11 16:29:49, gab wrote: > 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 Acknowledged. https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:516: if (stat_url.dwFlags & STATURLFLAG_ISTOPLEVEL) { 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: > 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? https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:520: row.hidden = true; Yes, it is a good idea! On 2014/12/11 16:29:49, gab wrote: > 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).
https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/ie_importer_win.cc (right): https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:520: row.hidden = true; 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: > Yes, it is a good idea! > > On 2014/12/11 16:29:49, gab wrote: > > 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). >
Replies below. Launched a few try jobs to catch errors early. Cheers, Gab https://codereview.chromium.org/800433002/diff/20001/chrome/browser/importer/... File chrome/browser/importer/ie_importer_browsertest_win.cc (right): https://codereview.chromium.org/800433002/diff/20001/chrome/browser/importer/... chrome/browser/importer/ie_importer_browsertest_win.cc:501: ADDURL_ADDTOHISTORYANDCACHE); On 2014/12/12 10:07:08, aseren wrote: > Here is documentation: > http://msdn.microsoft.com/ru-ru/aa767730 Thanks, can you add a comment above pointing at this documentation (i.e., the next reader will probably also have trouble finding it). > > On 2014/12/11 16:29:49, gab wrote: > > Any documentation for ADDURL_ADDTOHISTORYANDCACHE? All documentation I can > find > > for IUrlHistoryStg(2) reports that the last param is "Not Implemented"... > > > > (and I also can't find documentation for ADDURL_ADDTOHISTORYANDCACHE and > > ADDURL_ADDTOCACHE themselves -- besides seeing that they are indeed part of an > > undocumented enum in the SDK in UrlList.h ...) > > > > Please at least add a comment (ideally with supporting links) to describe what > > this does. > https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/ie_importer_win.cc (right): https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:492: enum_url->SetFilter(NULL, STATURL_QUERYFLAG_TOPLEVEL); On 2014/12/12 10:07:08, aseren wrote: > 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 Right, but in this case poszFilter is documented as [in] not [in, optional]... but I guess it's another documentation fluke... > > 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 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? > > 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? 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? > > On 2014/12/11 16:29:49, gab wrote: > > 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)? > https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:516: if (stat_url.dwFlags & STATURLFLAG_ISTOPLEVEL) { On 2014/12/12 10:07:08, aseren wrote: > 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: > > 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? > I see, so the "either or" documentation for dwFlags is incorrect... https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:520: row.hidden = true; On 2014/12/12 10:38:40, aseren wrote: > 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: > > Yes, it is a good idea! > > > > On 2014/12/11 16:29:49, gab wrote: > > > 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). > > > 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);) https://codereview.chromium.org/800433002/diff/40001/chrome/utility/importer/... File chrome/utility/importer/ie_importer_win.cc (right): https://codereview.chromium.org/800433002/diff/40001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:482: return; nit: Wrap in {} since conditional is multi-line.
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 Also 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 I think that IUrlHistory::STATURL_QUERYFLAG_ISCACHED and IUrlHistory::STATURL_QUERYFLAG_TOPLEVEL flags implicitly indicates that presence of iscached and istoplevel flags can be configured. And it is turned off by default for some reasons, may be for performance issues. https://codereview.chromium.org/800433002/diff/20001/chrome/browser/importer/... File chrome/browser/importer/ie_importer_browsertest_win.cc (right): https://codereview.chromium.org/800433002/diff/20001/chrome/browser/importer/... chrome/browser/importer/ie_importer_browsertest_win.cc:501: ADDURL_ADDTOHISTORYANDCACHE); On 2014/12/12 14:58:31, gab wrote: > On 2014/12/12 10:07:08, aseren wrote: > > Here is documentation: > > http://msdn.microsoft.com/ru-ru/aa767730 > > Thanks, can you add a comment above pointing at this documentation (i.e., the > next reader will probably also have trouble finding it). > > > > > > On 2014/12/11 16:29:49, gab wrote: > > > Any documentation for ADDURL_ADDTOHISTORYANDCACHE? All documentation I can > > find > > > for IUrlHistoryStg(2) reports that the last param is "Not Implemented"... > > > > > > (and I also can't find documentation for ADDURL_ADDTOHISTORYANDCACHE and > > > ADDURL_ADDTOCACHE themselves -- besides seeing that they are indeed part of > an > > > undocumented enum in the SDK in UrlList.h ...) > > > > > > Please at least add a comment (ideally with supporting links) to describe > what > > > this does. > > > Acknowledged. https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/ie_importer_win.cc (right): https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:492: enum_url->SetFilter(NULL, STATURL_QUERYFLAG_TOPLEVEL); On 2014/12/12 14:58:31, gab wrote: > On 2014/12/12 10:07:08, aseren wrote: > > 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 > > Right, but in this case poszFilter is documented as [in] not [in, optional]... > but I guess it's another documentation fluke... > > > > > > 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 > > > 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? > > 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. > > > > 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? > > 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? > > > > > On 2014/12/11 16:29:49, gab wrote: > > > 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)? > > > Ok, I will use OLESTR("") here and will expand the comment. https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:520: row.hidden = true; On 2014/12/12 14:58:31, gab wrote: > On 2014/12/12 10:38:40, aseren wrote: > > 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: > > > Yes, it is a good idea! > > > > > > On 2014/12/11 16:29:49, gab wrote: > > > > 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). > > > > > > > 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);) Ok, requesting the flag STATURLFLAG_ISCACHED will be very useful for clarifying and augumenting the documentation. https://codereview.chromium.org/800433002/diff/40001/chrome/utility/importer/... File chrome/utility/importer/ie_importer_win.cc (right): https://codereview.chromium.org/800433002/diff/40001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:482: return; On 2014/12/12 14:58:31, gab wrote: > nit: Wrap in {} since conditional is multi-line. Acknowledged.
SG, thanks for your replies, will await a new patch set with code comment updates and acknowledged nits. https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/ie_importer_win.cc (right): https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:492: enum_url->SetFilter(NULL, STATURL_QUERYFLAG_TOPLEVEL); On 2014/12/15 14:00:23, aseren wrote: > On 2014/12/12 14:58:31, gab wrote: > > On 2014/12/12 10:07:08, aseren wrote: > > > 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 > > > > Right, but in this case poszFilter is documented as [in] not [in, optional]... > > but I guess it's another documentation fluke... > > > > > > > > > > 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 > > > > > > 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? > > > > > > 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. 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. > > > > > > > > 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? > > > > 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? > > > > > > > > On 2014/12/11 16:29:49, gab wrote: > > > > 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)? > > > > > > > Ok, I will use OLESTR("") here and will expand the comment. L"" should do I think, I've never seen usage of OLESTR() in our codebase. https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:520: row.hidden = true; On 2014/12/15 14:00:23, aseren wrote: > On 2014/12/12 14:58:31, gab wrote: > > On 2014/12/12 10:38:40, aseren wrote: > > > 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: > > > > Yes, it is a good idea! > > > > > > > > On 2014/12/11 16:29:49, gab wrote: > > > > > 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). > > > > > > > > > > > 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);) > > Ok, requesting the flag STATURLFLAG_ISCACHED will be very useful for clarifying > and augumenting the documentation. > SG.
aseren@yandex-team.ru changed required reviewers: - isherman@chromium.org
Hello Gab. Thank you for reviews and comments. Please see code patchset with comments expanded. Best regards. aseren https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/ie_importer_win.cc (right): https://codereview.chromium.org/800433002/diff/20001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:492: enum_url->SetFilter(NULL, STATURL_QUERYFLAG_TOPLEVEL); On 2014/12/15 15:50:01, gab wrote: > On 2014/12/15 14:00:23, aseren wrote: > > On 2014/12/12 14:58:31, gab wrote: > > > On 2014/12/12 10:07:08, aseren wrote: > > > > 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 > > > > > > Right, but in this case poszFilter is documented as [in] not [in, > optional]... > > > but I guess it's another documentation fluke... > > > > > > > > > > > > > > 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 > > > > > > > > > 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? > > > > > > > > > > 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. > > 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. > > > > > > > > > > > > > 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? > > > > > > 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? > > > > > > > > > > > On 2014/12/11 16:29:49, gab wrote: > > > > > 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)? > > > > > > > > > > > Ok, I will use OLESTR("") here and will expand the comment. > > L"" should do I think, I've never seen usage of OLESTR() in our codebase. Acknowledged.
lvg, last couple nits below. https://codereview.chromium.org/800433002/diff/60001/chrome/browser/importer/... File chrome/browser/importer/ie_importer_browsertest_win.cc (right): https://codereview.chromium.org/800433002/diff/60001/chrome/browser/importer/... chrome/browser/importer/ie_importer_browsertest_win.cc:282: bool history_item_found_ = false; No '_' suffix for local variables. https://codereview.chromium.org/800433002/diff/60001/chrome/utility/importer/... File chrome/utility/importer/ie_importer_win.cc (right): https://codereview.chromium.org/800433002/diff/60001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:496: STATURL_QUERYFLAG_ISCACHED); 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 :-)!). https://codereview.chromium.org/800433002/diff/60001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:524: DCHECK(stat_url.dwFlags == STATURLFLAG_ISCACHED); DCHECK_EQ(stat_url.dwFlags, STATURLFLAG_ISCACHED); (Provides nicer error log message on mismatch)
Hello Gab, Please see my answers below. Thanks, aseren https://codereview.chromium.org/800433002/diff/60001/chrome/browser/importer/... File chrome/browser/importer/ie_importer_browsertest_win.cc (right): https://codereview.chromium.org/800433002/diff/60001/chrome/browser/importer/... chrome/browser/importer/ie_importer_browsertest_win.cc:282: bool history_item_found_ = false; On 2014/12/17 18:55:30, gab wrote: > No '_' suffix for local variables. Acknowledged. https://codereview.chromium.org/800433002/diff/60001/chrome/utility/importer/... File chrome/utility/importer/ie_importer_win.cc (right): https://codereview.chromium.org/800433002/diff/60001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:496: STATURL_QUERYFLAG_ISCACHED); On 2014/12/17 18:55:30, gab wrote: > 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 :-)!). 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. https://codereview.chromium.org/800433002/diff/60001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:524: DCHECK(stat_url.dwFlags == STATURLFLAG_ISCACHED); On 2014/12/17 18:55:30, gab wrote: > DCHECK_EQ(stat_url.dwFlags, STATURLFLAG_ISCACHED); > > (Provides nicer error log message on mismatch) Ok, I will change it if it will not be removed (due to comments above)
https://codereview.chromium.org/800433002/diff/60001/chrome/utility/importer/... File chrome/utility/importer/ie_importer_win.cc (right): https://codereview.chromium.org/800433002/diff/60001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:496: STATURL_QUERYFLAG_ISCACHED); On 2014/12/18 12:08:15, aseren wrote: > On 2014/12/17 18:55:30, gab wrote: > > 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 :-)!). > > 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. 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...). > > So should I remove DCHECK below or not? > If yes, I will change second argument and comment for SetFilter() call. 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?
Thank you for your comments/reviews. Regards. aseren https://codereview.chromium.org/800433002/diff/60001/chrome/utility/importer/... File chrome/utility/importer/ie_importer_win.cc (right): https://codereview.chromium.org/800433002/diff/60001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:496: STATURL_QUERYFLAG_ISCACHED); On 2014/12/18 15:48:01, gab wrote: > On 2014/12/18 12:08:15, aseren wrote: > > On 2014/12/17 18:55:30, gab wrote: > > > 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 :-)!). > > > > 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. > > 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...). > > > > > So should I remove DCHECK below or not? > > If yes, I will change second argument and comment for SetFilter() call. > > 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? 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.
Looking great, one last set of tweaks I think and we're good to go :-)! https://codereview.chromium.org/800433002/diff/80001/chrome/browser/importer/... File chrome/browser/importer/ie_importer_browsertest_win.cc (right): https://codereview.chromium.org/800433002/diff/80001/chrome/browser/importer/... chrome/browser/importer/ie_importer_browsertest_win.cc:499: ASSERT_TRUE(res == S_OK); It's kind of overkill to store |res| in a variable only to assert it. And it's ugly to re-use the same variable to do this over and over again. Could you please inline the calls inside the asserts and get rid of |res|? Thanks! https://codereview.chromium.org/800433002/diff/80001/chrome/utility/importer/... File chrome/utility/importer/ie_importer_win.cc (right): https://codereview.chromium.org/800433002/diff/80001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:491: // specify that STATURL::dwFlags will indicate whether this URL is dated I prefered your previous wording, e.g. something like: "... to specfic how STATURL::dwFlags will be filled." (I find the latest slightly confusing for a new reader) https://codereview.chromium.org/800433002/diff/80001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:523: DCHECK(!stat_url.dwFlags); As in code provided in previous comment, add this comment here: // dwFlags should only contain the STATURLFLAG_ISTOPLEVEL bit per // the filter set above.
Hello gab! Please see the last Patch Set. Hope it will be fine. https://codereview.chromium.org/800433002/diff/80001/chrome/browser/importer/... File chrome/browser/importer/ie_importer_browsertest_win.cc (right): https://codereview.chromium.org/800433002/diff/80001/chrome/browser/importer/... chrome/browser/importer/ie_importer_browsertest_win.cc:499: ASSERT_TRUE(res == S_OK); On 2014/12/23 22:00:58, gab (mostly OOO until Jan 5th) wrote: > It's kind of overkill to store |res| in a variable only to assert it. And it's > ugly to re-use the same variable to do this over and over again. > > Could you please inline the calls inside the asserts and get rid of |res|? > > Thanks! Acknowledged. https://codereview.chromium.org/800433002/diff/80001/chrome/utility/importer/... File chrome/utility/importer/ie_importer_win.cc (right): https://codereview.chromium.org/800433002/diff/80001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:491: // specify that STATURL::dwFlags will indicate whether this URL is dated On 2014/12/23 22:00:58, gab (mostly OOO until Jan 5th) wrote: > I prefered your previous wording, e.g. something like: > > "... to specfic how STATURL::dwFlags will be filled." > > (I find the latest slightly confusing for a new reader) Acknowledged. Changed to previous comment. https://codereview.chromium.org/800433002/diff/80001/chrome/utility/importer/... chrome/utility/importer/ie_importer_win.cc:523: DCHECK(!stat_url.dwFlags); On 2014/12/23 22:00:58, gab (mostly OOO until Jan 5th) wrote: > As in code provided in previous comment, add this comment here: > > // dwFlags should only contain the STATURLFLAG_ISTOPLEVEL bit per > // the filter set above. Acknowledged.
lgtm, thanks!
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/800433002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a32f1fb46916344f3975c3b1eddef6e34a0cd9da Cr-Commit-Position: refs/heads/master@{#310023} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
