191783 - Convert bookmark implemenation to use nsIFile instead of nsFileSpec`

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect, P1)

Description

[Benjamin Smedberg]

timless bugged me to convert the bookmark code to use nsIFile instead of
nsFileSpec. So I have, mostly. One problem I came across was that
nsILineInputStream probably doesn't convert UTF-8 properly into the AString. I
will attach a patch shortly. I don't know the proper solution to that problem...
would it be better to alter NS_ReadLine to support UTF8 encodings (with a flag?)
and what would that affect/break?

Comment 1

[Darin Fisher]

nsILineInputStream is something bz added to the tree... cc'ing him for his
thoughts about extending it to support UTF-8 -> UCS-2 conversion.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So the plan for nsILineInputStream (which I have not had time to implement is):

1) Make it an actual XPCOM object that can wrap any buffered stream and read it
   using ReadSegments.
2) Make the init() method take two arguments -- an nsIInputStream and an encoding.
3) Use an actual unicode decoder to do the charset conversion.

Of course if you're just looking for a quick fix, supporting only UTF8 is pretty
trivial... No need for a flag; right now it only supports ASCII, which is a
subset of UTF8 anyway.  The only issue to watch out for is that invalid ASCII
will currently be bit-stripped and appended, whereas using NS_ConvertUTF8toUCS2
will convert invalid UTF8 to the empty string....

Comment 3

[Benjamin Smedberg]

Attachment: patch with just the important stuff

I've done a lot of whitespace/bracing cleanup, and fixed up some NS_ENSURE_*
macros that bugged me in the file. But this patch only includes the significant
changes from nsFileSpec to nsIFile, and the change in NS_ReadLine to support
UTF8. I've done basic testing and it works on my tree. Are there QA bookmark
regression tests anywhere?

Comment 4

[Benjamin Smedberg]

Attachment: complete patch with whitespace

Complete patch with whitespace/bracing/nitpick changes.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 113499 [details] [diff] [review]
patch with just the important stuff

Like I said, NS_ConvertUTF8toUCS2 will just completely drop anything that is
not valid UTF-8.  At the least, you should add an NS_WARN_IF_FALSE for such
cases (if not fall back on treating it as ascii and bit-stripping it...)

Comment 6

[Benjamin Smedberg]

Attachment: revised, with whitespace

Revised patch with UTF8 sanity checks per bzbarsky. I also fixed leftover
variables named "fileSpec" to something better. Lastly, I fixed the PRInt64
code for platforms without longlong. I need to test whether this patch
handles symlinked "bookmarks.html" files correctly. Also, why don't we use
the system "temp" directory for the temporary file, rather than the profile
directory?

Comment 7

[Benjamin Smedberg]

Who is the proper reviewer for bookmark code?

Comment 8

[Boris Zbarsky [:bzbarsky]]

alecf is probably the right sr...

[e-mail] is likely the right reviewer.

ben would be good if you can ever get him to actually respond to mail.  ;)

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

>why don't we use the system "temp" directory for the temporary file,

moving files between filesystems is hard, because you can't just use rename()
for that, but need to copy and delete...

at least I guess that that's the reason.

Comment 10

[Benjamin Smedberg]

Attachment: version 3, with whitespace

Version 3 of this patch uses the system "temp" dir for the temporary copy of
the bookmarks file, then copies to the correct bookmarks file location using
"CopyFollowingSymlinks". Using any other directory than the system temporary
directory may cause security problems, especially when there is a customized
bookmarks file location, or symlinks.

To get the gist of this change, I still recommend the first patch I uploaded
without the whitespace and nitpick changes. This patch is really hard to read.

Comment 11

[Benjamin Smedberg]

Comment on attachment 114016 [details] [diff] [review]
version 3, with whitespace

I get mail bounced from [e-mail]... asking [e-mail] for review.

Comment 12

[Alec Flett]

Comment on attachment 114016 [details] [diff] [review]
version 3, with whitespace

could I get a diff -bw on this? I'm glad to see the spacing updated and all,
but I'd like to review only the relevant changes.

Comment 13

[Benjamin Smedberg]

Attachment: version 3, without whitespace

OK, no whitespace. This patch still has bracing/cleanup changes.

Comment 14

[Alec Flett]

Comment on attachment 114016 [details] [diff] [review]
version 3, with whitespace

to be honest, I think we've already done this work on the MiniMo branch. I'd
like to hold off on this until we land that branch. (which should be early in
1.4a)

Comment 15

[Benjamin Smedberg]

However you want to do it is fine with me; I don't think that this was updated
in MINIMO, it just links against XPCOM-obsolete. Do you want to land this into
MINIMO to avoid that dependency?

Comment 16

[Doug Turner (:dougt)]

there is no requirement that this land on minimo.

Comment 17

[Alec Flett]

Comment on attachment 114016 [details] [diff] [review]
version 3, with whitespace

Since this is definitely going to land with minimo (probably this week) I'm
removing this from my review queue

Comment 18

[Jan Varga [:janv]]

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MINIMO_01302003_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=month&mindate=&maxdate=&cvsroot=%2Fcvsroot

It seems that this hasn't been done on the MINIMO branch.

Comment 19

[Benjamin Smedberg]

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

Comment 20

[Benjamin Smedberg]

Attachment: bookmarks use nsIFile, version 4 complete

Now that MINIMO has landed, let's try this again. Updated patch to avoid merge
conflicts, and remove xpcom_obsolete which is no longer needed

Comment 21

[Benjamin Smedberg]

Attachment: bookmarks use nsIFile, version 4, without whitespace

patch as above without whitespace cleanup or makefile change

Comment 22

[Jan Varga [:janv]]

Nice work, could you hold this until we complete arch changes on the bookmarks
branch ?
Thanks

Comment 23

[Benjamin Smedberg]

Do you want to include this on the branch? And what is the ETL for that? The
patch tends to bitrot quickly because of the indenting changes.

Comment 24

[Jan Varga [:janv]]

Yes, landing on the branch is fine with me.
The branch name is BOOKMARKS_20030310_BRANCH.
This project is also included on the landing page.
Thanks

Comment 25

[Jan Varga [:janv]]

Another idea, would it be very painful for you to split this patch/bug to
nsIFile and indentation changes ?
I bet you'll get reviews for indentation changes w/o problems (r=varga on that)
So you can land it on the branch and trunk immediately.
We will block each other otherwise.

A comment on your patch:
Instead of adding the BREAKRV macro that adds additional code footprint
you could use a similar mechanism that fastload serialization/deserialization
code uses.

nsresult rv;

rv = strm->Write(KCSLEN(kDTOpen), &dummy);
rv |= strm->Write(uri.get(), uri.Length(), &dummy);
rv |= WriteBookmarkProperties(ds, strm, child, kNC_BookmarkAddDate,            
                                  NS_LITERAL_CSTRING(kAddDateEquals), PR_FALSE);
...
return rv;

please respond ASAP

Comment 26

[Benjamin Smedberg]

Let me see if I can separate the patches. Probably this evening sometime.

> rv = strm->Write(KCSLEN(kDTOpen), &dummy);
> rv |= strm->Write(uri.get(), uri.Length(), &dummy);

Is that "legal"? It sounds like a good idea, but the actual nsresult would be
munged to something useless, no?

Comment 27

[Jan Varga [:janv]]

>Let me see if I can separate the patches. Probably this evening sometime.
Thanks, the sooner the better.

>Is that "legal"? It sounds like a good idea, but the actual nsresult would be
>munged to something useless, no?

I don't think so, it's a cheap way to do this kind of checks.
See http://bugzilla.mozilla.org/show_bug.cgi?id=112064#c32

Comment 28

[Benjamin Smedberg]

Attachment: whitespace/brace style only

OK, breaking this patch up into pieces per Jan's request. This just updates
whitespace and bracing.

Comment 29

[Jan Varga [:janv]]

Comment on attachment 117612 [details] [diff] [review]
whitespace/brace style only

r=varga

Comment 30

[Jan Varga [:janv]]

Comment on attachment 117612 [details] [diff] [review]
whitespace/brace style only

sspitzer says rs=sspitzer

Comment 31

[Jan Varga [:janv]]

Comment on attachment 117612 [details] [diff] [review]
whitespace/brace style only

checked in

Comment 32

[Jan Varga [:janv]]

thanks for the patch Benjamin

Comment 33

[Benjamin Smedberg]

Attachment: bookmarks use nsIFile

OK, now there's a readable patch that might actually get reviewed. This patch
is for the trunk (there is at least one merge conflict with the BOOKMARKS_2003*
branch).

Comment 34

[Alec Flett]

Comment on attachment 117877 [details] [diff] [review]
bookmarks use nsIFile

what is this junk with checking if the file is too big? what's the issue there?
+    parser.Init(file, mInner, nsAutoString(), PR_TRUE);

don't pass nsAutoString(), pass nsString() at the very least.

-	 parser.Init(&ieFavoritesFile, mInner, nsAutoString());
+	 parser.Init(ieFavoritesFile, mInner, nsAutoString());

and here

+#define KCSLEN(str) str, sizeof(str)-1
+


I hate this macro. Not only is it poorly named (how about CSTRING_LENGTH or
something?) but it has different meanings depending on how its used. this makes
the call to Write() look really strange.. just use Write(kFileIntro,
CSTRING_LENGTH(kFileIntro),  etc..)


+    rv = NS_NewISupportsArray(getter_AddRefs(parentArray));
+    NS_ENSURE_SUCCESS(rv, rv);

I'll give you a nickel if you switch to nsCOMArray.. (not required but it gets
you good mozilla karma)

other than that, this really looks great.. nice cleanup. sr=alecf with the
above changes.. please attach a new patch anyhow, and re-ask for a review if
you DO switch to nsCOMArray

(a hint on nsCOMArray: pass around a reference to it, as in:
WriteBookmarksContainer(..., nsCOMArray<nsIRDFResource>& parentArray) {...})

Comment 35

[Jan Varga [:janv]]

>+    parser.Init(file, mInner, nsAutoString(), PR_TRUE);
We already removed this in the bookmarks branch.
Please hold this a bit longer, I'll review your patch then.

Comment 36

[Benjamin Smedberg]

Attachment: bookmarks use nsIFile, again

the PRInt64/file too big stuff is because I'm trying to read the file into a
char[] and I can't allocate one larger than PRUInt32.

Fixed bad KCSLEN macro to KSTRINGLEN
Switched to nsCOMArray for 'good Mozilla karma'.

Jan, since your branch has landed, this should be good to go?

Comment 37

[Jan Varga [:janv]]

comments on the patch:
- it doesn't regress performance, that's good
- personally, I don't like the KSTRINGLEN macro. We already use |sizeof(str)-1|
directly in this code, so why don't follow this style ?
- the rvs variable in WriteBookmarksContainer() could be eliminated, it would be
better to do the |if NS_FAILED(rv)| check after a block of Write() calls instead
of one at the end. I think you already did this in WriteBookmark()

other then that, looks pretty good
I think one more patch will make it.

Comment 38

[Benjamin Smedberg]

Attachment: bookmarks use nsIFile, again and again

Darn, I liked KSTRINGLEN, oh well, it was an easy regex-replace

Comment 39

[Jan Varga [:janv]]

Comment on attachment 118587 [details] [diff] [review]
bookmarks use nsIFile, again and again

please get rid of the |rvs| completely
r=varga

Comment 40

[Alec Flett]

Comment on attachment 118587 [details] [diff] [review]
bookmarks use nsIFile, again and again

sr=alecf
nice cleanup

Comment 41

[Benjamin Smedberg]

Attachment: final patch (spelling error wouldn't compile)

Oops, I mis-spelled NS_ENSURE_SUCESS and NS_ENSURE_SUCEES in #ifdef XP_WIN and
XP_BEOS sections... updated, carrying over r/sr

Comment 42

[Benjamin Smedberg]

Fixed on trunk. Thanks Jan and Alecf for your assistance!

For the record, this cause a 2k codesize increase on linux-'comet' and a 500byte
decrease on win32 'beast'. Go figure.

Comment 43

[Chris Petersen]

Verified per last comments

Comment 44

[Stephen Donner [:stephend] Not actively reading bugmail]

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