550816 - Import Bookmarks dialog should indicate that "Mozilla Firefox" does not include Fx 3

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Lisa Thompson]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en; rv:1.9.0.18) Gecko/2010021619 Camino/2.0.2 (like Firefox/3.0.18)
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en; rv:1.9.0.18) Gecko/2010021619 Camino/2.0.2 (like Firefox/3.0.18)

I was helping a friend configure Camino, went to the Import Bookmarks, "Mozilla Firefox" is the default choice, so we ran it and didn't get the expected bookmarks. Had to go to cbo to learn the right way. Not a good first impression. I'm sure you want to support importing Fx 3 bookmarks eventually, but in the meantime, it would be nice if the popup menu in the dialog specifically said "Firefox 2" (or Firefox 1 & 2, or whatever is accurate).


Reproducible: Always

Comment 1

[Chris Lawson (gone)]

Yeah, I think we need to do something about this, especially this late after the Places transition.

l10n implications for a 2.0.x release if we do it there. Probably at least wanted-2.1, though IMO it's more important to have an actual working Places importer in 2.1. (Our Firefox importer should probably be smarter, in other words.)

Just fixing the text is really easy if we want to go ahead and do that now and then fix it "for real" in another bug (say, by educating the import code we have already).

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

There's no l10n impact from changing the menu to "Mozilla Firefox 2" or "Mozilla Firefox 1.x-2" (note that for consistency's sake, "iCab" should change to "iCab 2" or "iCab 1-2" for the same reasons).

Beyond that, the simplest thing to do is look for places.sqlite in the Firefox profile and bail if it's there (assuming people don't export their HTML bookmarks from Firefox 3.x back into that profile).

That said, I don't know that bug 327035 would be too difficult to fix in the same way that CookieThief 1.1 works on cookies (without having to worry about asking Places to do anything), though I haven't looked at a sample places.sqlite, so grain-of-salt.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Confirming this bug because it's the same issue that prompted Stuart to file bug 439014, and even though he duped it, there are two issues, the current UI vagueness and bug 327035.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: WIP to implement comment 2

This patch basically almost implements both parts of my suggestion in comment 2: renaming "Mozilla Firefox" and "iCab" to mention the highest version that particular importer handles, and bails out of the Firefox codepath when it detects a places.sqlite file in that profile.

There's one real problem with the patch as-is (besides whatever code ugliness my changes have): I haven't yet been able to figure out how to bail out of the entire Firefox codepath without messing up the menu generation.

Right now the code doesn't add the "current" Firefox profiledir as a datasource when it sees a places.sqlite, but, because of the way we do fallback there, if someone has an old Firefox profile (~/Library/Firefox/) or a Phoenix one sitting around, we'd still detect that one and import even-older bookmarks, thereby making the import situation worse!

If I stick a return in the else clause, it does really funny things to the menu creation; the menu initially contains the correct options, but either "Select a file…" or the separator is the initial selection, rather than the first item in the menu.

So there's something I'm still missing.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I guess I should take this, at least for the moment.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Fix to implement comment 2

The solution to the problem above seems to be to pull the Firefox bookmarks import out into its own function, from which we can return early and not mess up the menu-generation/default-selected-item code.

This is pretty much a copypastach, so I'm sure there are areas for code quality improvement.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: v1.0a, with updated SeaMonkey comment

Stuart liked the approach outlined in comment 2, so here's the patch again for review (changes in this version are just an update to the updated SeaMonkey comment).

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: v1.1, with some irc comments from cl addressed

Comment 9

[Chris Lawson (gone)]

Argh, hit Submit before I could comment. r=me, but I'm (still) unable to test this patch. Smokey assures me he's tested it rigorously, though, and it's reasonably simple, so I'm OK with sending it to sr without having tested it myself.

Comment 10

[Stuart Morgan]

Comment on attachment 434769 [details] [diff] [review]
v1.1, with some irc comments from cl addressed

>+  NSString *foxPath;

Combine the declaration with the first use (down in the else). It would be great if you could do the same with mozPath up in the code you modeled this method on.

>+  NSString *maybePlacesBookmarkPath = [self saltedPlacesPathForProfile:@"~/Library/Application Support/Firefox/Profiles/"];

@"~/Library/Application Support/Firefox/Profiles/" is used twice in this method; make it a local variable instead (currentFirefoxProfileRoot maybe?)

>+  NSFileManager *fm = [NSFileManager defaultManager];
>+  BOOL hasPlacesBookmarks = ([fm fileExistsAtPath:fullPathString]);

No need for the parens here.

Maybe inline the [NSFileManager defaultManager] since you don't use it again, but I'm fine either way.

>+  if (hasPlacesBookmarks) {
>+    // There's a places.sqlite file in the current profile, so any bookmarks.html in
>+    // this profile, or in any older profile location, is old.
>+    return;
>+  }
>+  else {

No else needed after an early return.

>+    foxPath = [self saltedBookmarkPathForProfile:@"~/Library/Application Support/Firefox/Profiles/"];
>+    if (!foxPath)
>+      foxPath = [self saltedBookmarkPathForProfile:@"~/Library/Firefox/Profiles/default/"];
>+    if (!foxPath)
>+      foxPath = [self saltedBookmarkPathForProfile:@"~/Library/Phoenix/Profiles/default/"];
>+    if (foxPath)
>+      [self tryAddImportFromBrowser:@"Mozilla Firefox 2" withBookmarkPath:foxPath];

Add a blank line before the last block; as it is it reads like part of of the list of profile locations at first, and I had to re-parse it.

>+  if (lastModifiedSubDir) 
>+    return 
>+
>+  return nil;

This is 100% equivalent to:
  return [lastModifiedSubDir stringByAppendingPathComponent:@"places.sqlite"];

(Feel free to make the same simplification in the other method.)

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: v1.2, addresses sr comments

I think I got everything right :)

Comment 12

[Stuart Morgan]

Comment on attachment 435294 [details] [diff] [review]
v1.2, addresses sr comments

sr=smorgan. Thanks for the cleanup in the other methods :)

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on cvs trunk and the CAMINO_2_0_BRANCH in advance of 2.0.3.

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/8276a605ec3a