Closed Bug 482911 Opened 15 years ago Closed 12 years ago

[HTML5] Re-implement bookmarks.html parsing using the HTML5 parser

Categories

(Toolkit :: Places, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: hsivonen, Assigned: hsivonen)

References

(Blocks 1 open bug, )

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(3 files, 9 obsolete files)

145.91 KB, patch
mak
: review+
Details | Diff | Splinter Review
2.29 KB, patch
mak
: review+
Details | Diff | Splinter Review
1.05 KB, patch
mak
: review+
Details | Diff | Splinter Review
Legacy Netscape bookmarks.html format parsing uses the old HTML tokenizer with a special content sink. Need to re-implement bookmarks.html handling by using parsing into a DOM using the HTML5 parser and then walking the DOM.
Priority: -- → P3
Assignee: hsivonen → nobody
Blocks: 650775
Depends on: 651072
mak, the plan is still to do this in JS rather than C++, right?
Component: HTML: Parser → Places
Product: Core → Toolkit
QA Contact: parser → places
Version: Other Branch → Trunk
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
(In reply to Henri Sivonen (:hsivonen) from comment #2)
> mak, the plan is still to do this in JS rather than C++, right?

right, and use async APIs wherever possible.
that in case of bookmarks means nowhere unfortunately, but it should be done so that in future, when we have an async bookmarking api we can replace it in place without having to rewrite everything (call that fake-async, thus handle sync apis as if they were async)
(In reply to Dietrich Ayala (:dietrich) from comment #5)
> depends on bug 519514, per comment #4.

My implementation is a line-by-line port of the old C++ using the existing APIs. Let's leave migration to a new API to a follow-up, since that's not on the critical path for getting rid of the old HTML parser and it seems the new APIs aren't ready.

Adding the addon-compat keyword, because the importing part of nsIPlacesImportExportService is going to go away. I'm not planning to rename the interface so that JS-based extensions that only use the exporting part don't break.
No longer depends on: placesAsyncBookmarks
Keywords: addon-compat
(In reply to Henri Sivonen (:hsivonen) from comment #6)
> My implementation is a line-by-line port of the old C++ using the existing
> APIs. Let's leave migration to a new API to a follow-up, since that's not on
> the critical path for getting rid of the old HTML parser and it seems the
> new APIs aren't ready.

I agree, if we keep the old api for the export part.

> Adding the addon-compat keyword, because the importing part of
> nsIPlacesImportExportService is going to go away.

What does this mean? Why can't we import?
note that removing import is not just breaking add-ons, it's breaking us. default bookmarks are imported from an html file.
This patch replaces the C++-based bookmark imported with a JS-based line-by-line port that exposes an asynchronous API to the outside (but internally using the same APIs for interacting with the database as the old impl did).

I'm well aware that the JS code is needlessly complex because it examines one DOM node at a time instead of exploring the DOM tree more freely now that it would be possible look ahead it the tree. I wanted to keep the structure of the old code so that I don't regress anything in the conversion process.

What's missing is the ability to synchronously import the default bookmarks during clobbering IE or Safari profile migration from within C++. I noticed that Fennec keeps the default bookmarks in a JSON file instead of using a Netscape bookmark file. Do we to keep using the Netscape bookmark format for the default bookmarks on desktop?
Attachment #585699 - Attachment description: WIP; lacks a solution for importing the default bookmark during IE/Safari import → WIP; lacks a solution for importing the default bookmarks during IE/Safari import
(In reply to Henri Sivonen (:hsivonen) from comment #9)
> Do we to keep using the Netscape bookmark format for
> the default bookmarks on desktop?

Yes, we have a bookmarks.html file for default bookmarks on desktop, and we support importing an html file, since other browsers export their bookmarks in that format.
actually, the fact we don't import bookmarks.html from the migrators is something we may fix elsewhere, Mano was hitting the same problem with his Safari migrator and he could probably look into that.
Actually browserglue may take care of that (And it already does in the Chrome migrator case, though that was unexpected).
(In reply to Marco Bonardo [:mak] from comment #7)
> (In reply to Henri Sivonen (:hsivonen) from comment #6)
> > My implementation is a line-by-line port of the old C++ using the existing
> > APIs. Let's leave migration to a new API to a follow-up, since that's not on
> > the critical path for getting rid of the old HTML parser and it seems the
> > new APIs aren't ready.
> 
> I agree, if we keep the old api for the export part.

The old export API is staying.

> > Adding the addon-compat keyword, because the importing part of
> > nsIPlacesImportExportService is going to go away.
> 
> What does this mean? Why can't we import?

Since our own usage of the importer was through JS anyway (except for importing the default bookmark when migrating from IE or Safari in the clobbering mode), I put the new importer in a JSM instead of an XPCOM component.

(In reply to Marco Bonardo [:mak] from comment #8)
> note that removing import is not just breaking add-ons, it's breaking us.
> default bookmarks are imported from an html file.

Fennec keeps the default bookmarks as a JSON file. Is it important to keep the default bookmarks on desktops as an HTML file? If it is important to keep using HTML, would it be OK to change the IE and Safari importers not to expect the default bookmark import to happen synchronously?

(In reply to Marco Bonardo [:mak] from comment #10)
> (In reply to Henri Sivonen (:hsivonen) from comment #9)
> > Do we [want] to keep using the Netscape bookmark format for
> > the default bookmarks on desktop?
> 
> Yes, we have a bookmarks.html file for default bookmarks on desktop

But is important to keep using HTML for the defaults? Fennec doesn't.

> and we
> support importing an html file, since other browsers export their bookmarks
> in that format.

Importing an HTML file from the bookmark manager UI is supported by the WIP patch already.
Depends on: 715099
mak, there are a bunch of test_browserGlue_* tests that deliberately try to pretend that nsBrowserGlue initialization is synchronous. Why are those tests avoiding the observer mechanism in the first place? With the new code, nsBrowserGlue initialization becomes truly async as far as the import of default bookmarks go, so those tests break.
(In reply to Henri Sivonen (:hsivonen) from comment #13)
> Why are those
> tests avoiding the observer mechanism in the first place? With the new code,
> nsBrowserGlue initialization becomes truly async as far as the import of
> default bookmarks go, so those tests break.

We have hundreds of tests expecting synchronous behavior and we rewrite/fix them every time some part becomes async. So this is not surprising.
Some of those, from what I see, already wait for the smart bookmarks creation batch, that happens at notifications, in http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#248
Still to do:
 * Figuring out what's wrong with the profile migrator integration.
 * Updating all the tests that deliberately foil the initialization notification mechanism in nsBrowserGlue and expect it to initialize synchronously.

I could use help on both these points.
Attachment #585699 - Attachment is obsolete: true
Mano may help you figuring you the integration with the migrators, and I may help you with the tests.
Do you have a list of tests failing? a Tryrun?
(In reply to Marco Bonardo [:mak] from comment #16)
> Mano may help you figuring you the integration with the migrators, and I may
> help you with the tests.
> Do you have a list of tests failing? a Tryrun?

No up-to-date try run, but the failures are all visible by running 
make -C browser/components/places/tests/ xpcshell-tests
in the objdir.
(In reply to Marco Bonardo [:mak] from comment #16)
> Do you have a list of tests failing? a Tryrun?

Failures left:
test_bookmarksRestoreNotification.js
test_browserGlue_distribution.js
test_browserGlue_prefs.js
test_browserGlue_smartBookmarks.js

I could use help with those.
Attachment #588368 - Attachment is obsolete: true
Thanks, I'll look at those today with your patch applied.
hm, I have updated the patch and now it passes all of the above tests, unfortunately I figured out you forgot to call do_test_pending in other 3 tests, so now I have to fix those as well, but patience, I'm almost there.
Attached patch updated patch (obsolete) — Splinter Review
this should pass all tests.
Most of the bugs hit by tests were actual real code bugs though, you may check the interdiff for the changes I made (sorry, I should have made an on-top patch, but I figured that out too late).
I think the patch needs some cleanup generally. but it's not too far from being reviewable.
Probably the the migration changes should be handled differently, we have to import defaults in very specific cases, when the database is empty and we are migrating with isImportDefaults. Otherwise we may end up importing them on a database that already has them. Plus the ordering is sadly important, since defaults should be added before any other bookmark, otherwise they'd just be queued. I have to think about this yet.
Mano may have thoughts too, since he was already challenging the problem in the Safari migrator.
ah notice there is another issue still to fix, when invoking the callback you should catch its exceptions, otherwise we end up notifying a failed import just because a callback threw.
(In reply to Marco Bonardo [:mak] from comment #21)
> this should pass all tests.

Thank you!

> I think the patch needs some cleanup generally. but it's not too far from
> being reviewable.

I don't know what cleanup is needed without review, so requesting review. :-)

(In reply to Marco Bonardo [:mak] from comment #22)
> ah notice there is another issue still to fix, when invoking the callback
> you should catch its exceptions, otherwise we end up notifying a failed
> import just because a callback threw.

This patch makes that change.
Attachment #589341 - Attachment is obsolete: true
Attachment #589509 - Flags: review?(mak77)
Comment on attachment 589509 [details] [diff] [review]
JS impl. of bookmark import, with catch around the callback

Review of attachment 589509 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry if this took a while, lots of stuff ongoing around!
It looks great! Though, considered the risk I'd delay it to FF13. There's still some minor design things to figure out, also with Mano where possible).
This won't likely be the final form of the module, but I think it's a good step to proceed and it's fine for now to stick with the original structure and improve it later.

::: browser/components/migration/src/ProfileMigrator.js
@@ +28,5 @@
>      params.appendElement(migrator, false);
>      params.appendElement(aStartup, false);
>  
> +    // Tell nsBrowserGlue.js not to import default bookmarks again
> +    Services.prefs.setBoolPref("browser.places.importBookmarksHTML", false);

hm, so, I don't like this much, it may interact with the user or an addon setting this pref and it is exposing a pref that is usually hidden.
Plus we should do this only when an initial import happens (not on an import started from the ui).
I'm not even sure it works correctly, if the database has just been created and browserGlue::_initPlaces is invoked before any bookmark is added, it will try to restore from JSON or from an old bookmarks.html.

You may init browserGlue here and add an API to it to set an internal flag and skip later import if this flag is set.
I will keep thinking to other solutions, the init path is not that easy to correctly synchronize (not that shutdown is any better).

::: browser/components/places/content/placesOverlay.xul
@@ +56,5 @@
>      // A bunch of browser code depends on us defining these, sad but true :(
>      var Cc = Components.classes;
>      var Ci = Components.interfaces;
>      var Cr = Components.results;
> +    var Cu = Components.utils;

Atm, I'd avoid this change, at least till we figure out the final destination of these definitions (surely not a Places overlay, see the bug above...).
Please use Components.utils for now where you need it and it's not already available.

::: toolkit/components/places/BookmarkImporter.jsm
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

So, I think I would like a somehow more generic naming here, since I suppose we will port the Export part to it when possible.

Maybe BookmarkHTMLUtils.jsm (and the object named after it). It may expose functions (like ImportFromFile, ImportFromURL) to do the import and (in future) the export and use the BookmarkImporter object internally (and one day we may have a BookmarkExporter)

@@ +36,5 @@
> + * 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 ***** */

Please use MPL2 (http://www.mozilla.org/MPL/headers/) for new files

@@ +52,5 @@
> +const Container_Normal = 0;
> +const Container_Toolbar = 1;
> +const Container_Menu = 2;
> +const Container_Unfiled = 3;
> +const Container_Places = 4;

nit: I'd prefer const CONTAINER = { NORMAL: 0, ... }
though we may cleanup this later, having the first porting more 1:1 is fine.

@@ +55,5 @@
> +const Container_Unfiled = 3;
> +const Container_Places = 4;
> +
> +// The RESTORE_*_NSIOBSERVER_TOPIC constants should match the #defines of the
> +// same names in browser/components/places/src/nsPlacesImportExportService.cpp

I think you removed those from that file?

@@ +68,5 @@
> +const POST_DATA_ANNO = "bookmarkProperties/POSTData";
> +
> +function BookmarkImporter(allowRootChanges,
> +                          folder,
> +                          isImportDefaults) {

prefix arguments with "a" where possible.

aAllowRootChanges looks dangerous, and in all the callpoints is false, may we kill it please?

Some arguments can get better naming:
s/folder/aDestFolderId/
s/isImportDefaults/aInitialImport/

@@ +71,5 @@
> +                          folder,
> +                          isImportDefaults) {
> +
> +  this.allowRootChanges = allowRootChanges;
> +  this.isImportDefaults = isImportDefaults;

prefix internal properties/methods with "_", those inteded to be used from the outside are fine unprefixed

@@ +77,5 @@
> +  // initialize the root frame with the menu root
> +  let menuRoot;
> +  if (folder) {
> +    menuRoot = folder;
> +    this.folderSpecified = true;

s/folderSpecified/importInFolder/

@@ +80,5 @@
> +    menuRoot = folder;
> +    this.folderSpecified = true;
> +    this.folderObject = Cc["@mozilla.org/supports-PRInt64;1"].
> +                        createInstance(Ci.nsISupportsPRInt64);
> +    this.folderObject.data = folder;

s/folderObject/wrappedFolderId/

@@ +82,5 @@
> +    this.folderObject = Cc["@mozilla.org/supports-PRInt64;1"].
> +                        createInstance(Ci.nsISupportsPRInt64);
> +    this.folderObject.data = folder;
> +  } else {
> +    menuRoot = PlacesUtils.bookmarks.bookmarksMenuFolder;

never use PlacesUtils.bookmarks.someroot. always use PlacesUtils.somerootId, like PlacesUtils.bookmarksMenuFolderId, in this case (so you avoid crossing xpcom)

@@ +91,5 @@
> +  this.frames = new Array();
> +  this.frames.push(this.constructFrame(menuRoot));
> +}
> +
> +BookmarkImporter.serialNumber = 0; // for favicons

Is this intended to be global for the single instance or for an entire session? in the former case you may just put it in the prototype, in the latter case in a gobal in the module scope

@@ +100,5 @@
> +    if (str) {
> +      return str.trim();
> +    }
> +    return str;
> +  },

to avoid confusion and help search, better calling this safeTrim()

@@ +108,5 @@
> +  },
> +
> +  previousFrame: function previousFrame() {
> +    return this.frames[this.frames.length - 2];
> +  },

why are not these getters?

@@ +115,5 @@
> +   * This is called when there is a new folder found. The folder takes the
> +   * name from the previous frame's heading.
> +   */
> +  newFrame: function newFrame() {
> +    let ourID = 0;

I wonder if there has ever been a theirID :)

s/ourID/frameFolderId/ or containerId to cope with the rest. please init it to -1, that we usually use to identify invalid ids.

@@ +117,5 @@
> +   */
> +  newFrame: function newFrame() {
> +    let ourID = 0;
> +    let frame = this.curFrame();
> +    let containerName = frame.previousText;

nit: in Places we usually use "title" rather than "name"

@@ +132,5 @@
> +                                                   PlacesUtils.bookmarks.DEFAULT_INDEX);
> +        break;
> +      case Container_Places:
> +        // places root, never reparent here, when we're building the initial
> +        // hierarchy, it will only be defined at the top level

I think that once you remove allowRootsChange all the reparenting stuff may go away

@@ +137,5 @@
> +        ourID = PlacesUtils.placesRootId;
> +        break;
> +      case Container_Menu:
> +        // menu folder
> +        ourID = PlacesUtils.bookmarksMenuFolderId;

these comments are useless, you can remove them

@@ +155,5 @@
> +        ourID = PlacesUtils.toolbarFolderId;
> +        break;
> +      default:
> +        // NOT REACHED
> +        break;

you may throw New Error here, at least

@@ +168,5 @@
> +    if (frame.previousDateAdded > 0) {
> +      try {
> +        PlacesUtils.bookmarks.setItemDateAdded(ourID, frame.previousDateAdded);
> +      } catch(e) {
> +      }

global-nit: you can put the closing brace in line with the catch, if you wish

@@ +187,5 @@
> +
> +  constructFrame: function constructFrame(frameId) {
> +    return {
> +
> +      containerID: frameId,

Could you convert this to a proper Frame object with its prototype? and instead of having constructFrame(id) around having new Frame(id)

@@ +262,5 @@
> +  },
> +
> +  popFrame: function popFrame() {
> +    this.frames.pop();
> +  },

this is useless complication, just hardcode this.frames.pop() where needed

@@ +267,5 @@
> +
> +  /**
> +   * Handles <H1>. We check for the attribute PLACES_ROOT and reset the
> +   * container id if it's found. Otherwise, the default bookmark menu
> +   * root is assumed and imported things will go into the bookmarks menu.

the last part of the comment sounds strange

@@ +272,5 @@
> +   */
> +  handleHead1Begin: function handleHead1Begin(elt) {
> +    if (this.frames.length > 1) {
> +      return;
> +    }

comment here, what is this handling? when may it happen? Is this an error?

@@ +274,5 @@
> +    if (this.frames.length > 1) {
> +      return;
> +    }
> +    if (elt.hasAttribute("places_root")) {
> +      this.curFrame().containerID = PlacesUtils.bookmarks.placesRoot;

PlacesUtils.placesRootId

@@ +291,5 @@
> +
> +    // after a heading, a previous bookmark is not applicable (for example, for
> +    // the descriptions contained in a <dd>). Neither is any previous head type
> +    frame.previousLink = null;
> +    frame.lastContainerType = Container_Normal;

I don't understand the comment :(

@@ +443,5 @@
> +      } catch(e) {
> +      }
> +      if (icon || iconUriObject) {
> +        try {
> +          this.setFaviconForURI(frame.previousLink, iconUriObject, icon);

Here we should likely use the new favicons API in mozIAsyncFavicons... though I'd accept a follow-up to fix it later.

@@ +523,5 @@
> +
> +  /**
> +   * Saves the title for the given bookmark. This only writes the user title.
> +   * Any previous title will be untouched. If this is a new entry, it will have
> +   * an empty "official" title until you visit it.

Most of this comment is false, we save the title, and it will stick!

@@ +576,5 @@
> +
> +  openContainer: function openContainer(elt) {
> +    if (elt.namespaceURI != "http://www.w3.org/1999/xhtml") {
> +      return;
> +    }

should we report a corruption here? was the old code just continuing?

@@ +685,5 @@
> +  },
> +
> +  addText: function addText(str) {
> +    this.curFrame().previousText += str;
> +  },

rename to appendText since it's what it does

@@ +734,5 @@
> +    // case data will not be saved to the db but we will still continue.
> +    PlacesUtils.favicons.setFaviconDataFromDataURL(faviconURI, data, 0);
> +
> +    PlacesUtils.favicons.setFaviconUrlForPage(pageURI, faviconURI);
> +  },

ditto on the fact this should use the new API, as I said above,  a follow-up will be fine.

@@ +744,5 @@
> +    if (date) {
> +      return parseInt(date) * 1000000; // in bookmarks.html this value is in seconds, not microseconds
> +    } else {
> +      return 0;
> +    }

I'd prefer if you parseInt(date) and ensure it's not NaN, checking it's not null/undefined is not enough
Also 0 doesn't sounds like a good fallback data to set in the database, maybe just use Date.now()

@@ +750,5 @@
> +
> +  runBatched: function runBatched(doc) {
> +    if (!doc) {
> +      return;
> +    }

may this really ever happen?

::: toolkit/components/places/nsIPlacesImportExportService.idl
@@ +42,5 @@
>  
>  /**
> + * The PlacesImportExport interface provides methods for exporting Places data.
> + * The word "Import" is in the name for legacy reasons and was kept to avoid
> + * disrupting potential extension code using the export part.

please point to the new importer module

::: toolkit/components/places/nsNavBookmarks.cpp
@@ +1537,5 @@
>  nsNavBookmarks::SetItemDateAdded(PRInt64 aItemId, PRTime aDateAdded)
>  {
> +  if (aItemId < 1) {
> +    printf("Bad itemid %lld\n", aItemId);
> +  }

this should go away

@@ +1726,5 @@
>  nsNavBookmarks::SetItemTitle(PRInt64 aItemId, const nsACString& aTitle)
>  {
> +  if (aItemId < 1) {
> +    printf("Bad itemid %lld\n", aItemId);
> +  }

ditto

::: toolkit/components/places/nsPlacesImportExportService.cpp
@@ +184,5 @@
>    }
>    return escaped;
>  }
>  
> +PLACES_FACTORY_SINGLETON_IMPLEMENTATION(nsPlacesExportService, gImportExportService)

if you rename the class you should also rename the file containing it, and fix the Makefile to the new name.
Attachment #589509 - Flags: review?(mak77) → feedback+
(In reply to Marco Bonardo [:mak] from comment #24)
> ::: browser/components/migration/src/ProfileMigrator.js

> You may init browserGlue here and add an API to it to set an internal flag
> and skip later import if this flag is set.

Initializing BrowserGlue from an unusual place seems scary. I could use some more guidance here.

> ::: browser/components/places/content/placesOverlay.xul
> @@ +56,5 @@
> >      // A bunch of browser code depends on us defining these, sad but true :(
> >      var Cc = Components.classes;
> >      var Ci = Components.interfaces;
> >      var Cr = Components.results;
> > +    var Cu = Components.utils;
> 
> Atm, I'd avoid this change, at least till we figure out the final
> destination of these definitions (surely not a Places overlay, see the bug
> above...).
> Please use Components.utils for now where you need it and it's not already
> available.

Done.

> ::: toolkit/components/places/BookmarkImporter.jsm
> @@ +1,1 @@
> > +/* ***** BEGIN LICENSE BLOCK *****
> 
> So, I think I would like a somehow more generic naming here, since I suppose
> we will port the Export part to it when possible.
> 
> Maybe BookmarkHTMLUtils.jsm (and the object named after it). It may expose
> functions (like ImportFromFile, ImportFromURL) to do the import and (in
> future) the export and use the BookmarkImporter object internally (and one
> day we may have a BookmarkExporter)

I renamed the .jsm but continued to expose BookmarkImporter? Why is exposing the BookmarkImporter object a problem?

> @@ +36,5 @@
> > + * 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 ***** */
> 
> Please use MPL2 (http://www.mozilla.org/MPL/headers/) for new files

Done, although this isn't purely a new file but a repurposing of an old one.

> @@ +52,5 @@
> > +const Container_Normal = 0;
> > +const Container_Toolbar = 1;
> > +const Container_Menu = 2;
> > +const Container_Unfiled = 3;
> > +const Container_Places = 4;
> 
> nit: I'd prefer const CONTAINER = { NORMAL: 0, ... }
> though we may cleanup this later, having the first porting more 1:1 is fine.

Deferring for later.

> @@ +55,5 @@
> > +const Container_Unfiled = 3;
> > +const Container_Places = 4;
> > +
> > +// The RESTORE_*_NSIOBSERVER_TOPIC constants should match the #defines of the
> > +// same names in browser/components/places/src/nsPlacesImportExportService.cpp
> 
> I think you removed those from that file?

Removed the comment.

> @@ +68,5 @@
> > +const POST_DATA_ANNO = "bookmarkProperties/POSTData";
> > +
> > +function BookmarkImporter(allowRootChanges,
> > +                          folder,
> > +                          isImportDefaults) {
> 
> prefix arguments with "a" where possible.

Done.

> aAllowRootChanges looks dangerous, and in all the callpoints is false, may
> we kill it please?

Done.

> Some arguments can get better naming:
> s/folder/aDestFolderId/
> s/isImportDefaults/aInitialImport/

Done.

> @@ +71,5 @@
> > +                          folder,
> > +                          isImportDefaults) {
> > +
> > +  this.allowRootChanges = allowRootChanges;
> > +  this.isImportDefaults = isImportDefaults;
> 
> prefix internal properties/methods with "_", those inteded to be used from
> the outside are fine unprefixed

Done.

> @@ +77,5 @@
> > +  // initialize the root frame with the menu root
> > +  let menuRoot;
> > +  if (folder) {
> > +    menuRoot = folder;
> > +    this.folderSpecified = true;
> 
> s/folderSpecified/importInFolder/

Done.

> @@ +80,5 @@
> > +    menuRoot = folder;
> > +    this.folderSpecified = true;
> > +    this.folderObject = Cc["@mozilla.org/supports-PRInt64;1"].
> > +                        createInstance(Ci.nsISupportsPRInt64);
> > +    this.folderObject.data = folder;
> 
> s/folderObject/wrappedFolderId/

Done.

> @@ +82,5 @@
> > +    this.folderObject = Cc["@mozilla.org/supports-PRInt64;1"].
> > +                        createInstance(Ci.nsISupportsPRInt64);
> > +    this.folderObject.data = folder;
> > +  } else {
> > +    menuRoot = PlacesUtils.bookmarks.bookmarksMenuFolder;
> 
> never use PlacesUtils.bookmarks.someroot. always use PlacesUtils.somerootId,
> like PlacesUtils.bookmarksMenuFolderId, in this case (so you avoid crossing
> xpcom)

Done.

> @@ +91,5 @@
> > +  this.frames = new Array();
> > +  this.frames.push(this.constructFrame(menuRoot));
> > +}
> > +
> > +BookmarkImporter.serialNumber = 0; // for favicons
> 
> Is this intended to be global for the single instance or for an entire
> session? in the former case you may just put it in the prototype, in the
> latter case in a gobal in the module scope

Moved to module scope.

> @@ +100,5 @@
> > +    if (str) {
> > +      return str.trim();
> > +    }
> > +    return str;
> > +  },
> 
> to avoid confusion and help search, better calling this safeTrim()

Renamed.

> @@ +108,5 @@
> > +  },
> > +
> > +  previousFrame: function previousFrame() {
> > +    return this.frames[this.frames.length - 2];
> > +  },
> 
> why are not these getters?

Changed to getters.

> @@ +115,5 @@
> > +   * This is called when there is a new folder found. The folder takes the
> > +   * name from the previous frame's heading.
> > +   */
> > +  newFrame: function newFrame() {
> > +    let ourID = 0;
> 
> I wonder if there has ever been a theirID :)
> 
> s/ourID/frameFolderId/ or containerId to cope with the rest. please init it
> to -1, that we usually use to identify invalid ids.

Done.

> @@ +117,5 @@
> > +   */
> > +  newFrame: function newFrame() {
> > +    let ourID = 0;
> > +    let frame = this.curFrame();
> > +    let containerName = frame.previousText;
> 
> nit: in Places we usually use "title" rather than "name"

Changed. (All this naming stuff comes from the old code.)

> @@ +132,5 @@
> > +                                                   PlacesUtils.bookmarks.DEFAULT_INDEX);
> > +        break;
> > +      case Container_Places:
> > +        // places root, never reparent here, when we're building the initial
> > +        // hierarchy, it will only be defined at the top level
> 
> I think that once you remove allowRootsChange all the reparenting stuff may
> go away

Removed.

> @@ +137,5 @@
> > +        ourID = PlacesUtils.placesRootId;
> > +        break;
> > +      case Container_Menu:
> > +        // menu folder
> > +        ourID = PlacesUtils.bookmarksMenuFolderId;
> 
> these comments are useless, you can remove them

Done.

> @@ +155,5 @@
> > +        ourID = PlacesUtils.toolbarFolderId;
> > +        break;
> > +      default:
> > +        // NOT REACHED
> > +        break;
> 
> you may throw New Error here, at least

Done.

> @@ +168,5 @@
> > +    if (frame.previousDateAdded > 0) {
> > +      try {
> > +        PlacesUtils.bookmarks.setItemDateAdded(ourID, frame.previousDateAdded);
> > +      } catch(e) {
> > +      }
> 
> global-nit: you can put the closing brace in line with the catch, if you wish

Left as is given "if you wish".

> @@ +187,5 @@
> > +
> > +  constructFrame: function constructFrame(frameId) {
> > +    return {
> > +
> > +      containerID: frameId,
> 
> Could you convert this to a proper Frame object with its prototype? and
> instead of having constructFrame(id) around having new Frame(id)

Done.

> @@ +262,5 @@
> > +  },
> > +
> > +  popFrame: function popFrame() {
> > +    this.frames.pop();
> > +  },
> 
> this is useless complication, just hardcode this.frames.pop() where needed

Done.

> @@ +267,5 @@
> > +
> > +  /**
> > +   * Handles <H1>. We check for the attribute PLACES_ROOT and reset the
> > +   * container id if it's found. Otherwise, the default bookmark menu
> > +   * root is assumed and imported things will go into the bookmarks menu.
> 
> the last part of the comment sounds strange

This stuff comes from the old code.

> @@ +272,5 @@
> > +   */
> > +  handleHead1Begin: function handleHead1Begin(elt) {
> > +    if (this.frames.length > 1) {
> > +      return;
> > +    }
> 
> comment here, what is this handling? when may it happen? Is this an error?

No idea--just copying the old code.

> @@ +274,5 @@
> > +    if (this.frames.length > 1) {
> > +      return;
> > +    }
> > +    if (elt.hasAttribute("places_root")) {
> > +      this.curFrame().containerID = PlacesUtils.bookmarks.placesRoot;
> 
> PlacesUtils.placesRootId

done.

> @@ +291,5 @@
> > +
> > +    // after a heading, a previous bookmark is not applicable (for example, for
> > +    // the descriptions contained in a <dd>). Neither is any previous head type
> > +    frame.previousLink = null;
> > +    frame.lastContainerType = Container_Normal;
> 
> I don't understand the comment :(

Comes from the old code.

> @@ +443,5 @@
> > +      } catch(e) {
> > +      }
> > +      if (icon || iconUriObject) {
> > +        try {
> > +          this.setFaviconForURI(frame.previousLink, iconUriObject, icon);
> 
> Here we should likely use the new favicons API in mozIAsyncFavicons...
> though I'd accept a follow-up to fix it later.

I did the async livemark changes. Let leave other asyncness as follow-up.

> @@ +523,5 @@
> > +
> > +  /**
> > +   * Saves the title for the given bookmark. This only writes the user title.
> > +   * Any previous title will be untouched. If this is a new entry, it will have
> > +   * an empty "official" title until you visit it.
> 
> Most of this comment is false, we save the title, and it will stick!

Removed most of the comment.

> @@ +576,5 @@
> > +
> > +  openContainer: function openContainer(elt) {
> > +    if (elt.namespaceURI != "http://www.w3.org/1999/xhtml") {
> > +      return;
> > +    }
> 
> should we report a corruption here? was the old code just continuing?

The old parser didn't have these lines, because the old parser always emitted only HTML elements.

It probably doesn't really matter either way how elaborate corruption handling is here.

> @@ +685,5 @@
> > +  },
> > +
> > +  addText: function addText(str) {
> > +    this.curFrame().previousText += str;
> > +  },
> 
> rename to appendText since it's what it does

Done.

> @@ +734,5 @@
> > +    // case data will not be saved to the db but we will still continue.
> > +    PlacesUtils.favicons.setFaviconDataFromDataURL(faviconURI, data, 0);
> > +
> > +    PlacesUtils.favicons.setFaviconUrlForPage(pageURI, faviconURI);
> > +  },
> 
> ditto on the fact this should use the new API, as I said above,  a follow-up
> will be fine.

OK.

> @@ +744,5 @@
> > +    if (date) {
> > +      return parseInt(date) * 1000000; // in bookmarks.html this value is in seconds, not microseconds
> > +    } else {
> > +      return 0;
> > +    }
> 
> I'd prefer if you parseInt(date) and ensure it's not NaN, checking it's not
> null/undefined is not enough
> Also 0 doesn't sounds like a good fallback data to set in the database,
> maybe just use Date.now()

Done.

> @@ +750,5 @@
> > +
> > +  runBatched: function runBatched(doc) {
> > +    if (!doc) {
> > +      return;
> > +    }
> 
> may this really ever happen?

I don't recall anymore.

> ::: toolkit/components/places/nsIPlacesImportExportService.idl
> @@ +42,5 @@
> >  
> >  /**
> > + * The PlacesImportExport interface provides methods for exporting Places data.
> > + * The word "Import" is in the name for legacy reasons and was kept to avoid
> > + * disrupting potential extension code using the export part.
> 
> please point to the new importer module

Done.

> ::: toolkit/components/places/nsNavBookmarks.cpp
> @@ +1537,5 @@
> >  nsNavBookmarks::SetItemDateAdded(PRInt64 aItemId, PRTime aDateAdded)
> >  {
> > +  if (aItemId < 1) {
> > +    printf("Bad itemid %lld\n", aItemId);
> > +  }
> 
> this should go away
> 
> @@ +1726,5 @@
> >  nsNavBookmarks::SetItemTitle(PRInt64 aItemId, const nsACString& aTitle)
> >  {
> > +  if (aItemId < 1) {
> > +    printf("Bad itemid %lld\n", aItemId);
> > +  }
> 
> ditto

Removed both.

> ::: toolkit/components/places/nsPlacesImportExportService.cpp
> @@ +184,5 @@
> >    }
> >    return escaped;
> >  }
> >  
> > +PLACES_FACTORY_SINGLETON_IMPLEMENTATION(nsPlacesExportService, gImportExportService)
> 
> if you rename the class you should also rename the file containing it, and
> fix the Makefile to the new name.

Done.
Attachment #589170 - Attachment is obsolete: true
Attachment #589509 - Attachment is obsolete: true
Attachment #601580 - Flags: review?(mak77)
Depends on: 735625
bug 735625 is removing importHTMLFromFileToFolder API, this can likely remove some stuff from this patch.

Btw, I plan to review this tomorrow.
Comment on attachment 601580 [details] [diff] [review]
JS impl. of bookmark import, addressing most comments so far

Review of attachment 601580 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for fixing all of those comments, it looks much better now!
I think the next iteration will be good enough to land, no reasons to keep working on a huge patch when we can make small patches, and so we can also do some more testing on a product rather than on a custom build (and get Nightly testing). We will have all the FF14 bracket to fix minor stuff.

The major issue to fix is in ProfileMigrator.js, but I think I have a decent solution to suggest and try there, see below.
I'd still prefer the common utils object exposing import (and in future export) methods.
Before unbitrotting I suggesting waiting for bug 735625 to land (hope to get a review along today/tomorrow) or apply that patch in mq before this one.

::: browser/components/migration/src/ProfileMigrator.js
@@ +28,5 @@
>      params.appendElement(migrator, false);
>      params.appendElement(aStartup, false);
>  
> +    // Tell nsBrowserGlue.js not to import default bookmarks again
> +    Services.prefs.setBoolPref("browser.places.importBookmarksHTML", false);

nsBrowserGlue is part of the app-startup category, so must have been already inited at this point, by using it you should not cause any effect.
To simplify things, I suggest to add to nsBrowserGlue::observe a new case like "initial-migration" and track through it, with an internal property, whether we are doing initial migration. Then later you can just check this flag and don't reimport defaults.  You don't even need to addObserver since you can directly QI nsBrowserGlue to nsIObserver call directly into .observe.

reusing browser.places.importBookmarksHTML looks scary to me, cause those prefs are in a fragile equilibrium. We should first do a refactoring of those, and doesn't look a priority atm.

Rather, we should do this only if aStartup is defined, since on non-initial migration we should not import default bookmarks.

@@ +32,5 @@
> +    Services.prefs.setBoolPref("browser.places.importBookmarksHTML", false);
> +    
> +    let defaultBookmarks = "resource:///defaults/profile/bookmarks.html";
> +    Cu.import("resource://gre/modules/BookmarkHTMLUtils.jsm");
> +    let importer = new BookmarkImporter(0, true);

Well, one reason for the module naming vs object naming, is that we usually export an object with the same name as the module. Though clearly that's not mandatory.
A second reason is that defineLazyModuleGetter only exports one symbol and I expect people to use that for less important modules, like this one.
That's why I suggestes to have an importer internal object, but expose methods through an exposed BookmarkHTMLUtils object that will have both import and export methods, similarly to the old API (actually maybe we should call it BookmarksHTMLUtils, to cope with bookmarks.html name? Sounds like painting a bikeshed :p ).
Finally, less possibility of naming conflicts (I would like to add also a BookmarksJSONUtils module, and if both export a BookmarkImporter object...)

::: browser/components/migration/src/nsBrowserProfileMigratorUtils.cpp
@@ -231,5 @@
> -  
> -  nsresult rv = ImportBookmarksHTML(bookmarksFile, true, true, EmptyString().get());
> -  NS_ENSURE_SUCCESS(rv, rv);
> -  return NS_OK;
> -}

this code changed, not a big deal, but the patch will have to be updated and the new ImportDefaultBookmarks I introduced instead of this craziness should be removed.

::: browser/components/migration/src/nsIEProfileMigrator.cpp
@@ -1407,5 @@
> -    // Initialize the default bookmarks
> -    nsCOMPtr<nsIFile> profile;
> -    GetProfilePath(nsnull, profile);
> -    rv = InitializeBookmarks(profile);
> -    NS_ENSURE_SUCCESS(rv, rv);

this changed as well, should just remove the new code.

::: browser/components/migration/src/nsSafariProfileMigrator.cpp
@@ -979,5 @@
>    else {
> -    nsCOMPtr<nsIFile> profile;
> -    GetProfilePath(nsnull, profile);
> -    rv = InitializeBookmarks(profile);
> -    NS_ENSURE_SUCCESS(rv, rv);

ditto

::: browser/components/places/tests/unit/test_bookmarksRestoreNotification.js
@@ +279,5 @@
>                                           bmsvc.DEFAULT_INDEX);
>        print("  Sanity check: createFolder() should have succeeded");
>        do_check_true(this.folderId > 0);
>        try {
> +        let importer = new BookmarkImporter(this.folderId, false);

all tests importing into a folder are about to be removed in bug 735625, so they should not be added back.

@@ +310,5 @@
> +        importer.importFromFile(this.file, function (success) {
> +          if (!success) {
> +            do_throw("  Restore should not have failed");
> +          }
> +        });

to be removed as well

@@ +337,5 @@
> +        importer.importFromFile(this.file, function (success) {
> +          if (success) {
> +            do_throw("  Restore should have failed");
> +          }
> +        });

and this one too

::: browser/components/places/tests/unit/test_bookmarks_html.js
@@ +272,1 @@
>    } catch(ex) { do_throw("couldn't import the exported file to folder: " + ex); }

importHTMLFromFileToFolder test disappearing

@@ +298,5 @@
> +        });
> +      } else {
> +        do_throw("couldn't import the exported file to folder.");
> +      }
> +    });

another importHTMLFromFileToFolder test to kill

::: toolkit/components/places/BookmarkHTMLUtils.jsm
@@ +24,5 @@
> +const RESTORE_NSIOBSERVER_DATA = "html";
> +const RESTORE_INITIAL_NSIOBSERVER_DATA = "html-initial";
> +
> +const LMANNO_FEEDURI = "livemark/feedURI";
> +const LMANNO_SITEURI = "livemark/siteURI";

instead of defining these, directly use PlacesUtils.LMANNO_FEEDURI and PlacesUtils.LMANNO_SITEURI

@@ +30,5 @@
> +const LOAD_IN_SIDEBAR_ANNO = "bookmarkProperties/loadInSidebar";
> +const DESCRIPTION_ANNO = "bookmarkProperties/description";
> +const POST_DATA_ANNO = "bookmarkProperties/POSTData";
> +
> +let _serialNumber = 0; // for favicons

no need for the underscore imo, this is the module scope, it's not exposed regardless

@@ +104,5 @@
> +  this.previousDateAdded = 0;
> +  this.previousLastModifiedDate = 0;
> +}
> +
> +function BookmarkImporter(aDestFolderId, aInitialImport) {

the first argument (And all the code handling it) should be removed, since we killed importHtmlFromFileToFolder, we don't import anymore to a specified folder.
This should simplify the code quite a bit by allowing to remove _importInFolder and _wrappedFolderId

@@ +132,5 @@
> +  _safeTrim: function safeTrim(aStr) {
> +    if (aStr) {
> +      return aStr.trim();
> +    }
> +    return aStr;

return aStr ? aStr.trim() : aStr;

@@ +467,5 @@
> +        // The is a live bookmark.  We create it here since in HandleLinkBegin we
> +        // don't know the title.
> +        PlacesUtils.livemarks.addLivemark({
> +          "id": 0,
> +          "guid": "",

don't pass these, this is a jsval, so you can avoid any unneeded properties

@@ +470,5 @@
> +          "id": 0,
> +          "guid": "",
> +          "title": frame.previousText,
> +          "parentId": frame.containerId,
> +          "index": PlacesUtils.bookmarks.DEFAULT_INDEX,

if possible we should use the actual index here, since this is async DEFAULT_INDEX will end up inserting the livemark in the wrong place in future... though, if we don't have the index here I'm fine with handling this in future, when we'll make this use the async bookmarking api, since for now it will do the right thing thanks to internal implementation of the livemarks service.

@@ +473,5 @@
> +          "parentId": frame.containerId,
> +          "index": PlacesUtils.bookmarks.DEFAULT_INDEX,
> +          "feedURI": frame.previousFeed,
> +          "siteURI": frame.previousLink,
> +        }, null);

don't need to pass null, the callback is marked [optional]

@@ +758,5 @@
> +        } catch(ex) {
> +        }
> +      }
> +    }).bind(this);
> +    xhr.open("GET", aUrlString);

we found recently that xhr.open can throw, so may be worth to expand the below try to include this.
wouldn't make sense to use asynchronous request here?
Attachment #601580 - Flags: review?(mak77)
Blocks: 728174
(In reply to Marco Bonardo [:mak] from comment #27)
> I'd still prefer the common utils object exposing import (and in future
> export) methods.

Done.

> Before unbitrotting I suggesting waiting for bug 735625 to land (hope to get
> a review along today/tomorrow) or apply that patch in mq before this one.

OK.

> ::: browser/components/migration/src/ProfileMigrator.js
> @@ +28,5 @@
> >      params.appendElement(migrator, false);
> >      params.appendElement(aStartup, false);
> >  
> > +    // Tell nsBrowserGlue.js not to import default bookmarks again
> > +    Services.prefs.setBoolPref("browser.places.importBookmarksHTML", false);
> 
> nsBrowserGlue is part of the app-startup category, so must have been already
> inited at this point, by using it you should not cause any effect.
> To simplify things, I suggest to add to nsBrowserGlue::observe a new case
> like "initial-migration" and track through it, with an internal property,
> whether we are doing initial migration. Then later you can just check this
> flag and don't reimport defaults.  You don't even need to addObserver since
> you can directly QI nsBrowserGlue to nsIObserver call directly into .observe.

Done.

> Rather, we should do this only if aStartup is defined, since on non-initial
> migration we should not import default bookmarks.

Done.

> @@ +32,5 @@
> > +    Services.prefs.setBoolPref("browser.places.importBookmarksHTML", false);
> > +    
> > +    let defaultBookmarks = "resource:///defaults/profile/bookmarks.html";
> > +    Cu.import("resource://gre/modules/BookmarkHTMLUtils.jsm");
> > +    let importer = new BookmarkImporter(0, true);
> 
> Well, one reason for the module naming vs object naming, is that we usually
> export an object with the same name as the module. Though clearly that's not
> mandatory.
> A second reason is that defineLazyModuleGetter only exports one symbol and I
> expect people to use that for less important modules, like this one.
> That's why I suggestes to have an importer internal object, but expose
> methods through an exposed BookmarkHTMLUtils object that will have both
> import and export methods, similarly to the old API (actually maybe we
> should call it BookmarksHTMLUtils, to cope with bookmarks.html name? Sounds
> like painting a bikeshed :p ).
> Finally, less possibility of naming conflicts (I would like to add also a
> BookmarksJSONUtils module, and if both export a BookmarkImporter object...)

OK. I exposed a BookmarkHTMLUtils object.

> ::: browser/components/migration/src/nsBrowserProfileMigratorUtils.cpp
> @@ -231,5 @@
> > -  
> > -  nsresult rv = ImportBookmarksHTML(bookmarksFile, true, true, EmptyString().get());
> > -  NS_ENSURE_SUCCESS(rv, rv);
> > -  return NS_OK;
> > -}
> 
> this code changed, not a big deal, but the patch will have to be updated and
> the new ImportDefaultBookmarks I introduced instead of this craziness should
> be removed.

Removed the new code instead. 

> ::: toolkit/components/places/BookmarkHTMLUtils.jsm
> @@ +24,5 @@
> > +const RESTORE_NSIOBSERVER_DATA = "html";
> > +const RESTORE_INITIAL_NSIOBSERVER_DATA = "html-initial";
> > +
> > +const LMANNO_FEEDURI = "livemark/feedURI";
> > +const LMANNO_SITEURI = "livemark/siteURI";
> 
> instead of defining these, directly use PlacesUtils.LMANNO_FEEDURI and
> PlacesUtils.LMANNO_SITEURI

Removed. These were not actually used.

> @@ +30,5 @@
> > +const LOAD_IN_SIDEBAR_ANNO = "bookmarkProperties/loadInSidebar";
> > +const DESCRIPTION_ANNO = "bookmarkProperties/description";
> > +const POST_DATA_ANNO = "bookmarkProperties/POSTData";
> > +
> > +let _serialNumber = 0; // for favicons
> 
> no need for the underscore imo, this is the module scope, it's not exposed
> regardless

Removed the underscore.

> @@ +104,5 @@
> > +  this.previousDateAdded = 0;
> > +  this.previousLastModifiedDate = 0;
> > +}
> > +
> > +function BookmarkImporter(aDestFolderId, aInitialImport) {
> 
> the first argument (And all the code handling it) should be removed, since
> we killed importHtmlFromFileToFolder, we don't import anymore to a specified
> folder.
> This should simplify the code quite a bit by allowing to remove
> _importInFolder and _wrappedFolderId

Done.

> @@ +132,5 @@
> > +  _safeTrim: function safeTrim(aStr) {
> > +    if (aStr) {
> > +      return aStr.trim();
> > +    }
> > +    return aStr;
> 
> return aStr ? aStr.trim() : aStr;

Done.

> @@ +467,5 @@
> > +        // The is a live bookmark.  We create it here since in HandleLinkBegin we
> > +        // don't know the title.
> > +        PlacesUtils.livemarks.addLivemark({
> > +          "id": 0,
> > +          "guid": "",
> 
> don't pass these, this is a jsval, so you can avoid any unneeded properties

Removed.

> @@ +470,5 @@
> > +          "id": 0,
> > +          "guid": "",
> > +          "title": frame.previousText,
> > +          "parentId": frame.containerId,
> > +          "index": PlacesUtils.bookmarks.DEFAULT_INDEX,
> 
> if possible we should use the actual index here, since this is async
> DEFAULT_INDEX will end up inserting the livemark in the wrong place in
> future... though, if we don't have the index here I'm fine with handling
> this in future, when we'll make this use the async bookmarking api, since
> for now it will do the right thing thanks to internal implementation of the
> livemarks service.

Left for the future.

> @@ +473,5 @@
> > +          "parentId": frame.containerId,
> > +          "index": PlacesUtils.bookmarks.DEFAULT_INDEX,
> > +          "feedURI": frame.previousFeed,
> > +          "siteURI": frame.previousLink,
> > +        }, null);
> 
> don't need to pass null, the callback is marked [optional]

Removed.

> @@ +758,5 @@
> > +        } catch(ex) {
> > +        }
> > +      }
> > +    }).bind(this);
> > +    xhr.open("GET", aUrlString);
> 
> we found recently that xhr.open can throw, so may be worth to expand the
> below try to include this.

Expanded.

> wouldn't make sense to use asynchronous request here?

This is an async request.
Attachment #601580 - Attachment is obsolete: true
Blocks: 549736
Comment on attachment 606589 [details] [diff] [review]
JS impl. of bookmark import, review comments addressed

With this iteration of the patch, I believe I have addressed previous review comments, but now test_browserGlue_corrupt_nobackup.js hangs and I don't see why. Any ideas why that test hangs?
Attachment #606589 - Flags: feedback?(mak77)
yes, you didn't update the lazy module getter in nsBrowserGlue.js it's still trying to import BookmarkImporter and the test is correctly complaining :)
Though I have to rebuild locally to verify that.
Comment on attachment 606589 [details] [diff] [review]
JS impl. of bookmark import, review comments addressed

Review of attachment 606589 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/migration/src/ProfileMigrator.js
@@ +48,5 @@
> +      // Tell nsBrowserGlue.js not to import default bookmarks again
> +      Cc["@mozilla.org/browser/browserglue;1"]
> +        .getService(Ci.nsIBrowserGlue);
> +        .queryInterface(Ci.nsIObserver)
> +        .observe(null, "initial-migration", null);

this is broken, due to the semicolon
Btw, you can just Cc["@mozilla.org/browser/browserglue;1"].getService(Ci.nsIObserver).observe

@@ +54,5 @@
> +
> +    let defaultBookmarks = "resource:///defaults/profile/bookmarks.html";
> +    Cu.import("resource://gre/modules/BookmarkHTMLUtils.jsm");
> +    BookmarkHTMLUtils.importFromURL(defaultBookmarks, true, function() {
> +      Services.ww.openWindow(null,

this is still wrong, we should not import default bookmarks if it's not a startup migration. you need to define the callback function elsewhere, something like

function startMigrating() {
  all the window opening stuff
}

if (aStartup) {
  glue stuff
  BookmarkHTMLUtils.importFromURL(defaultBookmarks, true, startMigrating);
}
else {
  startMigrating()
}

::: browser/components/nsBrowserGlue.js
@@ +60,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
> +                                  "resource://gre/modules/PlacesUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "BookmarkImporter",
> +                                  "resource://gre/modules/BookmarkHTMLUtils.jsm");

this is importing the wrong symbol

@@ +1000,5 @@
>      var importBookmarksHTML = false;
>      try {
>        importBookmarksHTML =
>          Services.prefs.getBoolPref("browser.places.importBookmarksHTML");
> +      if (importBookmarksHTML && !this._initialMigrationPerformed)

rather than doing this, the code to change is this one:

if (dbStatus == PlacesUtils.history.DATABASE_STATUS_CREATE) {
  // If the database has just been created, but we already have any
  // bookmark, this is not the initial import.  This can happen after a
  // migration from a different browser since migrators run before us.
  // In such a case we should not import, unless some pref has been set.
  if (PlacesUtils.bookmarks.getIdForItemAt(PlacesUtils.bookmarksMenuFolderId, 0) != -1 ||
  PlacesUtils.bookmarks.getIdForItemAt(PlacesUtils.toolbarFolderId, 0) != -1)
  importBookmarks = false;
}

I suggest removing all of this code and changing importBookmarks initial value to

var importBookmarks = !this._initialMigrationPerformed &&
                      (dbStatus == PlacesUtils.history.DATABASE_STATUS_CREATE ||
                       dbStatus == PlacesUtils.history.DATABASE_STATUS_CORRUPT);

this should also be a nice Ts win.

::: browser/components/places/tests/unit/test_457441-import-export-corrupt-bookmarks-html.js
@@ +229,5 @@
>    var faviconURI = icos.getFaviconForPage(uri(TEST_FAVICON_PAGE_URL));
>    var dataURL = icos.getFaviconDataAsDataURL(faviconURI);
>    do_check_eq(TEST_FAVICON_DATA_URL, dataURL);
> +
> +  do_test_finished();

this test has 2 do_test_finished() calls, something went wrong in the merging, or maybe you added this for the timeout and now the test is stopping too early.
Attachment #606589 - Flags: feedback?(mak77)
Comment on attachment 606589 [details] [diff] [review]
JS impl. of bookmark import, review comments addressed

Review of attachment 606589 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/BookmarkHTMLUtils.jsm
@@ +137,5 @@
> +      case Container_Normal:
> +        // append a new folder
> +        containerId = PlacesUtils.bookmarks.createFolder(frame.containerId,
> +                                                   containerTitle,
> +                                                   PlacesUtils.bookmarks.DEFAULT_INDEX);

bogus indent, rather
containerId =
  PlacesUtils.bookmarks.createFolder(frame.containerId,
                                     containerTitle,
                                     PlacesUtils.bookmarks.DEFAULT_INDEX);
or
containerId = PlacesUtils.bookmarks
                         .createFolder(frame.containerId,
                                       containerTitle,
                                       PlacesUtils.bookmarks.DEFAULT_INDEX);

@@ +251,5 @@
> +      let modDate;
> +      if (modDate = aElt.getAttribute("last_modified")) {
> +        frame.previousLastModifiedDate = this._convertImportedDateToInternalDate(modDate);
> +      }
> +    }

this code looks different from the original code, see
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsPlacesImportExportService.cpp#774
the original code just sets dateAdded or lastModified.

also I'd prefer is being written as
let addDate = getAttr
let modDate = getAttr
if (addDate) something
else if (modDate) something

@@ +338,5 @@
> +    // Set the date added value, if we have it.
> +    if (dateAdded) {
> +      let convertedDateAdded =
> +        this._convertImportedDateToInternalDate(dateAdded);
> +      if (convertedDateAdded) {

when may this if be false? _convertImportedDateToInternalDate seems to always return a meaningful value

@@ +525,5 @@
> +                                                      frame.previousText,
> +                                                      0,
> +                                                      PlacesUtils.annotations.EXPIRE_NEVER);
> +
> +          }

nit: useless newline

@@ +736,5 @@
> +      }
> +    }).bind(this);
> +    this._notifyObservers(RESTORE_BEGIN_NSIOBSERVER_TOPIC);
> +    try {
> +      xhr.open("GET", aUrlString);

what I meant by saying async, is whether we want the third argument to open, doesn't xhr.open default to a sync request?

@@ +758,5 @@
> +  },
> +
> +};
> +
> +var BookmarkHTMLUtils = {

please move this to the top, before any other private object, and Object.freeze() it.

@@ +763,5 @@
> +  importFromURL: function importFromFile(aUrlString,
> +                                         aInitialImport,
> +                                         aCallback) {
> +	let importer = new BookmarkImporter(aInitialImport);
> +	importer.importFromURL(aUrlString, aCallback);

bogus indent

@@ +770,5 @@
> +  importFromFile: function importFromFile(aLocalFile,
> +                                          aInitialImport,
> +                                          aCallback) {
> +	let importer = new BookmarkImporter(aInitialImport);
> +	importer.importFromFile(aLocalFile, aCallback);

bogus indent
(In reply to Marco Bonardo [:mak] from comment #30)
> yes, you didn't update the lazy module getter in nsBrowserGlue.js it's still
> trying to import BookmarkImporter and the test is correctly complaining :)

Ooops... Thanks. Fixed.

(In reply to Marco Bonardo [:mak] from comment #31)
> ::: browser/components/migration/src/ProfileMigrator.js
> @@ +48,5 @@
> > +      // Tell nsBrowserGlue.js not to import default bookmarks again
> > +      Cc["@mozilla.org/browser/browserglue;1"]
> > +        .getService(Ci.nsIBrowserGlue);
> > +        .queryInterface(Ci.nsIObserver)
> > +        .observe(null, "initial-migration", null);
> 
> this is broken, due to the semicolon
> Btw, you can just
> Cc["@mozilla.org/browser/browserglue;1"].getService(Ci.nsIObserver).observe

Fixed.

> @@ +54,5 @@
> > +
> > +    let defaultBookmarks = "resource:///defaults/profile/bookmarks.html";
> > +    Cu.import("resource://gre/modules/BookmarkHTMLUtils.jsm");
> > +    BookmarkHTMLUtils.importFromURL(defaultBookmarks, true, function() {
> > +      Services.ww.openWindow(null,
> 
> this is still wrong, we should not import default bookmarks if it's not a
> startup migration. you need to define the callback function elsewhere,
> something like
> 
> function startMigrating() {
>   all the window opening stuff
> }
> 
> if (aStartup) {
>   glue stuff
>   BookmarkHTMLUtils.importFromURL(defaultBookmarks, true, startMigrating);
> }
> else {
>   startMigrating()
> }

Changed to what you suggested.

> ::: browser/components/nsBrowserGlue.js
> @@ +60,5 @@
> > +XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
> > +                                  "resource://gre/modules/PlacesUtils.jsm");
> > +
> > +XPCOMUtils.defineLazyModuleGetter(this, "BookmarkImporter",
> > +                                  "resource://gre/modules/BookmarkHTMLUtils.jsm");
> 
> this is importing the wrong symbol

Fixed.

> @@ +1000,5 @@
> >      var importBookmarksHTML = false;
> >      try {
> >        importBookmarksHTML =
> >          Services.prefs.getBoolPref("browser.places.importBookmarksHTML");
> > +      if (importBookmarksHTML && !this._initialMigrationPerformed)
> 
> rather than doing this, the code to change is this one:
> 
> if (dbStatus == PlacesUtils.history.DATABASE_STATUS_CREATE) {
>   // If the database has just been created, but we already have any
>   // bookmark, this is not the initial import.  This can happen after a
>   // migration from a different browser since migrators run before us.
>   // In such a case we should not import, unless some pref has been set.
>   if
> (PlacesUtils.bookmarks.getIdForItemAt(PlacesUtils.bookmarksMenuFolderId, 0)
> != -1 ||
>   PlacesUtils.bookmarks.getIdForItemAt(PlacesUtils.toolbarFolderId, 0) != -1)
>   importBookmarks = false;
> }
> 
> I suggest removing all of this code and changing importBookmarks initial
> value to

Removing what code? I removed the if (dbStatus == PlacesUtils.history.DATABASE_STATUS_CREATE) { block in this iteration of the patch. I'm seeing 3 xpcshell test failures whose cause isn't obvious to me.

> var importBookmarks = !this._initialMigrationPerformed &&
>                       (dbStatus ==
> PlacesUtils.history.DATABASE_STATUS_CREATE ||
>                        dbStatus ==
> PlacesUtils.history.DATABASE_STATUS_CORRUPT);

OK.

> :::
> browser/components/places/tests/unit/test_457441-import-export-corrupt-
> bookmarks-html.js
> @@ +229,5 @@
> >    var faviconURI = icos.getFaviconForPage(uri(TEST_FAVICON_PAGE_URL));
> >    var dataURL = icos.getFaviconDataAsDataURL(faviconURI);
> >    do_check_eq(TEST_FAVICON_DATA_URL, dataURL);
> > +
> > +  do_test_finished();
> 
> this test has 2 do_test_finished() calls, something went wrong in the
> merging, or maybe you added this for the timeout and now the test is
> stopping too early.

Bad merge. Fixed.

(In reply to Marco Bonardo [:mak] from comment #32)
> Comment on attachment 606589 [details] [diff] [review]
> JS impl. of bookmark import, review comments addressed
> 
> Review of attachment 606589 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/places/BookmarkHTMLUtils.jsm
> @@ +137,5 @@
> > +      case Container_Normal:
> > +        // append a new folder
> > +        containerId = PlacesUtils.bookmarks.createFolder(frame.containerId,
> > +                                                   containerTitle,
> > +                                                   PlacesUtils.bookmarks.DEFAULT_INDEX);
> 
> bogus indent, rather
> containerId =
>   PlacesUtils.bookmarks.createFolder(frame.containerId,
>                                      containerTitle,
>                                      PlacesUtils.bookmarks.DEFAULT_INDEX);
> or
> containerId = PlacesUtils.bookmarks
>                          .createFolder(frame.containerId,
>                                        containerTitle,
>                                        PlacesUtils.bookmarks.DEFAULT_INDEX);

Fixed.

> @@ +251,5 @@
> > +      let modDate;
> > +      if (modDate = aElt.getAttribute("last_modified")) {
> > +        frame.previousLastModifiedDate = this._convertImportedDateToInternalDate(modDate);
> > +      }
> > +    }
> 
> this code looks different from the original code, see
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> nsPlacesImportExportService.cpp#774
> the original code just sets dateAdded or lastModified.
> 
> also I'd prefer is being written as
> let addDate = getAttr
> let modDate = getAttr
> if (addDate) something
> else if (modDate) something

Fixed.

> @@ +338,5 @@
> > +    // Set the date added value, if we have it.
> > +    if (dateAdded) {
> > +      let convertedDateAdded =
> > +        this._convertImportedDateToInternalDate(dateAdded);
> > +      if (convertedDateAdded) {
> 
> when may this if be false? _convertImportedDateToInternalDate seems to
> always return a meaningful value

Removed the falsiness check.

> @@ +525,5 @@
> > +                                                      frame.previousText,
> > +                                                      0,
> > +                                                      PlacesUtils.annotations.EXPIRE_NEVER);
> > +
> > +          }
> 
> nit: useless newline

Removed.

> @@ +736,5 @@
> > +      }
> > +    }).bind(this);
> > +    this._notifyObservers(RESTORE_BEGIN_NSIOBSERVER_TOPIC);
> > +    try {
> > +      xhr.open("GET", aUrlString);
> 
> what I meant by saying async, is whether we want the third argument to open,
> doesn't xhr.open default to a sync request?

The default is async. I leave out the 3rd param to pretend sync XHR doesn't even exist.

> @@ +758,5 @@
> > +  },
> > +
> > +};
> > +
> > +var BookmarkHTMLUtils = {
> 
> please move this to the top, before any other private object, and
> Object.freeze() it.

Done.

> @@ +763,5 @@
> > +  importFromURL: function importFromFile(aUrlString,
> > +                                         aInitialImport,
> > +                                         aCallback) {
> > +	let importer = new BookmarkImporter(aInitialImport);
> > +	importer.importFromURL(aUrlString, aCallback);
> 
> bogus indent
> 
> @@ +770,5 @@
> > +  importFromFile: function importFromFile(aLocalFile,
> > +                                          aInitialImport,
> > +                                          aCallback) {
> > +	let importer = new BookmarkImporter(aInitialImport);
> > +	importer.importFromFile(aLocalFile, aCallback);
> 
> bogus indent

How is the indent bogus in these cases?
Attachment #606589 - Attachment is obsolete: true
Attachment #607552 - Flags: feedback?(mak77)
(In reply to Henri Sivonen (:hsivonen) from comment #33)

> > @@ +1000,5 @@
> > >      var importBookmarksHTML = false;
> > >      try {
> > >        importBookmarksHTML =
> > >          Services.prefs.getBoolPref("browser.places.importBookmarksHTML");
> > > +      if (importBookmarksHTML && !this._initialMigrationPerformed)
> > 
> > rather than doing this, the code to change is this one:
> > 
> > if (dbStatus == PlacesUtils.history.DATABASE_STATUS_CREATE) {
> >   // If the database has just been created, but we already have any
> >   // bookmark, this is not the initial import.  This can happen after a
> >   // migration from a different browser since migrators run before us.
> >   // In such a case we should not import, unless some pref has been set.
> >   if
> > (PlacesUtils.bookmarks.getIdForItemAt(PlacesUtils.bookmarksMenuFolderId, 0)
> > != -1 ||
> >   PlacesUtils.bookmarks.getIdForItemAt(PlacesUtils.toolbarFolderId, 0) != -1)
> >   importBookmarks = false;
> > }
> > 
> > I suggest removing all of this code and changing importBookmarks initial
> > value to
> 
> Removing what code? I removed the if (dbStatus ==
> PlacesUtils.history.DATABASE_STATUS_CREATE) { block in this iteration of the
> patch. I'm seeing 3 xpcshell test failures whose cause isn't obvious to me.

Yes, that's what I meant.  I will check the failures, are those in browser/components/places/tests?

> > @@ +736,5 @@
> > what I meant by saying async, is whether we want the third argument to open,
> > doesn't xhr.open default to a sync request?
> 
> The default is async. I leave out the 3rd param to pretend sync XHR doesn't
> even exist.

ah, I see, it's optional but with default value true... a bit confusing since we usually consider undefined like false... btw, fine!

> How is the indent bogus in these cases?

hm, looks like you have some tabs instead of spaces there.
(In reply to Marco Bonardo [:mak] from comment #34)
> I will check the failures, are those in
> browser/components/places/tests?

Yes, they are there. Thanks.

> hm, looks like you have some tabs instead of spaces there.

OK.
ah, I was wrong on the if/elseif stuff on dates.  The previous code was inside a for loop, so it was correct what you did originally, handling both attributes :( sorry, just revert to:

let addDate = aElt.getAttribute("add_date");
if (addDate) {
  frame.previousDateAdded =
    this._convertImportedDateToInternalDate(addDate);
}
let modDate = aElt.getAttribute("last_modified");
if (modDate) {
  frame.previousLastModifiedDate =
    this._convertImportedDateToInternalDate(modDate);
}

there is still a failure in test_browserGlue_migrate.js, but that's a positive thing since that code is exactly testing the code we removed! Just modify the code like this:

// A migrator would run before nsBrowserGlue Places initialization, so mimic
// that behavior adding a bookmark and notifying the migration.
PlacesUtils.bookmarks.insertBookmark(PlacesUtils.bookmarks.bookmarksMenuFolder,
                                     uri("http://mozilla.org/"),
                                     PlacesUtils.bookmarks.DEFAULT_INDEX,
                                     "migrated");
let bg = Cc["@mozilla.org/browser/browserglue;1"].
         getService(Ci.nsIObserver);
bg.observe(null, "initial-migration", null);
hm, I'm seeing something weird on initial migration, investigating.
ah, right, on startup migration we can't use Places since the profile folder is not yet setup, until aStartup.doStartup() is invoked, previously this import was done by the migrators later, so it was not a problem.
We may need to enqueue to bug 718608 to properly find a place to put this puzzle piece, luckily that's almost done, I'll try to test it tonight so we could push it and build on top of it.
Attached patch unbitrot (obsolete) — Splinter Review
well while testing it I ended up fixing the tests and unbitrotting, so attaching what I did to save some work to henri
Attachment #607552 - Attachment is obsolete: true
Attachment #607552 - Flags: feedback?(mak77)
Comment on attachment 607656 [details] [diff] [review]
unbitrot

(In reply to Marco Bonardo [:mak] from comment #39)
> well while testing it I ended up fixing the tests and unbitrotting, so
> attaching what I did to save some work to henri

Thank you.

(In reply to Marco Bonardo [:mak] from comment #38)
> We may need to enqueue to bug 718608 to properly find a place to put this
> puzzle piece, 

Does that mean bitrotting this patch again? Could we not land this as-is so that this patch stops rotting and then let the remaining problem be fixed in later refactorings?
Attachment #607656 - Flags: review?(mak77)
> (In reply to Marco Bonardo [:mak] from comment #38)
> > We may need to enqueue to bug 718608 to properly find a place to put this
> > puzzle piece, 
> 
> Does that mean bitrotting this patch again? Could we not land this as-is so
> that this patch stops rotting and then let the remaining problem be fixed in
> later refactorings?

nope, the problem is that this patch as is completely breaks Places initialization when initial migration runs.  Though the bitrot is limited to ProfileMigrator.js and we may even handle it while looking for a hook point for the import, so don't worry about that, it's a minimal change.
just as a note Bug 727637 is bitrotting the modules import at the top of nsBrowserGlue, could be merged today or tomorrow moening.  bug 718608 is just waiting for SR, may land today and be merged tomorrow.
So after discussing with Mano, we agree to proceed with the patch without default bookmarks import.  Mano will take care of that in bug 737381.
Fx-team should soon merge, then we can then just unbitrot on top of m-c and remove the import defaults code from ProfileMigrator.js.
Attached patch RebasedSplinter Review
Attachment #607656 - Attachment is obsolete: true
Attachment #608326 - Flags: review?(mak77)
Attachment #607656 - Flags: review?(mak77)
Comment on attachment 608326 [details] [diff] [review]
Rebased

Review of attachment 608326 [details] [diff] [review]:
-----------------------------------------------------------------

Man, I still can't believe this is happening.  Thank you very much for doing this.  Can't wait to see the export part die too :)

Asking SR for the removal of the import methods from nsIPlacesImportExportService, the module name, symbol and methods exported by BookmarkHTMLUtils.jsm

::: browser/components/places/tests/unit/test_browserGlue_migrate.js
@@ +31,3 @@
>  
>    // A migrator would run before nsBrowserGlue, so we mimic that behavior
>    // adding a bookmark.

I think I suggested updating this comment in a comment above (The same comment where I suggested adding initial-migration handling)
Attachment #608326 - Flags: superreview?(gavin.sharp)
Attachment #608326 - Flags: review?(mak77)
Attachment #608326 - Flags: review+
Flags: in-testsuite+
Keywords: dev-doc-needed
Blocks: 738263
filed bug 738263 to track remaining default bookmarks work
No longer blocks: 738263
Depends on: 738263
Attachment #608326 - Flags: superreview?(gavin.sharp) → superreview+
(In reply to Marco Bonardo [:mak] from comment #45)
> >    // A migrator would run before nsBrowserGlue, so we mimic that behavior
> >    // adding a bookmark.
> 
> I think I suggested updating this comment in a comment above (The same
> comment where I suggested adding initial-migration handling)

Changed the comment as suggested earlier.

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d2693e86769d

Thanks for all the help, the r and the sr!
...and backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e793817bfbee
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e17d25beb07
...due to crashes on Mac in mozilla::storage::StatementCache<mozIStorageStatement>::GetCachedStatement :
https://tbpl.mozilla.org/php/getParsedLog.php?id=10310267&tree=Mozilla-Inbound&full=1#error1
https://tbpl.mozilla.org/php/getParsedLog.php?id=10310271&tree=Mozilla-Inbound&full=1#error1

###!!! ASSERTION: The statement 'SELECT b.id, h.url, b.title, b.position, b.fk, b.parent, b.type, b.dateAdded, b.lastModified, b.guid, t.guid, t.parent FROM moz_bookmarks b LEFT JOIN moz_bookmarks t ON t.id = b.parent LEFT JOIN moz_places h ON h.id = b.fk WHERE b.id = :item_id' failed to compile with the error message ''.: 'Error', file ../../../dist/include/mozilla/storage/StatementCache.h, line 162

Do we have different sqlite on Mac vs. the other platforms? Do we need the latest sqlite update that fixed some LEFT JOIN stuff?
this just means the test has a wrong finish, maybe finishing too early
that's cause the test initializes browserGlue, that tries to start an import, the import begins asynchronously, and tries to use the service after the connection has been closed.
For now I'd say to just notify "initial-migration" in the test just after we init gluesvc, add a comment above that saying "// Avoid default bookmarks import.".
Comment on attachment 608648 [details] [diff] [review]
Tell nsBrowserGlue not to import initial bookmarks in tests that crash on Mac

Review of attachment 608648 [details] [diff] [review]:
-----------------------------------------------------------------

yes, for now will be fine, try to run the 2 tests locally just in case.

just please, add a space between // and the comment.
Attachment #608648 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/404738948156
https://hg.mozilla.org/mozilla-central/rev/adc7f3ed2d78
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Comment on attachment 609118 [details] [diff] [review]
(AAv1) Remove DEFAULT_BOOKMARKS leftover
[Checked in: Comment 57]

Review of attachment 609118 [details] [diff] [review]:
-----------------------------------------------------------------

as I said in the other bug, technically correct, but this file is going away regardless, so not really worth to clean it.
Attachment #609118 - Flags: review?(mak77) → review+
Comment on attachment 609118 [details] [diff] [review]
(AAv1) Remove DEFAULT_BOOKMARKS leftover
[Checked in: Comment 57]

https://hg.mozilla.org/mozilla-central/rev/3db28151b12f
Attachment #609118 - Attachment description: (AAv1) Remove DEFAULT_BOOKMARKS leftover → (AAv1) Remove DEFAULT_BOOKMARKS leftover [Checked in: Comment 57]
Depends on: 753205
Depends on: 767776
No longer depends on: 767776
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: