236300 - (SPM) Safari Profile Migrator

Status: VERIFIED FIXED

(Firefox :: Migration, defect, P2)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

Need to finish implementation of Safari Profile Migrator

Comment 1

[José Jeria]

*** Bug 260731 has been marked as a duplicate of this bug. ***

Comment 2

[Asaf Romano (gone)]

*** Bug 269961 has been marked as a duplicate of this bug. ***

Comment 3

[Asa Dotzler [:asa]]

moving blocking1.0mac bugs to Firefox1.1 Target Milestone.

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: partial work

implements all settings, bookmarks, user style sheet and saved search values.
Work Needed:
- history
- cookies
- testing (*none* has been done so far, other than compilation)

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

I will not be implementing password import, rather I believe we should
investigate using Keychain Services on Mac so we can access the Safari/Camino
password store. Not yet decided on this however. 

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch, almost complete.

All the migration sections are complete. Need to connect up to the Migration
API and test now.

Comment 7

[Asaf Romano (gone)]

taking QA

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: complete, but untested.

needs debugging.

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: works

OK, after much wailing and gnashing of teeth, this works, more or less.

Comment 10

[Asaf Romano (gone)]

Some bugs i find out:
  #1: it doesn't import bookmarks with non western characters in their title (in
my case, hebrew)
  #2: it hasn't import my default download folder
  #3 It hasn't import minimum font size

Comment 11

[Asaf Romano (gone)]

(In reply to comment #10)
> Some bugs i find out:
>   #1: it doesn't import bookmarks with non western characters in their title (in
> my case, hebrew)
>   #2: it hasn't import my default download folder
>   #3 It hasn't import minimum font size

Forgot to mention (don't know if it's expected to work) it doesn't list the
stuff it is going to import / was imported.

Comment 12

[Matthias Versen [:Matti]]

*** Bug 275271 has been marked as a duplicate of this bug. ***

Comment 13

[Asaf Romano (gone)]

*** Bug 275898 has been marked as a duplicate of this bug. ***

Comment 14

[Matthias Versen [:Matti]]

*** Bug 285259 has been marked as a duplicate of this bug. ***

Comment 15

[g-o1]

You actually can just use Safari to export bookmarks in a form Firefox can read.
First thing I found on Google:

http://www.macworld.com/weblogs/mac911/2005/04/safariin/index.php

Comment 16

[Asaf Romano (gone)]

Attachment: patch

Changes from Ben's last patch:
  * fixed various CF leaks
  * bookmarks (and other plist entries) with a non-western title are now
imported.
  * SetDefaultCharset, and its dependencies work (The orginal code was very
wrong/broken).
  * Thw wizard exposes the imported items.
  * The style sheet importer /still/ doens't work. However, it is much closer
;)
  * footprint fixes.

Comment 17

[Asaf Romano (gone)]

FYI:
(In reply to comment #10)
>   #2: it hasn't import my default download folder

Actually that's not a problem in the migrator itself, it is under the shade of
both mac-specific and XP bugs on this pref.

>   #3 It hasn't import minimum font size

I was wrong, it has/does.

Comment 18

[Asaf Romano (gone)]

Attachment: patch

Changes:
 * Working safari style sheet importer.
 * Only offer to import the safari-style-sheet if the active pofile doesn't
contain a userContent.css file.

Comment 19

[Josh Aas]

Comment on attachment 187337 [details] [diff] [review]
patch

+#define NC_URI(property) \
+  NS_LITERAL_CSTRING("http://home.netscape.com/NC-rdf#"#property)

String concat/expansion is weird in macros - have you tested that this does
indeed produce the string you want?

+#define SAFARI_HISTORY_DATE_OFFSET 978307200

If its not already somewhere nearby in the code, can you explain that number in
a comment above the macro? Just makes it a little easier to read.

Otherwise this looks good so far. I need to test it and then decide on r.

Comment 20

[Josh Aas]

Comment on attachment 187337 [details] [diff] [review]
patch

compiles, no warnings, runs great.

Lets try to get this into Alpha 2 for testing...

Comment 21

[Simon Fraser [no longer active]]

Comment on attachment 187337 [details] [diff] [review]
patch

>Index: browser/components/migration/src/nsSafariProfileMigrator.cpp
>===================================================================

>-nsresult
>-GetPListFromFile(nsILocalFile* aPListFile, CFPropertyListRef* aResult)
>+CFPropertyListRef GetPListFromFile(nsILocalFile* aPListFile)
> {

This should be renamed to follow CF conventions about whether
the caller should release the returned object. It looks like the
answer is yes, so this function should have 'create' or 'copy' in
the name. 'get' indicates no releasing necessary.

>+CFDictionaryRef GetSafariPrefs()
>+{

Ditto.

>+PRBool
>+GetDictionaryStringValue(CFDictionaryRef aDictionary, CFStringRef aKey,
>+                         nsAString& aResult)
>+{
>+  CFStringRef value = (CFStringRef)::CFDictionaryGetValue(aDictionary, aKey);
>+  if (value) {
>+    char buf[1024];
>+    if (::CFStringGetCString(value, buf, sizeof(buf), kCFStringEncodingUTF8)) {
>+      CopyUTF8toUTF16(buf, aResult);

Can't you use CFStringGetCharacters() here?

>+PRBool
>+GetArrayStringValue(CFArrayRef aArray, PRInt32 aIndex, nsAString& aResult)
>+{
>+  CFStringRef value = (CFStringRef)::CFArrayGetValueAtIndex(aArray, aIndex);
>+  if (value) {
>+    char buf[1024];
>+    if (::CFStringGetCString(value, buf, sizeof(buf), kCFStringEncodingUTF8)) {
>+      CopyUTF8toUTF16(buf, aResult);

Can't you use CFStringGetCharacters() here?


> static
> nsSafariProfileMigrator::PrefTransform gTransforms[] = {

Make this |const|.

>+static charsetEntry gCharsets[] = {

Make this |const|.


>     char* buf = nsnull;
>     SInt32 size = 256;
>     do {
>       buf = (char*)malloc((unsigned int)size+1);
>       if (!buf)
>         break;
>-        
>+
>       error = ::ICGetPref(internetConfig, kICWWWHomePage, &dummy, buf, &size);
>       if (error != noErr && error != icTruncatedErr) {
>         free(buf);
>         *buf = nsnull;

You've just crashed.

>       }
>-      else 
>+      else
>         buf[size] = '\0';

Won't this chop off the last char of the string, since the first char
is the size (pascal string)?

>-      
>+
>       size *= 2;
>     }
>     while (error == icTruncatedErr);
>-    
>+
>     if (*buf != 0) {

Don't you need to test buf here too? It could be null.

>       // This is kind of a hack... skip the first byte on the pascal string
>       // (which is the length).
>       branch->SetCharPref("browser.startup.homepage", buf + 1);
>     }
>-    
>+
>     if (buf) {
>       free(buf);
>       *buf = nsnull;

Crashy again.

>+nsresult
>+nsSafariProfileMigrator::ParseBookmarksFolder(CFArrayRef aChildren,
>+                                              nsIRDFResource* aParentResource,
>+                                              nsIBookmarksService* aBookmarksService,
>+                                              PRBool aIsAtRootLevel)
>+{
...
>+        rv |= ParseBookmarksFolder(children,
>+                                   toolbarFolder,
>+                                   aBookmarksService,
>+                                   PR_FALSE);

ORing nsresult values doesn't make much sense.

Comment 22

[Asaf Romano (gone)]

Attachment: patch

Comment 23

[Asaf Romano (gone)]

Attachment: patch

Comment 24

[Asaf Romano (gone)]

> 
> >+nsresult
> >+nsSafariProfileMigrator::ParseBookmarksFolder(CFArrayRef aChildren,
> >+                                              nsIRDFResource* aParentResource,
> >+                                              nsIBookmarksService*
aBookmarksService,
> >+                                              PRBool aIsAtRootLevel)
> >+{
> ...
> >+        rv |= ParseBookmarksFolder(children,
> >+                                   toolbarFolder,
> >+                                   aBookmarksService,
> >+                                   PR_FALSE);
> 
> ORing nsresult values doesn't make much sense.
> 

I just verified and it seems we do support that very well (that's, the various
nsresult macros (such as NS_FAILED) are checking for result & VAL, not result ==
VAL), going to restore those bits.

Comment 25

[Asaf Romano (gone)]

Attachment: patch

Comment 26

[Josh Aas]

Attachment: patche file

Comment 27

[Mike Connor [:mconnor]]

drive-by review comments (still reading the patch)

  // Since "x-unicode" in the font dialog means "Other Languages" (that is,
  // langauges which doesn't have a

s/langauges/languages/
s/doesn't/don't/
a what? 

Comment 28

[Josh Aas]

So - 3 issues me and Simon and Mano found so far:

1. use nsCRT::free() consistently, instead of just free()
2. fix memory leak in CopyFormData()
3. fix potential bad memory access because of buf not getting set to null after
a free() in CopyPreferences()

Comment 29

[Simon Fraser [no longer active]]

> #include <InternetConfig.h>

Prefer <Carbon/Carbon.h>

> 
> #define SAFARI_PREFERENCES_FILE_NAME NS_LITERAL_STRING("com.apple.Safari.plist")
> #define SAFARI_BOOKMARKS_FILE_NAME NS_LITERAL_STRING("Bookmarks.plist")
> #define SAFARI_HISTORY_FILE_NAME NS_LITERAL_STRING("History.plist")
> #define SAFARI_COOKIES_FILE_NAME NS_LITERAL_STRING("Cookies.plist")
> #define SAFARI_COOKIE_BEHAVIOR_FILE_NAME
NS_LITERAL_STRING("com.apple.WebFoundation.plist")
> #define SAFARI_HISTORY_DATE_OFFSET 978307200
> #define MIGRATION_BUNDLE "chrome://browser/locale/migration/migration.properties"

I'd be anal and align these for easier reading:

#define SAFARI_PREFERENCES_FILE_NAME       
NS_LITERAL_STRING("com.apple.Safari.plist")
#define SAFARI_BOOKMARKS_FILE_NAME          NS_LITERAL_STRING("Bookmarks.plist")
#define SAFARI_HISTORY_FILE_NAME            NS_LITERAL_STRING("History.plist")
#define SAFARI_COOKIES_FILE_NAME            NS_LITERAL_STRING("Cookies.plist")
#define SAFARI_COOKIE_BEHAVIOR_FILE_NAME   
NS_LITERAL_STRING("com.apple.WebFoundation.plist")
#define SAFARI_HISTORY_DATE_OFFSET          978307200
#define MIGRATION_BUNDLE                   
"chrome://browser/locale/migration/migration.properties"


> NS_IMETHODIMP
> nsSafariProfileMigrator::Migrate(PRUint16 aItems, nsIProfileStartup* aStartup,
>                                  const PRUnichar* aProfile)
> {
>   nsresult rv = NS_OK;
> 
>   PRBool aReplace = PR_FALSE;

Don't use 'aFoo' for local vars.

> PRBool
> GetDictionaryCStringValue(CFDictionaryRef aDictionary, CFStringRef aKey,
>                           nsACString& aResult, CFStringEncoding aEncoding)
> {
>   CFStringRef value = (CFStringRef)::CFDictionaryGetValue(aDictionary, aKey);
>   if (value) {
>     char buf[1024];
>     if (::CFStringGetCString(value, buf, sizeof(buf), aEncoding)) {

What if it's longer than 1024?

>       error = ::ICGetPref(internetConfig, kICWWWHomePage, &dummy, buf, &size);
>       if (error != noErr && error != icTruncatedErr)
>         free(buf);

Set buf to null here.

Comment 30

[Asaf Romano (gone)]

Attachment: patch

Comment 31

[Asaf Romano (gone)]

Comment on attachment 187754 [details] [diff] [review]
patch

r=mconnor+josh a=asa on irc, landing.

Comment 32

[Asaf Romano (gone)]

checked in.

Comment 33

[Frederic Bezies]

It breaks building with gcc 4.0 - the xcode 2.1 version, and setting SDK 10.3.9
by default.

Here is the crash :

nsSafariProfileMigrator.cpp
c++ -o nsSafariProfileMigrator.o -c  -DMOZILLA_INTERNAL_API
-DOSTYPE=\"Darwin8.1.0\" -DOSARCH=\"Darwin\" -DBUILD_ID=2005063012 
-I../../../../dist/include/xpcom -I../../../../dist/include/xpcom_obsolete
-I../../../../dist/include/string -I../../../../dist/include/necko
-I../../../../dist/include/history -I../../../../dist/include/libreg
-I../../../../dist/include/browsercomps -I../../../../dist/include/toolkitcomps
-I../../../../dist/include/pref -I../../../../dist/include/rdf
-I../../../../dist/include/satchel -I../../../../dist/include/bookmarks
-I../../../../dist/include/intl -I../../../../dist/include/unicharutil
-I../../../../dist/include/windowwatcher -I../../../../dist/include/dom
-I../../../../dist/include/docshell -I../../../../dist/include/xulapp
-I../../../../dist/include/migration -I../../../../dist/include
-I../../../../dist/include/nspr    -I/usr/X11R6/include   -mdynamic-no-pic 
-I/usr/X11R6/include -fno-rtti -fno-exceptions -Wall -Wconversion
-Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy
-Wno-non-virtual-dtor -Wno-long-long -nostdinc -nostdinc++
-I/Developer/SDKs/MacOSX10.3.9.sdk/usr/include/gcc/darwin/4.0/c++
-I/Developer/SDKs/MacOSX10.3.9.sdk/usr/include/gcc/darwin/4.0/c++/powerpc-apple-darwin7
-I/Developer/SDKs/MacOSX10.3.9.sdk/usr/include/gcc/darwin/4.0/c++/backward
-isystem
/Developer/SDKs/MacOSX10.3.9.sdk/usr/lib/gcc/powerpc-apple-darwin7/4.0.0/include
-isystem /Developer/SDKs/MacOSX10.3.9.sdk/usr/include
-F/Developer/SDKs/MacOSX10.3.9.sdk/System/Library/Frameworks
-F/Developer/SDKs/MacOSX10.3.9.sdk/Library/Frameworks -fpascal-strings
-no-cpp-precomp -fno-common -fshort-wchar
-I/Developer/SDKs/MacOSX10.3.9.sdk/Developer/Headers/FlatCarbon -pipe  -DNDEBUG
-DTRIMMED -O2  -I/usr/X11R6/include -DMOZILLA_CLIENT -include
../../../../mozilla-config.h -Wp,-MD,.deps/nsSafariProfileMigrator.pp
nsSafariProfileMigrator.cpp
nsSafariProfileMigrator.cpp: In member function 'nsresult
nsSafariProfileMigrator::CopyPreferences(PRBool)':
nsSafariProfileMigrator.cpp:670: error: invalid conversion from 'const
nsSafariProfileMigrator::PrefTransform*' to
'nsSafariProfileMigrator::PrefTransform*'
make[6]: *** [nsSafariProfileMigrator.o] Error 1
make[5]: *** [libs] Error 2
make[4]: *** [libs] Error 2
make[3]: *** [libs] Error 2
make[2]: *** [tier_99] Error 2
make[1]: *** [default] Error 2
make: *** [build] Error 2

Sorry to spam, but it is annoying for people who wants to use gcc 4.0

Comment 34

[Asaf Romano (gone)]

Frederic, please file a separate bug, cc me and mark mentovai, thanks.

Comment 35

[Asaf Romano (gone)]

*** Bug 305037 has been marked as a duplicate of this bug. ***