Closed Bug 236300 (SPM) Opened 19 years ago Closed 17 years ago

Safari Profile Migrator

Categories

(Firefox :: Migration, defect, P2)

PowerPC
macOS
defect

Tracking

()

VERIFIED FIXED
Firefox1.5

People

(Reporter: bugs, Assigned: asaf)

References

Details

Attachments

(1 file, 10 obsolete files)

Need to finish implementation of Safari Profile Migrator
Status: NEW → ASSIGNED
Flags: blocking1.0+
Priority: -- → P2
Target Milestone: --- → Firefox1.0
Flags: blocking1.0+ → blocking1.0mac+
Target Milestone: Firefox1.0 → Firefox1.0Mac
*** Bug 260731 has been marked as a duplicate of this bug. ***
*** Bug 269961 has been marked as a duplicate of this bug. ***
moving blocking1.0mac bugs to Firefox1.1 Target Milestone.
Target Milestone: Firefox1.0Mac → Firefox1.1
Attached patch partial work (obsolete) — Splinter Review
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)
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. 
Blocks: macmeta
Attached patch patch, almost complete. (obsolete) — Splinter Review
All the migration sections are complete. Need to connect up to the Migration
API and test now.
Attachment #167813 - Attachment is obsolete: true
taking QA
QA Contact: bugs.mano
Attached patch complete, but untested. (obsolete) — Splinter Review
needs debugging.
Attachment #167963 - Attachment is obsolete: true
Attached patch works (obsolete) — Splinter Review
OK, after much wailing and gnashing of teeth, this works, more or less.
Attachment #167965 - Attachment is obsolete: true
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
(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.
*** Bug 275271 has been marked as a duplicate of this bug. ***
*** Bug 275898 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.1?
*** Bug 285259 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1+
Flags: blocking-aviary1.0mac+
Depends on: 289367
Blocks: deermac
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
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #167993 - Attachment is obsolete: true
Depends on: 298801
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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #187299 - Attachment is obsolete: true
Attachment #187337 - Flags: review?(joshmoz)
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 on attachment 187337 [details] [diff] [review]
patch

compiles, no warnings, runs great.

Lets try to get this into Alpha 2 for testing...
Attachment #187337 - Flags: review?(joshmoz) → review+
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.
Attachment #187337 - Flags: superreview-
Attachment #187337 - Flags: review?(joshmoz)
Attachment #187337 - Flags: review+
Attached patch patch (obsolete) — Splinter Review
Assignee: bugs → bugs.mano
Attachment #187591 - Flags: review?(joshmoz)
Attachment #187337 - Attachment is obsolete: true
Attachment #187337 - Flags: review?(joshmoz)
Attached patch patch (obsolete) — Splinter Review
Attachment #187591 - Attachment is obsolete: true
Attachment #187594 - Flags: review?(joshmoz)
Attachment #187591 - Flags: review?(joshmoz)
> 
> >+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.
Attached patch patch (obsolete) — Splinter Review
Attachment #187594 - Attachment is obsolete: true
Attachment #187623 - Flags: review?(joshmoz)
Attachment #187594 - Flags: review?(joshmoz)
Alias: SPM
Attached file patche file (obsolete) —
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? 
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()
> #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.

Attached patch patchSplinter Review
Attachment #187623 - Attachment is obsolete: true
Attachment #187748 - Attachment is obsolete: true
Comment on attachment 187754 [details] [diff] [review]
patch

r=mconnor+josh a=asa on irc, landing.
Attachment #187754 - Flags: review+
Attachment #187623 - Flags: review?(joshmoz)
checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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
Frederic, please file a separate bug, cc me and mark mentovai, thanks.
*** Bug 305037 has been marked as a duplicate of this bug. ***
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.