Last Comment Bug 384370 - use JSON as the on disk, lossless format for our bookmark backup
: use JSON as the on disk, lossless format for our bookmark backup
Status: VERIFIED FIXED
: highrisk
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: P1 normal with 7 votes (vote)
: Firefox 3
Assigned To: Dietrich Ayala (:dietrich)
:
Mentors:
: 406208 (view as bug list)
Depends on: 412531 423126 444601 382711 386789 387522 394205 397536 398898 406478 414802 419895 421943 422877 422917 422919 423040 423080 423081 423082 423085 423154 423187 423470 424669 425112 426104 448584 448804 508767
Blocks: 413944 374528 382379 389987 390750 395528 396806 405938 406114 406208 407774 415389 417228 418592
  Show dependency treegraph
 
Reported: 2007-06-13 17:06 PDT by (not reading, please use seth@sspitzer.org instead)
Modified: 2014-06-14 02:04 PDT (History)
49 users (show)
mconnor: blocking‑firefox3+
mozillamarcia.knous: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip patch (47.28 KB, patch)
2007-08-01 18:06 PDT, Christine Yen
no flags Details | Diff | Review
[wip] import instead of separate JS service (35.91 KB, patch)
2007-08-06 12:55 PDT, Christine Yen
no flags Details | Diff | Review
wip (20.20 KB, patch)
2007-09-25 10:09 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
more (45.99 KB, patch)
2007-10-02 11:14 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
more (63.62 KB, patch)
2007-10-03 09:23 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
more (65.76 KB, patch)
2007-10-03 14:00 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
more (80.74 KB, patch)
2007-10-04 17:30 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
more (80.78 KB, patch)
2007-10-04 23:29 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
more (81.32 KB, patch)
2007-10-06 13:28 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
more (100.90 KB, patch)
2007-10-08 12:49 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
fix v1 (80.47 KB, patch)
2007-10-11 14:11 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
fix v1 (80.42 KB, patch)
2007-10-11 16:01 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
partially unrotted (16.15 KB, patch)
2007-11-29 10:34 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
mostly de-rotted (16.72 KB, patch)
2007-11-29 12:34 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
making PlacesUtils more modular (34.70 KB, patch)
2007-11-29 19:12 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
more (115.05 KB, patch)
2007-12-20 00:20 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
more (119.60 KB, patch)
2008-01-10 11:15 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
testing (127.33 KB, patch)
2008-01-16 14:18 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
more (182.78 KB, patch)
2008-01-27 22:43 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
more (89.43 KB, patch)
2008-02-06 14:57 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
more (89.62 KB, patch)
2008-02-06 23:10 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
fix (91.37 KB, patch)
2008-02-08 11:46 PST, Dietrich Ayala (:dietrich)
asaf: review-
Details | Diff | Review
strings (2.76 KB, patch)
2008-02-21 23:35 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
strings (entities rev'd) (2.77 KB, patch)
2008-02-21 23:43 PST, Dietrich Ayala (:dietrich)
reed: review-
Details | Diff | Review
strings, take 3 (2.74 KB, patch)
2008-02-21 23:51 PST, Dietrich Ayala (:dietrich)
mbeltzner: review+
mbeltzner: approval1.9+
Details | Diff | Review
most comments fixed (130.08 KB, patch)
2008-02-22 00:11 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
more (209.14 KB, patch)
2008-02-23 22:34 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
fix (208.06 KB, patch)
2008-02-24 19:09 PST, Dietrich Ayala (:dietrich)
asaf: review-
Details | Diff | Review
fix (252.73 KB, patch)
2008-02-25 23:52 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
fix (252.51 KB, patch)
2008-02-26 00:16 PST, Dietrich Ayala (:dietrich)
asaf: review-
Details | Diff | Review
fix (257.08 KB, patch)
2008-02-26 13:41 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
fix (255.14 KB, patch)
2008-02-26 14:59 PST, Dietrich Ayala (:dietrich)
asaf: review+
Details | Diff | Review
fix (256.44 KB, patch)
2008-02-26 16:37 PST, Dietrich Ayala (:dietrich)
dietrich: review+
Details | Diff | Review
for checkin (261.05 KB, patch)
2008-02-26 17:42 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
passes unit tests (261.14 KB, patch)
2008-02-26 18:22 PST, Dietrich Ayala (:dietrich)
mconnor: review+
asaf: review+
Details | Diff | Review
interdiff (622 bytes, text/plain)
2008-02-26 20:35 PST, Dietrich Ayala (:dietrich)
no flags Details
unrotted (261.68 KB, patch)
2008-02-26 22:46 PST, Dietrich Ayala (:dietrich)
dietrich: review+
Details | Diff | Review
fix - removes postplaces import support (8.88 KB, patch)
2008-02-27 12:18 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
fixed patch (197.24 KB, patch)
2008-02-28 10:39 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
fixes the leak (198.01 KB, patch)
2008-02-28 11:08 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
+ more Ts tweaks (198.64 KB, patch)
2008-02-28 13:56 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
interdiff for previous patch (3.40 KB, patch)
2008-02-28 13:57 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
sans tm getter (197.85 KB, patch)
2008-02-28 14:23 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
with tests again (209.44 KB, patch)
2008-02-28 16:44 PST, Dietrich Ayala (:dietrich)
asaf: review+
mbeltzner: approval1.9b4+
Details | Diff | Review
include toolkit js files inline (204.89 KB, patch)
2008-03-04 10:18 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
fix: use inline includes in favor of Cu.import() where possible (207.78 KB, patch)
2008-03-04 14:31 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
fix: inline v2 (207.42 KB, patch)
2008-03-05 18:10 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
with global scope getter instead of #include (205.23 KB, patch)
2008-03-07 15:47 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
w/ smart getter (205.20 KB, patch)
2008-03-08 09:57 PST, Dietrich Ayala (:dietrich)
asaf: review+
Details | Diff | Review
w/ more smart getter (213.52 KB, patch)
2008-03-12 14:16 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
comment fixed (213.76 KB, patch)
2008-03-13 11:23 PDT, Dietrich Ayala (:dietrich)
asaf: review+
Details | Diff | Review
revert debug.js changes (3.60 KB, patch)
2008-03-13 14:21 PDT, Dietrich Ayala (:dietrich)
asaf: review+
Details | Diff | Review
followup toolbar fix (967 bytes, patch)
2008-03-13 15:16 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
remove NS_ASSERT from toolkit utils.js (11.44 KB, patch)
2008-03-13 15:37 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
remove NS_ASSERT from toolkit utils.js (v2) (8.96 KB, patch)
2008-03-13 17:25 PDT, Dietrich Ayala (:dietrich)
asaf: review+
Details | Diff | Review
remove more NS_ASSERTs during startup (12.89 KB, patch)
2008-03-13 18:21 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
per irc (12.97 KB, patch)
2008-03-13 18:49 PDT, Dietrich Ayala (:dietrich)
asaf: review+
Details | Diff | Review
more s/Utils/UIUtils/ fixes (2.62 KB, patch)
2008-03-14 09:10 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review

Description (not reading, please use seth@sspitzer.org instead) 2007-06-13 17:06:28 PDT
use JSON as the on disk, lossless format for our bookmark backup

As we all know, using bookmarks.html is not going to cut it for a lossless format of our places data.  

(Specifically, I'm thinking about bookmarks data.)

What about JSON as our on disk format?   We write out bookmarks.json, and on schema change, we can re-import that format.  (XML could be our format as well.)

Sayre (who is always ahead of the curve), is using JSON for sync:  See https://bugzilla.mozilla.org/show_bug.cgi?id=379517

And for JavaScript 1.8, it sounds like toJSONString, ParseJSONString might be coming, see https://bugzilla.mozilla.org/show_bug.cgi?id=340987

More thoughts:

1) In Firefox 2, there was a pref to specify where to read/write the bookmarks.html file on disk.  I think we'd need a new pref for where to read / write the .json format?

2) There's a RFE bug logged importing bookmarks from a URL (see https://bugzilla.mozilla.org/show_bug.cgi?id=382933).  

But imagine:  "Import / Export Bookmarks from a Web Service", using JSONRequest() (see https://bugzilla.mozilla.org/show_bug.cgi?id=360666) or an XMLHttpRequest.
Comment 1 (not reading, please use seth@sspitzer.org instead) 2007-06-13 17:07:24 PDT
dietrich writes:

session restore already uses json as an on-disk format, and it proved very simple and flexible.

here's a couple of other things we should consider:

1. performance - sessionstore info is never very large, even with 100+ tabs. bookmarks might be much larger, especially if we expand beyond what is exported in bookmarks.html. once we get the bookmarks exposed as json for sync, we should do some perf testing.

2. gah, 3 bookmarks files on disk at close! the firefox build forum has already had numerous comments  (and some bugs files) due to confusion about where the source of truth is wrt to the 2 files we have now. if we do this, we may want to plan an EOL for bookmarks.html.
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-06-14 02:53:54 PDT
The advantage of bookmarks.html above bookmarks.json is that a html file is more easily readable, I think.
Comment 3 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-06-14 06:49:18 PDT
I think we should rather keep the current bookmarks.html file in the profile root and use JSON for bookmakrkbackups/ only. As for schema revisions, I don't find the current "drop all table and re-create from another format" as a long-term solution (post Fx3), it's fine for alphas/nightlies while we keep changing the schema often.
Comment 4 (not reading, please use seth@sspitzer.org instead) 2007-06-20 00:48:54 PDT
thunder, for your synch demo, how did you turn the bookmark data into a json
object?
Comment 5 Mike Connor [:mconnor] 2007-06-28 13:42:14 PDT
Needs more discussion, but I think Mano's suggestion makes sense in general.
Comment 6 (not reading, please use seth@sspitzer.org instead) 2007-07-30 17:57:48 PDT
over to christine, the author of our json -> places / places -> json code (in http://lxr.mozilla.org/seamonkey/source/browser/components/places/content/utils.js) 
Comment 7 (not reading, please use seth@sspitzer.org instead) 2007-07-30 18:21:17 PDT
some thoughts:

1) add a new interface to mozilla/browser/components/places/public/nsISomething.idl

2) move the json <-> places node code from utils.js into a service.  I hope/think we can get away with continuing to implement this in JS.

3) fix utils.js to call the new service

4) fix our code in nsBrowserGlue.js to backup bookmarks to .json format

5) fix our code so that on detection of corrupt places.sqlite (which you can do by putting a non-sqlite file as places.sqlite), we automatically restore from the .json backup.

Note, we will continue to need the migrate from bookmarks.html code (for fx2-migration).

a) should our json backup include history?

b) should our json backup exclude favicons (see related bug #389987).

c) should our json backup exclude microsummary data (I believe we currently exclude some of this from bookmarks.html, our rationale being that it will get updated.)

d) should we support fx3 UI driven import / export to .json? (in addition to html)
Comment 8 (not reading, please use seth@sspitzer.org instead) 2007-07-30 18:37:18 PDT
asking for reconsideration for blocking fx 3.
Comment 9 (not reading, please use seth@sspitzer.org instead) 2007-07-31 00:24:16 PDT
note, as mano suggested in comment #3:  we still need to keep the code that exports to bookmarks.html (of the lossy, fx 2 bookmarks html format) for a while, to support backwards migration to fx 2 (for people who try betas of fx 3).

Comment 10 Christine Yen 2007-08-01 16:46:40 PDT
A couple questions, assuming we are excluding history (for now) and just saving to bookmarkbackups/ instead of adding a bookmarks.js file to the Profile directory...

1) How are we restoring from the exported file? Do we assume that places.sqlite has been corrupted/that the places.sqlite history doesn't correspond to the bookmark backup file, and essentially empty the entire table + reinstantiate all the bookmarks? Or should it just delete the bookmarks root and reinstantiate all the bookmarks on top of existing history?

2) Should more information be wrapped up than is currently wrapped in utils.js wrapNode()?
http://lxr.mozilla.org/seamonkey/source/browser/components/places/content/utils.js#457
Currently saved off:
  id, title, uri, title, parent, index, keyword, annos, and type.

Things we display in the Bookmarks Organizer that we'd lose if we didn't backup history as well:
  Visit Count (keyed on URI)
  Last Visited (keyed on URI)
  Date Added
  Date Modified
Comment 11 Christine Yen 2007-08-01 17:17:29 PDT
Also - there's a lot of similar code in nsSessionStartup relating to saving a JSONified string to disk/reading the string back in and eval'ing it... Dietrich, do you think there's a good way to share code between the two of us? At this point, I pretty much have duplicated chunks of _readFile() and _writeFile() inside my patch-to-be...

writeFile(): http://mxr.mozilla.org/seamonkey/source/browser/components/sessionstore/src/nsSessionStore.js#1984

readFile(): http://mxr.mozilla.org/seamonkey/source/browser/components/sessionstore/src/nsSessionStartup.js#315
Comment 12 Christine Yen 2007-08-01 18:06:01 PDT
Created attachment 274866 [details] [diff] [review]
wip patch

Available, if anyone wants to take a look. Comments are scattered around in it, but wrapNode/unwrapNodes/parseJSON/toJSONString and such have all been moved to the service successfully. Methods lower in the service are similar to the sessionstore methods referred to above...
Comment 13 Dan Mills [:thunder] 2007-08-01 20:28:06 PDT
(In reply to comment #7)

> b) should our json backup exclude favicons (see related bug #389987).
> 
> c) should our json backup exclude microsummary data (I believe we currently
> exclude some of this from bookmarks.html, our rationale being that it will get
> updated.)

Related to this, I think we should have a list of annotations that are OK to export.  An extension adding a new annotation would add it to the list if it makes sense to do so.  Annotations not on the list would be ignored on export.

What do you guys think?
Comment 14 Dietrich Ayala (:dietrich) 2007-08-01 21:58:53 PDT
(In reply to comment #11)
> Also - there's a lot of similar code in nsSessionStartup relating to saving a
> JSONified string to disk/reading the string back in and eval'ing it...
> Dietrich, do you think there's a good way to share code between the two of us?
> At this point, I pretty much have duplicated chunks of _readFile() and
> _writeFile() inside my patch-to-be...
> 
> writeFile():
> http://mxr.mozilla.org/seamonkey/source/browser/components/sessionstore/src/nsSessionStore.js#1984
> 
> readFile():
> http://mxr.mozilla.org/seamonkey/source/browser/components/sessionstore/src/nsSessionStartup.js#315
> 

iirc, the session restore code currently uses non-standard json de/serialization methods. there's a bug for standardizing it, as well as a bug for adding json natively. we should not block on those.

(In reply to comment #13)
> (In reply to comment #7)
> 
> > b) should our json backup exclude favicons (see related bug #389987).
> > 
> > c) should our json backup exclude microsummary data (I believe we currently
> > exclude some of this from bookmarks.html, our rationale being that it will get
> > updated.)
> 
> Related to this, I think we should have a list of annotations that are OK to
> export.  An extension adding a new annotation would add it to the list if it
> makes sense to do so.  Annotations not on the list would be ignored on export.
> 
> What do you guys think?
> 

i think that we should import/export all annotations. in the event of places.sqlite corruption, we want 100% recoverability. possibly there could be arguments for the json import/export to specify which or how many, but the default should be all annotations.

if we do decide to import/export a subset, we could utilize the heretofore unused "flags" parameter to whitelist those annos.
Comment 15 Dietrich Ayala (:dietrich) 2007-08-02 09:46:15 PDT
iirc, don't we get penalized on Ts for every new JS XPCOM service? should we create a general nsBrowserPlacesService for this stuff, and separate it via includes?
Comment 16 Dan Mills [:thunder] 2007-08-02 12:02:42 PDT
(In reply to comment #14)

> i think that we should import/export all annotations. in the event of
> places.sqlite corruption, we want 100% recoverability. possibly there could be
> arguments for the json import/export to specify which or how many, but the
> default should be all annotations.

Perhaps we should do the reverse, then, and allow blacklisting certain annos?  In the case of sqlite corruption, I'd be much more concerned with getting my bookmarks back than with getting every single favicon along with them.

But my primary motivation is not local disk space.  I want this format to be suitable for sync.  So having extra options to the exporter would work.
Comment 17 Dietrich Ayala (:dietrich) 2007-08-02 12:14:10 PDT
(In reply to comment #16)
> (In reply to comment #14)
> 
> > i think that we should import/export all annotations. in the event of
> > places.sqlite corruption, we want 100% recoverability. possibly there could be
> > arguments for the json import/export to specify which or how many, but the
> > default should be all annotations.
> 
> Perhaps we should do the reverse, then, and allow blacklisting certain annos? 

another approach is to use expiration policies as a mechanism for determining what to export. ie: EXPIRE_NEVER should obviously be exported. but EXPIRE_SESSION shouldn't, as they'll be deleted at shutdown anyways. not sure about the date-variable expiration types.

> In the case of sqlite corruption, I'd be much more concerned with getting my
> bookmarks back than with getting every single favicon along with them.
> 

we already have bookmarks backup. i think the change of format is more about being able to expand beyond just bookmarks. the fallout from bug 319455 landing showed just how important annotations are to basic bookmark functionality: descriptions, livemarks and microsummaries all depend on them.
Comment 18 Dan Mills [:thunder] 2007-08-02 12:56:00 PDT
(In reply to comment #17)

> > Perhaps we should do the reverse, then, and allow blacklisting certain annos? 
> 
> another approach is to use expiration policies as a mechanism for determining
> what to export. ie: EXPIRE_NEVER should obviously be exported. but
> EXPIRE_SESSION shouldn't, as they'll be deleted at shutdown anyways. not sure
> about the date-variable expiration types.

That's not a bad idea.  Do we currently use the date-variable expiration types for anything?

> > In the case of sqlite corruption, I'd be much more concerned with getting my
> > bookmarks back than with getting every single favicon along with them.
> 
> we already have bookmarks backup.

This format would be a replacement, not an addition, right?

> the fallout from bug 319455 landing
> showed just how important annotations are to basic bookmark functionality:
> descriptions, livemarks and microsummaries all depend on them.

Right, and those things would not get blacklisted.  I'm more concerned about extensions adding large annotations in the future, and that having unintended impact on sync providers.
Comment 19 Dietrich Ayala (:dietrich) 2007-08-02 17:03:12 PDT
> 
> That's not a bad idea.  Do we currently use the date-variable expiration types
> for anything?

no

> 
> > > In the case of sqlite corruption, I'd be much more concerned with getting my
> > > bookmarks back than with getting every single favicon along with them.
> > 
> > we already have bookmarks backup.
> 
> This format would be a replacement, not an addition, right?
> 

right. we're still going to export to bookmarks.html at shutdown. the json backup will be used for the bookmarkbackups dir (ie: the daily backups), as well as sync, extensions, etc.

> > the fallout from bug 319455 landing
> > showed just how important annotations are to basic bookmark functionality:
> > descriptions, livemarks and microsummaries all depend on them.
> 
> Right, and those things would not get blacklisted.  I'm more concerned about
> extensions adding large annotations in the future, and that having unintended
> impact on sync providers.
> 

hm, i guess in that scenario i'm more inclined to use a whitelist: i'd rather have extension authors explicitly call out annos they do want exported/synced, than rely on them to explicitly call out annos that they *don't*.
Comment 20 Christine Yen 2007-08-03 17:20:48 PDT
per irc:
<dietrich> sayrer: see scrollback - wondering about over xpcom-ification
<dietrich> do we still take a hit for each js service, at startup?
<sayrer> yes, if it is called on startup
<sayrer> and you take a runtime perf hit too
<sayrer> because XPCOM JS components are called like JS -> XPConnect -> JS
<dietrich> so it's better to subscript load instead if it's utility functions that don't need to be a service?
<sayrer> oh god yes

in-tree docs for Components.utils.import:
http://lxr.mozilla.org/mozilla/source/js/src/xpconnect/idl/xpccomponents.idl#174

<thunder> dietrich, yen: you'll want to add to Makefile.in
<thunder> EXTRA_JS_MODULES = foo.js
<thunder> that will make foo.js be available via resource://modules/foo.js
<thunder> yen: yeah, you also need to export the symbols
<yen> where symbols are the method names?
<thunder> right
Comment 21 Christine Yen 2007-08-06 12:55:06 PDT
Created attachment 275460 [details] [diff] [review]
[wip] import instead of separate JS service
Comment 22 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-08-06 13:21:55 PDT
Looks like that patch partially conflicts with the one in bug 386789.
Comment 23 Robert Sayre 2007-08-06 13:28:38 PDT
(In reply to comment #21)
> Created an attachment (id=275460) [details]
> [wip] import instead of separate JS service
> 

Christine and I determined that using the subscript loader from a JS module isn't working. I think that's probably due to security restrictions. I think this is blocked  while waiting for C.u.import supports chrome URLs.
Comment 24 Ray Kiddy 2007-08-09 12:36:52 PDT
There are several bugs, including some old ones, that are requesting backup and restore of profiles.

If you all work out a pretty and clever way of backing up and restoring just Places, is that going to make the task of backing up and restoring profiles harder or easier? Are there things you are doing for this that will be re-usable for backing up and restoring other things in the profle, or the entire profile?

I will give an example. I am doing an extension to replace the Profile Manager with one that has backup and restore. I am going to just copy the directory. If there is a notification that I can send that tells the browser "time to copy your profile info to disk now", I will. I am thinking there is no such notification. If I backup a profile by copying the directory, will the UI you are envisioning give the user the impression that the Places info has not been backed up?

Here are some of the relevant bugs I mentioned.

https://bugzilla.mozilla.org/show_bug.cgi?id=22689
Import/Export Profile (Backup/Restore)

https://bugzilla.mozilla.org/show_bug.cgi?id=193567
Automatically backup all profile data at beginning of each session in case of crash and offer method of restoring profile from backup data after crash

https://bugzilla.mozilla.org/show_bug.cgi?id=17457
Need formalized way to "import" a profile

https://bugzilla.mozilla.org/show_bug.cgi?id=55309
Need an option to store migrated profiles in a user's choice of location
Comment 25 (not reading, please use seth@sspitzer.org instead) 2007-08-13 13:44:12 PDT
> Christine and I determined that using the subscript loader from a JS module
> isn't working. I think that's probably due to security restrictions. I think
> this is blocked  while waiting for C.u.import supports chrome URLs.

robert / christine:  is there a bug on that issue?
Comment 26 Dietrich Ayala (:dietrich) 2007-09-19 17:07:04 PDT
taking.

(In reply to comment #25)
> > Christine and I determined that using the subscript loader from a JS module
> > isn't working. I think that's probably due to security restrictions. I think
> > this is blocked  while waiting for C.u.import supports chrome URLs.
> 
> robert / christine:  is there a bug on that issue?
> 

fixed in bug 386789.
Comment 27 Dietrich Ayala (:dietrich) 2007-09-21 18:53:21 PDT
I did some profiling of using PlacesUtils.wrapNode() (JS code that does JSON serialization) vs the nsIPlacesImportExportService (C++ code that does HTML serialization):

PO_HTML: 482-486, 5 call(s), 76.49ms total, 7.94ms min, 34.65ms max, 15.3ms avg
PO_JSON: 487-489, 5 call(s), 603.65ms total, 91.79ms min, 210.3ms max, 120.73ms avg
JSON is 87% slower

PO_HTML: 482-486, 5 call(s), 46.16ms total, 7.68ms min, 13.74ms max, 9.23ms avg
PO_JSON: 487-489, 5 call(s), 471.54ms total, 86.62ms min, 110.63ms max, 94.31ms avg
JSON is 90% slower

These results are not really about either format, each is a fairly thin wrapper around the data. It's likely that the results say more about the performance costs of JS vs C++, XPConnect crossing, and possibly under-optimization in the JS code, as both approaches use the same Places APIs to get at the data.
Comment 28 Dietrich Ayala (:dietrich) 2007-09-22 17:54:53 PDT
> JSON is 90% slower
> 
> These results are not really about either format, each is a fairly thin wrapper
> around the data. It's likely that the results say more about the performance
> costs of JS vs C++, XPConnect crossing, and possibly under-optimization in the
> JS code, as both approaches use the same Places APIs to get at the data.
> 

I added some optimizations, including: reducing db calls, caching variables that required XPConnect traversal, and writing directly to an output buffer instead of storing the whole object mass in memory. JSON/JS was then only 84% slower than HTML/C++. I tested using small and large bookmarks collections, and the results were consistent.
Comment 29 Dietrich Ayala (:dietrich) 2007-09-24 17:30:54 PDT
The biggest culprit turned out to be the JSON stringification code in http://mxr.mozilla.org/seamonkey/source/js/src/xpconnect/loader/JSON.jsm, which comprises about 60% of the time spent:

serializeNodeToJSONStream: 1929-2018, 130 call(s) (max depth 3), 381.34ms total, 0ms min, 84.31ms max, 2.93ms avg, excluding calls: 85.36ms total, 0ms min, 18.04ms max, 0.66ms avg

JSON_toString: 74-143, 130 call(s), 222.82ms total, 0.96ms min, 7.57ms max, 1.71ms avg, excluding calls: 15.12ms total, 0.09ms min, 0.45ms max, 0.12ms avg

append_piece: 84-138, 2715 call(s) (max depth 3), 207.7ms total, 0ms min, 7.43ms max, 0.08ms avg, excluding calls: 207.7ms total, 0ms min, 7.43ms max, 0.08ms avg
Comment 30 Dietrich Ayala (:dietrich) 2007-09-24 21:00:45 PDT
For comparison, I ran the same test using toSource(), and it was only 30% slower than the C++/HTML code.
Comment 31 Dietrich Ayala (:dietrich) 2007-09-25 10:09:01 PDT
Created attachment 282276 [details] [diff] [review]
wip

- abstracts the node->JSON serialization code, writes to an output stream for re-usability
- modifies copy to use the new serializer
- performance tweaks in the serializer code
- implements backup and restore functionality using JSON, but not activated and needs tests

we can't enable this until bug 340987 gets fixed, due to the slowness of JSON.jsm. if bug 340987 is not going to be fixed for M9, we'll need to look at other options for complete places backup.
Comment 32 Dietrich Ayala (:dietrich) 2007-10-02 11:14:57 PDT
Created attachment 283214 [details] [diff] [review]
more

- hooks up automatic backups and archiving at shutdown
- more serialization efficiencies
Comment 33 Dietrich Ayala (:dietrich) 2007-10-03 09:23:31 PDT
Created attachment 283372 [details] [diff] [review]
more

got restore working (mostly)
Comment 34 Dietrich Ayala (:dietrich) 2007-10-03 14:00:58 PDT
Created attachment 283427 [details] [diff] [review]
more

- fixed restore order
- fixed serialization of saved searches
- re-added html export at shutdown (but need to disable auto-archiving)
Comment 35 Dietrich Ayala (:dietrich) 2007-10-04 17:30:03 PDT
Created attachment 283643 [details] [diff] [review]
more

This patch now does recovery of bookmarks from the json backups instead of bookmarks.html

It also adds two new attributes to nsINavHistoryService:

- adds a "first run" attribute, for detecting if the database was newly created (therefore browser should import bookmarks.html)

- adds an "import bookmarks" attribute, for detecting if we should import a bookmarks backup due to a schema migration

These properties add a distinction between upgrade and schema-change-migration, as well as history vs bookmark migration. It's cleaner than the current approach because it decouples the event in the service (db was created, etc) from the behavior of consumers (eg: import bookmarks.html, and it removes the browser-specific pref from toolkit code.
Comment 36 Dietrich Ayala (:dietrich) 2007-10-04 23:29:40 PDT
Created attachment 283677 [details] [diff] [review]
more

more fixes, and rolled back some of the changes in my last comment: firstRun was hogwash, should only ever import bookmarks.html if the pref is set, or we need to restore and there's no valid backup to restore from.
Comment 37 Dietrich Ayala (:dietrich) 2007-10-06 13:28:55 PDT
Created attachment 283858 [details] [diff] [review]
more
Comment 38 Dietrich Ayala (:dietrich) 2007-10-08 12:49:17 PDT
Created attachment 284025 [details] [diff] [review]
more

- adds support for date-added and last-modified
- adds tests, now passes same tests that HTML import/export does
Comment 39 Dietrich Ayala (:dietrich) 2007-10-08 13:27:51 PDT
note to self, and qa: this is the smoke test i've been using to test this:

- new profile, confirm default bookmarks were imported, confirm that browser.places.importBookmarksHTML is false
- upgrade an Fx2 profile, confirm that bookmarks.html was imported, confirm that browser.places.importBookmarksHTML is false
- shutdown, confirm bookmarks.html written to profile dir, and that a new bookmarks-{date}.json was written to the bookmarkbackups dir, and that a bookmarks-{date}.html was *not* written to the bookmarkbackups dir
- set browser.places.importBookmarksHTML to true, close browser, copy a different bookmarks.html file to the profile dir, start browser, confirm that the bookmarks.html was imported, appended to existing bookmarks, confirm that browser.places.importBookmarksHTML is false
- replace your places.sqlite file w/ any non-sqlite file (empt file, whatever), start browser, confirm that your bookmarks were restored from the latest json backup in /bookmarkbackups
- confirm you have a bookmarks.html in your profile dir, delete your bookmarkbackups dir, replace your places.sqlite file w/ something else, start browser, confirm that your bookmarks were restored from bookmarks.html
- backup bookmarks via the organizer, make some changes, restore bookmarks from the backup, confirm that the original set were restored
- test copy/paste bookmark, separator, folder, livemark
- test cut/paste bookmark, separator, folder, livemark
- test drag/drop bookmark, separator, folder, livemark
Comment 40 Dietrich Ayala (:dietrich) 2007-10-08 18:12:38 PDT
it's impossible to test the copy/paste parts of this due to bug 398898, which causes an immediate crash (then lays waste to all other running applications).
Comment 41 Dietrich Ayala (:dietrich) 2007-10-11 14:11:39 PDT
Created attachment 284522 [details] [diff] [review]
fix v1
Comment 42 Dietrich Ayala (:dietrich) 2007-10-11 14:42:33 PDT
Comment on attachment 284522 [details] [diff] [review]
fix v1

nope.
Comment 43 Dietrich Ayala (:dietrich) 2007-10-11 16:01:46 PDT
Created attachment 284550 [details] [diff] [review]
fix v1

ok, needed to remove GROUP_BY_FOLDER, now that it's behavior has changed.

however, export of large bookmarks sets still takes too long. this still cannot be checked in until native JSON is implemented.
Comment 44 Dietrich Ayala (:dietrich) 2007-10-13 10:23:56 PDT
Comment on attachment 284550 [details] [diff] [review]
fix v1

we don't have native json yet, but it's worth getting the review process going, as this patch probably has other issues worth discussing.
Comment 45 Al Billings [:abillings] 2007-10-15 18:32:12 PDT
QA should add a litmus case or two for this once it is checked in since it is a big change.
Comment 46 Dietrich Ayala (:dietrich) 2007-10-19 11:13:25 PDT
A couple of unresolved items:

1. The patch doesn't create a new bookmarks.json in the profile root directory. just saves/restores from {profdir}/bookmarkbackups. I'm not sure that we want a new crutch in the profile dir. We should instead focus on webservices and IPC for integration.

2. Right now backup/restore and import/export all expect bookmarks.html format. After this patch, backup = json, restore/import can handle both, and export = html. All of which == confusing. UX input would be good here.
Comment 47 Mike Connor [:mconnor] 2007-10-25 12:04:31 PDT
doesn't look like we'll have native JSON for M9, pushing this out.
Comment 48 Mike Beltzner [:beltzner, not reading bugmail] 2007-11-16 10:40:06 PST
Removing ui-wanted; please re-add, but I can't find anything here that needs our attention.
Comment 49 Dietrich Ayala (:dietrich) 2007-11-16 11:12:30 PST
(In reply to comment #48)
> Removing ui-wanted; please re-add, but I can't find anything here that needs
> our attention.
> 

Sorry for not providing details with the request. Here's the situation: This patch introduces a new serialization format into an area where the only option available thus far has been the HTML format.

Currently, the "Import and Backup" menu in the Places organizer currently has 4 options, which all use the HTML format:

- Import
- Export
- Backup
- Restore

This patch makes these changes:

- Import/Restore can take either JSON or HTML formats.
- Backup saves as JSON.

Import/Restore is probably fine ("It just works!"). However, we need to clarify what Backup and Export mean, and I'd rather not expose "save as JSON" in the UI, as that's meaningless to just about everyone, and should stay that way.

The approach I'm currently considering is to change both Backup and Export to use JSON, and add a new "Save Bookmarks as HTML" option. The reason for this is that the HTML format doesn't contain your un-filed bookmarks, your tags or annotations, so we really shouldn't be advertising it as a "backup". (Note: We should add un-filed bookmarks to the HTML export regardless)

A secondary issue is that "Backup" might be confusing for non-technical users who are suddenly thrown into a view of their profile directory, which could be scary. It might be better to have that option instead just write the bookmarks backup directly to the {profdir}/bookmarkbackups directory, and throw up a dialog saying something nice like "Your bookmarks have been backed up. You may restore this backup from the Restore menu.", etc.

If we don't go that route, then Backup and Export are basically the same, differing only in the default location in the filepicker (backup = profdir, export = desktop). Maybe we should consolidate them. 
Comment 50 Dan Mills [:thunder] 2007-11-16 11:58:30 PST
(In reply to comment #49)

> If we don't go that route, then Backup and Export are basically the same,
> differing only in the default location in the filepicker (backup = profdir,
> export = desktop). Maybe we should consolidate them. 

Maybe I have a slightly outdated build, but in my copy (without the json patch), both point to the desktop, and the only difference is the default filename--backup has the date in it.  Why change either of them to the profile dir?  I don't think there's any reason to have either backup or export default to the profile directory...
Comment 51 Dietrich Ayala (:dietrich) 2007-11-17 14:17:44 PST
Marking as high-risk, as there's still no movement on bug 340987. Rob Sayre said it should be up for review this last week, but not there yet.

Once there's a usable patch on that bug, I'll perf test this to see if native JSON enables this patch to perform acceptably, or whether there are other problems.
Comment 52 Mike Beltzner [:beltzner, not reading bugmail] 2007-11-27 16:52:28 PST
(In reply to comment #51)
> Marking as high-risk, as there's still no movement on bug 340987. Rob Sayre
> said it should be up for review this last week, but not there yet.

I think Rob's doing that work in bug 387522, actually. Update dependencies?

(ui-stuff coming RSN!)
Comment 53 Mike Beltzner [:beltzner, not reading bugmail] 2007-11-28 02:38:58 PST
(In reply to comment #49)
> The approach I'm currently considering is to change both Backup and Export to
> use JSON, and add a new "Save Bookmarks as HTML" option. The reason for this 

I'd rather eliminate options from that menu, if we can do it.

Couple of ideas on how to do this, in order of preference:

1. "It always works":
 - Export and Backup should always export to HTML with the JSON serialization embedded in a <!-- comment --!>; our import should always look for that comment and use it if present, otherwise use the HTML import.
 - Backup always saves as JSON, since it's only ever meant to be restored by Firefox (not strictly necessary, but saves some filespace overhead)

2. Save as type
 - add "JSON" to the drop-down for "Save as file type" in the Export... dialog
 - two types: "HTML" (default) and "Firefox 3 Bookmarks (JSON)"
 - backup always uses JSON

> If we don't go that route, then Backup and Export are basically the same,
> differing only in the default location in the filepicker (backup = profdir,
> export = desktop). Maybe we should consolidate them. 

If we can get the consolidation of file types as described above, then I think we can, as backup *requires* JSON, but most of the export tasks I can think of (for online services, other browsers, etc) require HTML.

if we can get the unified HTML/JSON file type, then ...
 - remove "Backup...", and just keep "Export..."; the user can then either import or restore from that file they've exported, and they also have 5 days of backups they can always restore from

if we can't get the unified HTML/JSON file type, then...
 - keep both "Backup..." and "Export..."
 - "Export..." defaults to HTML
 - "Backup..." only saves as JSON
 - both have default save locations of the user's documents folder
Comment 54 Mike Beltzner [:beltzner, not reading bugmail] 2007-11-28 10:31:24 PST
Just spoke with Dietrich, can't get the HTML/JSON combotype, alas.

I know the design proposed above in comment 53 has two entries that basically do the same thing except for filetype; the issue it's solving is that "Export" means "use in another application" so we need to have that export in a format that anyone else can use. "Backup" implies "create a full backup" which shouldn't be lossy in terms of metadata.
Comment 55 Dietrich Ayala (:dietrich) 2007-11-29 10:34:19 PST
Created attachment 290707 [details] [diff] [review]
partially unrotted
Comment 56 Dietrich Ayala (:dietrich) 2007-11-29 12:34:12 PST
Created attachment 290727 [details] [diff] [review]
mostly de-rotted
Comment 57 Dietrich Ayala (:dietrich) 2007-11-29 19:12:05 PST
Created attachment 290813 [details] [diff] [review]
making PlacesUtils more modular
Comment 58 Peter Gerdes 2007-12-04 02:37:09 PST
Alright, I presume there is an obvious answer to this silly question but why bother with a new format at all?

The bookmark data is already stored in a sqlite DB so why not just copy this somewhere?  Or just do a .dump sort of command and save the output.

Now if the goal is to share this data with other programs this might not be so useful but then it doesn't seem like the real goal is backup.  If the worry is use by future versions I still don't see the problem since they must be able to convert the old DBs anyway on upgrade.

Now I like the idea of having bookmarks that can be exported in a nice readable form but if that's the only reason for this choice wouldn't it make more sense to leave it as something easily browsable in any web browser (i.e. html)?

So is the real goal here to create a  bookmark exchange format not a backup format?
Comment 59 Dietrich Ayala (:dietrich) 2007-12-17 18:40:15 PST
(In reply to comment #58)
> Alright, I presume there is an obvious answer to this silly question but why
> bother with a new format at all?
> 
> The bookmark data is already stored in a sqlite DB so why not just copy this
> somewhere?  Or just do a .dump sort of command and save the output.
> 
> Now if the goal is to share this data with other programs this might not be so
> useful but then it doesn't seem like the real goal is backup.  If the worry is
> use by future versions I still don't see the problem since they must be able to
> convert the old DBs anyway on upgrade.
> 
> Now I like the idea of having bookmarks that can be exported in a nice readable
> form but if that's the only reason for this choice wouldn't it make more sense
> to leave it as something easily browsable in any web browser (i.e. html)?
> 
> So is the real goal here to create a  bookmark exchange format not a backup
> format?
> 

The goal is to create a backup format as well as an exchange format. Some benefits:

- very lightweight serialization format
- easy to get customized subsets of data serialized via the query api
- lots of parser implementations

Also, we want to migrate on-disk consumers away from the HTML serialization, which is limited by backwards compatibility reasons, and is not easy to work with.

I've thought about just copying the sqlite file, but then we're only solving the backup problem, and even that has issues: less flexibility wrt to import over vs merge scenarios, higher tool requirements for external consumers. 
Comment 60 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-12-18 04:07:17 PST
Thanks for your answer Dietrich.
To be honest, I don't really understand those benefits.
The only thing I know is that I can open a html file directly inside a browser and a .json file not. I wouldn't know what to do with a .json file.
But perhaps this shouldn't be discussed in a bug, rather at dev.apps.firefox or something?
Comment 61 Dietrich Ayala (:dietrich) 2007-12-18 08:51:18 PST
(In reply to comment #60)
> Thanks for your answer Dietrich.
> To be honest, I don't really understand those benefits.
> The only thing I know is that I can open a html file directly inside a browser
> and a .json file not. I wouldn't know what to do with a .json file.
> But perhaps this shouldn't be discussed in a bug, rather at dev.apps.firefox or
> something?
> 

"human and browser readable" is not a goal of moving to JSON. We have the HTML serialization for that, which you can access via the export option in the Library.

The use-cases for the JSON serialization are backup/restore and external integration. Sorry if this was not clear in my earlier comment.
Comment 62 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-12-18 08:55:16 PST
No, it was clear to me this is for backup/restore, so no need to apologize. Not sure what you mean with external integration, though.
Comment 63 Dietrich Ayala (:dietrich) 2007-12-18 09:02:04 PST
(In reply to comment #62)
> No, it was clear to me this is for backup/restore, so no need to apologize. Not
> sure what you mean with external integration, though.
> 

- web services (eg: del.icio.us, foxmarks)
- local apps that index bookmark and tag data (eg: spotlight, beagle)

Comment 64 Dietrich Ayala (:dietrich) 2007-12-20 00:20:45 PST
Created attachment 294006 [details] [diff] [review]
more
Comment 65 Dietrich Ayala (:dietrich) 2008-01-10 11:15:53 PST
Created attachment 296372 [details] [diff] [review]
more

- unrotted
- use native json
Comment 66 Dietrich Ayala (:dietrich) 2008-01-16 14:18:46 PST
Created attachment 297423 [details] [diff] [review]
testing
Comment 67 Dietrich Ayala (:dietrich) 2008-01-27 22:43:22 PST
Created attachment 299691 [details] [diff] [review]
more
Comment 68 Dietrich Ayala (:dietrich) 2008-02-06 14:57:09 PST
Created attachment 301756 [details] [diff] [review]
more
Comment 69 Reed Loden [:reed] (use needinfo?) 2008-02-06 15:01:03 PST
er, how did you lose 106KB between the last two patches?
Comment 70 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-02-06 16:43:36 PST
Dietrich is just a smart guy, reed.
Comment 71 Dietrich Ayala (:dietrich) 2008-02-06 23:10:05 PST
Created attachment 301831 [details] [diff] [review]
more
Comment 72 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-02-07 06:29:14 PST
Comment on attachment 301756 [details] [diff] [review]
more

Random non-binding flyby review!

>     if (this._profileStarted) {
>-      // final places cleanup
>+      // Shutdown Places
>+      // XXX why is this not in onProfileShutdown()?
>       this._shutdownPlaces();
>     }
>   },

Yeah, why is that?  File for it?

>+  get PlacesUtils() {
>+    if(typeof(PlacesUtils) != "function") { // we should dynamically load the script
>+      var loader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"].
>+                   getService(Components.interfaces.mozIJSSubScriptLoader);
>+      loader.loadSubScript("chrome://global/content/debug.js");
>+      loader.loadSubScript("chrome://browser/content/places/utils.js");
>+    }
>+    return PlacesUtils;
>+  },

Would using the module-import for Fx3 work better here?  I think it has better performance characteristics, especially if you could end up importing multiple times in the lifetime of the app (even into different scopes).  Not sure if we fastload, but we're more likely to do so in the future for import than for subscript loading.

>+        // only back up pre-places bookmarks.html if we plan on overwriting it
>+        if (prefBranch.getBoolPref("browser.bookmarks.overwrite")) {
>+          // backup pre-places bookmarks.html
>+          // XXXtodo remove this before betas, after import/export is solid
>+          var profDir = dirService.get("ProfD", Ci.nsILocalFile);
>+          var bookmarksBackup = profDir.clone();
>+          bookmarksBackup.append("bookmarks.preplaces.html");

XXXtodo now?  I know you were just reindenting that passage, but it seems like it's worth revisiting in the context of this bug.  (Given that we now back up as JSON, though, we would never overwrite .html, perhaps?  In which case that code can just go?)
Comment 73 Dietrich Ayala (:dietrich) 2008-02-07 16:53:23 PST
> >     if (this._profileStarted) {
> >-      // final places cleanup
> >+      // Shutdown Places
> >+      // XXX why is this not in onProfileShutdown()?
> >       this._shutdownPlaces();
> >     }
> >   },
> 
> Yeah, why is that?  File for it?

fixed to do places cleanup when we receive quit-application-granted.

currently, we're doing /browser places shutdown tasks *after* /toolkit places shuts down due to receiving xpcom-shutdown: wtf.

> Would using the module-import for Fx3 work better here?  I think it has better
> performance characteristics, especially if you could end up importing multiple
> times in the lifetime of the app (even into different scopes).  Not sure if we
> fastload, but we're more likely to do so in the future for import than for
> subscript loading.

yup, fixed here and wherever else we were subscript loading utils.js

> 
> >+        // only back up pre-places bookmarks.html if we plan on overwriting it
> >+        if (prefBranch.getBoolPref("browser.bookmarks.overwrite")) {
> >+          // backup pre-places bookmarks.html
> >+          // XXXtodo remove this before betas, after import/export is solid
> >+          var profDir = dirService.get("ProfD", Ci.nsILocalFile);
> >+          var bookmarksBackup = profDir.clone();
> >+          bookmarksBackup.append("bookmarks.preplaces.html");
> 
> XXXtodo now?  I know you were just reindenting that passage, but it seems like
> it's worth revisiting in the context of this bug.  (Given that we now back up
> as JSON, though, we would never overwrite .html, perhaps?  In which case that
> code can just go?)
> 

Hm, yes, since we're no longer clobbering bookmarks.html unless the user explicitly asks us to via browser.bookmarks.autoExportHTML=true, this pref serves no purpose.
Comment 74 Dietrich Ayala (:dietrich) 2008-02-08 11:46:46 PST
Created attachment 302160 [details] [diff] [review]
fix

reduced code duplication down to the nodeIs* methods (and a few consts/vars). serialization is moved to toolkit. restoration isn't yet because of larger dependencies on utils.js, but there's no need to block on that.

this fixes shaver's comments, saved-searches, and fixes livemark save/restore to not duplicate feed and site uris.

i need to finish my FFTs for this (and publish them), as well as remove the perf debugging stuff, but it's ready for first review now.
Comment 75 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-02-15 23:47:29 PST
Index: browser/components/nsBrowserGlue.js
===================================================================
 
-const Ci = Components.interfaces;
-const Cc = Components.classes;
-const Cr = Components.results;
-const Cu = Components.utils;
+let Ci = Components.interfaces;
+let Cc = Components.classes;
+let Cr = Components.results;
+let Cu = Components.utils;

why?

 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource:///modules/distribution.js");
 
+// Backup bookmarks once per day on 5 mins idle.
+const BOOKMARKS_ARCHIVE_IDLE_TIME = 60 * 5;
+

The delay should be much longer IMO.


 // Factory object
 const BrowserGlueServiceFactory = {
   _instance: null,
   createInstance: function (outer, iid) 
   {
     if (outer != null)
       throw Components.results.NS_ERROR_NO_AGGREGATION;
     return this._instance == null ?
@@ -97,20 +100,29 @@ BrowserGlue.prototype = {
         this._onQuitRequest(subject, data);
         break;
       case "quit-application-granted":
         if (this._saveSession) {
           var prefBranch = Cc["@mozilla.org/preferences-service;1"].
                            getService(Ci.nsIPrefBranch);
           prefBranch.setBoolPref("browser.sessionstore.resume_session_once", true);
         }
+        this._shutdownPlaces();

and here you could remove the observer rather than clearing _shutdownPlaces.

+        break;
+      case "idle":
+        if (this.idleService.idleTime > BOOKMARKS_ARCHIVE_IDLE_TIME * 1000) {
+          // Back up bookmarks.
+          this._archiveBookmarks();
+          // Remove the idle observer.
+          this.idleService.removeIdleObserver(this, BOOKMARKS_ARCHIVE_IDLE_TIME);


What if I leave the browser open for few days? What if I reopen it twice a day? Is archive done per launch rather than "on day" as your comment says?

 
   _onProfileChange: function()
   {
-    // this block is for code that depends on _onProfileStartup() having 
-    // been called.
-    if (this._profileStarted) {
-      // final places cleanup
-      this._shutdownPlaces();
-    }
   },

why did you leave the empty function in place?


+    else {
+      var dirService = Cc["@mozilla.org/file/directory_service;1"].
+                       getService(Ci.nsIProperties);
+      var bookmarksFile = dirService.get("BMarks", Ci.nsILocalFile);
+
+      if (bookmarksFile.exists()) {
+        // import the file
+        try {
+          var importer = 
+            Cc["@mozilla.org/browser/places/import-export-service;1"].
+            getService(Ci.nsIPlacesImportExportService);
+          importer.importHTMLFromFile(bookmarksFile, true);
+        } catch(ex) {
+        } finally {
+          prefBranch.setBoolPref("browser.places.importBookmarksHTML", false);
         }

nit: remove the catch block
       }
     }
+
+    // Initialize bookmark archiving on idle.
+    // Once a day, either on idle or shutdown, bookmarks are backed up.
+    this.idleService.addIdleObserver(this, BOOKMARKS_ARCHIVE_IDLE_TIME);

Again, once a day?

* - export bookmarks as HTML, if so configured
+   *
+   * Note: quit-application-granted notification is received twice
+   *       so replace this method with a no-op when first called.

see notes above.


+  _archivedBookmarks: false,
+  _archiveBookmarks: function nsBrowserGlue__archiveBookmarks() {
+    // Do nothing if bookmarks already backed up.
+    if (this._archivedBookmarks)
+      return;

but how would we get here if the observer is gone?


Index: browser/components/places/content/places.js
===================================================================
     if (fileList.length == 0)
       return;
 
     // populate menu
     for (var i = 0; i < fileList.length; i++) {
       var m = restorePopup.insertBefore
         (document.createElement("menuitem"),
          document.getElementById("restoreFromFile"));
       var dateStr = fileList[i].leafName.replace("bookmarks-", "").
-        replace(".html", "");
+        replace(".html", "").replace(".json", "");

use regexp please, and make sure that it's on the end.

@@ -409,82 +410,74 @@ var PlacesOrganizer = {
   onRestoreMenuItemClick: function PO_onRestoreMenuItemClick(aMenuItem) {
     var dirSvc = Cc["@mozilla.org/file/directory_service;1"].
                  getService(Ci.nsIProperties);
     var bookmarksFile = dirSvc.get("ProfD", Ci.nsIFile);
     bookmarksFile.append("bookmarkbackups");
     bookmarksFile.append(aMenuItem.getAttribute("value"));
     if (!bookmarksFile.exists())
       return;
+    PlacesUtils.restoreBookmarksFromFile(bookmarksFile);
+  },
+
+  /**
+   * Called when 'Choose File...' is selected from the restore menu.
+   * Prompts for a file and restores bookmarks to those in the file.
+   */
+  onRestoreBookmarksFromFile: function PO_onRestoreBookmarksFromFile() {
+    var fp = Cc["@mozilla.org/filepicker;1"].createInstance(Ci.nsIFilePicker);
+    fp.init(window, PlacesUtils.getString("bookmarksRestoreTitle"),
+            Ci.nsIFilePicker.modeOpen);
+
+    var dirSvc = Cc["@mozilla.org/file/directory_service;1"].
+                 getService(Ci.nsIProperties);
+    var backupsDir = dirSvc.get("Desk", Ci.nsILocalFile);
+    fp.displayDirectory = backupsDir;

why Desk?



   backupBookmarks: function PO_backupBookmarks() {
     var fp = Cc["@mozilla.org/filepicker;1"].createInstance(Ci.nsIFilePicker);
     fp.init(window, PlacesUtils.getString("bookmarksBackupTitle"),
             Ci.nsIFilePicker.modeSave);
-    fp.appendFilters(Ci.nsIFilePicker.filterHTML);

no filter is forced?


Index: browser/components/places/content/utils.js
===================================================================
RCS file: /cvsroot/mozilla/browser/components/places/content/utils.js,v
retrieving revision 1.100
diff -u -8 -p -r1.100 utils.js
--- browser/components/places/content/utils.js	2 Feb 2008 00:39:05 -0000	1.100
+++ browser/components/places/content/utils.js	8 Feb 2008 19:42:11 -0000
@@ -18,16 +18,17 @@
  * Portions created by the Initial Developer are Copyright (C) 2005
  * the Initial Developer. All Rights Reserved.
  *
  * Contributor(s):
  *   Ben Goodger <beng@google.com>
  *   Myk Melez <myk@mozilla.org>
  *   Asaf Romano <mano@mozilla.com>
  *   Sungjoon Steve Won <stevewon@gmail.com>
+ *   Dietrich Ayala <dietrich@mozilla.com>
  *
  * Alternatively, the contents of this file may be used under the terms of
  * either the GNU General Public License Version 2 or later (the "GPL"), or
  * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
  * in which case the provisions of the GPL or the LGPL are applicable instead
  * of those above. If you wish to allow use of your version of this file only
  * under the terms of either the GPL or the LGPL, and not to allow others to
  * use your version of this file under the terms of the MPL, indicate your
@@ -37,21 +38,29 @@
  * the terms of any one of the MPL, the GPL or the LGPL.
  *
  * ***** END LICENSE BLOCK ***** */
 
 function LOG(str) {
   dump("*** " + str + "\n");
 }
 
+var EXPORTED_SYMBOLS = ["PlacesUtils"];
+
 var Ci = Components.interfaces;
 var Cc = Components.classes;
 var Cr = Components.results;
 
-Components.utils.import("resource://gre/modules/JSON.jsm");
+if (typeof(NS_ASSERT) != "function") {
+  var loader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"].
+               getService(Components.interfaces.mozIJSSubScriptLoader);
+  loader.loadSubScript("chrome://global/content/debug.js");
+}
+

this bothers me, can yoy make sure that browser.xul actually load debug.js before placesOverlay?

+const JSON = Cc["@mozilla.org/dom/json;1"].createInstance(Ci.nsIJSON);

why const? why is it in the global context?

   },
 
   get ptm() {
     delete this.ptm;
     return this.ptm = Cc["@mozilla.org/browser/placesTransactionsService;1"].
                       getService(Ci.nsIPlacesTransactionsService);
   },
 
+  get pio() {
+    delete this.pio;
+    Components.utils.import("resource://gre/modules/PlacesIO.js");
+    return this.pio = PlacesIO;

Should you just define a PlaceIO smart getter in the global scope? this is kinda complex for no good reason.


@@ -478,89 +493,36 @@ var PlacesUtils = {
   wrapNode: function PU_wrapNode(aNode, aType, aOverrideURI) {
     var self = this;
 
     // when wrapping a node, we want all the items, even if the original
     // query options are excluding them.
     // this can happen when copying from the left hand pane of the bookmarks
     // organizer
     function convertNode(cNode) {
-      try {
-        if (self.nodeIsFolder(cNode) && cNode.queryOptions.excludeItems)
-          return self.getFolderContents(cNode.itemId, false, true).root;
-      }
-      catch (e) {
+      if (self.nodeIsFolder(cNode)) {
+        this.asQuery(cNode);
+        if (cNode.queryOptions.excludeItems)

hrm, you could do asQuery(cNode).queryOptions.excludeItems and keep it in one block.


-                       type: self.TYPE_X_MOZ_PLACE_CONTAINER };
-              bNode.containerOpen = wasOpen;
-            }
+        var writer = {
+          value: "",
+          write: function PU_wrapNode__write(aStr, aLen) {
+            this.value += aStr;
           }
-          return node;
-        }
-        return JSON.toString(gatherDataPlace(convertNode(aNode)));
+        };
+        self.pio.serializeNodeAsJSONToOutputStream(aNode, writer);

Why is PlacesIO loaded lazily anyway?


+    // tag folders go through the tag service

not anymore, please use the transactions.

-      break;
-    case this.TYPE_X_MOZ_PLACE_SEPARATOR:
-      if (copy) {
+          /*
+          if (data.livemark && data.annos) {
+            // Place is a Livemark Container
+            var feedURI = null;
+            var siteURI = null;
+            data.annos.forEach(function(aAnno) {
+              if (aAnno.name == LMANNO_FEEDURI)
+                feedURI = this._uri(aAnno.value);
+              if (aAnno.name == LMANNO_SITEURI)
+                siteURI = this._uri(aAnno.value);
+            }, this);
+            return this.ptm.createLivemark(feedURI, siteURI, data.title, container,
+                                           index, data.annos);
+          }
+          else
+            return this._getFolderCopyTransaction(data, container, index);
+          */

?

+  /**
+   * Restores bookmarks/tags from a JSON or HTML file.
+   * WARNING: This method *removes* any bookmarks in the collection before
+   * restoring from the file.
+   */
+  restoreBookmarksFromFile: function PU_restoreBookmarksFromFile(aFile) {
+    var ioSvc = Cc["@mozilla.org/network/io-service;1"].
+                 getService(Ci.nsIIOService);
+    var fileURL = ioSvc.newFileURI(aFile).QueryInterface(Ci.nsIURL);
+    var fileExtension = fileURL.fileExtension.toLowerCase();
+    if (fileExtension == "html")
+      this.iesvc.importHTMLFromFile(aFile, true);
+    else if (fileExtension == "json")
+      this.restoreBookmarksFromJSONFile(aFile);
+    else
+      alert("unsupported file type"); // FIXME

Yeah, l10n freeze is in 6 days, so please :)

+  restoreBookmarksFromJSONString:
+  function PU_restoreBookmarksFromJSONString(aString) {
+    // convert string to JSON
+    var nodes = null;
+    try {
+      nodes = this.unwrapNodes(aString, this.TYPE_X_MOZ_PLACE_CONTAINER);
+    } catch (ex) {
+      LOG("restoreBookmarksFromFile(): " + ex);
+      return;
+    }
+
+    if (nodes.length == 0 || !nodes[0].children ||
+        nodes[0].children.length == 0)
+      return; // nothing to restore
+

what about nodes[1]?


+      if (!node.children || node.children.length == 0) {
+        LOG("no kids\n");
+        return; // nothing to restore for this root
+      }

not useful :)

+      var container = this.placesRootId; // default to places root
+      switch (root) {
+        case "bookmarksMenuFolder":
+          container = this.bookmarksMenuFolderId;
+          break;
+        case "tagsFolder":
+          container = this.tagsFolderId;
+          break;
+        case "unfiledBookmarksFolder":
+          container = this.unfiledBookmarksFolderId;
+          break;
+        case "toolbarFolder":
+          container = this.toolbarFolderId;
+          break;
+      }
+

You default to the placeroot (no idea why), but you don't handle that case.
'
+      // create transactions for inserting the data into the db
+      var transactions = [];
+      try {
+        for (var j = 0; j < node.children.length; j++) {
+          var child = node.children[j];
+          var index = child.index;
+          transactions.push(this.makeTransaction(child, child.type, 
+                                                 container, index, true));
+        }
+      } catch(ex) {
+        LOG("restoreBookmarksFromFile(): " + ex);
+      }
+
+      if (transactions.length != 0) {
+        // remove old contents only if we have new data to replace it with
+        if (root != "tagsFolder")
+          this.bookmarks.removeFolderChildren(container);
+        else {
+          // must remove tags via the tagging service
+          // so it doesn't get out of sync
+          var tags = this.tagging.allTags;
+          var uris = [];
+          for (let i in tags) {
+            var tagURIs = this.tagging.getURIsForTag(tags[i]);
+            for (let i in tagURIs)
+              this.tagging.untagURI(tagURIs[i], [tags[i]]);

Use the transactions.

+  archiveBookmarksFile:
+  function PU_archiveBookmarksFile(aNumberOfBackups, aForceArchive) {
  
document the arguments please, what is aNumberOfBackups?

+    // get/create backups directory
+    var dirService = Cc["@mozilla.org/file/directory_service;1"].
+                     getService(Ci.nsIProperties);
+    var bookmarksBackupDir = dirService.get("ProfD", Ci.nsILocalFile);
+    bookmarksBackupDir.append("bookmarkbackups");
+    if (!bookmarksBackupDir.exists())
+      bookmarksBackupDir.create(Ci.nsIFile.DIRECTORY_TYPE, 0700);
+    if (!bookmarksBackupDir.exists())
+      return; // unable to create directory!
+

that should be nested.

+    // construct the new leafname
+    // Use YYYY-MM-DD (ISO 8601) as it doesn't contain illegal characters
+    // and makes the alphabetical order of multiple backup files more useful.
+    var date = (new Date).toLocaleFormat("%Y-%m-%d");
+    var backupFilenamePrefix = this.getString("bookmarksArchiveFilenamePrefix");
+    var backupFilenameSuffix = this.getString("bookmarksArchiveFilenameSuffix");
+    var backupFilename = backupFilenamePrefix + date + backupFilenameSuffix;

one formatted string would be nice...

+    var backupFile = null;
+    if (!aForceArchive) {
+      var backupFileNames = [];
+      var hasCurrentBackup = false;

unused.


+  /**
+   * Get the most recent backup file.
+   * @returns nsIFile backup file
+   */
+  getMostRecentBackup: function PU_getMostRecentBackup() {
+    var dirService = Cc["@mozilla.org/file/directory_service;1"].
+                     getService(Ci.nsIProperties);
+    var bookmarksBackupDir = dirService.get("ProfD", Ci.nsILocalFile);
+    bookmarksBackupDir.append("bookmarkbackups");
+    if (!bookmarksBackupDir.exists())
+      return null;
+
+    var backupFilenamePrefix = this.getString("bookmarksArchiveFilenamePrefix");
+

Still you could use one string along with a regexp.

Index: browser/components/places/src/nsPlacesTransactionsService.js
===================================================================

 const loadInSidebarAnno = "bookmarkProperties/loadInSidebar";
 const descriptionAnno = "bookmarkProperties/description";
 const CLASS_ID = Components.ID("c0844a84-5a12-4808-80a8-809cb002bb4f");
 const CONTRACT_ID = "@mozilla.org/browser/placesTransactionsService;1";
 
-var loader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"].
-             getService(Components.interfaces.mozIJSSubScriptLoader);
-loader.loadSubScript("chrome://global/content/debug.js");
-loader.loadSubScript("chrome://browser/content/places/utils.js");
+function NS_ASSERT(aAssertion, aMsg) {
+  if (!aAssertion)
+    throw (aMsg);
+}

hrm?

Index: browser/components/places/tests/unit/head_bookmarks.js
===================================================================
  * use your version of this file under the terms of the MPL, indicate your
  * decision by deleting the provisions above and replace them with the notice
  * and other provisions required by the GPL or the LGPL. If you do not delete
  * the provisions above, a recipient may use your version of this file under
  * the terms of any one of the MPL, the GPL or the LGPL.
  *
  * ***** END LICENSE BLOCK ***** */
 
+version(170);
+

what's this about?

Index: browser/components/places/tests/unit/test_384370.js
===================================================================

Please add a license header.

Index: browser/components/places/tests/unit/test_placesTxn.js
===================================================================

Why did you comment out tests here?

Index: toolkit/components/places/src/Makefile.in

 
Index: toolkit/components/places/src/PlacesIO.js
===================================================================

+
+const JSON = Cc["@mozilla.org/dom/json;1"].createInstance(Ci.nsIJSON);
+const history = Cc["@mozilla.org/browser/nav-history-service;1"].getService(Ci.nsINavHistoryService);
+const bookmarks = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].getService(Ci.nsINavBookmarksService);
+const livemarks = Cc["@mozilla.org/browser/livemark-service;2"].getService(Ci.nsILivemarkService);
+const annotations= Cc["@mozilla.org/browser/annotation-service;1"].getService(Ci.nsIAnnotationService);

Don't get services in the global scope.'

I didn't review the serialization code yet, will do in the next round.
Comment 76 Dietrich Ayala (:dietrich) 2008-02-21 23:35:02 PST
Created attachment 304914 [details] [diff] [review]
strings

strings only, for l10n freeze.
Comment 77 Dietrich Ayala (:dietrich) 2008-02-21 23:43:39 PST
Created attachment 304915 [details] [diff] [review]
strings (entities rev'd)
Comment 78 Reed Loden [:reed] (use needinfo?) 2008-02-21 23:46:47 PST
Comment on attachment 304915 [details] [diff] [review]
strings (entities rev'd)

>-bookmarksBackupFilename=Bookmarks %S.html
>+bookmarksBackupFilenameJSON=Bookmarks %S.json

...

>-bookmarksRestoreAlert=This will replace all of your current bookmarks with the backup. Are you sure?
>+bookmarksRestoreAlertTags=This will replace all of your current bookmarks and tags with the backup. Are you sure?

Since you're not committing the entire patch, you can't actually change the old entities. You can only add the new ones until the patch lands...
Comment 79 Dietrich Ayala (:dietrich) 2008-02-21 23:51:18 PST
Created attachment 304916 [details] [diff] [review]
strings, take 3
Comment 80 Mike Beltzner [:beltzner, not reading bugmail] 2008-02-21 23:53:44 PST
Comment on attachment 304916 [details] [diff] [review]
strings, take 3

r+uir+a=beltzner
Comment 81 Dietrich Ayala (:dietrich) 2008-02-21 23:56:52 PST
Checking in browser/locales/en-US/chrome/browser/places/places.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.properties,v  <--  places.properties
new revision: 1.38; previous revision: 1.37
done
Checking in toolkit/locales/en-US/chrome/places/places.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/places/places.properties,v  <--  places.properties
new revision: 1.4; previous revision: 1.3
done
Comment 82 Dietrich Ayala (:dietrich) 2008-02-22 00:11:35 PST
Created attachment 304919 [details] [diff] [review]
most comments fixed
Comment 83 Dietrich Ayala (:dietrich) 2008-02-23 22:34:25 PST
Created attachment 305293 [details] [diff] [review]
more

more
Comment 84 Dietrich Ayala (:dietrich) 2008-02-24 19:09:11 PST
Created attachment 305424 [details] [diff] [review]
fix
Comment 85 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-02-24 20:49:12 PST
Comment on attachment 305424 [details] [diff] [review]
fix

Index: browser/base/content/browser-places.js
===================================================================

@@ -620,17 +620,17 @@ var BookmarksEventHandler = {  
    * Menus and submenus from the folder buttons bubble up to this handler.
    * Opens the item.
    * @param aEvent 
    *        DOMEvent for the command
    */
   onCommand: function BM_onCommand(aEvent) {
     var target = aEvent.originalTarget;
     if (target.node) {
-      PlacesUtils.getViewForNode(target)
+      PlacesUIUtils.getViewForNode(target)
                  .controller
                  .openSelectedNodeWithEvent(aEvent);
                  

nit: fix alignment here and in other places like this across the patch (I noticed few in browser.js for instance).


Index: browser/components/nsBrowserGlue.js
===================================================================
RCS file: /cvsroot/mozilla/browser/components/nsBrowserGlue.js,v
retrieving revision 1.56
diff -u -8 -p -r1.56 nsBrowserGlue.js
--- browser/components/nsBrowserGlue.js	23 Feb 2008 09:32:09 -0000	1.56
+++ browser/components/nsBrowserGlue.js	25 Feb 2008 03:06:46 -0000
@@ -40,16 +40,23 @@
 const Ci = Components.interfaces;
 const Cc = Components.classes;
 const Cr = Components.results;
 const Cu = Components.utils;
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource:///modules/distribution.js");
 

+// Check to see if bookmarks need backing up once per
+// day on 1 hour idle.
+const BOOKMARKS_ARCHIVE_IDLE_TIME = 60 * 60;

3600.

+
+// Backup bookmarks once every 24 hours.
+const BOOKMARKS_ARCHIVE_INTERVAL = 86400 * 1000;
+
86400000

:)
 // Factory object
 const BrowserGlueServiceFactory = {
   _instance: null,
   createInstance: function (outer, iid) 
   {
     if (outer != null)
       throw Components.results.NS_ERROR_NO_AGGREGATION;
     return this._instance == null ?
@@ -98,25 +105,32 @@ BrowserGlue.prototype = {
         break;
       case "quit-application-requested":
         this._onQuitRequest(subject, data);
         break;
       case "quit-application-granted":
         if (this._saveSession) {
           this._setPrefToSaveSession();
         }
+        this._shutdownPlaces();
         break;
       case "session-save":
         this._setPrefToSaveSession();
         subject.QueryInterface(Ci.nsISupportsPRBool);
         subject.data = true;
         break;
+      case "idle":
+        if (this.idleService.idleTime > BOOKMARKS_ARCHIVE_IDLE_TIME * 1000) {
+          // Back up bookmarks.
+          this._archiveBookmarks();
+        }
+        break;
     }
-  }
-, 
+  }, 
+
   // initialization (called on application startup) 
   _init: function() 
   {
     // observer registration
     const osvr = Cc['@mozilla.org/observer-service;1'].
                  getService(Ci.nsIObserverService);
     osvr.addObserver(this, "quit-application", false);
     osvr.addObserver(this, "xpcom-shutdown", false);
@@ -319,88 +333,139 @@ BrowserGlue.prototype = {
     if(typeof(Sanitizer) != "function") { // we should dynamically load the script
       Cc["@mozilla.org/moz/jssubscript-loader;1"].
       getService(Ci.mozIJSSubScriptLoader).
       loadSubScript("chrome://browser/content/sanitize.js", null);
     }
     return Sanitizer;
   },
 
+  _idleService: null,
+  get idleService() {
+    if (!this._idleService)
+      this._idleService = Cc["@mozilla.org/widget/idleservice;1"].
+                          getService(Ci.nsIIdleService);
+    return this._idleService;
+  },
+
   /**
    * Initialize Places
    * - imports the bookmarks html file if bookmarks datastore is empty
    */
   _initPlaces: function bg__initPlaces() {
     // we need to instantiate the history service before we check the 
     // the browser.places.importBookmarksHTML pref, as 
     // nsNavHistory::ForceMigrateBookmarksDB() will set that pref
     // if we need to force a migration (due to a schema change)
     var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
                   getService(Ci.nsINavHistoryService);
 
+    var prefBranch = Cc["@mozilla.org/preferences-service;1"].
+                     getService(Ci.nsIPrefBranch);
+
     var importBookmarks = false;
     try {
-      var prefBranch = Cc["@mozilla.org/preferences-service;1"].
-                       getService(Ci.nsIPrefBranch);
       importBookmarks = prefBranch.getBoolPref("browser.places.importBookmarksHTML");
     } catch(ex) {}
 
+    dump("importing? " + importBookmarks + "\n");

Make sure not to leave these in when landing, dump()s in components code don't
follow the pref.
'
     if (!importBookmarks) {
       // Call it here for Fx3 profiles created before the Places folder
       // has been added, otherwise it's called during import.
       this.ensurePlacesDefaultQueriesInitialized();
-      return;
     }
-
-    var dirService = Cc["@mozilla.org/file/directory_service;1"].
-                     getService(Ci.nsIProperties);
-
-    var bookmarksFile = dirService.get("BMarks", Ci.nsILocalFile);
-
-    if (bookmarksFile.exists()) {
-      // import the file
-      try {
-        var importer = 
-          Cc["@mozilla.org/browser/places/import-export-service;1"].
-          getService(Ci.nsIPlacesImportExportService);
-        importer.importHTMLFromFile(bookmarksFile, true);
-      } catch(ex) {
-      } finally {
-        prefBranch.setBoolPref("browser.places.importBookmarksHTML", false);
+    else {
+      return;

?

Index: browser/components/places/content/utils.js
===================================================================

+var PlacesUIUtils = {

Did you ensure each get*String directs to the right object?


 
Index: browser/components/places/src/nsPlacesTransactionsService.js
===================================================================

-var loader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"].
-             getService(Components.interfaces.mozIJSSubScriptLoader);
-loader.loadSubScript("chrome://global/content/debug.js");
-loader.loadSubScript("chrome://browser/content/places/utils.js");
-
 Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
+Components.utils.import("resource://gre/modules/utils.js");
+
+#include ../../../../toolkit/content/debug.js

I think we should make debug.js a resource too and import it everywhere.


Index: browser/components/places/tests/unit/head_bookmarks.js
===================================================================

+version(170);
+

So, I still don't know what's this about


Index: browser/components/places/tests/unit/test_384370.js
===================================================================
+ * The Original Code is mozilla.com code.

Bug ###### code.


+ * Alternatively, the contents of this file may be used under the terms of
+ * either the GNU General Public License Version 2 or later (the "GPL"), or
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].getService(Ci.nsINavHistoryService);
+var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].getService(Ci.nsINavBookmarksService);
+var livemarksvc = Cc["@mozilla.org/browser/livemark-service;2"].getService(Ci.nsILivemarkService);
+var annosvc = Cc["@mozilla.org/browser/annotation-service;1"].getService(Ci.nsIAnnotationService);
+var tagsvc = Cc["@mozilla.org/browser/tagging-service;1"].getService(Ci.nsITaggingService);
+
+
+Components.utils.import("resource://gre/modules/utils.js");

nit: use the getters from utils.js :)



Index: toolkit/components/places/jar.mn
===================================================================
RCS file: toolkit/components/places/jar.mn
diff -N toolkit/components/places/jar.mn
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ toolkit/components/places/jar.mn	25 Feb 2008 03:06:48 -0000
@@ -0,0 +1,2 @@
+toolkit.jar:
+*   content/global/placesUtils.js       (src/utils.js)

hrm, utils.js is fine.

I don't get this, if it's a resource, why should it be jarred as well?

browser's utils.js should just import it.



Index: toolkit/components/places/src/utils.js
===================================================================
+#include ../../../content/debug.js
+

See my comment for the other instance.

+const LOAD_IN_SIDEBAR_ANNO = "bookmarkProperties/loadInSidebar";
+const DESCRIPTION_ANNO = "bookmarkProperties/description";
+const POST_DATA_ANNO = "bookmarkProperties/POSTData";

+const ORGANIZER_FOLDER_ANNO = "PlacesOrganizer/OrganizerFolder";
+const ORGANIZER_QUERY_ANNO = "PlacesOrganizer/OrganizerQuery";
+

Frontend stuff.

+#ifdef XP_MACOSX
+// On Mac OSX, the transferable system converts "\r\n" to "\n\n", where we
+// really just want "\n".
+const NEWLINE= "\n";
+#else
+// On other platforms, the transferable system converts "\r\n" to "\n".
+const NEWLINE = "\r\n";
+#endif
+

Probably that too, it's only used by d & d code as far as I remember.


+var PlacesUtils = {
+  // Place entries that are containers, e.g. bookmark folders or queries.
+  TYPE_X_MOZ_PLACE_CONTAINER: "text/x-moz-place-container",
...
+  TYPE_UNICODE: "text/unicode",
+

ditto.


+  get globalHistory() {

Could you please just remove this and QI this.history to nsIBrowserHistory in
the one place it's used?

get RDF()
get localStore()
get ptm()
get microsummaries()

Frontend.

get clipboard()
get URIFixup()

Probably these too.



+  getFormattedString: function PU_getFormattedString(key, params) {
+    try {
+      return this._bundle.formatStringFromName(key, params, params.length);
+    }
+    catch(ex) {
+      LOG("getFormattedString(): key " + key + " threw: " + ex);
+    }
+  },
+
+  getString: function PU_getString(key) {
+    try {
+      return this._bundle.GetStringFromName(key);
+    }
+    catch(ex) {
+      LOG("getFormattedString(): key " + key + " threw: " + ex);
+    }
+  },
+

Don't catch the exceptions here.

_getURIItemCopyTransaction()
_getBookmarkItemCopyTransaction()
_getFolderCopyTransaction()
_getLivemarkCopyTransaction()
makeTransaction()
setPostDataForBookmark()
getPostDataForBookmark()
getURLAndPostDataForKeyword()
getItemDescription()
maybeFewMorePleaseDoubleCheck()

Frontend.

+  markPageAsTyped: function PU_markPageAsTyped(aURL) {
+    this.globalHistory.markPageAsTyped(this.createFixedURI(aURL));

see my comment about the globalHistory getter.

+  checkURLSecurity: function PU_checkURLSecurity(aURINode) {
+    if (!this.nodeIsBookmark(aURINode)) {
+      var uri = this._uri(aURINode.uri);
+      if (uri.schemeIs("javascript") || uri.schemeIs("data")) {
+        const BRANDING_BUNDLE_URI = "chrome://branding/locale/brand.properties";
+        var brandShortName = Cc["@mozilla.org/intl/stringbundle;1"].
+                             getService(Ci.nsIStringBundleService).
+                             createBundle(BRANDING_BUNDLE_URI).
+                             GetStringFromName("brandShortName");
+        var promptService = Cc["@mozilla.org/embedcomp/prompt-service;1"].
+                            getService(Ci.nsIPromptService);
+
+        var errorStr = this.getString("load-js-data-url-error");

That belongs to the frontend file as well, please make sure the string is in the right file...


+  /**
+   * Get the description associated with a document, as specified in a <META>
+   * element.
+   * @param   doc
+   *          A DOM Document to get a description for
+   * @returns A description string if a META element was discovered with a
+   *          "description" or "httpequiv" attribute, empty string otherwise.
+   */
+  getDescriptionFromDocument: function PU_getDescriptionFromDocument(doc) {
  
Didn't you keep this in the frontend file?


+        // remove old root contents
+        var aggregate = this.ptm.aggregateTransactions("REMOVE " + root, removeOldTransactions);
+        this.ptm.doTransaction(aggregate);
+
+        // add new root contents
+        aggregate = this.ptm.aggregateTransactions("RESTORE " + root, transactions);

Er, you cannot use PTM in toolkit code, nor I think you should, this does not need to be undoable(!).


That said, we should make sure to batch when restoting from JSON.


+    // construct the new leafname
+    // Use YYYY-MM-DD (ISO 8601) as it doesn't contain illegal characters
+    // and makes the alphabetical order of multiple backup files more useful.
+    var date = (new Date).toLocaleFormat("%Y-%m-%d");

Normally we do new Date() IIRC.

+    var backupFilename = this.getFormattedString("bookmarksArchiveFilename", [date]);
+
+    var backupFile = null;
+    if (!aForceArchive) {
+      var backupFileNames = [];
+      var backupFilenamePrefix = backupFilename.substr(0, backupFilename.indexOf("-"));
+      var entries = bookmarksBackupDir.directoryEntries;
+      while (entries.hasMoreElements()) {
+        var entry = entries.getNext().QueryInterface(Ci.nsIFile);
+        var backupName = entry.leafName;
+        if (backupName.substr(0, backupFilenamePrefix.length) == backupFilenamePrefix) {
+          if (backupName == backupFilename)
+            backupFile = entry;
+          backupFileNames.push(backupName);
+        }
+      }

Please convert this to regexp usage if possible.


+PlacesUtils.placesFlavors = [PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER,
+                             PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR,
+                             PlacesUtils.TYPE_X_MOZ_PLACE];
+
+PlacesUtils.GENERIC_VIEW_DROP_TYPES = [PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER,
+                                       PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR,
+                                       PlacesUtils.TYPE_X_MOZ_PLACE,
+                                       PlacesUtils.TYPE_X_MOZ_URL,
+                                       PlacesUtils.TYPE_UNICODE];

Frontend

Index: toolkit/components/places/tests/bookmarks/test_384370.js

hrm, missing parts?
Comment 86 Shawn Wilsher :sdwilsh 2008-02-24 20:52:54 PST
I don't understand why you would want to make those constants more ambiguous.  Those expressions would only be evaluated once, at script load time, and only help to make the code more understandable.
Comment 87 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-02-25 05:02:56 PST
I agree, I don't understand the motivation at all.  If you are that concerned with parse time (where the constant folding happens), I recommend you profile and see if using shorter variable names wouldn't be a bigger win. :P
Comment 88 Damian Shaw [Quan] 2008-02-25 06:37:07 PST
Don't suppose this'll make beta 4? Going to need a fair bit of testing and people can't yet back-up tags and the like...
Comment 89 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-02-25 10:39:48 PST
I don't fine these more readable but whatever.
Comment 90 Dietrich Ayala (:dietrich) 2008-02-25 23:52:51 PST
Created attachment 305698 [details] [diff] [review]
fix
Comment 91 Dietrich Ayala (:dietrich) 2008-02-26 00:02:54 PST
> Index: browser/components/places/content/utils.js
> ===================================================================
> 
> +var PlacesUIUtils = {
> 
> Did you ensure each get*String directs to the right object?

i'll do another pass

> +#include ../../../../toolkit/content/debug.js
> 
> I think we should make debug.js a resource too and import it everywhere.

this is better than subscript loading, and doesn't require any runtime loading. why favor Cu.import here?

> 
> 
> Index: browser/components/places/tests/unit/head_bookmarks.js
> ===================================================================
> 
> +version(170);
> +
> 
> So, I still don't know what's this about

xpcshell is at js 1.6, so no |let| support.

> 
> +const LOAD_IN_SIDEBAR_ANNO = "bookmarkProperties/loadInSidebar";
> +const DESCRIPTION_ANNO = "bookmarkProperties/description";
> +const POST_DATA_ANNO = "bookmarkProperties/POSTData";
> 
> +const ORGANIZER_FOLDER_ANNO = "PlacesOrganizer/OrganizerFolder";
> +const ORGANIZER_QUERY_ANNO = "PlacesOrganizer/OrganizerQuery";
> +
> 
> Frontend stuff.

fixed, except for postdata, which keeping in toolkit utils means that the txn service only has to pull in the one file.

> 
> +#ifdef XP_MACOSX
> +// On Mac OSX, the transferable system converts "\r\n" to "\n\n", where we
> +// really just want "\n".
> +const NEWLINE= "\n";
> +#else
> +// On other platforms, the transferable system converts "\r\n" to "\n".
> +const NEWLINE = "\r\n";
> +#endif
> +
> 
> Probably that too, it's only used by d & d code as far as I remember.

used in un/wrapNodes()

> 
> +  checkURLSecurity: function PU_checkURLSecurity(aURINode) {
> +    if (!this.nodeIsBookmark(aURINode)) {
> +      var uri = this._uri(aURINode.uri);
> +      if (uri.schemeIs("javascript") || uri.schemeIs("data")) {
> +        const BRANDING_BUNDLE_URI =
> "chrome://branding/locale/brand.properties";
> +        var brandShortName = Cc["@mozilla.org/intl/stringbundle;1"].
> +                             getService(Ci.nsIStringBundleService).
> +                             createBundle(BRANDING_BUNDLE_URI).
> +                             GetStringFromName("brandShortName");
> +        var promptService = Cc["@mozilla.org/embedcomp/prompt-service;1"].
> +                            getService(Ci.nsIPromptService);
> +
> +        var errorStr = this.getString("load-js-data-url-error");
> 
> That belongs to the frontend file as well, please make sure the string is in
> the right file...
> 

called by markPageAs*()

> 
> Index: toolkit/components/places/tests/bookmarks/test_384370.js
> 
> hrm, missing parts?
> 

keeping the tests in /browser because they add microsummaries, etc.
Comment 92 Dietrich Ayala (:dietrich) 2008-02-26 00:16:05 PST
Created attachment 305701 [details] [diff] [review]
fix

fix other getString() etc
Comment 93 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-02-26 01:10:27 PST
> this is better than subscript loading, and doesn't require any runtime loading.
> why favor Cu.import here?

Because it duplicates code and breaks my js console. With your latest patch, I'm pretty sure NS_ASSERT is defined twice in few windows (globalOverlay #includes debug.js)
Comment 94 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-02-26 01:30:05 PST
Comment on attachment 305701 [details] [diff] [review]
fix

Index: browser/components/places/content/editBookmarkOverlay.js
===================================================================
@@ -319,17 +319,17 @@ var gEditItemOverlay = {
@@ -491,91 +491,91 @@ var gEditItemOverlay = {
Index: browser/components/places/content/bookmarkProperties.js
===================================================================
@@ -585,17 +585,17 @@ var BookmarkPropertiesPanel = {
Index: browser/components/places/content/treeView.js
===================================================================
@@ -234,17 +234,17 @@ PlacesTreeView.prototype = {

nit: alignment

Index: browser/components/places/content/places.js
===================================================================

+    if (aFile && aFile.leafName.match("\.json$")) {
+      // restore a JSON backup
+      PlacesUtils.restoreBookmarksFromJSONFile(aFile);
+    }
+    else {
+      var importer = Cc["@mozilla.org/browser/places/import-export-service;1"].
+                     getService(Ci.nsIPlacesImportExportService);
+      importer.importHTMLFromFile(aFile, true /* overwrite existing */);
+    }

Why null check aFile? This then goes to the html route in that case...

Index: browser/components/places/content/utils.js
===================================================================
+    try {
+      return this._bundle.formatStringFromName(key, params, params.length);
     }

hey!    


+  _getLivemarkCopyTransaction:
+  function PU__getLivemarkCopyTransaction(aData, aContainer, aIndex) {
+    NS_ASSERT(aData.livemark && aData.annos, "node is not a livemark");
+    // Place is a Livemark Container
+    var feedURI = null;
+    var siteURI = null;
+    aData.annos.forEach(function(aAnno) {
+      if (aAnno.name == LMANNO_FEEDURI)
+        feedURI = PlacesUtils._uri(aAnno.value);
+      if (aAnno.name == LMANNO_SITEURI)
+        siteURI = PlacesUtils._uri(aAnno.value);
+    }, this);
+    return this.ptm.createLivemark(feedURI, siteURI, aData.title, aContainer,
+                                   aIndex, aData.annos);

could you instead put feedURI and siteURI as properties for aData? this is kinda
ugly.

Index: browser/components/places/src/nsPlacesTransactionsService.js
===================================================================

-var loader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"].
-             getService(Components.interfaces.mozIJSSubScriptLoader);
-loader.loadSubScript("chrome://global/content/debug.js");
-loader.loadSubScript("chrome://browser/content/places/utils.js");
-
 Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
+Components.utils.import("resource://gre/modules/utils.js");
+
+#include ../../../../toolkit/content/debug.js

See previous comment..

Index: toolkit/components/places/src/nsTaggingService.js
===================================================================

Did you intend to include this change?
 
Index: toolkit/components/places/src/utils.js
===================================================================

+var EXPORTED_SYMBOLS = ["PlacesUtils"];
+
+var Ci = Components.interfaces;
+var Cc = Components.classes;
+var Cr = Components.results;
+
+#include ../../../content/debug.js
+

As I said, this brings the second copy of debug.js to browser.xul.


+   * Allows opening of javascript/data URI only if the given node is
+   * bookmarked (see bug 224521).
+   * @param aURINode
+   *        a URI node
+   * @return true if it's safe to open the node in the browser, false otherwise.
+   *
+   */
+  checkURLSecurity: function PU_checkURLSecurity(aURINode) {

Still, this doesn't belong to toolkit.

+      }
+    };
+    
+    try {
+      this.bookmarks.runInBatchMode(batch, null);
+    }
+    catch(ex) {
+      Components.utils.reportError(ex);
+    }

why do you catch it?

+  },
+
+  /**
+   * Takes a JSON-serialized node and inserts it into the db.
+   *
+   * @param   data
+   *          The unwrapped data blob of dropped or pasted data.
+   * @param   container
+   *          The container the data was dropped or pasted into
+   * @param   index
+   *          The index within the container the item was dropped or pasted at
+   */
+  importJSONNode: function PU_importJSONNode(aData, aContainer, aIndex) {
  
nit: fix argument names in the header.

+    LOG("importJSONNode(" + aData.title + ", " + aContainer + ", " + aIndex + ")");
+    // create item
+    var id = null;

Make it -1.

...
+          if (feedURI && siteURI)
+            id = this.livemarks.createLivemark(aContainer, aData.title, siteURI, feedURI, aIndex);

siteURI is optional(!).

+  /**
+   * Serializes the given node (and all it's descendents) as JSON
+   * and writes the serialization to the given output stream.
+   */ 


Is this spec'ed somewhere?
Comment 95 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-02-26 05:05:37 PST
(In reply to comment #91)
> > +#include ../../../../toolkit/content/debug.js
> > 
> > I think we should make debug.js a resource too and import it everywhere.
> 
> this is better than subscript loading, and doesn't require any runtime loading.
> why favor Cu.import here?

Cu.import will compile the code and define the functions once for all imports, rather than duplicating compilation time and resultant space for all #inclusions.

> > Index: browser/components/places/tests/unit/head_bookmarks.js
> > ===================================================================
> > 
> > +version(170);
> > +
> > 
> > So, I still don't know what's this about
> 
> xpcshell is at js 1.6, so no |let| support.

OMFG, gross.  Bug 419165 will save you here, hopefully within the hour.
Comment 96 Dietrich Ayala (:dietrich) 2008-02-26 13:40:24 PST
> > xpcshell is at js 1.6, so no |let| support.
> 
> OMFG, gross.  Bug 419165 will save you here, hopefully within the hour.
> 

wrong bug, but yeah that'd be great.

> +  _getLivemarkCopyTransaction:
> +  function PU__getLivemarkCopyTransaction(aData, aContainer, aIndex) {
> +    NS_ASSERT(aData.livemark && aData.annos, "node is not a livemark");
> +    // Place is a Livemark Container
> +    var feedURI = null;
> +    var siteURI = null;
> +    aData.annos.forEach(function(aAnno) {
> +      if (aAnno.name == LMANNO_FEEDURI)
> +        feedURI = PlacesUtils._uri(aAnno.value);
> +      if (aAnno.name == LMANNO_SITEURI)
> +        siteURI = PlacesUtils._uri(aAnno.value);
> +    }, this);
> +    return this.ptm.createLivemark(feedURI, siteURI, aData.title, aContainer,
> +                                   aIndex, aData.annos);
> 
> could you instead put feedURI and siteURI as properties for aData? this is
> kinda
> ugly.

the alternative is to put the ugly in the serializer, where we'd have to filter them out of the annos array.

> Index: toolkit/components/places/src/nsTaggingService.js
> ===================================================================
> 
> Did you intend to include this change?

yes. creating a new query there causes "endUpdateBatch w/o a begin!" errors, or something to that effect. i'll file a followup.

this uses the existing query node, so probably more efficient anyway.

> +  /**
> +   * Serializes the given node (and all it's descendents) as JSON
> +   * and writes the serialization to the given output stream.
> +   */ 
> 
> 
> Is this spec'ed somewhere?
> 

no. i'll put up a wiki for it.
Comment 97 Dietrich Ayala (:dietrich) 2008-02-26 13:41:06 PST
Created attachment 305835 [details] [diff] [review]
fix
Comment 98 Dietrich Ayala (:dietrich) 2008-02-26 14:59:58 PST
Created attachment 305854 [details] [diff] [review]
fix

with the fixes discussed on irc
Comment 99 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-02-26 15:39:24 PST
Comment on attachment 305854 [details] [diff] [review]
fix

You probably shouldn't alter the incoming annos array, it's a little bit weird to alter the input in that context, may you copy over it at the function top.

Please also:
 1. remove debug.js from toolkit/content/jar.mn
 2. Make globalOverlay.js import debug.js rather than include.
 3. Drop the strings removal for now.

r=mano otherwise .
Comment 100 Dietrich Ayala (:dietrich) 2008-02-26 16:37:53 PST
Created attachment 305877 [details] [diff] [review]
fix

implemented those changes, and fixed a couple more s/PlacesUtils/PlacesUIUtils/ instances.
Comment 101 Dietrich Ayala (:dietrich) 2008-02-26 17:42:08 PST
Created attachment 305893 [details] [diff] [review]
for checkin

de-rotted and updated to work with bug 419549, bug 403263 and friends.
Comment 102 Dietrich Ayala (:dietrich) 2008-02-26 18:22:41 PST
Created attachment 305903 [details] [diff] [review]
passes unit tests

2 line change to add keyword import to importJSONNode() in toolkit utils.
Comment 103 Dietrich Ayala (:dietrich) 2008-02-26 20:35:13 PST
Created attachment 305923 [details]
interdiff
Comment 104 Mike Connor [:mconnor] 2008-02-26 21:50:47 PST
Comment on attachment 305903 [details] [diff] [review]
passes unit tests

r=me based on the incredibly obvious interdiff

w00t!
Comment 105 Dietrich Ayala (:dietrich) 2008-02-26 22:46:43 PST
Created attachment 305946 [details] [diff] [review]
unrotted

unrotted, for checkin, carrying over r+.
Comment 106 Dietrich Ayala (:dietrich) 2008-02-27 10:14:51 PST
Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.290; previous revision: 1.289
done
Checking in browser/base/content/browser-menubar.inc;
/cvsroot/mozilla/browser/base/content/browser-menubar.inc,v  <--  browser-menubar.inc
new revision: 1.130; previous revision: 1.129
done
Checking in browser/base/content/browser-places.js;
/cvsroot/mozilla/browser/base/content/browser-places.js,v  <--  browser-places.js
new revision: 1.98; previous revision: 1.97
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.978; previous revision: 1.977
done
Checking in browser/base/content/nsContextMenu.js;
/cvsroot/mozilla/browser/base/content/nsContextMenu.js,v  <--  nsContextMenu.js
new revision: 1.38; previous revision: 1.37
done
Checking in browser/components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v  <--  nsBrowserGlue.js
new revision: 1.58; previous revision: 1.57
done
Checking in browser/components/places/content/bookmarkProperties.js;
/cvsroot/mozilla/browser/components/places/content/bookmarkProperties.js,v  <--  bookmarkProperties.js
new revision: 1.70; previous revision: 1.69
done
Checking in browser/components/places/content/bookmarksPanel.js;
/cvsroot/mozilla/browser/components/places/content/bookmarksPanel.js,v  <--  bookmarksPanel.js
new revision: 1.7; previous revision: 1.6
done
Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  controller.js
new revision: 1.209; previous revision: 1.208
done
Checking in browser/components/places/content/editBookmarkOverlay.js;
/cvsroot/mozilla/browser/components/places/content/editBookmarkOverlay.js,v  <--  editBookmarkOverlay.js
new revision: 1.22; previous revision: 1.21
done
Checking in browser/components/places/content/history-panel.js;
/cvsroot/mozilla/browser/components/places/content/history-panel.js,v  <--  history-panel.js
new revision: 1.26; previous revision: 1.25
done
Checking in browser/components/places/content/menu.xml;
/cvsroot/mozilla/browser/components/places/content/menu.xml,v  <--  menu.xml
new revision: 1.105; previous revision: 1.104
done
Checking in browser/components/places/content/moveBookmarks.js;
/cvsroot/mozilla/browser/components/places/content/moveBookmarks.js,v  <--  moveBookmarks.js
new revision: 1.15; previous revision: 1.14
done
Checking in browser/components/places/content/places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.132; previous revision: 1.131
done
Checking in browser/components/places/content/places.xul;
/cvsroot/mozilla/browser/components/places/content/places.xul,v  <--  places.xul
new revision: 1.109; previous revision: 1.108
done
Checking in browser/components/places/content/placesOverlay.xul;
/cvsroot/mozilla/browser/components/places/content/placesOverlay.xul,v  <--  placesOverlay.xul
new revision: 1.22; previous revision: 1.21
done
Checking in browser/components/places/content/sidebarUtils.js;
/cvsroot/mozilla/browser/components/places/content/sidebarUtils.js,v  <--  sidebarUtils.js
new revision: 1.5; previous revision: 1.4
done
Checking in browser/components/places/content/toolbar.xml;
/cvsroot/mozilla/browser/components/places/content/toolbar.xml,v  <--  toolbar.xml
new revision: 1.127; previous revision: 1.126
done
Checking in browser/components/places/content/tree.xml;
/cvsroot/mozilla/browser/components/places/content/tree.xml,v  <--  tree.xml
new revision: 1.98; previous revision: 1.97
done
Checking in browser/components/places/content/treeView.js;
/cvsroot/mozilla/browser/components/places/content/treeView.js,v  <--  treeView.js
new revision: 1.41; previous revision: 1.40
done
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.106; previous revision: 1.105
done
Checking in browser/components/places/src/Makefile.in;
/cvsroot/mozilla/browser/components/places/src/Makefile.in,v  <--  Makefile.in
new revision: 1.27; previous revision: 1.26
done
Checking in browser/components/places/src/nsPlacesImportExportService.cpp;
/cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp,v  <--  nsPlacesImportExportService.cpp
new revision: 1.45; previous revision: 1.44
done
Checking in browser/components/places/src/nsPlacesTransactionsService.js;
/cvsroot/mozilla/browser/components/places/src/nsPlacesTransactionsService.js,v  <--  nsPlacesTransactionsService.js
new revision: 1.19; previous revision: 1.18
done
Checking in browser/components/places/tests/unit/head_bookmarks.js;
/cvsroot/mozilla/browser/components/places/tests/unit/head_bookmarks.js,v  <--  head_bookmarks.js
new revision: 1.14; previous revision: 1.13
done
RCS file: /cvsroot/mozilla/browser/components/places/tests/unit/test_384370.js,v
done
Checking in browser/components/places/tests/unit/test_384370.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_384370.js,v  <--  test_384370.js
initial revision: 1.1
done
Checking in browser/components/places/tests/unit/test_398914.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_398914.js,v  <--  test_398914.js
new revision: 1.8; previous revision: 1.7
done
Checking in browser/components/places/tests/unit/test_bookmarks_html.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_bookmarks_html.js,v  <--  test_bookmarks_html.js
new revision: 1.20; previous revision: 1.19
done
Checking in browser/components/sidebar/src/nsSidebar.js;
/cvsroot/mozilla/browser/components/sidebar/src/nsSidebar.js,v  <--  nsSidebar.js
new revision: 1.31; previous revision: 1.30
done
Checking in toolkit/components/places/src/Makefile.in;
/cvsroot/mozilla/toolkit/components/places/src/Makefile.in,v  <--  Makefile.in
new revision: 1.30; previous revision: 1.29
done
Checking in toolkit/components/places/src/nsTaggingService.js;
/cvsroot/mozilla/toolkit/components/places/src/nsTaggingService.js,v  <--  nsTaggingService.js
new revision: 1.12; previous revision: 1.11
done
RCS file: /cvsroot/mozilla/toolkit/components/places/src/utils.js,v
done
Checking in toolkit/components/places/src/utils.js;
/cvsroot/mozilla/toolkit/components/places/src/utils.js,v  <--  utils.js
initial revision: 1.1
done
Checking in toolkit/content/Makefile.in;
/cvsroot/mozilla/toolkit/content/Makefile.in,v  <--  Makefile.in
new revision: 1.7; previous revision: 1.6
done
Checking in toolkit/content/debug.js;
/cvsroot/mozilla/toolkit/content/debug.js,v  <--  debug.js
new revision: 1.8; previous revision: 1.7
done
Checking in toolkit/content/globalOverlay.js;
/cvsroot/mozilla/toolkit/content/globalOverlay.js,v  <--  globalOverlay.js
new revision: 1.35; previous revision: 1.34
done
Checking in toolkit/content/jar.mn;
/cvsroot/mozilla/toolkit/content/jar.mn,v  <--  jar.mn
new revision: 1.35; previous revision: 1.34
done
Comment 107 Reed Loden [:reed] (use needinfo?) 2008-02-27 11:36:48 PST
Please get approval1.9b4+ on your next iteration, as per tree rules.
Comment 108 Dietrich Ayala (:dietrich) 2008-02-27 12:18:49 PST
Created attachment 306075 [details] [diff] [review]
fix - removes postplaces import support

change to address Ts: the previous patch supported detection of bookmarks.postplaces.html for import in the event of first-run or db recreation, but no json backup available.

however, now that we don't export bookmarks.postplaces.html anymore, and any of those beta users will after a single app restart have a json backup, there's no need to do ever import the old legacy files.
Comment 109 Justin Dolske [:Dolske] 2008-02-27 19:38:42 PST
Looks like /browser/components/places/tests/unit/test_384370.js was missed in the backout, I added an early |return| run run_tests() to disable it. Revert it when this relands.
Comment 110 Justin Dolske [:Dolske] 2008-02-27 19:55:46 PST
Hmm, sayrer noted that test_384370.js was only failing on Windows... It was still passing on OS X and Linux, even though the rest of this bug was backed out. Is it testing the right things?

On Windows it was failing with:

ReferenceError: PlacesUtils is not defined
*** FAIL ***
Exception: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.remove]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: ../../../../_tests/xpcshell-simple/test_browser_places/unit/head_bookmarks.js :: cleanUp :: line 130"  data: no]uncaught exception: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: ../../../../_tests/xpcshell-simple/test_browser_places/unit/test_384370.js :: <TOP_LEVEL> :: line 44"  data: no]
Comment 111 Mike Beltzner [:beltzner, not reading bugmail] 2008-02-28 05:40:41 PST
Dietrich: did you mean to request approval on attachment 306075 [details] [diff] [review]?
Comment 112 Dietrich Ayala (:dietrich) 2008-02-28 10:39:35 PST
Created attachment 306326 [details] [diff] [review]
fixed patch

patch with the fix described previously

beltzner: no, not ready for review.

this fix mitigates the Ts hit when importing, but doesn't fix the hit we're seeing in the test, since this import code should only be executed in the first of the 10 runs that Ts does (Ts takes the best score of 10).
Comment 113 Dietrich Ayala (:dietrich) 2008-02-28 11:08:31 PST
Created attachment 306332 [details] [diff] [review]
fixes the leak

fixes the leak by removing the idle observer at shutdown
Comment 114 Dietrich Ayala (:dietrich) 2008-02-28 13:56:53 PST
Created attachment 306376 [details] [diff] [review]
+ more Ts tweaks

the changes:

- remove idle observer to fix leak
- don't import bookmarks.postplaces.html (was transitional, now just use json)
- lazy import utils.js in places txn service
- lazy import debug.js in places txn service
- lazy get transaction manager in places txn service

lazily importing toolkit utils.js in the transaction service means that it's not interpreted until far later in startup. oddly, importing toolkit utils.js seems to cost more in the transaction service, than when included the browser utils.js file:

dtrace when utils.js is imported in old patch:
nsPlacesTransactions func  import    134855 (2nd longest exclusive elapsed time)

dtrace when utils.js is imported in new patch:
utils.js             func  import    46095   (17th longest)
Comment 115 Dietrich Ayala (:dietrich) 2008-02-28 13:57:27 PST
Created attachment 306377 [details] [diff] [review]
interdiff for previous patch
Comment 116 Justin Dolske [:Dolske] 2008-02-28 13:59:43 PST
plz to be including browser/components/places/tests/unit/test_384370.js!
Comment 117 Dietrich Ayala (:dietrich) 2008-02-28 14:04:21 PST
(In reply to comment #116)
> plz to be including browser/components/places/tests/unit/test_384370.js!
> 

this is a checkpoint really.

i need to investigate why that test seemed to be passing after the APIs it exercises were backed out (!). i won't check this in without resolving the test issue.
Comment 118 Dietrich Ayala (:dietrich) 2008-02-28 14:23:53 PST
Created attachment 306383 [details] [diff] [review]
sans tm getter
Comment 119 Dietrich Ayala (:dietrich) 2008-02-28 16:44:28 PST
Created attachment 306407 [details] [diff] [review]
 with tests again

yesterday i forgot to back out the newly added files, which were toolkit utils.js and tests. seeing as the tests are testing utils.js, makes sense that they'd keep passing after the backout :P

however, i don't know why the failure started after the PGO clobber. the error indicated that the test wasn't able to load toolkit utils.js any longer.

anyways, these tests are passing locally.
Comment 120 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-02-28 23:56:02 PST
Comment on attachment 306407 [details] [diff] [review]
 with tests again

r=mano
Comment 121 Mike Beltzner [:beltzner, not reading bugmail] 2008-02-28 23:59:28 PST
Comment on attachment 306407 [details] [diff] [review]
 with tests again

a1.9b4=beltzner
Comment 122 Dietrich Ayala (:dietrich) 2008-02-29 08:45:41 PST
tree was green when i was getting my checkin ready, red by the time i checked in, so this might get backed out again depending on the reason for red.

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.292; previous revision: 1.291
done
Checking in browser/base/content/browser-menubar.inc;
/cvsroot/mozilla/browser/base/content/browser-menubar.inc,v  <--  browser-menubar.inc
new revision: 1.132; previous revision: 1.131
done
Checking in browser/base/content/browser-places.js;
/cvsroot/mozilla/browser/base/content/browser-places.js,v  <--  browser-places.js
new revision: 1.101; previous revision: 1.100
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.982; previous revision: 1.981
done
Checking in browser/base/content/nsContextMenu.js;
/cvsroot/mozilla/browser/base/content/nsContextMenu.js,v  <--  nsContextMenu.js
new revision: 1.40; previous revision: 1.39
done
Checking in browser/components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v  <--  nsBrowserGlue.js
new revision: 1.61; previous revision: 1.60
done
Checking in browser/components/places/content/bookmarkProperties.js;
/cvsroot/mozilla/browser/components/places/content/bookmarkProperties.js,v  <--  bookmarkProperties.js
new revision: 1.72; previous revision: 1.71
done
Checking in browser/components/places/content/bookmarksPanel.js;
/cvsroot/mozilla/browser/components/places/content/bookmarksPanel.js,v  <--  bookmarksPanel.js
new revision: 1.9; previous revision: 1.8
done
Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  controller.js
new revision: 1.211; previous revision: 1.210
done
Checking in browser/components/places/content/editBookmarkOverlay.js;
/cvsroot/mozilla/browser/components/places/content/editBookmarkOverlay.js,v  <--  editBookmarkOverlay.js
new revision: 1.24; previous revision: 1.23
done
Checking in browser/components/places/content/history-panel.js;
/cvsroot/mozilla/browser/components/places/content/history-panel.js,v  <--  history-panel.js
new revision: 1.28; previous revision: 1.27
done
Checking in browser/components/places/content/menu.xml;
/cvsroot/mozilla/browser/components/places/content/menu.xml,v  <--  menu.xml
new revision: 1.107; previous revision: 1.106
done
Checking in browser/components/places/content/moveBookmarks.js;
/cvsroot/mozilla/browser/components/places/content/moveBookmarks.js,v  <--  moveBookmarks.js
new revision: 1.17; previous revision: 1.16
done
Checking in browser/components/places/content/places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.134; previous revision: 1.133
done
Checking in browser/components/places/content/places.xul;
/cvsroot/mozilla/browser/components/places/content/places.xul,v  <--  places.xul
new revision: 1.111; previous revision: 1.110
done
Checking in browser/components/places/content/placesOverlay.xul;
/cvsroot/mozilla/browser/components/places/content/placesOverlay.xul,v  <--  placesOverlay.xul
new revision: 1.24; previous revision: 1.23
done
Checking in browser/components/places/content/sidebarUtils.js;
/cvsroot/mozilla/browser/components/places/content/sidebarUtils.js,v  <--  sidebarUtils.js
new revision: 1.7; previous revision: 1.6
done
Checking in browser/components/places/content/toolbar.xml;
/cvsroot/mozilla/browser/components/places/content/toolbar.xml,v  <--  toolbar.xml
new revision: 1.129; previous revision: 1.128
done
Checking in browser/components/places/content/tree.xml;
/cvsroot/mozilla/browser/components/places/content/tree.xml,v  <--  tree.xml
new revision: 1.100; previous revision: 1.99
done
Checking in browser/components/places/content/treeView.js;
/cvsroot/mozilla/browser/components/places/content/treeView.js,v  <--  treeView.js
new revision: 1.43; previous revision: 1.42
done
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.108; previous revision: 1.107
done
Checking in browser/components/places/src/Makefile.in;
/cvsroot/mozilla/browser/components/places/src/Makefile.in,v  <--  Makefile.in
new revision: 1.29; previous revision: 1.28
done
Checking in browser/components/places/src/nsPlacesImportExportService.cpp;
/cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp,v  <--  nsPlacesImportExportService.cpp
new revision: 1.47; previous revision: 1.46
done
Checking in browser/components/places/src/nsPlacesTransactionsService.js;
/cvsroot/mozilla/browser/components/places/src/nsPlacesTransactionsService.js,v  <--  nsPlacesTransactionsService.js
new revision: 1.21; previous revision: 1.20
done
Checking in browser/components/places/tests/unit/head_bookmarks.js;
/cvsroot/mozilla/browser/components/places/tests/unit/head_bookmarks.js,v  <--  head_bookmarks.js
new revision: 1.16; previous revision: 1.15
done
Checking in browser/components/places/tests/unit/test_384370.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_384370.js,v  <--  test_384370.js
new revision: 1.4; previous revision: 1.3
done
Checking in browser/components/places/tests/unit/test_398914.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_398914.js,v  <--  test_398914.js
new revision: 1.10; previous revision: 1.9
done
Checking in browser/components/places/tests/unit/test_bookmarks_html.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_bookmarks_html.js,v  <--  test_bookmarks_html.js
new revision: 1.22; previous revision: 1.21
done
Checking in browser/components/sidebar/src/nsSidebar.js;
/cvsroot/mozilla/browser/components/sidebar/src/nsSidebar.js,v  <--  nsSidebar.js
new revision: 1.33; previous revision: 1.32
done
Checking in toolkit/components/places/src/Makefile.in;
/cvsroot/mozilla/toolkit/components/places/src/Makefile.in,v  <--  Makefile.in
new revision: 1.33; previous revision: 1.32
done
Checking in toolkit/components/places/src/nsTaggingService.js;
/cvsroot/mozilla/toolkit/components/places/src/nsTaggingService.js,v  <--  nsTaggingService.js
new revision: 1.14; previous revision: 1.13
done
Checking in toolkit/content/Makefile.in;
/cvsroot/mozilla/toolkit/content/Makefile.in,v  <--  Makefile.in
new revision: 1.9; previous revision: 1.8
done
Checking in toolkit/content/debug.js;
/cvsroot/mozilla/toolkit/content/debug.js,v  <--  debug.js
new revision: 1.10; previous revision: 1.9
done
Checking in toolkit/content/globalOverlay.js;
/cvsroot/mozilla/toolkit/content/globalOverlay.js,v  <--  globalOverlay.js
new revision: 1.37; previous revision: 1.36
done
Checking in toolkit/content/jar.mn;
/cvsroot/mozilla/toolkit/content/jar.mn,v  <--  jar.mn
new revision: 1.37; previous revision: 1.36
done
Comment 123 Dietrich Ayala (:dietrich) 2008-02-29 09:33:00 PST
backed out again. still a 20ms Ts regression on bl-bldlnx03. Also looks like a clear Txul regression there as well.
Comment 124 Dietrich Ayala (:dietrich) 2008-02-29 11:15:40 PST
Comment on attachment 306407 [details] [diff] [review]
 with tests again


>+        var dirService = Cc["@mozilla.org/file/directory_service;1"].
>+                         getService(Ci.nsIProperties);
>+        var bookmarksFile = dirService.get("BMarks", Ci.nsILocalFile);
>+
>+        // import the file
>+        try {
>+          var importer = Cc["@mozilla.org/browser/places/import-export-service;1"].
>+                         getService(Ci.nsIPlacesImportExportService);
>+          importer.importHTMLFromFile(bookmarksFile, true /* overwrite existing */);
>+        } finally {
>+          prefBranch.setBoolPref("browser.places.importBookmarksHTML", false);
>         }

this should only execute for the first Ts run.

>       }
>     }
>+
>+    // Initialize bookmark archiving on idle.
>+    // Once a day, either on idle or shutdown, bookmarks are backed up.
>+    this.idleService.addIdleObserver(this, BOOKMARKS_ARCHIVE_IDLE_TIME);

penalty: this is getting run every startup


> var Cr = Components.results;
> 
>-Components.utils.import("resource://gre/modules/JSON.jsm");
>+Components.utils.import("resource://gre/modules/utils.js");
>+Components.utils.import("resource://gre/modules/debug.js");
> 

could hurt. utils.js gets pulled in at window load. previously it was all one file, but now the pulled-in file does this as well.

> 
>-var loader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"].
>-             getService(Components.interfaces.mozIJSSubScriptLoader);
>-loader.loadSubScript("chrome://global/content/debug.js");
>-loader.loadSubScript("chrome://browser/content/places/utils.js");
>-
> Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> 
> // The minimum amount of transactions we should tell our observers to begin
> // batching (rather than letting them do incremental drawing).
> const MIN_TRANSACTIONS_FOR_BATCH = 5;  
> 
> function placesTransactionsService() {
>   this.mTransactionManager = Cc["@mozilla.org/transactionmanager;1"].
>                              createInstance(Ci.nsITransactionManager);
>+  Components.utils.import("resource://gre/modules/utils.js");
> }
> 

this should improve Ts, since:

1. loading a smaller utils.js.

2. loading utils.js only when the svc is started, instead of when it's interpreted. this service is not used until after startup.

> 
> function placesMoveItemTransactions(aItemId, aNewContainer, aNewIndex) {
>+  Components.utils.import("resource://gre/modules/debug.js");
>   NS_ASSERT(aNewIndex >= -1, "invalid insertion index");
>   this._id = aItemId;

ditto, certainly not loading this at startup anymore.

>Index: toolkit/content/globalOverlay.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/content/globalOverlay.js,v
>retrieving revision 1.36
>diff -u -8 -p -r1.36 globalOverlay.js
>--- toolkit/content/globalOverlay.js	27 Feb 2008 19:05:55 -0000	1.36
>+++ toolkit/content/globalOverlay.js	29 Feb 2008 00:28:35 -0000
>@@ -191,9 +191,9 @@ function FillInTooltip ( tipElement )
>       var node = document.createTextNode(tipText);
>       textNode.appendChild(node);
>       retVal = true;
>     }
>   }
>   return retVal;
> }
> 
>-#include debug.js
>+Components.utils.import("resource://gre/modules/debug.js");

certainly a potential pain point, will happen every time the app loads.
Comment 125 Dietrich Ayala (:dietrich) 2008-03-04 10:18:08 PST
Created attachment 307273 [details] [diff] [review]
include toolkit js files inline

this is to measure the Ts hit caused by the increased runtime file i/o when using Components.utils(). i'll back out after a bl-bldlnx03 run completes.
Comment 126 Dietrich Ayala (:dietrich) 2008-03-04 14:15:14 PST
changing to inline include of toolkit utils.js and debug.js resulted in a Ts regression of 3-4ms. removing the Cu.import() of debug.js in globalOverlay.js might mitigate that regression. new patch coming up.
Comment 127 Dietrich Ayala (:dietrich) 2008-03-04 14:31:43 PST
Created attachment 307315 [details] [diff] [review]
fix: use inline includes in favor of Cu.import() where possible

i still need to fixup one of the tests, but this is ready for review of this approach.

yeah, this sucks. it duplicates code in the packaged app and in memory. but it doesn't regress Ts.
Comment 128 Robert Sayre 2008-03-04 14:58:08 PST
Comment on attachment 307315 [details] [diff] [review]
fix: use inline includes in favor of Cu.import() where possible

>
>+#include ../../../../toolkit/content/debug.js
>+#include ../../../../toolkit/components/places/src/utils.js

This looks like a case of eagerly importing things. This file is all functions, so the Cu.import() call these includes replace need not be eager, interupting IO from XUL.mfasl.

>
> function placesTransactionsService() {
>   this.mTransactionManager = Cc["@mozilla.org/transactionmanager;1"].
>                              createInstance(Ci.nsITransactionManager);
>+  Components.utils.import("resource://gre/modules/utils.js");
> }
> 

This doesn't make much sense. Each call to placesTransactionsService() will assign the symbols in utils.js to the script's global object.

A better strategy would be something like this:

var _utils = {};
function getUtils() {
  if (!_utils.foo) {
    Cu.import("...utils.js", _utils);
  }
  return _utils;
}
Comment 129 Dan Mills [:thunder] 2008-03-04 15:40:52 PST
Has json export been tested with cjkt charsets in bookmark titles?  In the weave add-on I'm running into bug #420411 which I'd guess would show up here too.
Comment 130 Dietrich Ayala (:dietrich) 2008-03-04 17:30:34 PST
(In reply to comment #128)
> (From update of attachment 307315 [details] [diff] [review])
> >
> >+#include ../../../../toolkit/content/debug.js
> >+#include ../../../../toolkit/components/places/src/utils.js
> 
> This looks like a case of eagerly importing things. This file is all functions,
> so the Cu.import() call these includes replace need not be eager, interupting
> IO from XUL.mfasl.

i don't understand what you mean. if by "eager" you mean that we should include them until necessary: these functions need to be available in window scope during startup. we can't push them off until afterwards, without also pushing back loading of the toolbar, for example.

> 
> >
> > function placesTransactionsService() {
> >   this.mTransactionManager = Cc["@mozilla.org/transactionmanager;1"].
> >                              createInstance(Ci.nsITransactionManager);
> >+  Components.utils.import("resource://gre/modules/utils.js");
> > }
> > 
> 
> This doesn't make much sense. Each call to placesTransactionsService() will
> assign the symbols in utils.js to the script's global object.
> 

it's a service, this code should only ever be executed once.
Comment 131 Robert Sayre 2008-03-05 15:38:06 PST
 
> i don't understand what you mean. if by "eager" you mean that we should include
> them until necessary: these functions need to be available in window scope
> during startup. we can't push them off until afterwards, without also pushing
> back loading of the toolbar, for example.

By "eager", I meant you want to put off calling Cu.import until one of the functions is actually called. Also, if debug.js is causing a runtime hit of any kind, in should not be included in release builds. I know JS lacks a macro system to make this easy. :/

> > 
> > >
> > > function placesTransactionsService() {
> > >   this.mTransactionManager = Cc["@mozilla.org/transactionmanager;1"].
> > >                              createInstance(Ci.nsITransactionManager);
> > >+  Components.utils.import("resource://gre/modules/utils.js");
> > > }
> > > 
> > 
> > This doesn't make much sense. Each call to placesTransactionsService() will
> > assign the symbols in utils.js to the script's global object.
> > 
> 
> it's a service, this code should only ever be executed once.
> 

Still bad style points. Suggest fixing.
Comment 132 Dietrich Ayala (:dietrich) 2008-03-05 18:10:41 PST
Created attachment 307628 [details] [diff] [review]
fix: inline v2
Comment 133 Dietrich Ayala (:dietrich) 2008-03-05 18:20:45 PST
(In reply to comment #131)
> By "eager", I meant you want to put off calling Cu.import until one of the
> functions is actually called. Also, if debug.js is causing a runtime hit of any
> kind, in should not be included in release builds. I know JS lacks a macro
> system to make this easy. :/

those functions are still going to be called during startup, so putting off calling Cu.import until those funcs are called still involves calling Cu.import during startup, which is slower than #include'ing the file. i must be misunderstanding what you're recommending...

about debug.js: it's a single function, and #include'ing it doesn't seem to cause a hit to Ts, only Cu.import()ing it.

> > it's a service, this code should only ever be executed once.
> > 
> 
> Still bad style points. Suggest fixing.

fixed in latest patch
Comment 134 Robert Sayre 2008-03-06 10:16:05 PST
(In reply to comment #133)
>  i must be misunderstanding what you're recommending...

The thinking is that calling Cu.import (and into XPC.mfasl) at that point is a bad idea. Hopefully, delaying it until it is needed by a function call would get rid of the Ts regression altogether. 3-4ms is still 1-2%, right?
Comment 135 Dietrich Ayala (:dietrich) 2008-03-06 10:33:45 PST
(In reply to comment #134)
> (In reply to comment #133)
> >  i must be misunderstanding what you're recommending...
> 
> The thinking is that calling Cu.import (and into XPC.mfasl) at that point is a
> bad idea. Hopefully, delaying it until it is needed by a function call would
> get rid of the Ts regression altogether. 3-4ms is still 1-2%, right?
> 

Hm, I don't understand the internals of Cu.import (and don't even know what XPC.mfasl is). Are you saying that maybe this delay would put the call *after* the Ts measurement ends? If so, this seems like more gaming of Ts, since the hit still occurs, just after we stop measuring :/

The only solvent approach so far has been to not use Cu.import() at all, thereby removing the additional runtime loading of files from disk. The makeup of browser.js is testament to this approach. Sure it kinda sucks, but it doesn't suck so bad that it should delay the release, nor the landing of this patch (in my biased opinion ;) ).

About the remaining 3-4ms: I took out my change to load debug.js via Cu.import in the global overlay (ie: keep the status quo), which may fix this.
Comment 136 Robert Sayre 2008-03-06 13:29:01 PST
(In reply to comment #135)
> 
> Hm, I don't understand the internals of Cu.import (and don't even know what
> XPC.mfasl is). Are you saying that maybe this delay would put the call *after*
> the Ts measurement ends?

No, that's not what I'm saying. We have two fastload files, XUL.mfasl and XPC.mfasl. I think we're having problems here due to interspersing loads from each. The best way to speed startup time is to not do things that don't need to be done at startup. It's not "gaming" Ts. I don't know how much can be put off, in this case.

> 
> The only solvent approach so far has been to not use Cu.import() at all,

Isn't that the only approach you've tried?

> thereby removing the additional runtime loading of files from disk.

You're still loading from disk no matter what you do.

 The makeup
> of browser.js is testament to this approach. Sure it kinda sucks, but it
> doesn't suck so bad that it should delay the release, nor the landing of this
> patch (in my biased opinion ;) ).

When we find an area that affects a metric, we should be able to make it faster. I don't have an opinion about landing with this particular regression.
Comment 137 Dietrich Ayala (:dietrich) 2008-03-06 20:23:14 PST
(In reply to comment #136)
> No, that's not what I'm saying. We have two fastload files, XUL.mfasl and
> XPC.mfasl. I think we're having problems here due to interspersing loads from
> each. The best way to speed startup time is to not do things that don't need to
> be done at startup. It's not "gaming" Ts. I don't know how much can be put off,
> in this case.

ok, thanks for clarifying. is there a way to determine whether that is in fact the problem, maybe from the shark profiles i sent?

> > The only solvent approach so far has been to not use Cu.import() at all,
> 
> Isn't that the only approach you've tried?

yes. i'll try detecting the first consumer, and loading there (fragile, but fine for testing).

is there way to memo-ize objects in the global scope? eg: we currently have many consumers of PlacesUtils.*. if i could replace that object on first access, it'd be easier and safer than changing all the callers.

> > thereby removing the additional runtime loading of files from disk.
> 
> You're still loading from disk no matter what you do.

note the word "additional" above.

>  The makeup
> > of browser.js is testament to this approach. Sure it kinda sucks, but it
> > doesn't suck so bad that it should delay the release, nor the landing of this
> > patch (in my biased opinion ;) ).
> 
> When we find an area that affects a metric, we should be able to make it
> faster. I don't have an opinion about landing with this particular regression.

of course we should. my point is that doing that doesn't need to block this bug.

Comment 138 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-07 05:08:25 PST
This patch shows exactly why is #include a bad practice - you're loading NS_ASSERT twice in browser.js (once via utils.js, once via globalOverlay.js)
Comment 139 Ondrej Brablc 2008-03-07 06:42:54 PST
I suggest that the s/PlacesUtils/PlacesUIUtils/ change is done in separate bug which would be checked in very quickly (or postponed). We have many pending patches (some waiting for landing - can someone help) and this will break them all.
Comment 140 Dietrich Ayala (:dietrich) 2008-03-07 07:11:56 PST
(In reply to comment #138)
> This patch shows exactly why is #include a bad practice - you're loading
> NS_ASSERT twice in browser.js (once via utils.js, once via globalOverlay.js)
> 

1. easily fixed
2. agreed! as stated many times above: i don't think it's ideal, only that it *works* w/o regressing perf. i'm happy to file a followup bug to figure out how to get rid of #include.

(In reply to comment #139)
> I suggest that the s/PlacesUtils/PlacesUIUtils/ change is done in separate bug
> which would be checked in very quickly (or postponed). We have many pending
> patches (some waiting for landing - can someone help) and this will break them
> all.
> 

one option would be to just keep it a single object, in /toolkit. it would then only be included once in chrome via <script> (as in the status quo), with no #includes required. we'd be pulling useless code into nsBrowserGlue, but that's trivial compared to regressing Ts and is well worth moving forward.
Comment 141 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-03-07 07:51:22 PST
(In reply to comment #137)
> is there way to memo-ize objects in the global scope? eg: we currently have
> many consumers of PlacesUtils.*. if i could replace that object on first
> access, it'd be easier and safer than changing all the callers.

I can't get global getters to work in content (haven't tried in chrome), but if you have a Places.Utils.* then you can indeed do lazy memoization.  It might work better in the global case for chrome, or components, but possibly not.

<script>
function realMyThingGetter()
{
  return this.i++;
}

function setUpMyThing()
{
  document.write("first call, setting up real getter<br>\n");
  delete this.thing; // not necessary in the simple case, better for generality
  this.__defineGetter__("thing", realMyThingGetter);
  return this.thing;
}

var My = { i: 0 };
My.__defineGetter__("thing", setUpMyThing);

var v = My.thing;
document.write("My.thing: ", v, "<br>");
v = My.thing;
document.write("My.thing: ", v, "<br>");
</script>
Comment 142 Adam Kowalczyk 2008-03-07 08:49:58 PST
(In reply to comment #141)
> (In reply to comment #137)
> > is there way to memo-ize objects in the global scope? eg: we currently have
> > many consumers of PlacesUtils.*. if i could replace that object on first
> > access, it'd be easier and safer than changing all the callers.
> 
> I can't get global getters to work in content (haven't tried in chrome)

It was bug 420585 which has just been fixed, have you tried with the today's nightly? I use global getters without a problem.
Comment 143 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-03-07 09:33:01 PST
Boo-yah, indeed.  I leave updating my example as an exercise to the reader.
Comment 144 Dietrich Ayala (:dietrich) 2008-03-07 15:47:04 PST
Created attachment 308062 [details] [diff] [review]
with global scope getter instead of #include

thanks for the tips. shaver, in your example, the "real" function was executed every time - was never memoized.

i ended up with:

__defineGetter__("PlacesUtils", function() {
  if (!this._tmp) {
    var tmpScope = {};
    Components.utils.import("resource://gre/modules/utils.js", tmpScope);
    this._tmp = tmpScope.PlacesUtils;
  }
  return this._tmp;
});

which should provide the lazy-loading functionality we need to test sayrer's fastload idea.
Comment 145 Dietrich Ayala (:dietrich) 2008-03-07 15:48:26 PST
btw, this doesn't have debug.js in globalOverlay.js converted to Cu.import. i'll provide a followup patch for that if this doesn't regress Ts.
Comment 146 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-07 22:58:30 PST
Dietrich, can you rather use a smart getter here, i.e. convert "PlacesUtils" to a simple variable after the first access to this getter?
Comment 147 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-03-08 04:12:28 PST
(In reply to comment #144)
> Created an attachment (id=308062) [details]
> with global scope getter instead of #include
> 
> thanks for the tips. shaver, in your example, the "real" function was executed
> every time - was never memoized.

Well sure, because I used __defineGetter__ again; if you don't need any dynamic behaviour after the initial setup:

<script>
function setUpMyThing()
{
  document.write("first call, setting up real getter<br>\n");
  delete this.thing; // not necessary in the simple case, better for generality
  this.thing = "5";
  return this.thing;
}

var My = { };
My.__defineGetter__("thing", setUpMyThing);

var v = My.thing;
document.write("My.thing: ", v, "<br>");
v = My.thing;
document.write("My.thing: ", v, "<br>");
</script>
Comment 148 Dietrich Ayala (:dietrich) 2008-03-08 08:05:00 PST
(In reply to comment #146)
> Dietrich, can you rather use a smart getter here, i.e. convert "PlacesUtils" to
> a simple variable after the first access to this getter?

i didn't do this before because doing so inside the getter results in recursion badness. i tried redefining the getter from inside the getter, but JS gave an error something like "innappropriate use of getters". can you suggest an alternate approach?

(In reply to comment #147)
> 
> Well sure, because I used __defineGetter__ again; if you don't need any dynamic
> behaviour after the initial setup:

apologies, i didn't make that clear.

Comment 149 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-03-08 08:24:06 PST
Delete the property before redefining the getter?  With a code-snippet I could give better advice, perhaps, but my example in comment 147 makes My.thing a simple variable, modified here to use a global property:

<script>
function setUpMyThing()
{
  document.write("first call, setting up real property<br>\n");
  delete this.thing;
  this.thing = "5";
  return this.thing;
}

this.__defineGetter__("thing", setUpMyThing);

var v = thing;
document.write("thing: ", v, "<br>");
v = thing;
document.write("thing: ", v, "<br>");
</script>
Comment 150 Dietrich Ayala (:dietrich) 2008-03-08 09:57:16 PST
Created attachment 308177 [details] [diff] [review]
w/ smart getter

grr, i thought this caused the recursion problem earlier. yep this works, thanks shaver sir.
Comment 151 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-08 10:26:59 PST
Comment on attachment 308177 [details] [diff] [review]
w/ smart getter


> function placesMoveItemTransactions(aItemId, aNewContainer, aNewIndex) {
>+  Components.utils.import("resource://gre/modules/debug.js");
>   NS_ASSERT(aNewIndex >= -1, "invalid insertion index");
>   this._id = aItemId;
>   this._oldContainer = PlacesUtils.bookmarks.getFolderIdForItem(this._id);
>   this._oldIndex = PlacesUtils.bookmarks.getItemIndex(this._id);
>   NS_ASSERT(this._oldContainer > 0 && this._oldIndex >= 0, "invalid item");

Just remove these asserts.

r=mano otherwise.
Comment 152 Dietrich Ayala (:dietrich) 2008-03-10 11:10:57 PDT
checked for a test run:

Ts before: 846/843

Ts after: 828/824

This is significantly faster than using #include, which seemed keep Ts on par (not surprising since that's basically not changing from status quo, other than the addition of a few methods), and clearly and suspiciously faster than without the patch.

Over IRC, Rob said that it may be a combination of:

1. not interrupting XUL.mfasl
2. loading utils.js after Ts stops measuring

The downside of this patch is that all the debug+leak boxes started failing the alive test:

*** loading utils.js
  ###!!! ASSERTION: redundant multiplexed document?: 'docMapEntry->mString == nsnull', file /builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-53.1.13.el5_Depend/mozilla/xpcom/io/nsFastLoadFile.cpp, line 1404

log: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1205168640.1205169774.13100.gz&fulltext=1
Comment 153 Brendan Eich [:brendan] 2008-03-10 11:34:27 PDT
Look for an open bug on that assertion, or file a new one -- it should be a warning since AFAIK the code after it copes. It says that someone is not playing by the FastLoad API rules, which is a small ineffeciency, maybe.

/be
Comment 154 Dietrich Ayala (:dietrich) 2008-03-10 12:49:11 PDT
(In reply to comment #153)
> Look for an open bug on that assertion, or file a new one -- it should be a
> warning since AFAIK the code after it copes. It says that someone is not
> playing by the FastLoad API rules, which is a small ineffeciency, maybe.
> 
> /be
> 

Filed bug 421943.
Comment 155 Dietrich Ayala (:dietrich) 2008-03-10 13:38:08 PDT
a few things:

1. utils.js is loaded before first page load (Ts test end). twice actually, due to bug 421939.

2. The lazy getter function seems to be executed when assigned as the getter, before the getter itself is accessed. in this dtrace log, the import calls are inside our smart getter function:

  0 6879160887       utils.js               -> __defineGetter__
  0 6879161088       utils.js               <- __defineGetter__
  0 6879161653       utils.js               -> LOG
  0 6879161706       utils.js                 -> dump
  0 6879161778       utils.js                 <- dump
  0 6879161806       utils.js               <- LOG
  0 6879161892       utils.js               -> import
  1 6879183853       utils.js                 -> LOG
  1 6879183913       utils.js                   -> dump
  1 6879184052       utils.js                   <- dump
  1 6879184097       utils.js                 <- LOG
  1 6879184740       utils.js                 -> import
  1 6879187758       utils.js                 <- import
  1 6879188612       utils.js               <- import
Comment 156 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-03-10 15:00:11 PDT
It looks to me like __defineGetter_ as already returned when the imports happen, no?  So it's not the getter assignment that does it, unless I'm misreading that snippet.
Comment 157 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-03-10 15:36:40 PDT
Dietrich: if you put a "debugger;" in the smart getter and run in a debug build, XPConnect should print the stack for you.  That'd tell you where it's getting triggered from, I suspect -- __defineGetter__ doesn't call its argument, as far as I can tell from testing and reading source.
Comment 158 Dietrich Ayala (:dietrich) 2008-03-10 17:03:17 PDT
(In reply to comment #157)
> Dietrich: if you put a "debugger;" in the smart getter and run in a debug
> build, XPConnect should print the stack for you.  That'd tell you where it's
> getting triggered from, I suspect -- __defineGetter__ doesn't call its
> argument, as far as I can tell from testing and reading source.
> 

yep, that's correct, i was reading the output wrong.
Comment 159 Dietrich Ayala (:dietrich) 2008-03-12 14:16:16 PDT
Created attachment 308957 [details] [diff] [review]
w/ more smart getter

changes in this patch:

- unrotted some /browser bits
- re-added debug.js module conversion
- use smart getter for other Cu.import calls
- use nsIConverterOutputStream to ensure proper charset conversion (bug 415250)
Comment 160 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-13 05:53:53 PDT
Index: toolkit/content/globalOverlay.js
===================================================================

-#include debug.js
+Components.utils.import("resource://gre/modules/debug.js");

That makes the utils.js's NS_ASEERT smart getter fix nothing in the context of a browser window (I don't know if it did before).
Comment 161 Dietrich Ayala (:dietrich) 2008-03-13 10:32:03 PDT
(In reply to comment #160)
> Index: toolkit/content/globalOverlay.js
> ===================================================================
> 
> -#include debug.js
> +Components.utils.import("resource://gre/modules/debug.js");
> 
> That makes the utils.js's NS_ASEERT smart getter fix nothing in the context of
> a browser window (I don't know if it did before).
> 

i'll make that a smart getter as well. it's possible that toolkit utils.js can be loaded prior to this, via nsBrowserGlue on first run or migration.
Comment 162 Dietrich Ayala (:dietrich) 2008-03-13 11:23:35 PDT
Created attachment 309167 [details] [diff] [review]
comment fixed

fixed comment #160
Comment 163 Dietrich Ayala (:dietrich) 2008-03-13 12:26:31 PDT
Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.309; previous revision: 1.308
done
Checking in browser/base/content/browser-menubar.inc;
/cvsroot/mozilla/browser/base/content/browser-menubar.inc,v  <--  browser-menubar.inc
new revision: 1.151; previous revision: 1.150
done
Checking in browser/base/content/browser-places.js;
/cvsroot/mozilla/browser/base/content/browser-places.js,v  <--  browser-places.js
new revision: 1.116; previous revision: 1.115
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.1005; previous revision: 1.1004
done
Checking in browser/base/content/nsContextMenu.js;
/cvsroot/mozilla/browser/base/content/nsContextMenu.js,v  <--  nsContextMenu.js
new revision: 1.52; previous revision: 1.51
done
Checking in browser/components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v  <--  nsBrowserGlue.js
new revision: 1.75; previous revision: 1.74
done
Checking in browser/components/places/content/bookmarkProperties.js;
/cvsroot/mozilla/browser/components/places/content/bookmarkProperties.js,v  <--  bookmarkProperties.js
new revision: 1.82; previous revision: 1.81
done
Checking in browser/components/places/content/bookmarksPanel.js;
/cvsroot/mozilla/browser/components/places/content/bookmarksPanel.js,v  <--  bookmarksPanel.js
new revision: 1.20; previous revision: 1.19
done
Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  controller.js
new revision: 1.221; previous revision: 1.220
done
Checking in browser/components/places/content/editBookmarkOverlay.js;
/cvsroot/mozilla/browser/components/places/content/editBookmarkOverlay.js,v  <--  editBookmarkOverlay.js
new revision: 1.35; previous revision: 1.34
done
Checking in browser/components/places/content/history-panel.js;
/cvsroot/mozilla/browser/components/places/content/history-panel.js,v  <--  history-panel.js
new revision: 1.39; previous revision: 1.38
done
Checking in browser/components/places/content/menu.xml;
/cvsroot/mozilla/browser/components/places/content/menu.xml,v  <--  menu.xml
new revision: 1.121; previous revision: 1.120
done
Checking in browser/components/places/content/moveBookmarks.js;
/cvsroot/mozilla/browser/components/places/content/moveBookmarks.js,v  <--  moveBookmarks.js
new revision: 1.27; previous revision: 1.26
done
Checking in browser/components/places/content/places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.147; previous revision: 1.146
done
Checking in browser/components/places/content/places.xul;
/cvsroot/mozilla/browser/components/places/content/places.xul,v  <--  places.xul
new revision: 1.123; previous revision: 1.122
done
Checking in browser/components/places/content/placesOverlay.xul;
/cvsroot/mozilla/browser/components/places/content/placesOverlay.xul,v  <--  placesOverlay.xul
new revision: 1.34; previous revision: 1.33
done
Checking in browser/components/places/content/sidebarUtils.js;
/cvsroot/mozilla/browser/components/places/content/sidebarUtils.js,v  <--  sidebarUtils.js
new revision: 1.17; previous revision: 1.16
done
Checking in browser/components/places/content/toolbar.xml;
/cvsroot/mozilla/browser/components/places/content/toolbar.xml,v  <--  toolbar.xml
new revision: 1.142; previous revision: 1.141
done
Checking in browser/components/places/content/tree.xml;
/cvsroot/mozilla/browser/components/places/content/tree.xml,v  <--  tree.xml
new revision: 1.110; previous revision: 1.109
done
Checking in browser/components/places/content/treeView.js;
/cvsroot/mozilla/browser/components/places/content/treeView.js,v  <--  treeView.js
new revision: 1.57; previous revision: 1.56
done
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.122; previous revision: 1.121
done
Checking in browser/components/places/src/nsPlacesImportExportService.cpp;
/cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp,v  <--  nsPlacesImportExportService.cpp
new revision: 1.60; previous revision: 1.59
done
Checking in browser/components/places/src/nsPlacesTransactionsService.js;
/cvsroot/mozilla/browser/components/places/src/nsPlacesTransactionsService.js,v  <--  nsPlacesTransactionsService.js
new revision: 1.32; previous revision: 1.31
done
Checking in browser/components/places/tests/unit/head_bookmarks.js;
/cvsroot/mozilla/browser/components/places/tests/unit/head_bookmarks.js,v  <--  head_bookmarks.js
new revision: 1.26; previous revision: 1.25
done
Checking in browser/components/places/tests/unit/test_384370.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_384370.js,v  <--  test_384370.js
new revision: 1.14; previous revision: 1.13
done
Checking in browser/components/places/tests/unit/test_398914.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_398914.js,v  <--  test_398914.js
new revision: 1.20; previous revision: 1.19
done
Checking in browser/components/places/tests/unit/test_bookmarks_html.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_bookmarks_html.js,v  <--  test_bookmarks_html.js
new revision: 1.32; previous revision: 1.31
done
Checking in browser/components/sidebar/src/nsSidebar.js;
/cvsroot/mozilla/browser/components/sidebar/src/nsSidebar.js,v  <--  nsSidebar.js
new revision: 1.43; previous revision: 1.42
done
Checking in toolkit/components/places/src/Makefile.in;
/cvsroot/mozilla/toolkit/components/places/src/Makefile.in,v  <--  Makefile.in
new revision: 1.43; previous revision: 1.42
done
Checking in toolkit/components/places/src/nsTaggingService.js;
/cvsroot/mozilla/toolkit/components/places/src/nsTaggingService.js,v  <--  nsTaggingService.js
new revision: 1.24; previous revision: 1.23
done
Checking in toolkit/components/places/src/utils.js;
/cvsroot/mozilla/toolkit/components/places/src/utils.js,v  <--  utils.js
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/content/Makefile.in;
/cvsroot/mozilla/toolkit/content/Makefile.in,v  <--  Makefile.in
new revision: 1.13; previous revision: 1.12
done
Checking in toolkit/content/debug.js;
/cvsroot/mozilla/toolkit/content/debug.js,v  <--  debug.js
new revision: 1.14; previous revision: 1.13
done
Checking in toolkit/content/globalOverlay.js;
/cvsroot/mozilla/toolkit/content/globalOverlay.js,v  <--  globalOverlay.js
new revision: 1.41; previous revision: 1.40
done
Checking in toolkit/content/jar.mn;
/cvsroot/mozilla/toolkit/content/jar.mn,v  <--  jar.mn
new revision: 1.41; previous revision: 1.40
done
Comment 164 Dietrich Ayala (:dietrich) 2008-03-13 13:25:00 PDT
bl-bldlnx03 (old Ts): 765ms -> 779ms (increase 14ms)
qm-plinux-fast01 (new Ts): 1357ms -> 1350ms (decrease 7ms)

hm. i could revert the debug.js changes, back to #include, which is how it was when we got the 20ms Ts win on bl-bldlnx03.
Comment 165 Dietrich Ayala (:dietrich) 2008-03-13 14:21:57 PDT
Created attachment 309226 [details] [diff] [review]
revert debug.js changes

potential Ts fix.
Comment 166 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-13 14:50:41 PDT
Comment on attachment 309226 [details] [diff] [review]
revert debug.js changes

r=mano to get Ts fixed.

Though, I would rather just remove all these asserts from the nodeIs* functions at this point.
Comment 167 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-13 14:51:52 PDT
This does load NS_ASSERT into browser.xul twice though, doesn't it?
Comment 168 Dietrich Ayala (:dietrich) 2008-03-13 14:57:24 PDT
(In reply to comment #167)
> This does load NS_ASSERT into browser.xul twice though, doesn't it?
> 

only loaded via globalOverlay.js, afaict.
Comment 169 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-13 15:08:15 PDT
toolkit's utils.js is loaded into browser.xul via browser's utils.js.
Comment 170 Dietrich Ayala (:dietrich) 2008-03-13 15:16:24 PDT
Created attachment 309248 [details] [diff] [review]
followup toolbar fix

irc-r=mano

Checking in browser/components/places/content/toolbar.xml;
/cvsroot/mozilla/browser/components/places/content/toolbar.xml,v  <--  toolbar.xml
new revision: 1.143; previous revision: 1.142
done
Comment 171 Dietrich Ayala (:dietrich) 2008-03-13 15:37:14 PDT
Created attachment 309255 [details] [diff] [review]
remove NS_ASSERT from toolkit utils.js
Comment 172 Dietrich Ayala (:dietrich) 2008-03-13 17:25:27 PDT
Created attachment 309285 [details] [diff] [review]
remove NS_ASSERT from toolkit utils.js (v2)

remove debug.js from toolkit utils.js, leave Cu.import in globalOverlay.js
Comment 173 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-13 17:56:31 PDT
Comment on attachment 309285 [details] [diff] [review]
remove NS_ASSERT from toolkit utils.js (v2)

r=mano assuming you've tested there's no other NS_ASSERT usage during startup.
Comment 174 Dietrich Ayala (:dietrich) 2008-03-13 18:21:33 PDT
Created attachment 309306 [details] [diff] [review]
remove more NS_ASSERTs during startup
Comment 175 Dietrich Ayala (:dietrich) 2008-03-13 18:49:26 PDT
Created attachment 309312 [details] [diff] [review]
per irc
Comment 176 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-13 18:54:16 PDT
Comment on attachment 309312 [details] [diff] [review]
per irc

r=mano
Comment 177 Dietrich Ayala (:dietrich) 2008-03-13 18:58:38 PDT
Checking in browser/components/places/content/toolbar.xml;
/cvsroot/mozilla/browser/components/places/content/toolbar.xml,v  <--  toolbar.xml
new revision: 1.144; previous revision: 1.143
done
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.123; previous revision: 1.122
done
Checking in toolkit/components/places/src/utils.js;
/cvsroot/mozilla/toolkit/components/places/src/utils.js,v  <--  utils.js
new revision: 1.3; previous revision: 1.2
done
Comment 178 pal-moz 2008-03-14 00:05:38 PDT
after 20080313_1302_firefox-3.0b5pre.en-US.win32.zip

[Bonsai Query]
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1205432520&maxdate=1205438519


"Bookmarks">bookmark sub folder,
middle click on it (to open bookmarks in sub folder in tabs) does not work.

intended or regression ?
Comment 179 pal-moz 2008-03-14 00:08:56 PDT
(In reply to comment #178)
> after 20080313_1302_firefox-3.0b5pre.en-US.win32.zip
> 
> [Bonsai Query]
> http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1205432520&maxdate=1205438519
> 
> 
> "Bookmarks">bookmark sub folder,
> middle click on it (to open bookmarks in sub folder in tabs) does not work.
> 
> intended or regression ?
> 

error in Console2,

Error: PlacesUtils.openContainerNodeInTabs is not a function
Source file: chrome://browser/content/browser.js
Line: 6273
Comment 180 PikeUK 2008-03-14 05:44:32 PDT
Filed two bugs caused by this:

Bug 422917 – "Save as type" blank in Backup & Restore dialogs in Library 
Bug 422919 – Bookmark Import and Restore do not intelligently handle two different bookmark formats

I wont attempt to fiddle about with the blocking/depends fields, if someone else could do that (assuming it applies here) I would be grateful.
Comment 181 Dietrich Ayala (:dietrich) 2008-03-14 09:10:01 PDT
Created attachment 309440 [details] [diff] [review]
more s/Utils/UIUtils/ fixes

fixes comment 178/179, as well as an occurrence from bug 394252.

Checking in browser/base/content/browser-places.js;
/cvsroot/mozilla/browser/base/content/browser-places.js,v  <--  browser-places.js
new revision: 1.118; previous revision: 1.117
done
Checking in browser/components/places/content/editBookmarkOverlay.js;
/cvsroot/mozilla/browser/components/places/content/editBookmarkOverlay.js,v  <--  editBookmarkOverlay.js
new revision: 1.37; previous revision: 1.36
done
Comment 182 Timo Tolonen 2008-03-14 09:19:17 PDT
There's one more at http://mxr.mozilla.org/mozilla/source/browser/components/places/content/places.js#254

Middle-clicking a bookmark folder in "Library" fails with "Error: PlacesUtils.openContainerNodeInTabs is not a function
Source File: chrome://browser/content/places/places.js
Line: 235"

I filed bug 422877 for these but I guess it was unnecessary.
Comment 183 Dietrich Ayala (:dietrich) 2008-03-14 09:31:11 PDT
The followup patch for Ts last night did not resolve the ~1% Ts hit. If we want to pursue that ~1%, lets do it in another bug. Rolling back to #include for debug.js is what gave us the 20ms Ts win, so that's an avenue to explore.

The bugs filed against this change so far are not critical enough to roll this back, so I'm closing this bug. Please file new bugs for any followup issues.
Comment 184 Valentin Laube 2008-03-14 16:01:44 PDT
another one: http://mxr.mozilla.org/firefox/source/browser/components/places/src/nsPlacesTransactionsService.js#921

Happens when you try to cancel after adding a tag to a bookmark.
Here is the error from the console:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 

(NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.removeItem]"  nsresult: "0x80070057 

(NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: 

file:///D:/Programme/Internet/firefox/components/nsPlacesTransactionsService.js :: 

PTU_undoTransaction :: line 921"  data: no]
Comment 185 Ryan VanderMeulen [:RyanVM] 2008-03-14 16:04:15 PDT
Valentin, thanks for your bug hunting. However, instead of continuing to post in an already-resolved bug, please file new bugs for the problems you're finding and mark them as blocking this bug. That makes it easier for developers to deal with individual issues and to keep the various bugs from getting too cluttered.
Comment 186 Valentin Laube 2008-03-14 16:21:35 PDT
Sorry about that, I didn't notice that the bug was already closed
Filed Bug 423040 – Cancel after adding a tag does not work
Comment 187 Laurens Holst 2008-03-16 07:13:38 PDT
Just a question - why did you choose JSON for the format and not XML?

XML has a DOM with useful tree traversal methods such as nextSibling and parentNode. Additionally, when one would want to read this file outside the browser (e.g. in some kind of bookmarks sharing or system administration project), XML parsers are readily available in pretty much all programming languages, whereas JSON would likely need the user to either create a custom parser or try to find one.
Comment 188 Damian Shaw [Quan] 2008-03-16 08:38:19 PDT
(In reply to comment #187)
> Just a question - why did you choose JSON for the format and not XML?
> 
> XML has a DOM with useful tree traversal methods such as nextSibling and
> parentNode. Additionally, when one would want to read this file outside the
> browser (e.g. in some kind of bookmarks sharing or system administration
> project), XML parsers are readily available in pretty much all programming
> languages, whereas JSON would likely need the user to either create a custom
> parser or try to find one.
> 

I asked that question on IRC the other night and got told that Firefox's JSON parser is much much faster that its XML one.
Comment 189 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-03-16 09:57:07 PDT
Also see comment 59 and further.
Comment 190 timeless 2008-03-16 14:15:31 PDT
*** Bug 406208 has been marked as a duplicate of this bug. ***
Comment 191 Marcia Knous [:marcia - use ni] 2008-04-04 17:14:48 PDT
https://litmus.mozilla.org/show_test.cgi?id=5234 has been added to Litmus, as well a few other test cases related to Backup.
Comment 192 neil@parkwaycc.co.uk 2009-01-27 06:54:33 PST
Comment on attachment 309167 [details] [diff] [review]
comment fixed

>+EXTRA_JS_MODULES = debug.js
>+EXTRA_PP_JS_MODULES = debug.js
It's lucky that EXTRA_PP_JS_MODULES takes effect after EXTRA_JS_MODULES does...
Comment 193 David Dahl :ddahl 2009-08-10 14:36:29 PDT
TEST-UNEXPECTED-FAIL | /builds/moz2_slave/mozilla-central-linux-unittest-everythingelse/build/xpcshell/tests/test_browser_places/unit/test_384370.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/moz2_slave/mozilla-central-linux-unittest-everythingelse/build/xpcshell/tests/test_browser_places/unit/test_384370.js | 5 == 4 - See following stack:
Unknown Error: command finished with exit code: 1

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1249937644.1249939751.5169.gz

went orange today :(
Comment 194 Dietrich Ayala (:dietrich) 2009-08-10 14:42:26 PDT
that orange is bug 506042.
Comment 195 Gervase Markham [:gerv] 2009-11-26 06:33:24 PST
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Note You need to log in before you can comment on or make changes to this bug.