Closed Bug 384370 Opened 17 years ago Closed 16 years ago

use JSON as the on disk, lossless format for our bookmark backup

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3

People

(Reporter: moco, Assigned: dietrich)

References

(Depends on 1 open bug)

Details

(Keywords: highrisk)

Attachments

(5 files, 53 obsolete files)

2.74 KB, patch
beltzner
: review+
beltzner
: approval1.9+
Details | Diff | Splinter Review
213.76 KB, patch
asaf
: review+
Details | Diff | Splinter Review
967 bytes, patch
Details | Diff | Splinter Review
12.97 KB, patch
asaf
: review+
Details | Diff | Splinter Review
2.62 KB, patch
Details | Diff | Splinter Review
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.
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.
Blocks: 374529
The advantage of bookmarks.html above bookmarks.json is that a html file is more easily readable, I think.
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.
thunder, for your synch demo, how did you turn the bookmark data into a json
object?
Flags: blocking-firefox3?
OS: Mac OS X → All
Hardware: PC → All
Needs more discussion, but I think Mano's suggestion makes sense in general.
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3]
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) 
Assignee: nobody → christineyen+bugs
Target Milestone: --- → Firefox 3 M8
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)
asking for reconsideration for blocking fx 3.
Flags: blocking-firefox3- → blocking-firefox3?
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).

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
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
Attached patch wip patch (obsolete) — Splinter Review
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...
(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?
(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.
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?
(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.
(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.
(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.
Blocks: 382379
> 
> 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*.
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
Attachment #274866 - Attachment is obsolete: true
Looks like that patch partially conflicts with the one in bug 386789.
(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.
Depends on: 386789
No longer blocks: 374529
Blocks: 374528
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
Assignee: christineyen+bugs → thunder
Flags: blocking-firefox3? → blocking-firefox3+
> 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?
Target Milestone: Firefox 3 M8 → Firefox 3 M9
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.
Assignee: thunder → dietrich
Blocks: 396806
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.
> 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.
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
For comparison, I ran the same test using toSource(), and it was only 30% slower than the C++/HTML code.
Depends on: 340987
Whiteboard: [wanted-firefox3]
Attached patch wip (obsolete) — Splinter Review
- 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.
Attachment #275460 - Attachment is obsolete: true
Attached patch more (obsolete) — Splinter Review
- hooks up automatic backups and archiving at shutdown
- more serialization efficiencies
Attachment #282276 - Attachment is obsolete: true
Attached patch more (obsolete) — Splinter Review
got restore working (mostly)
Attachment #283214 - Attachment is obsolete: true
Attached patch more (obsolete) — Splinter Review
- fixed restore order
- fixed serialization of saved searches
- re-added html export at shutdown (but need to disable auto-archiving)
Attachment #283372 - Attachment is obsolete: true
Attached patch more (obsolete) — Splinter Review
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.
Attachment #283427 - Attachment is obsolete: true
Attached patch more (obsolete) — Splinter Review
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.
Attachment #283643 - Attachment is obsolete: true
Attached patch more (obsolete) — Splinter Review
Attachment #283677 - Attachment is obsolete: true
Attached patch more (obsolete) — Splinter Review
- adds support for date-added and last-modified
- adds tests, now passes same tests that HTML import/export does
Attachment #283858 - Attachment is obsolete: true
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
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).
Status: NEW → ASSIGNED
Depends on: 398898
Attached patch fix v1 (obsolete) — Splinter Review
Attachment #284025 - Attachment is obsolete: true
Comment on attachment 284522 [details] [diff] [review]
fix v1

nope.
Attachment #284522 - Attachment is obsolete: true
Attached patch fix v1 (obsolete) — Splinter Review
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 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.
Attachment #284550 - Flags: review?(sspitzer)
QA should add a litmus case or two for this once it is checked in since it is a big change.
Flags: in-litmus?
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.
Keywords: uiwanted
doesn't look like we'll have native JSON for M9, pushing this out.
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Priority: -- → P1
Attachment #284550 - Flags: review?(sspitzer)
Removing ui-wanted; please re-add, but I can't find anything here that needs our attention.
Keywords: uiwanted
(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. 
Keywords: uiwanted
(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...
Blocks: 374521
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.
Keywords: highrisk
Whiteboard: [waiting on native JSON]
(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!)
(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
Keywords: uiwanted
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.
Attached patch partially unrotted (obsolete) — Splinter Review
Attachment #284550 - Attachment is obsolete: true
Attached patch mostly de-rotted (obsolete) — Splinter Review
Attachment #290707 - Attachment is obsolete: true
Attached patch making PlacesUtils more modular (obsolete) — Splinter Review
Attachment #290727 - Attachment is obsolete: true
Blocks: 394205
Depends on: 406371
No longer blocks: 394205
Depends on: 394205
Depends on: 406478
Depends on: 387522
No longer depends on: 340987
Priority: P1 → P2
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?
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Blocks: 407774
Priority: P2 → P1
(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. 
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?
(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.
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.
(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)

Attached patch more (obsolete) — Splinter Review
Attachment #290813 - Attachment is obsolete: true
Depends on: 382711
Whiteboard: [waiting on native JSON]
Blocks: 390750
No longer blocks: 374521
No longer depends on: 406371
Attached patch more (obsolete) — Splinter Review
- unrotted
- use native json
Attachment #294006 - Attachment is obsolete: true
Depends on: 412531
Attached patch testing (obsolete) — Splinter Review
Blocks: 413944
Attached patch more (obsolete) — Splinter Review
Attachment #296372 - Attachment is obsolete: true
Attachment #297423 - Attachment is obsolete: true
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
Depends on: 414802
Attached patch more (obsolete) — Splinter Review
Attachment #299691 - Attachment is obsolete: true
er, how did you lose 106KB between the last two patches?
Dietrich is just a smart guy, reed.
Attached patch more (obsolete) — Splinter Review
Attachment #301756 - Attachment is obsolete: true
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?)
> >     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.
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #301831 - Attachment is obsolete: true
Attachment #302160 - Flags: review?(mano)
Whiteboard: [has patch][needs review mano]
Keywords: late-l10n
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.
Attachment #302160 - Flags: review?(mano) → review-
Whiteboard: [has patch][needs review mano] → [needs new patch]
Blocks: 405938
Blocks: 406114
Attached patch strings (obsolete) — Splinter Review
strings only, for l10n freeze.
Attachment #304914 - Flags: review?
Attachment #304914 - Flags: review?(beltzner)
Attachment #304914 - Flags: review?
Attachment #304914 - Flags: approval1.9?
Attached patch strings (entities rev'd) (obsolete) — Splinter Review
Attachment #304914 - Attachment is obsolete: true
Attachment #304915 - Flags: review?(beltzner)
Attachment #304915 - Flags: approval1.9?
Attachment #304914 - Flags: review?(beltzner)
Attachment #304914 - Flags: approval1.9?
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...
Attachment #304915 - Flags: review?(beltzner)
Attachment #304915 - Flags: review-
Attachment #304915 - Flags: approval1.9?
Attached patch strings, take 3Splinter Review
Attachment #304915 - Attachment is obsolete: true
Attachment #304916 - Flags: review?(beltzner)
Attachment #304916 - Flags: approval1.9?
Comment on attachment 304916 [details] [diff] [review]
strings, take 3

r+uir+a=beltzner
Attachment #304916 - Flags: review?(beltzner)
Attachment #304916 - Flags: review+
Attachment #304916 - Flags: approval1.9?
Attachment #304916 - Flags: approval1.9+
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
Attached patch most comments fixed (obsolete) — Splinter Review
Attachment #302160 - Attachment is obsolete: true
Attached patch more (obsolete) — Splinter Review
more
Attachment #304919 - Attachment is obsolete: true
Attached patch fix (obsolete) — Splinter Review
Attachment #305293 - Attachment is obsolete: true
Attachment #305424 - Flags: review?(mano)
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?
Attachment #305424 - Flags: review?(mano) → review-
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.
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
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...
I don't fine these more readable but whatever.
Attached patch fix (obsolete) — Splinter Review
Attachment #305424 - Attachment is obsolete: true
Attachment #305698 - Flags: review?(mano)
> 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.
Attached patch fix (obsolete) — Splinter Review
fix other getString() etc
Attachment #305698 - Attachment is obsolete: true
Attachment #305701 - Flags: review?(mano)
Attachment #305698 - Flags: review?(mano)
> 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 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?
Attachment #305701 - Flags: review?(mano) → review-
(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.
> > 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.
Attached patch fix (obsolete) — Splinter Review
Attachment #305701 - Attachment is obsolete: true
Attachment #305835 - Flags: review?(mano)
Attached patch fix (obsolete) — Splinter Review
with the fixes discussed on irc
Attachment #305835 - Attachment is obsolete: true
Attachment #305854 - Flags: review?(mano)
Attachment #305835 - Flags: review?(mano)
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 .
Attachment #305854 - Flags: review?(mano) → review+
Attached patch fix (obsolete) — Splinter Review
implemented those changes, and fixed a couple more s/PlacesUtils/PlacesUIUtils/ instances.
Attachment #305854 - Attachment is obsolete: true
Attachment #305877 - Flags: review+
Attached patch for checkin (obsolete) — Splinter Review
de-rotted and updated to work with bug 419549, bug 403263 and friends.
Attachment #305877 - Attachment is obsolete: true
Attached patch passes unit tests (obsolete) — Splinter Review
2 line change to add keyword import to importJSONNode() in toolkit utils.
Attachment #305893 - Attachment is obsolete: true
Attachment #305903 - Flags: review?(mano)
Attached file interdiff (obsolete) —
Comment on attachment 305903 [details] [diff] [review]
passes unit tests

r=me based on the incredibly obvious interdiff

w00t!
Attachment #305903 - Flags: review?(mano) → review+
Attachment #305903 - Flags: review+
Whiteboard: [needs new patch]
Attached patch unrotted (obsolete) — Splinter Review
unrotted, for checkin, carrying over r+.
Attachment #305903 - Attachment is obsolete: true
Attachment #305946 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Depends on: 419895
Please get approval1.9b4+ on your next iteration, as per tree rules.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Target Milestone: Firefox 3 beta4 → Firefox 3
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.
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]
Dietrich: did you mean to request approval on attachment 306075 [details] [diff] [review]?
Attached patch fixed patch (obsolete) — Splinter Review
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).
Attachment #305946 - Attachment is obsolete: true
Attachment #306075 - Attachment is obsolete: true
Attached patch fixes the leak (obsolete) — Splinter Review
fixes the leak by removing the idle observer at shutdown
Attachment #306326 - Attachment is obsolete: true
Keywords: late-l10n
Attached patch + more Ts tweaks (obsolete) — Splinter Review
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)
Attachment #306332 - Attachment is obsolete: true
Attachment #306376 - Flags: review?(mano)
Attached patch interdiff for previous patch (obsolete) — Splinter Review
plz to be including browser/components/places/tests/unit/test_384370.js!
(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.
Attachment #306377 - Attachment is patch: true
Attachment #306377 - Attachment mime type: application/octet-stream → text/plain
Attached patch sans tm getter (obsolete) — Splinter Review
Attachment #306376 - Attachment is obsolete: true
Attachment #306376 - Flags: review?(mano)
Attached patch with tests again (obsolete) — Splinter Review
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.
Attachment #306383 - Attachment is obsolete: true
Attachment #306407 - Flags: review?(mano)
Blocks: 406208
Comment on attachment 306407 [details] [diff] [review]
 with tests again

r=mano
Attachment #306407 - Flags: review?(mano)
Attachment #306407 - Flags: review+
Attachment #306407 - Flags: approval1.9b4?
Comment on attachment 306407 [details] [diff] [review]
 with tests again

a1.9b4=beltzner
Attachment #306407 - Flags: approval1.9b4? → approval1.9b4+
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
backed out again. still a 20ms Ts regression on bl-bldlnx03. Also looks like a clear Txul regression there as well.
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.
Blocks: 415389
Attached patch include toolkit js files inline (obsolete) — Splinter Review
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.
Attachment #307273 - Attachment is patch: true
Attachment #307273 - Attachment mime type: application/octet-stream → text/plain
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.
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.
Attachment #305923 - Attachment is obsolete: true
Attachment #306377 - Attachment is obsolete: true
Attachment #306407 - Attachment is obsolete: true
Attachment #307273 - Attachment is obsolete: true
Attachment #307315 - Flags: review?(mano)
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;
}
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.
(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.
Whiteboard: [swag: 1d][needs review mano]
 
> 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.
Attached patch fix: inline v2 (obsolete) — Splinter Review
Attachment #307315 - Attachment is obsolete: true
Attachment #307628 - Flags: review?(mano)
Attachment #307315 - Flags: review?(mano)
(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
(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?
(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.
(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.
(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.

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)
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.
(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.
(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>
(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.
Blocks: 418592
Boo-yah, indeed.  I leave updating my example as an exercise to the reader.
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.
Attachment #308062 - Flags: review?(sayrer)
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.
Dietrich, can you rather use a smart getter here, i.e. convert "PlacesUtils" to a simple variable after the first access to this getter?
(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>
(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.

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>
Attached patch w/ smart getter (obsolete) — Splinter Review
grr, i thought this caused the recursion problem earlier. yep this works, thanks shaver sir.
Attachment #307628 - Attachment is obsolete: true
Attachment #308062 - Attachment is obsolete: true
Attachment #308177 - Flags: review?(mano)
Attachment #307628 - Flags: review?(mano)
Attachment #308062 - Flags: review?(sayrer)
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.
Attachment #308177 - Flags: review?(mano) → review+
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
Whiteboard: [swag: 1d][needs review mano] → [swag: 1d]
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
(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.
Depends on: 421943
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
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.
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.
(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.
Blocks: 421981
No longer blocks: 421981
Blocks: 389987
Attached patch w/ more smart getter (obsolete) — Splinter Review
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)
Attachment #308177 - Attachment is obsolete: true
Attachment #308957 - Flags: review?(mano)
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).
(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.
Attached patch comment fixedSplinter Review
fixed comment #160
Attachment #308957 - Attachment is obsolete: true
Attachment #309167 - Flags: review?(mano)
Attachment #308957 - Flags: review?(mano)
Attachment #309167 - Flags: review?(mano) → review+
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
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.
Blocks: 417228
Attached patch revert debug.js changes (obsolete) — Splinter Review
potential Ts fix.
Attachment #309226 - Flags: review?(mano)
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.
Attachment #309226 - Flags: review?(mano) → review+
This does load NS_ASSERT into browser.xul twice though, doesn't it?
(In reply to comment #167)
> This does load NS_ASSERT into browser.xul twice though, doesn't it?
> 

only loaded via globalOverlay.js, afaict.
toolkit's utils.js is loaded into browser.xul via browser's utils.js.
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
Attachment #309226 - Attachment is obsolete: true
Attachment #309255 - Flags: review?(mano)
remove debug.js from toolkit utils.js, leave Cu.import in globalOverlay.js
Attachment #309255 - Attachment is obsolete: true
Attachment #309285 - Flags: review?(mano)
Attachment #309255 - Flags: review?(mano)
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.
Attachment #309285 - Flags: review?(mano) → review+
Attachment #309285 - Attachment is obsolete: true
Attachment #309306 - Flags: review?(mano)
Attachment #309306 - Attachment is patch: true
Attachment #309306 - Attachment mime type: application/x-javascript → text/plain
Attached patch per ircSplinter Review
Attachment #309306 - Attachment is obsolete: true
Attachment #309306 - Flags: review?(mano)
Attachment #309312 - Flags: review?(mano)
Comment on attachment 309312 [details] [diff] [review]
per irc

r=mano
Attachment #309312 - Flags: review?(mano) → review+
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
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 ?
(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
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.
Depends on: 422917
Depends on: 422919
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
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.
Depends on: 422877
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.
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
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]
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.
Depends on: 423040
Sorry about that, I didn't notice that the bug was already closed
Filed Bug 423040 – Cancel after adding a tag does not work
Depends on: 423080
Depends on: 423081
Depends on: 423082
Depends on: 423083
Depends on: 423085
Depends on: 423126
Depends on: 423154
Depends on: 423187
No longer depends on: 423083
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.
(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.
Also see comment 59 and further.
No longer depends on: 423187
Depends on: 423187
Depends on: 423470
Status: RESOLVED → VERIFIED
Depends on: 426104
https://litmus.mozilla.org/show_test.cgi?id=5234 has been added to Litmus, as well a few other test cases related to Backup.
Flags: in-litmus? → in-litmus+
Depends on: 448804
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...
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 :(
Whiteboard: [swag: 1d] → [swag: 1d] [orange]
Depends on: 508767
that orange is bug 506042.
Whiteboard: [swag: 1d] [orange]
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.