First cut at Safari Import - Home Page & History Only.

First cut at Safari Import - Home Page & History Only.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=22179

[jeremy, 2009-07-30 23:18:57]

This is just a first stab, sending it out for review now to make sure everyone
is on board with the direction this is going in.

Reviewers:
Stuart: Mac stuff.
Rahul: Importer stuff.
Sky: Does the history translation look ok to you?

cced:
Mark: Time conversion funcs and changes to mac_utils seem like things you should
weigh in on.
Brett: History system master.

[Mark Mentovai, 2009-07-30 23:50:54]

I've only looked at mac_util and the time stuff, which I think is all you wanted
me to look at.  LGTM on that front.  Let me know if you want me to review
anything else here.

http://codereview.chromium.org/159668/diff/1/4
File base/mac_util.mm (right):

http://codereview.chromium.org/159668/diff/1/4#newcode57
Line 57: NSArray* dirs =
Wonky indent.

http://codereview.chromium.org/159668/diff/1/4#newcode67
Line 67: return FilePath();
I'd structure the function so that success comes last (and you don't need "else"
after return):

if ([dirs count == 0)
  return

NSString* library_dir =
if (!library_dir)
  return

return FilePath

http://codereview.chromium.org/159668/diff/1/10
File chrome/browser/importer/safari_importer.mm (right):

http://codereview.chromium.org/159668/diff/1/10#newcode93
Line 93: const double kCFToUnixEpochDelta = 978307200;
This number seems like magic.  You should add to the comment that the Unix epoch
is 1970-01-01 00:00:00.0 UTC and the CF epoch is 2001-01-01 00:00:00.0 UTC.

http://codereview.chromium.org/159668/diff/1/10#newcode96
Line 96: return CFStringGetDoubleValue((CFStringRef)history_time) +
C++ cast

http://codereview.chromium.org/159668/diff/1/10#newcode97
Line 97: kCFToUnixEpochDelta;
Move this over so it lines up just to the right of the (.  When things fit that
way, it's usually more readable than just giving the extra 4-space indent.

[jeremy, 2009-07-31 00:01:54]

All fixed, thanks!

[sky, 2009-07-31 15:08:01]

http://codereview.chromium.org/159668/diff/18/1021
File base/mac_util.mm (right):

http://codereview.chromium.org/159668/diff/18/1021#newcode58
Line 58: NSSearchPathForDirectoriesInDomains(NSLibraryDirectory,
nit: this should be 4 space indented.

http://codereview.chromium.org/159668/diff/18/1025
File chrome/browser/importer/safari_importer.h (right):

http://codereview.chromium.org/159668/diff/18/1025#newcode23
Line 23: SafariImporter(const FilePath& library_dir);
explicit

http://codereview.chromium.org/159668/diff/18/1025#newcode43
Line 43: NSString* ParseHomePage();
description?

http://codereview.chromium.org/159668/diff/18/1025#newcode47
Line 47: double HistoryTimeToEpochTime(NSString *history_time);
nit: space should be after *, not before it.

http://codereview.chromium.org/159668/diff/18/1025#newcode50
Line 50: void ParseHistoryItems(std::vector<history::URLRow> *history_items);
nit: space should be after *, not before it.

http://codereview.chromium.org/159668/diff/18/1026
File chrome/browser/importer/safari_importer.mm (right):

http://codereview.chromium.org/159668/diff/18/1026#newcode28
Line 28: if (!url.is_valid())
nit: this could just be:
  return url.is_valid()

http://codereview.chromium.org/159668/diff/18/1026#newcode36
Line 36: SafariImporter::SafariImporter(const FilePath& library_dir) :
nit: the ":" should be on the following line.
You should also NULL out writer_ too.

http://codereview.chromium.org/159668/diff/18/1026#newcode44
Line 44: uint16 items, ProfileWriter* writer,
nit: spacing is off.

http://codereview.chromium.org/159668/diff/18/1026#newcode48
Line 48: // source_path_ = profile_info.source_path;
If you don't need these commented out lines, please remove them.

http://codereview.chromium.org/159668/diff/18/1026#newcode60
Line 60: }
Doesn't Safari support different search engines?

http://codereview.chromium.org/159668/diff/18/1026#newcode75
Line 75: }
How about some NOTIMPLEMENTEDs or at least TODOs here.

http://codereview.chromium.org/159668/diff/18/1026#newcode115
Line 115: NSDictionary *history_dict = [NSDictionary
nit: space should be after *, not before it.

http://codereview.chromium.org/159668/diff/18/1026#newcode120
Line 120: for(NSDictionary *history_item in safari_history_items) {
nit: space should be after *, not before it.
I'm not intimately familiar with this enhanced for syntax. Does it properly deal
if safari_history_items is NULL?

http://codereview.chromium.org/159668/diff/18/1026#newcode165
Line 165: string16 hompeage_str = base::SysNSStringToUTF16(ParseHomePage());
Does this work of if ParseHomePage returns NULL?

[jeremy, 2009-07-31 15:59:22]

All fixed, ready for another look, thanks!

http://codereview.chromium.org/159668/diff/18/1021
File base/mac_util.mm (right):

http://codereview.chromium.org/159668/diff/18/1021#newcode58
Line 58: NSSearchPathForDirectoriesInDomains(NSLibraryDirectory,
On 2009/07/31 15:08:01, sky wrote:
> nit: this should be 4 space indented.

Done.

http://codereview.chromium.org/159668/diff/18/1025
File chrome/browser/importer/safari_importer.h (right):

http://codereview.chromium.org/159668/diff/18/1025#newcode23
Line 23: SafariImporter(const FilePath& library_dir);
On 2009/07/31 15:08:01, sky wrote:
> explicit

Done.

http://codereview.chromium.org/159668/diff/18/1025#newcode43
Line 43: NSString* ParseHomePage();
On 2009/07/31 15:08:01, sky wrote:
> description?

Done.

http://codereview.chromium.org/159668/diff/18/1025#newcode47
Line 47: double HistoryTimeToEpochTime(NSString *history_time);
On 2009/07/31 15:08:01, sky wrote:
> nit: space should be after *, not before it.

Done.

http://codereview.chromium.org/159668/diff/18/1025#newcode50
Line 50: void ParseHistoryItems(std::vector<history::URLRow> *history_items);
On 2009/07/31 15:08:01, sky wrote:
> nit: space should be after *, not before it.

Done.

http://codereview.chromium.org/159668/diff/18/1026
File chrome/browser/importer/safari_importer.mm (right):

http://codereview.chromium.org/159668/diff/18/1026#newcode28
Line 28: if (!url.is_valid())
I purposefully went with the same pattern as ff_importer since this function is
a stand-in for if we need to filter URLs in which case the structure is correct.

http://codereview.chromium.org/159668/diff/18/1026#newcode36
Line 36: SafariImporter::SafariImporter(const FilePath& library_dir) :
On 2009/07/31 15:08:01, sky wrote:
> nit: the ":" should be on the following line.
> You should also NULL out writer_ too.

Done.

http://codereview.chromium.org/159668/diff/18/1026#newcode44
Line 44: uint16 items, ProfileWriter* writer,
On 2009/07/31 15:08:01, sky wrote:
> nit: spacing is off.

Done.

http://codereview.chromium.org/159668/diff/18/1026#newcode48
Line 48: // source_path_ = profile_info.source_path;
On 2009/07/31 15:08:01, sky wrote:
> If you don't need these commented out lines, please remove them.

Done.

http://codereview.chromium.org/159668/diff/18/1026#newcode60
Line 60: }
On 2009/07/31 15:08:01, sky wrote:
> Doesn't Safari support different search engines?

Done.

http://codereview.chromium.org/159668/diff/18/1026#newcode75
Line 75: }
On 2009/07/31 15:08:01, sky wrote:
> How about some NOTIMPLEMENTEDs or at least TODOs here.

Done.

http://codereview.chromium.org/159668/diff/18/1026#newcode115
Line 115: NSDictionary *history_dict = [NSDictionary
On 2009/07/31 15:08:01, sky wrote:
> nit: space should be after *, not before it.

Done.

http://codereview.chromium.org/159668/diff/18/1026#newcode120
Line 120: for(NSDictionary *history_item in safari_history_items) {
On 2009/07/31 15:08:01, sky wrote:
> nit: space should be after *, not before it.
> I'm not intimately familiar with this enhanced for syntax. Does it properly
deal
> if safari_history_items is NULL?

Done.

http://codereview.chromium.org/159668/diff/18/1026#newcode165
Line 165: string16 hompeage_str = base::SysNSStringToUTF16(ParseHomePage());
Good catch, thanks!

[sky, 2009-07-31 16:17:26]

LGTM

[kuchhal, 2009-07-31 16:22:16]

lgtm.

some nits below.

http://codereview.chromium.org/159668/diff/1034/29
File chrome/browser/importer/safari_importer.h (right):

http://codereview.chromium.org/159668/diff/1034/29#newcode30
Line 30: MessageLoop* delagate_loop,
delegate_loop

http://codereview.chromium.org/159668/diff/1034/29#newcode64
Line 64: DISALLOW_EVIL_CONSTRUCTORS(SafariImporter);
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/159668/diff/1034/34
File chrome/test/data/safari_import/README (right):

http://codereview.chromium.org/159668/diff/1034/34#newcode2
Line 2: this directory where written by Safari v4.0.2 on OS X 10.5.7.  In the
were written

[stuartmorgan, 2009-07-31 16:46:06]

http://codereview.chromium.org/159668/diff/1034/25
File base/mac_util.mm (right):

http://codereview.chromium.org/159668/diff/1034/25#newcode63
Line 63: if (!library_dir)
It's impossible for an NSArray to contain nil, so this is an odd check.

http://codereview.chromium.org/159668/diff/1034/29
File chrome/browser/importer/safari_importer.h (right):

http://codereview.chromium.org/159668/diff/1034/29#newcode47
Line 47: // and convert them to seconds since UNIX epoch.
Maybe tweak this comment's structure; it doesn't follow the descriptive (vs
declarative) suggestion of the style guide like the rest here do.

http://codereview.chromium.org/159668/diff/1034/30
File chrome/browser/importer/safari_importer.mm (right):

http://codereview.chromium.org/159668/diff/1034/30#newcode103
Line 103: const double kCFToUnixEpochDelta = 978307200;
kCFAbsoluteTimeIntervalSince1970

http://codereview.chromium.org/159668/diff/1034/30#newcode107
Line 107: kCFToUnixEpochDelta;
Fix alignment; this looks like a parameter to CFStringGetDoubleValue, but isn't.

http://codereview.chromium.org/159668/diff/1034/30#newcode120
Line 120: stringByAppendingPathComponent:@"History.plist"];
Fix indentation.

http://codereview.chromium.org/159668/diff/1034/30#newcode128
Line 128: for(NSDictionary* history_item in safari_history_items) {
Space after for

http://codereview.chromium.org/159668/diff/1034/30#newcode186
Line 186: // Get a hold of ~/Library/Preferences/com.apple.Safari.plist
Why not just use CFPreferencesCopyAppValue?

http://codereview.chromium.org/159668/diff/1034/30#newcode203
Line 203: // static
?

http://codereview.chromium.org/159668/diff/1034/31
File chrome/browser/importer/safari_importer_unittest.mm (right):

http://codereview.chromium.org/159668/diff/1034/31#newcode39
Line 39: ASSERT_EQ(base::SysNSStringToUTF8(home_page),
"http://www.example.com/");
EXPECT_EQ

http://codereview.chromium.org/159668/diff/1034/31#newcode56
Line 56: ASSERT_EQ(it1.url(), GURL("http://www.firsthistoryitem.com/"));
Everything from here down looks like it should be EXPECT_EQ.

http://codereview.chromium.org/159668/diff/1034/31#newcode75
Line 75: 
Remove the blank lines here

[jeremy, 2009-07-31 18:46:06]

All fixed, ready for another look.

http://codereview.chromium.org/159668/diff/1034/25
File base/mac_util.mm (right):

http://codereview.chromium.org/159668/diff/1034/25#newcode63
Line 63: if (!library_dir)
On 2009/07/31 16:46:06, stuartmorgan wrote:
> It's impossible for an NSArray to contain nil, so this is an odd check.

Done.

http://codereview.chromium.org/159668/diff/1034/29
File chrome/browser/importer/safari_importer.h (right):

http://codereview.chromium.org/159668/diff/1034/29#newcode30
Line 30: MessageLoop* delagate_loop,
On 2009/07/31 16:22:17, kuchhal wrote:
> delegate_loop

Done.

http://codereview.chromium.org/159668/diff/1034/29#newcode47
Line 47: // and convert them to seconds since UNIX epoch.
On 2009/07/31 16:46:06, stuartmorgan wrote:
> Maybe tweak this comment's structure; it doesn't follow the descriptive (vs
> declarative) suggestion of the style guide like the rest here do.

Done.

http://codereview.chromium.org/159668/diff/1034/29#newcode64
Line 64: DISALLOW_EVIL_CONSTRUCTORS(SafariImporter);
On 2009/07/31 16:22:17, kuchhal wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/159668/diff/1034/30
File chrome/browser/importer/safari_importer.mm (right):

http://codereview.chromium.org/159668/diff/1034/30#newcode103
Line 103: const double kCFToUnixEpochDelta = 978307200;
On 2009/07/31 16:46:06, stuartmorgan wrote:
> kCFAbsoluteTimeIntervalSince1970

Done.

http://codereview.chromium.org/159668/diff/1034/30#newcode107
Line 107: kCFToUnixEpochDelta;
On 2009/07/31 16:46:06, stuartmorgan wrote:
> Fix alignment; this looks like a parameter to CFStringGetDoubleValue, but
isn't.

Done.

http://codereview.chromium.org/159668/diff/1034/30#newcode120
Line 120: stringByAppendingPathComponent:@"History.plist"];
On 2009/07/31 16:46:06, stuartmorgan wrote:
> Fix indentation.

Done.

http://codereview.chromium.org/159668/diff/1034/30#newcode128
Line 128: for(NSDictionary* history_item in safari_history_items) {
On 2009/07/31 16:46:06, stuartmorgan wrote:
> Space after for

Done.

http://codereview.chromium.org/159668/diff/1034/30#newcode186
Line 186: // Get a hold of ~/Library/Preferences/com.apple.Safari.plist
That would be much easier, but doing things this way means I can write what
seems like a more robust unit test?

http://codereview.chromium.org/159668/diff/1034/30#newcode203
Line 203: // static
This is just a first stab at the import code, I'm including this function in
case I need it further along, I'll remove it in the next CL if I don't.  You can
ignore it for now.

http://codereview.chromium.org/159668/diff/1034/31
File chrome/browser/importer/safari_importer_unittest.mm (right):

http://codereview.chromium.org/159668/diff/1034/31#newcode39
Line 39: ASSERT_EQ(base::SysNSStringToUTF8(home_page),
"http://www.example.com/");
On 2009/07/31 16:46:06, stuartmorgan wrote:
> EXPECT_EQ

Done.

http://codereview.chromium.org/159668/diff/1034/31#newcode56
Line 56: ASSERT_EQ(it1.url(), GURL("http://www.firsthistoryitem.com/"));
On 2009/07/31 16:46:06, stuartmorgan wrote:
> Everything from here down looks like it should be EXPECT_EQ.

Done.

http://codereview.chromium.org/159668/diff/1034/31#newcode75
Line 75: 
On 2009/07/31 16:46:06, stuartmorgan wrote:
> Remove the blank lines here

Done.

http://codereview.chromium.org/159668/diff/1034/34
File chrome/test/data/safari_import/README (right):

http://codereview.chromium.org/159668/diff/1034/34#newcode2
Line 2: this directory where written by Safari v4.0.2 on OS X 10.5.7.  In the
On 2009/07/31 16:22:17, kuchhal wrote:
> were written

Done.

[stuartmorgan, 2009-07-31 18:59:56]

http://codereview.chromium.org/159668/diff/1034/30
File chrome/browser/importer/safari_importer.mm (right):

http://codereview.chromium.org/159668/diff/1034/30#newcode186
Line 186: // Get a hold of ~/Library/Preferences/com.apple.Safari.plist
On 2009/07/31 18:46:06, jeremy wrote:
> That would be much easier, but doing things this way means I can write what
> seems like a more robust unit test?

Yeah, I thought of that as well, but it seems better to have a robust, trivial
implementation that uses the OS-provided API for doing exactly what you want to
do, than to re-invent an existing function just to unit-test the implementation.
Unless we don't think CFPreferencesCopyAppValue is reliable, which seems
unlikely.

If you still wanted to test it, perhaps you could have the Safari bundle-id be a
parameter (or have a test-only call to override it), and then have your test
create this pref for a bogus bunde ID, flush that to disk, call the importer,
then clean up the bogus pref?

http://codereview.chromium.org/159668/diff/1059/1066
File chrome/browser/importer/safari_importer.h (right):

http://codereview.chromium.org/159668/diff/1059/1066#newcode46
Line 46: // Convert history time stored by Safari as a double serialized as a
string,
s/Convert/Converts/

http://codereview.chromium.org/159668/diff/1059/1067
File chrome/browser/importer/safari_importer.mm (right):

http://codereview.chromium.org/159668/diff/1059/1067#newcode108
Line 108: const double kCFAbsoluteTimeIntervalSince1970 = 978307200;
Sorry, my comment wasn't clear. I meant that there is an existing constant,
kCFAbsoluteTimeIntervalSince1970, that is the value you need; you don't have to
define it.

http://codereview.chromium.org/159668/diff/1059/1068
File chrome/browser/importer/safari_importer_unittest.mm (right):

http://codereview.chromium.org/159668/diff/1059/1068#newcode55
Line 55: EXPECT_EQ(history_items.size(), 2U);
This should stay ASSERT, or you should have other checks below, or the test will
crash if it comes back with <2.

[jeremy, 2009-07-31 19:53:01]

All fixed, ready for another round.

[stuartmorgan, 2009-07-31 19:57:01]

LGTM with leak fix.

http://codereview.chromium.org/159668/diff/1074/53
File chrome/browser/importer/safari_importer.mm (right):

http://codereview.chromium.org/159668/diff/1074/53#newcode179
Line 179: CFSTR("com.apple.Safari")));
This needs releasing; Copy gives you an owned reference.