Implement support for importing favorites from Edge on Windows 10.

Implement support for importing favorites from Edge on Windows 10.
This CL adds support for importing favorties from the Edge browser on
Windows 10. It supports the old style importation from internet shortcuts
(without supporting order due to structure changes) as well as importing
from Edge 13 database format.

BUG=539746
Committed: https://crrev.com/57e648081288644895bf6f5fe184597a63282336
Cr-Commit-Position: refs/heads/master@{#362977}

[forshaw, 2015-11-20 22:36:34]

Patchset #2 (id:20001) has been deleted

[forshaw, 2015-11-20 22:41:50]

Patchset #2 (id:40001) has been deleted

[forshaw, 2015-11-20 22:42:00]

Patchset #2 (id:60001) has been deleted

[forshaw, 2015-11-20 22:42:16]

Patchset #1 (id:1) has been deleted

[forshaw, 2015-11-25 14:22:19]

forshaw@chromium.org changed reviewers:
+ isherman@chromium.org, jochen@chromium.org

[forshaw, 2015-11-25 14:22:19]

isherman@: Please can you review changes in both the metrics code as well as
importer which you seem to be an owner.

jochen@: Please can do a more general review of changes, but with an eye for
security as in theory a sufficiently privileged renderer could invoke the import
functionality.

Obviously being the time of year I'm not necessarily expecting an immediate
response.

Thank very much.

[Ilya Sherman, 2015-11-26 02:04:45]

Whew!  That's a large CL =)

It would be nice to add some more thorough test coverage for the various
possible edge cases.  Not critical though.

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
File chrome/browser/importer/edge_importer_browsertest_win.cc (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/edge_importer_browsertest_win.cc:45: :
ProfileWriter(NULL),
nit: nullptr

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/edge_importer_browsertest_win.cc:47:
bookmark_entries_(bookmark_entries),
nit: Maybe prefix this with "expected_"?

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/edge_importer_browsertest_win.cc:49:
favicon_groups_(favicon_groups),
Ditto

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/edge_importer_browsertest_win.cc:85:
EXPECT_EQ(favicon_groups_.size(), usage.size());
nit: This seems like it should be an ASSERT_EQ, since you index into both lists
using a single index

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/edge_importer_browsertest_win.cc:87:
EXPECT_EQ(usage[i].urls.size(), 1);
nit: I think you might need to write "1U"

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/edge_importer_browsertest_win.cc:87:
EXPECT_EQ(usage[i].urls.size(), 1);
Actually, this expectation seems oddly placed -- is it not redundant with the
one on line 95?

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/edge_importer_browsertest_win.cc:112: uint16
importer_items_;
nit: Please document these member vars.

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/edge_importer_browsertest_win.cc:115: class ScopedGzFile
{
nit: s/Gz/Gzip

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/edge_importer_browsertest_win.cc:124: gz_file_ =
nullptr;
nit: Why is this line needed?

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/edge_importer_browsertest_win.cc:178: }
Can you just slurp in the whole file and then call compression:GzipUncompress?

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/edge_importer_browsertest_win.cc:206:
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
nit: Would it be appropriate to simply call this as part of the constructor?

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/edge_importer_browsertest_win.cc:348: new
TestObserver(importer::FAVORITES, bookmark_entries, favicon_groups);
Who owns the memory for the observer?

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
File chrome/browser/importer/importer_list.cc (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/importer_list.cc:34: // IE always exists and doesn't
have multiple profiles.
Is it still true that IE always exists on Windows 10?

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/importer_list.cc:46: importer::SourceProfile* edge = new
importer::SourceProfile;
Hrm.  Not as part of this CL, but if you have time, it would be really nice to
clean up the memory model here, so that we're not passing around
implicitly-owned pointers.

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/importer_list.cc:50: edge->app_path.clear();
These two lines don't do anything -- right?  I do see that they're also present
in the DetectIEProfiles implementation above, but it seems pretty weird to
include them.

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
File chrome/browser/importer/importer_uma.cc (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/importer_uma.cc:25: IMPORTER_METRICS_EDGE            =
7,
nit: Should this be guarded with an "#if defined(OS_WIN)"?

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
File chrome/common/importer/edge_importer_utils_win.cc (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.cc:35: L"torage\\";
nit: This string is pretty oddly wrapped.  Could you please re-wrap it so that
the first line is longer, and the line break comes immediately after a "\\"?

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.cc:39: return base_key;
nit: What does the "base" in "base_key" mean?

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.cc:42: base::string16
GetPotentiallyOverridenIEKey(
nit: s/IE/Edge?

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.cc:44: base::string16
test_reg_override(
nit: s/reg/registry

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.cc:63: wchar_t buffer[MAX_PATH];
I noticed that a lot -- but not all -- of our other code initializes this buffer
to have a nul-terminator character at index 0, i.e. to be an empty C-string. 
I'm not sure why that's done, so I don't know whether its' needed here... but
maybe you know?

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.cc:64: if
(::SHGetFolderPath(NULL, CSIDL_LOCAL_APPDATA, NULL, SHGFP_TYPE_CURRENT,
Why do you need the leading "::"?

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.cc:65: buffer) != S_OK)
nit: I noticed that other code uses FAILED rather than comparing to S_OK.  I am
not a Windows programmer, but I wonder if that's the preferred style, or
possibly even functionally different somehow.

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.cc:78: DWORD ese_enabled = 0;
nit: I don't know what "ese" is.  Please either expand the acronym or document
it.

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
File chrome/common/importer/edge_importer_utils_win.h (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.h:14: // Overridable by tests via
IEImporterTestRegistryOverrider.
Why does the overrider have "IE" in its name, if it's overriding Edge data?

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.h:24: // Returns true if Edge
favorites is current in legacy (pre-Edge 13) mode.
nit: "current in" -> "currently in"?

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
File chrome/utility/importer/edge_importer_win.cc (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:26: 
nit: This line break is typically omitted in Chromium code.  (The other line
breaks separating includes are typically kept, though.)

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:46: WCHAR error_message[1024] = {};
nit: Why 1024?

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:60: bool set_last_error(JET_ERR
error) {
Please document what the return value means.  Also, since this is not just a
trivial setter, it's probably appropriate to name it SetLastError.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:64: return false;
nit: "return error == JET_errSuccess;"

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:83: table_id_ = JET_tableidNil;
Why is this line needed, given that it's in the destructor?

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:100: bool& is_null) {
Please pass these by pointer rather than by reference.  Passing by non-const
reference is pretty much disallowed by the Chromium style guide.  (Please fix
this throughout.)

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:110: set_last_error(err);
Is it important whether or not set_last_error is ever called for the "success"
cases?  In Reset() and Next() it is; whereas here it isn't.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:127: bool RetrieveColumn(const
base::string16& column_name, T& value) {
Chromium style generally speaking frowns upon default parameter values, which is
essentially what this function overload is providing.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:139: (column_data.size() >=
sizeof(value))) {
Why is this >= rather than simply =?  Truncating data seems bad.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:164: reinterpret_cast<const
base::char16*>(&column_data[0]);
I may be wrong, I don't think that this is a safe use of reinterpret_cast.  I
think that you want to memcpy instead.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:251: instance_id_ =
JET_instanceNil;
nit: Why is this line needed?

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:261: nullptr, JET_sesidNil,
JET_paramDatabasePageSize, 8192, nullptr)))
Why 8192?  Is that a guaranteed constant, or might it change with new Edge
releases?

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:296: return
scoped_ptr<EdgeDatabaseTableEnumerator>(
nit: You can use make_scoped_ptr here.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:326:
std::vector<EdgeFavoriteEntry*> children;
nit: Can the items be const?

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:348: return lhs->order_number <
rhs->order_number;
nit: Please implement this using std::tie: https://chromium-cpp.appspot.com/

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:355: base::FilePath
FindSpartanDatabase(base::FilePath profile_path) {
nit: Pass by const-ref?

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:357: profile_path.empty() ?
importer::GetEdgeDataFilePath() : profile_path;
Are both of these cases possible?  I'd kind of expect that profile_path is
either always empty or never empty -- no?

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:392:
favicon_base::FaviconUsageDataList* favicons,
nit: Outparams are usually written after the in-params.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:463: // From Edge 13 (released with
Windows 10 TH2) Favorites are stored in a JET
nit: Please add a comma after the closing paren.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:490: DLOG(ERROR) << "Error opening
database " << database.GetErrorMessage();
Optional nit: You probably want DVLOG or just LOG.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:506: :
source_path_.Append(L"DataStore");
nit: I'd move the .Append(L"DataStore") out of the ternary operator.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:509: do {
Is it possible for the enumerator to be empty?

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:546: for (auto& e :
database_entries) {
nit: s/e/entry

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:546: for (auto& e :
database_entries) {
nit: Can the entry be const?

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
File chrome/utility/importer/edge_importer_win.h (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.h:28: typedef
std::vector<ImportedBookmarkEntry> BookmarkVector;
nit: This is not actually used in the header.  I'd prefer that you remove it
entirely -- IMO this sort of typedef hurts readability more than it helps.  But,
if you decide to keep it, please either use it in the header, or move it to the
implementation file.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
File chrome/utility/importer/ie_importer_win.cc (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/ie_importer_win.cc:434: DCHECK(items ==
importer::FAVORITES);
nit: DCHECK_EQ

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/ie_importer_win.cc:437: source_path_ =
importer::GetEdgeDataFilePath();
Why isn't this just set on the SourceProfiler?

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/ie_importer_win.cc:798: // Source path exists during
testing as well as importing from Edge.
nit: "as well as importing" -> "as well as when importing"

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
File chrome/utility/importer/ie_importer_win.h (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/ie_importer_win.h:79: // Set to true when importing the
favorite order in old Edge on Windows 10.
nit: s/favorite/favorites

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/ie_importer_win.h:80: bool edge_import_mode_;
It's a little semantically weird for the IE importer to support Edge importing. 
Is it actually worthwhile to support importing from old versions of Edge?  How
likely are these to appear in the wild?  (Though since the cost of support is
low, maybe it's worth keeping even for a relatively small benefit.)

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
File chrome/utility/importer/importer_creator.cc (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/importer_creator.cc:26: Importer*
CreateImporterByType(ImporterType type) {
Here, too, it would be nice to have a scoped_ptr... but again, not necessarily
as part of this CL.

[Ilya Sherman, 2015-11-26 02:07:18]

Oh, also, I should note: I am not a Windows programmer, and so don't feel super
comfortable reviewing the windows-isms.  Jochen, if you're comfortable with
those, please pay extra close attention to them, as I have only looked at them
relatively superficially.

[jochen (gone - plz use gerrit), 2015-11-26 13:19:49]

jochen@chromium.org changed reviewers:
+ jschuh@chromium.org

[jochen (gone - plz use gerrit), 2015-11-26 13:19:49]

+jschuh because security and windows and stuff

[forshaw, 2015-11-30 12:57:59]

Made updates from review, PTAL.

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
File chrome/browser/importer/edge_importer_browsertest_win.cc (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/edge_importer_browsertest_win.cc:45: :
ProfileWriter(NULL),
On 2015/11/26 02:04:43, Ilya Sherman wrote:
> nit: nullptr

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/edge_importer_browsertest_win.cc:47:
bookmark_entries_(bookmark_entries),
On 2015/11/26 02:04:43, Ilya Sherman wrote:
> nit: Maybe prefix this with "expected_"?

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/edge_importer_browsertest_win.cc:49:
favicon_groups_(favicon_groups),
On 2015/11/26 02:04:43, Ilya Sherman wrote:
> Ditto

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/edge_importer_browsertest_win.cc:87:
EXPECT_EQ(usage[i].urls.size(), 1);
On 2015/11/26 02:04:43, Ilya Sherman wrote:
> Actually, this expectation seems oddly placed -- is it not redundant with the
> one on line 95?

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/edge_importer_browsertest_win.cc:87:
EXPECT_EQ(usage[i].urls.size(), 1);
On 2015/11/26 02:04:43, Ilya Sherman wrote:
> nit: I think you might need to write "1U"

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/edge_importer_browsertest_win.cc:178: }
On 2015/11/26 02:04:43, Ilya Sherman wrote:
> Can you just slurp in the whole file and then call compression:GzipUncompress?

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/edge_importer_browsertest_win.cc:348: new
TestObserver(importer::FAVORITES, bookmark_entries, favicon_groups);
On 2015/11/26 02:04:43, Ilya Sherman wrote:
> Who owns the memory for the observer?

Good question, this was basically taken from the IEImporter browser test. I'll
use a scoped_refptr like the FF importer browser test does.

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
File chrome/browser/importer/importer_list.cc (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/importer_list.cc:34: // IE always exists and doesn't
have multiple profiles.
On 2015/11/26 02:04:43, Ilya Sherman wrote:
> Is it still true that IE always exists on Windows 10?

Yes it's still there, things like the favorites are actually baked into the
Explorer shell so even if you remove the IE GUI you still have IE installed in a
sense.

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/importer_list.cc:46: importer::SourceProfile* edge = new
importer::SourceProfile;
On 2015/11/26 02:04:43, Ilya Sherman wrote:
> Hrm.  Not as part of this CL, but if you have time, it would be really nice to
> clean up the memory model here, so that we're not passing around
> implicitly-owned pointers.

Will look at it, does seem pretty nasty.

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/importer_list.cc:50: edge->app_path.clear();
On 2015/11/26 02:04:43, Ilya Sherman wrote:
> These two lines don't do anything -- right?  I do see that they're also
present
> in the DetectIEProfiles implementation above, but it seems pretty weird to
> include them.

Nope probably not, just copied again from above.

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
File chrome/browser/importer/importer_uma.cc (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/browser/importe...
chrome/browser/importer/importer_uma.cc:25: IMPORTER_METRICS_EDGE            =
7,
On 2015/11/26 02:04:43, Ilya Sherman wrote:
> nit: Should this be guarded with an "#if defined(OS_WIN)"?

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
File chrome/common/importer/edge_importer_utils_win.cc (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.cc:35: L"torage\\";
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> nit: This string is pretty oddly wrapped.  Could you please re-wrap it so that
> the first line is longer, and the line break comes immediately after a "\\"?

Will fix, clearly cl format doesn't understand the best place to break path
strings.

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.cc:39: return base_key;
On 2015/11/26 02:04:43, Ilya Sherman wrote:
> nit: What does the "base" in "base_key" mean?

Will rename.

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.cc:63: wchar_t buffer[MAX_PATH];
On 2015/11/26 02:04:43, Ilya Sherman wrote:
> I noticed that a lot -- but not all -- of our other code initializes this
buffer
> to have a nul-terminator character at index 0, i.e. to be an empty C-string. 
> I'm not sure why that's done, so I don't know whether its' needed here... but
> maybe you know?

This is a Windows coding idiom to initialize arrays and objects, it isn't
necessary unless the value of the buffer was to be later used. In this case the
buffer is not used unless SHGetFolderPath returns S_OK so doesn't need to be
initialized but will do for consistency.

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.cc:64: if
(::SHGetFolderPath(NULL, CSIDL_LOCAL_APPDATA, NULL, SHGFP_TYPE_CURRENT,
On 2015/11/26 02:04:43, Ilya Sherman wrote:
> Why do you need the leading "::"?

Again a common Windows idiom to ensure you access the global namespace as all
Windows APIs are within that namespace.

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.cc:65: buffer) != S_OK)
On 2015/11/26 02:04:43, Ilya Sherman wrote:
> nit: I noticed that other code uses FAILED rather than comparing to S_OK.  I
am
> not a Windows programmer, but I wonder if that's the preferred style, or
> possibly even functionally different somehow.

I think the other code is technically wrong, but would work in 99% of cases. In
this case the function is documented to return S_OK on success but can return
S_FALSE in certain error conditions. S_FALSE is considered to be a success from
the FAILED macro perspective. Confusing I realize.

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.cc:78: DWORD ese_enabled = 0;
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> nit: I don't know what "ese" is.  Please either expand the acronym or document
> it.

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
File chrome/common/importer/edge_importer_utils_win.h (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.h:14: // Overridable by tests via
IEImporterTestRegistryOverrider.
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> Why does the overrider have "IE" in its name, if it's overriding Edge data?

I repurpose the IE registry overrider which is has the exact same functionality
I'd need for supporting the edge tests, there seemed little reason to implement
the same functionality twice, but also not sure it was worth renaming the class.
I could do.

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.h:24: // Returns true if Edge
favorites is current in legacy (pre-Edge 13) mode.
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> nit: "current in" -> "currently in"?

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
File chrome/utility/importer/edge_importer_win.cc (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:26: 
On 2015/11/26 02:04:45, Ilya Sherman wrote:
> nit: This line break is typically omitted in Chromium code.  (The other line
> breaks separating includes are typically kept, though.)

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:46: WCHAR error_message[1024] = {};
On 2015/11/26 02:04:45, Ilya Sherman wrote:
> nit: Why 1024?

Just an arbitrary size, the API provides no way of determining the error message
size and there's no constant or define which describes a maximum size for this
parameter.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:60: bool set_last_error(JET_ERR
error) {
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> Please document what the return value means.  Also, since this is not just a
> trivial setter, it's probably appropriate to name it SetLastError.

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:64: return false;
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> nit: "return error == JET_errSuccess;"

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:83: table_id_ = JET_tableidNil;
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> Why is this line needed, given that it's in the destructor?

I tend to do this to reduce risk of breakage if it gets moved around at a later
date and accidentally gets released twice. But will remove it.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:100: bool& is_null) {
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> Please pass these by pointer rather than by reference.  Passing by non-const
> reference is pretty much disallowed by the Chromium style guide.  (Please fix
> this throughout.)

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:110: set_last_error(err);
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> Is it important whether or not set_last_error is ever called for the "success"
> cases?  In Reset() and Next() it is; whereas here it isn't.

Not really important, just in most cases there's no need to distinguish between
two "success" codes so a straight call to set_last_error seems reasonable. In
this case as I need to distinguish between the success and the Null column I had
to add a check anyway so didn't seem to make much of a difference. But I'll move
it for consistency anyway.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:127: bool RetrieveColumn(const
base::string16& column_name, T& value) {
On 2015/11/26 02:04:45, Ilya Sherman wrote:
> Chromium style generally speaking frowns upon default parameter values, which
is
> essentially what this function overload is providing.

Well I'd argue that it isn't providing default parameter values, it's hiding
information being returned that we don't actually care about. This was used when
I thought I'd need to distinguish between Null columns and not Null columns. I
could just remove that functionality entirely now.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:139: (column_data.size() >=
sizeof(value))) {
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> Why is this >= rather than simply =?  Truncating data seems bad.

The database engine is one of the ones where the type enforcement is done by the
calling application not the engine. There's no technical reason that a 4 byte
'int' field couldn't actually be considerably larger. I did this just from a
resilience perspective if Edge devs ever messed up and added too much data to
the column or they overloaded the data to add additional information. Maybe
unlikely but I'd expect that any truncated data is only ever going to be data
which wouldn't matter to the importer. I can change if you feel strongly enough.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:164: reinterpret_cast<const
base::char16*>(&column_data[0]);
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> I may be wrong, I don't think that this is a safe use of reinterpret_cast.  I
> think that you want to memcpy instead.

Reworked

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:251: instance_id_ =
JET_instanceNil;
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> nit: Why is this line needed?

As above but removed.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:261: nullptr, JET_sesidNil,
JET_paramDatabasePageSize, 8192, nullptr)))
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> Why 8192?  Is that a guaranteed constant, or might it change with new Edge
> releases?

In theory it could change but it's unlikely to do so. But I'll push it out as a
constant anyway.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:296: return
scoped_ptr<EdgeDatabaseTableEnumerator>(
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> nit: You can use make_scoped_ptr here.

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:326:
std::vector<EdgeFavoriteEntry*> children;
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> nit: Can the items be const?

I've moved the sorting of the children out of the recursive loop so that this
can now be const.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:348: return lhs->order_number <
rhs->order_number;
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> nit: Please implement this using std::tie: https://chromium-cpp.appspot.com/

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:355: base::FilePath
FindSpartanDatabase(base::FilePath profile_path) {
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> nit: Pass by const-ref?

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:357: profile_path.empty() ?
importer::GetEdgeDataFilePath() : profile_path;
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> Are both of these cases possible?  I'd kind of expect that profile_path is
> either always empty or never empty -- no?

GetEdgeDataFilePath could also return an empty path so the second check guards
again that function failing. The logic is supposed to be if the passed profile
path has overridden the location use that. If not try and get the "real" profile
path but verify if that fails.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:392:
favicon_base::FaviconUsageDataList* favicons,
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> nit: Outparams are usually written after the in-params.

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:463: // From Edge 13 (released with
Windows 10 TH2) Favorites are stored in a JET
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> nit: Please add a comma after the closing paren.

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:490: DLOG(ERROR) << "Error opening
database " << database.GetErrorMessage();
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> Optional nit: You probably want DVLOG or just LOG.

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:506: :
source_path_.Append(L"DataStore");
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> nit: I'd move the .Append(L"DataStore") out of the ternary operator.

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:509: do {
On 2015/11/26 02:04:45, Ilya Sherman wrote:
> Is it possible for the enumerator to be empty?

Yes it could be, this would be caught by the loop failing. But I'll add a check
for the return code from Reset just in case.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:546: for (auto& e :
database_entries) {
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> nit: s/e/entry

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:546: for (auto& e :
database_entries) {
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> nit: Can the entry be const?

In the loop? Don't think so unless I'm missing something as we're changing the
children member. Or do you mean adding into the children member? I've changed
that as above.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:546: for (auto& e :
database_entries) {
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> nit: s/e/entry

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:546: for (auto& e :
database_entries) {
On 2015/11/26 02:04:44, Ilya Sherman wrote:
> nit: Can the entry be const?

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
File chrome/utility/importer/edge_importer_win.h (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.h:28: typedef
std::vector<ImportedBookmarkEntry> BookmarkVector;
On 2015/11/26 02:04:45, Ilya Sherman wrote:
> nit: This is not actually used in the header.  I'd prefer that you remove it
> entirely -- IMO this sort of typedef hurts readability more than it helps. 
But,
> if you decide to keep it, please either use it in the header, or move it to
the
> implementation file.

Removed, just a holdover from previous IE based implementation.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
File chrome/utility/importer/ie_importer_win.cc (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/ie_importer_win.cc:434: DCHECK(items ==
importer::FAVORITES);
On 2015/11/26 02:04:45, Ilya Sherman wrote:
> nit: DCHECK_EQ

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/ie_importer_win.cc:437: source_path_ =
importer::GetEdgeDataFilePath();
On 2015/11/26 02:04:45, Ilya Sherman wrote:
> Why isn't this just set on the SourceProfiler?

Moved this to being initialized in importer_list.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/ie_importer_win.cc:798: // Source path exists during
testing as well as importing from Edge.
On 2015/11/26 02:04:45, Ilya Sherman wrote:
> nit: "as well as importing" -> "as well as when importing"

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
File chrome/utility/importer/ie_importer_win.h (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/ie_importer_win.h:79: // Set to true when importing the
favorite order in old Edge on Windows 10.
On 2015/11/26 02:04:45, Ilya Sherman wrote:
> nit: s/favorite/favorites

Acknowledged.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/ie_importer_win.h:80: bool edge_import_mode_;
On 2015/11/26 02:04:45, Ilya Sherman wrote:
> It's a little semantically weird for the IE importer to support Edge
importing. 
> Is it actually worthwhile to support importing from old versions of Edge?  How
> likely are these to appear in the wild?  (Though since the cost of support is
> low, maybe it's worth keeping even for a relatively small benefit.)

Well technically Edge is stripped down IE code so the old favorites model was
based on what was already in IE. Imo it therefore makes sense and also removes
code duplication. It would be nice to not implement it. At least for consumer
versions of Windows 10 that is likely to be using the latest version of Edge but
for Enterprise customers they're on a slower release cycle which means they'll
be using the older version of Edge for some time.

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
File chrome/utility/importer/importer_creator.cc (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/utility/importe...
chrome/utility/importer/importer_creator.cc:26: Importer*
CreateImporterByType(ImporterType type) {
On 2015/11/26 02:04:45, Ilya Sherman wrote:
> Here, too, it would be nice to have a scoped_ptr... but again, not necessarily
> as part of this CL.

Acknowledged.

[forshaw, 2015-11-30 17:12:21]

forshaw@chromium.org changed reviewers:
+ ananta@chromium.org
- jschuh@chromium.org

[forshaw, 2015-11-30 17:12:22]

ananta@ any chance you could check out the Windows specific code (which is
mostly in edge_*) from this CL?

Thanks very much.

[Ilya Sherman, 2015-12-01 07:31:18]

Thanks!  LGTM % the remaining nits, though I'd still like ananta@ or someone
else quite familiar with Windows to take a look.

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
File chrome/common/importer/edge_importer_utils_win.h (right):

https://codereview.chromium.org/1465853002/diff/200001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.h:14: // Overridable by tests via
IEImporterTestRegistryOverrider.
On 2015/11/30 12:57:58, forshaw wrote:
> On 2015/11/26 02:04:44, Ilya Sherman wrote:
> > Why does the overrider have "IE" in its name, if it's overriding Edge data?
> 
> I repurpose the IE registry overrider which is has the exact same
functionality
> I'd need for supporting the edge tests, there seemed little reason to
implement
> the same functionality twice, but also not sure it was worth renaming the
class.
> I could do.

I think it might be useful to rename the class.  For example, you could just
drop the "IE" prefix, or possibly replace it with "Windows".  It's not a big
deal, though.

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
File chrome/utility/importer/edge_importer_win.cc (right):

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:68: static const size_t
kErrorMessagesize = 1024;
nit: Please move this to the top of the file, as with the string constants.

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:68: static const size_t
kErrorMessagesize = 1024;
Since you mentioned that this is an arbitrarily chosen size, let's make sure to
document that the choice is arbitrary.  It might help future code spelunkers =)

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:68: static const size_t
kErrorMessagesize = 1024;
nit: s/size/Size

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:85: }
nit: You can now drop the curly braces.

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:157:
value_ref.resize(column_data.size() / sizeof(base::char16));
nit: Reuse char_length here?

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:215: // 0 means indicates an
invalid column.
nit: s/means indicates/indicates

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:244: }
nit: You can remove the curly braces here.

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:384: void
BuildBookmarkEntries(const EdgeFavoriteEntry* current_entry,
nit: Pass by const-ref?

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:481: DVLOG(ERROR) << "Error opening
database " << database.GetErrorMessage();
nit: Does this compile?  I think DVLOG expects a number rather than "ERROR" etc.
 That is, I'd expect this to be DVLOG(1).

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:499: return;
nit: I'd move this up, above the favicon_base computation.

[forshaw, 2015-12-01 11:12:13]

Thanks isherman@ for the review, it's very much appreciated. I've fixed the nits
and renamed the class as suggested. I'll try and get one of the Windows devs to
do a sanity pass over everything before committing it.

Also once this patch is committed I'll submit a CL to fix up the pointer
handling as requested.

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
File chrome/utility/importer/edge_importer_win.cc (right):

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:68: static const size_t
kErrorMessagesize = 1024;
On 2015/12/01 07:31:18, Ilya Sherman wrote:
> nit: Please move this to the top of the file, as with the string constants.

Acknowledged.

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:68: static const size_t
kErrorMessagesize = 1024;
On 2015/12/01 07:31:18, Ilya Sherman wrote:
> Since you mentioned that this is an arbitrarily chosen size, let's make sure
to
> document that the choice is arbitrary.  It might help future code spelunkers
=)

Acknowledged.

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:68: static const size_t
kErrorMessagesize = 1024;
On 2015/12/01 07:31:18, Ilya Sherman wrote:
> nit: s/size/Size

Acknowledged.

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:85: }
On 2015/12/01 07:31:18, Ilya Sherman wrote:
> nit: You can now drop the curly braces.

Acknowledged.

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:157:
value_ref.resize(column_data.size() / sizeof(base::char16));
On 2015/12/01 07:31:18, Ilya Sherman wrote:
> nit: Reuse char_length here?

Acknowledged.

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:215: // 0 means indicates an
invalid column.
On 2015/12/01 07:31:18, Ilya Sherman wrote:
> nit: s/means indicates/indicates

Acknowledged.

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:244: }
On 2015/12/01 07:31:18, Ilya Sherman wrote:
> nit: You can remove the curly braces here.

Acknowledged.

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:384: void
BuildBookmarkEntries(const EdgeFavoriteEntry* current_entry,
On 2015/12/01 07:31:18, Ilya Sherman wrote:
> nit: Pass by const-ref?

Acknowledged.

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:481: DVLOG(ERROR) << "Error opening
database " << database.GetErrorMessage();
On 2015/12/01 07:31:18, Ilya Sherman wrote:
> nit: Does this compile?  I think DVLOG expects a number rather than "ERROR"
etc.
>  That is, I'd expect this to be DVLOG(1).

Acknowledged.

https://codereview.chromium.org/1465853002/diff/240001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:499: return;
On 2015/12/01 07:31:18, Ilya Sherman wrote:
> nit: I'd move this up, above the favicon_base computation.

Acknowledged.

[ananta, 2015-12-01 19:36:35]

https://codereview.chromium.org/1465853002/diff/260001/chrome/common/importer...
File chrome/common/importer/edge_importer_utils_win.cc (right):

https://codereview.chromium.org/1465853002/diff/260001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.cc:74: if
(key.ReadValueDW(L"FavoritesESEEnabled", &ese_enabled) == ERROR_SUCCESS)
This defaults to true?

https://codereview.chromium.org/1465853002/diff/260001/chrome/utility/importe...
File chrome/utility/importer/edge_importer_win.cc (right):

https://codereview.chromium.org/1465853002/diff/260001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:43: class EdgeErrorObject {
Please move the Edge database reading classes to their own files. Would provide
a way to unit test them as well.

[forshaw, 2015-12-02 15:35:05]

Patchset #11 (id:280001) has been deleted

[forshaw, 2015-12-02 18:21:54]

ananta@ I've split out the edge database classes and added a set of unit tests
for it. PTAL.

Thanks.

https://codereview.chromium.org/1465853002/diff/260001/chrome/common/importer...
File chrome/common/importer/edge_importer_utils_win.cc (right):

https://codereview.chromium.org/1465853002/diff/260001/chrome/common/importer...
chrome/common/importer/edge_importer_utils_win.cc:74: if
(key.ReadValueDW(L"FavoritesESEEnabled", &ese_enabled) == ERROR_SUCCESS)
On 2015/12/01 19:36:35, ananta wrote:
> This defaults to true?

Yes, if the registry key doesn't exist then legacy favorites is in use (as when
Edge 13 upgrades it then writes the key).

https://codereview.chromium.org/1465853002/diff/260001/chrome/utility/importe...
File chrome/utility/importer/edge_importer_win.cc (right):

https://codereview.chromium.org/1465853002/diff/260001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:43: class EdgeErrorObject {
On 2015/12/01 19:36:35, ananta wrote:
> Please move the Edge database reading classes to their own files. Would
provide
> a way to unit test them as well.

Acknowledged.

[ananta, 2015-12-02 21:33:11]

lgtm % nits/questions

https://codereview.chromium.org/1465853002/diff/320001/chrome/utility/importe...
File chrome/utility/importer/edge_database_reader_unittest_win.cc (right):

https://codereview.chromium.org/1465853002/diff/320001/chrome/utility/importe...
chrome/utility/importer/edge_database_reader_unittest_win.cc:21: 
Thanks for adding these tests.

https://codereview.chromium.org/1465853002/diff/320001/chrome/utility/importe...
File chrome/utility/importer/edge_database_reader_win.cc (right):

https://codereview.chromium.org/1465853002/diff/320001/chrome/utility/importe...
chrome/utility/importer/edge_database_reader_win.cc:163:
SetLastError(JET_errInvalidColumnType);
Should there be a trace error here?

https://codereview.chromium.org/1465853002/diff/320001/chrome/utility/importe...
File chrome/utility/importer/edge_importer_win.cc (right):

https://codereview.chromium.org/1465853002/diff/320001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:214: DVLOG(1) << "Error opening
database " << database.GetErrorMessage();
Should this be a regular log error or a check perhaps to see if we hit this in
the field?

https://codereview.chromium.org/1465853002/diff/320001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:221: DVLOG(1) << "Error opening
database table " << database.GetErrorMessage();
Ditto for replacing this with a log error and or a check

[forshaw, 2015-12-02 21:56:12]

ananta@ thanks for the review. If you want me to really sort out the nits I
will, it seems mainly down to logging which I might let slide for now unless
there a compelling reason.

https://codereview.chromium.org/1465853002/diff/320001/chrome/utility/importe...
File chrome/utility/importer/edge_database_reader_win.cc (right):

https://codereview.chromium.org/1465853002/diff/320001/chrome/utility/importe...
chrome/utility/importer/edge_database_reader_win.cc:163:
SetLastError(JET_errInvalidColumnType);
On 2015/12/02 21:33:11, ananta wrote:
> Should there be a trace error here?

I expect if this fails then the entire import would fail. Not sure if it should
be exposed at a higher level?

https://codereview.chromium.org/1465853002/diff/320001/chrome/utility/importe...
File chrome/utility/importer/edge_importer_win.cc (right):

https://codereview.chromium.org/1465853002/diff/320001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:214: DVLOG(1) << "Error opening
database " << database.GetErrorMessage();
On 2015/12/02 21:33:11, ananta wrote:
> Should this be a regular log error or a check perhaps to see if we hit this in
> the field?

I did have this originally as a normal LOG but was changed from a previous
review. I can change it back if you feel strongly enough.

[Ilya Sherman, 2015-12-02 21:59:32]

https://codereview.chromium.org/1465853002/diff/320001/chrome/utility/importe...
File chrome/utility/importer/edge_importer_win.cc (right):

https://codereview.chromium.org/1465853002/diff/320001/chrome/utility/importe...
chrome/utility/importer/edge_importer_win.cc:214: DVLOG(1) << "Error opening
database " << database.GetErrorMessage();
On 2015/12/02 21:56:11, forshaw wrote:
> On 2015/12/02 21:33:11, ananta wrote:
> > Should this be a regular log error or a check perhaps to see if we hit this
in
> > the field?
> 
> I did have this originally as a normal LOG but was changed from a previous
> review. I can change it back if you feel strongly enough.

FWIW, you previously had DLOG, which isn't going to be hit in Release builds. 
I'd suggested either upgrading to a LOG, or changing to a DVLOG.  I'm fine with
either option.

[forshaw, 2015-12-02 22:01:35]

On 2015/12/02 21:59:32, Ilya Sherman wrote:
>
https://codereview.chromium.org/1465853002/diff/320001/chrome/utility/importe...
> File chrome/utility/importer/edge_importer_win.cc (right):
> 
>
https://codereview.chromium.org/1465853002/diff/320001/chrome/utility/importe...
> chrome/utility/importer/edge_importer_win.cc:214: DVLOG(1) << "Error opening
> database " << database.GetErrorMessage();
> On 2015/12/02 21:56:11, forshaw wrote:
> > On 2015/12/02 21:33:11, ananta wrote:
> > > Should this be a regular log error or a check perhaps to see if we hit
this
> in
> > > the field?
> > 
> > I did have this originally as a normal LOG but was changed from a previous
> > review. I can change it back if you feel strongly enough.
> 
> FWIW, you previously had DLOG, which isn't going to be hit in Release builds. 
> I'd suggested either upgrading to a LOG, or changing to a DVLOG.  I'm fine
with
> either option.

Ah sorry I misspoke, I have DVLOG atm, in theory that's okay?

[Ilya Sherman, 2015-12-02 22:03:30]

On 2015/12/02 22:01:35, forshaw wrote:
> On 2015/12/02 21:59:32, Ilya Sherman wrote:
> >
>
https://codereview.chromium.org/1465853002/diff/320001/chrome/utility/importe...
> > File chrome/utility/importer/edge_importer_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/1465853002/diff/320001/chrome/utility/importe...
> > chrome/utility/importer/edge_importer_win.cc:214: DVLOG(1) << "Error opening
> > database " << database.GetErrorMessage();
> > On 2015/12/02 21:56:11, forshaw wrote:
> > > On 2015/12/02 21:33:11, ananta wrote:
> > > > Should this be a regular log error or a check perhaps to see if we hit
> this
> > in
> > > > the field?
> > > 
> > > I did have this originally as a normal LOG but was changed from a previous
> > > review. I can change it back if you feel strongly enough.
> > 
> > FWIW, you previously had DLOG, which isn't going to be hit in Release
builds. 
> > I'd suggested either upgrading to a LOG, or changing to a DVLOG.  I'm fine
> with
> > either option.
> 
> Ah sorry I misspoke, I have DVLOG atm, in theory that's okay?

The difference is basically whether you want to include these logs in Release
builds or not.  DVLOG is compiled out of Release builds.  LOG is compiled and
enabled in Release builds.  DLOG is compiled in, but off by default -- kind of
the worst of both worlds.

[Nico, 2015-12-02 22:08:31]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-12-02 22:08:31]

.gypi and .gn files lgtm

https://codereview.chromium.org/1465853002/diff/320001/chrome/chrome_utility....
File chrome/chrome_utility.gypi (right):

https://codereview.chromium.org/1465853002/diff/320001/chrome/chrome_utility....
chrome/chrome_utility.gypi:154: 'esent.lib',
(why isn't this needed for wininet.dll?)

https://codereview.chromium.org/1465853002/diff/320001/chrome/chrome_utility....
chrome/chrome_utility.gypi:155: ]
add , after ]

[forshaw, 2015-12-03 11:59:55]

Patchset #13 (id:340001) has been deleted

[forshaw, 2015-12-03 12:01:11]

Thanks everyone for the reviews, it's much appreciated.

[forshaw, 2015-12-03 12:01:19]

The CQ bit was checked by forshaw@chromium.org

[forshaw, 2015-12-03 12:01:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1465853002/#ps320001
(title: "Added missing include files")

[commit-bot: I haz the power, 2015-12-03 12:02:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1465853002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1465853002/320001

[commit-bot: I haz the power, 2015-12-03 14:08:27]

Committed patchset #12 (id:320001)

[commit-bot: I haz the power, 2015-12-03 14:09:27]

Description was changed from

==========
Implement support for importing favorites from Edge on Windows 10.
This CL adds support for importing favorties from the Edge browser on
Windows 10. It supports the old style importation from internet shortcuts
(without supporting order due to structure changes) as well as importing
from Edge 13 database format.

BUG=539746
==========

to

==========
Implement support for importing favorites from Edge on Windows 10.
This CL adds support for importing favorties from the Edge browser on
Windows 10. It supports the old style importation from internet shortcuts
(without supporting order due to structure changes) as well as importing
from Edge 13 database format.

BUG=539746

Committed: https://crrev.com/57e648081288644895bf6f5fe184597a63282336
Cr-Commit-Position: refs/heads/master@{#362977}
==========

[commit-bot: I haz the power, 2015-12-03 14:09:28]

Patchset 12 (id:??) landed as
https://crrev.com/57e648081288644895bf6f5fe184597a63282336
Cr-Commit-Position: refs/heads/master@{#362977}

[Nico, 2015-12-09 18:23:06]

https://codereview.chromium.org/1465853002/diff/320001/chrome/utility/importe...
File chrome/utility/importer/edge_database_reader_unittest_win.cc (right):

https://codereview.chromium.org/1465853002/diff/320001/chrome/utility/importe...
chrome/utility/importer/edge_database_reader_unittest_win.cc:237:
EXPECT_EQ(row_count, 16);
FYI: It's EXPECT_EQ(expected, actual), not EXPECT_EQ(actual, expected). This
matters for the "expected $foo but got $bar" message when the test fails.