We should switch the SeaMonkey bookmarks UI to using places, import all bookmarks from the old backend into places, and then kill off the old backend.
This will enable the options (nobody is forced to use those) to have tags for bookmarks, feed-driven live bookmarks, and keyword, tag and bookmarks search results from the location bar if people want those.

Created attachment 412621[details]
Screenshot of Seamonkey Bookmark Manager and Firefox Places
What I mean is it seems that the firefox places ui is theme independent... Notice the back buttons and the toolbar background. If it was directly ported and to seamonkey it would like the toolkit addon manager be yet another inconsistent ui. All I was trying to point out that its a great idea however the ui namely the toolbar must be seamonkeyized so that it will fit into the suite's ui. Also since places is much more then a simple bookmark manager it stands to reason that it also needs a menu bar and a status bar possibly including a component-bar overlay into the status bar.
This would provide the advanced functionality of places without the cost of an inconsistent ui.

(In reply to comment #1)
> I am in favor of this change as long as the look and feel of the bookmarks
> manager remains consistent with the seamonkey ui.
That is the plan, though slight changes might be in order to integrate possible new features - but we want to stay similar to the current "old" bookmarks UI while using the new backend.

The work was getting scarily large even without yet doing bookmarks management UI or removing old files, so I decided to break this apart into a series of smaller patches. My current WIP state with all those created in some way is in my MQ, which is being backed up to http://hg.mozilla.org/users/kairo_kairo.at/comm-central-patches/file/tip at random intervals.
Those are the parts/stages I decided to split this into:
Part 1: Places services
Add the microsummary and places transactions services, along with a directory
provider change needed by the microsummary service.
Straight port from Firefox, can be checked in before actively using it.
Part 2: Base files
Add controller.js, menu.xml, places.css, placesOverlay.xul, toolbar.xml,
treeView.js, utils.js and probably tree.xml into common/bookmarks with full
functionality.
Those are the core files used by all the places UI coming in later parts.
Straight port from Firefox, also can be checked in before actively using it.
Part 3: Glue
nsSuiteGlue.js changes and default prefs, including auto-import of old
bookmarks when no places bookmarks are saved yet.
Needs bug 546942, parts of the current patch could be pulled out into a
separate bug as they're not specific to places bookmarks, those that are only
make sense when the UI lands at the same time, though.
Part 4: Browser UI
Places bookmarks UI in the main browser window - bookmarks menu, toolbar
button and personal bookmarks toolbar, including context menus, etc. to
support those parts of UI.
Contains calls to management UI but not those parts themselves. Only can land
with glue and management UI.
Part 5: Management UI
Bookmarks manager, and add bookmark UI
Must land together with glue and browser UI.
Part 6: Remove old code
Removes old bookmark code files from the tree, separated from the others just
to not clutter the other parts if possible and therefore ease review.
Can land at any time once the other patches have removed all uses of those
files and their being packaged into builds.
This cuts down individual patch sizes and should ease review, as well as possible make it possible to land in stages if wanted, as parts 1,2 and 6 can land at any time as long as they're in the right order. Only parts 3-5 definitely need to land together.
Parts 1, 2 and possibly 5 will probably be the larger parts, 1 and 2 are already almost 160K and 290K in their current WIP states (though any more changes there should be minor - with the exception of tree.xml, which I don't have in my tree yet as only the manager UI needs it).
This is just for your interest, so everyone here knows where I'm headed here.
Also, parts 1-4 are mostly done (apart from possibly minor glitches) and working well as far as they can, 5 and 6 haven't been really started though.

Oh, actually, depending on what Neil wants, there might be a 7th part of merging history and bookmarks core files (i.e. those in part 2 here), possibly moving them to a shared place like common/places, but not sure if that's best to do in part 2 here or in a followup. For now, my patches don't merge them yet.

Created attachment 432429[details][diff][review]
Part 1, v1: add services uses by places bookmarks
Here's the part 1 patch, adding the microsummaries and places transactions services to SeaMonkey. This code will only be actually used in later stages, the current patch is a relatively straight port from Firefox code.
Marco: I wonder if those services need to live on the application side, and why they don't live in toolkit, requesting feedback here to get a statement on that.

(In reply to comment #9)
> Marco: I wonder if those services need to live on the application side, and why
> they don't live in toolkit, requesting feedback here to get a statement on
> that.
Transactions service is mostly related to frontend. While it's true we could support transactions OOB in toolkit, it is something that is built on top of the backend and can act differently based on the implementer. Actually i think it is also a bad implementation, it is well tested but really slow due to its nature of acting on item ids (having to fire a bunch of queries just to gather some data that we already know). It should probably act on UI nodes instead (but we have a couple missing points to do the change, like being able to build a single bookmark node). An alternative could even be to refactor all the toolkit part, in such a case we could make something really cool like using sqlite named transactions. Unfortunatly this would mean create Places2, not backwards compatible with Places. Thus i don't think it will be moved to toolkit anytime soon.
As of microsummaries, i think it's in browser because it's a component that relies on Places and was initially build as an experimental feature. Probably this is also due to the fact Places initially was built thinking to something wider, not only related to history and bookmarks, thus all pieces only related to H&B were moved to browser. I never evaluated the microsummaries designs, so i guess you should ask Dietrich about moving them. They could be moved into toolkit/places/src folder imo.

(In reply to comment #11)
> ps: imo transactions service should not even be a service, but a simple module
> in browser/components/places, used as an UI helper.
Well, if there is a bug on doing that, I'll make sure to watch it. For now, I'll port what Firefox has, I don't pretend to know things well enough to completely refactor them... Thanks for your comments, I'll ask dietrich about the microsummary service.

I told Neil that my bookmarks work and history could or should share the core places files, which is why I'll import the new files into a new common/places/ directory and in a later step will try to switch over history to use them as well.
I did diffs of those files in my development tree that has the new files, see below. "file" is the file name, "hist" is the line count in history/, "places" is the line count of the new, to-be-shared places/ file, "(+)" and "(-)" are the lines added and removed to go from the history/ implementation to the places/ one:
file hist places (+) (-)
controller.js 610 1655 1166 121
sidebarUtils.js 124 131 15 8
tree.xml 246 807 559 98
treeView.js 1141 1575 655 221
utils.js 358 1456 1141 43

Created attachment 433474[details][diff][review]
parts 1&2, v1: places core files
Here are the core places files, and as the transactions service is the only service left, I did incorporate it in the core patch, so this is "parts 1&2" now.
Most of this is a pretty straight port from Firefox.

Created attachment 433480[details][diff][review]
part 6, v1: remove old, obsolete files
This last patch removes the old, now-obsolete files. There is some calls from the search code that I could not fully resolve, but I think we may just leave them with my crude rip-out for the moment and go for a better replacement once we can enable the toolkit search service in bug 410613.

Work like a charm.
I enable bookmark sync in weave addon, and have synced bookmarks between seamonkey and firefoxes.
Browse with tis build for a 2 hour, add, move, tag, rename and edit description bookmark.
All ok.

Created attachment 439780[details][diff][review]
part 1, v1.1: service/module
Here's an updated series, including a number of changes that happened on the Firefox side in the last month.
As PlacesUIUtils moved to a module, I've split up parts 1 and 2 again, making part 1 contain the transaction service and the utils module, part 2 is the rest of the places core, as originally imagined. I hope this eases review a bit.

Created attachment 439784[details][diff][review]
part 5, v1.1: management UI
As before, parts 1 and 2 can land without the rest, basically independently.
Parts 3 through 5 need to land together or something is broken for users.
Part 6 has no changes over v1, it's just removal of what's dead afterwards, it also can land independently after all other parts.

Created attachment 447977[details][diff][review]
part 1, v1.2: module
Here comes the series that should be ready for review!
The first part is being split into the PlacesUIUtils module part for review and a transactions service part that's not for review but just testing (bug 553467 moves it into a toolkit module that we'll reuse), all the other parts stay the same in concept.

Created attachment 447982[details][diff][review]
part 2, v1.2: places core
Places core now has the deXBLified menu/toolbar code and is diffed against history as far as possible. History should be driven by this and the module in the future, but I'll only switch it over in a followup.

Created attachment 447991[details][diff][review]
part 6, v1.2: remove old, obsolete files
The removal stuff hasn't changed much since the beginning, and I guess is the easiest to review despite its large size ;-)

If you run into problems applying any of the patches, it might be because I have the bug 423282 patch in my mq before those. I hope that will get reviews and land soon, and then everything here should apply fine (I hope).

Comment on attachment 447977[details][diff][review]
part 1, v1.2: module
>+EXTRA_PP_JS_MODULES = \
Why PP?
>+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>+
>+XPCOMUtils.defineLazyGetter(this, "Services", function() {
>+ Components.utils.import("resource://gre/modules/Services.jsm");
>+ return Services;
>+});
That's just too lazy ;-) Just import Services.jsm directly. After all, it's already compiled by utilityOverlay.js, so we're not gaining anything.
>+XPCOMUtils.defineLazyGetter(this, "PlacesUtils", function() {
>+ Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
>+ return PlacesUtils;
> });
Not convinced that this is too lazy, I think I moaned at someone before for something similar, but I forget now what the result was.
>+const ORGANIZER_LEFTPANE_VERSION = 6;
So, we're getting a left pane?
>+ function getChildItemsTransactions(aChildren) {
[Why does this exist when it only has one caller?]
[Note that Places abuses the transaction manager, since it relies on the transactions batching themselves, instead of getting the tm to do it.]
>+ if (aMinimalUI)
>+ features = "centerscreen,chrome,dialog,resizable,modal";
>+ else
>+ features = "centerscreen,chrome,modal,resizable=no";
Nit: inconsistent. First version should probably say
"centerscreen,chrome,modal,resizable=yes"
>+ _getCurrentActiveWin: function PUIU__getCurrentActiveWin() {
>+ return this.fm.activeWindow;
Which is actually *less* to type than _getCurrentActiveWin() ...
>+ // XXXmano: somehow we reach the xul document here!
Well, it is the parent node of the root element...
>+ checkURLSecurity: function PUIU_checkURLSecurity(aURINode) {
>+ if (!PlacesUtils.nodeIsBookmark(aURINode)) {
Would look nicer written as
if (PlacesUtils.nodeIsBookmark(aURINode))
return true;
>-delete.hostname.true=Delete History for %S
>-delete.hostname.false=Delete History for Website
>-delete.hostname.accesskey=s
>-delete.domain.true=Delete History for *.%S
>-delete.domain.false=Delete History for Domain
>-delete.domain.accesskey=H
So, what happens to these?

(In reply to comment #41)
> >+XPCOMUtils.defineLazyGetter(this, "PlacesUtils", function() {
> >+ Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
> >+ return PlacesUtils;
> > });
> Not convinced that this is too lazy, I think I moaned at someone before for
> something similar, but I forget now what the result was.
mak says this one "should be fine lazy instead", and given that this is a larger module and PlacesUIUtils being loaded on browser load, it probably is good to be loaded lazily.
> >+const ORGANIZER_LEFTPANE_VERSION = 6;
> So, we're getting a left pane?
That's what I'm planning, yes, though I want to follow up in a way that possibly can make it possible to collapse/hide it.
> >+ function getChildItemsTransactions(aChildren) {
> [Why does this exist when it only has one caller?]
> [Note that Places abuses the transaction manager, since it relies on the
> transactions batching themselves, instead of getting the tm to do it.]
There are plans for reducing suckiness of places transactions after bug 553467, I hope this will deal with your problems there.
mak says: "that function is large enough to have its own method... sure the transactions stuff needs love, first iteration won't change suckiness, but next ones should... btw, as it is i'd not inline that large util"
> >+ if (aMinimalUI)
> >+ features = "centerscreen,chrome,dialog,resizable,modal";
> >+ else
> >+ features = "centerscreen,chrome,modal,resizable=no";
> Nit: inconsistent. First version should probably say
> "centerscreen,chrome,modal,resizable=yes"
mak says: "I have a patch up somewhere that remakes all of that code using a dependent window, but our mac code does not have them, so I'll probably have to convert all dialogs to panels (if only they'd work as expected); getCurrentActiveWin is bogus and should go away (same patch as above); that is bug 562998"
> >+ _getCurrentActiveWin: function PUIU__getCurrentActiveWin() {
> >+ return this.fm.activeWindow;
> Which is actually *less* to type than _getCurrentActiveWin() ...
I'd like to leave that for the moment and then port the changes mentioned above when they happen on the Firefox side, is that acceptable?
> >+ // XXXmano: somehow we reach the xul document here!
> Well, it is the parent node of the root element...
I'll change the comment to reflect that, then ;-)
> >-delete.hostname.true=Delete History for %S
> >-delete.hostname.false=Delete History for Website
> >-delete.hostname.accesskey=s
> >-delete.domain.true=Delete History for *.%S
> >-delete.domain.false=Delete History for Domain
> >-delete.domain.accesskey=H
> So, what happens to these?
They may come back with bug 560111 but as long as it's bookmarks only, they wouldn't be used here, and I want to save localizers the work to copy them over for now.
> >+ return aUrlString.substr(0, aUrlString.indexOf(":"));
> [Or you can use replace or split or ... !]
Is any of those better (faster, shorter) than the variant here?
I have incorporated all other mentioned changes from the last 3 comments in my local patches and will attach a patch in bug 570788 to match the Firefox side as well.
Should I upload new versions with that for both patches you have commented on or only for the first?

(In reply to comment #0)
> We should switch the SeaMonkey bookmarks UI to using places,
For those of use new to this issue, could you explain what 'places' is
and/or make a pointer to it? Then it would be easier to see if
bug 571966 and bug 571965 will be solved by the changes.
Thanks!

(In reply to comment #45)
> For those of use new to this issue, could you explain what 'places' is
> and/or make a pointer to it?
Not in this bug. http://home.kairo.at/blog/tags/places should give a few pointers, for example.
This bug is about doing that switch, not about explaining it or discussing if it should be done. Please only comment here if you can help getting this done.

(In reply to comment #49)
> (From update of attachment 447983[details][diff][review])
> >-pref("browser.chrome.image_icons.max_size", 1024);
> Removed by mistake?
No, it's set to that value in all.js anyhow, and Firefox also uses the value from there, no need to have it in browser-prefs.js as well.
> >- this._onProfileShutdown();
> Why did this move?
Because it calls the final places shutdown, and I guess we want to make sure this is done when we remove the observer on places-shutdown.
I copied this from how Firefox does it, IIRC.
> >+ * Initialize Places
> [Whoa, that's a lot of work... will skim later]
Yup, those functions are mainly why I did put this in a separate patch.
> >+ callback: function(aNotificationBar, aButton) {
> >+ browser.selectedTab = browser.addTab(url);
> This needs to open Help.
Ah, right, the version in the patch tries to open SUMO, which isn't what we want, of course. Hmm, that means I'll need to write up a help topic as well, I guess - but that will be done in a followup.

(In reply to comment #50)
> Hmm, that means I'll need to write up a help topic as well, I
> guess - but that will be done in a followup.
Filed bug 573353 - I still need to test the local fix to call this topic from there, but I need to figure out how to lock the DB so I can test this.

(In reply to comment #48)
> Perhaps the documentation at <https://developer.mozilla.org/en/Places> will
> help you.
Yes, thanks. I worry that I will lose functionality I rely on. There seems to be no overall description of what the system will look like to a user. Where is that? (sorry for the interruption folks - something this important should be well documented, and I was just getting lost in that and other pages.)

Tom, please keep this out of this bug, and read the blog entries, if you like them or not, they contain the information you are looking for. And, no, please not even a comment that you understand. Any comment that doesn't help solving this bug is unwelcome here.

Created attachment 455592[details][diff][review]
part 1, v1.3: module
Here's a new patch series, with all changes Firefox has seen in the mean time, including the move of the transaction service to toolkit (i.e. we don't need to add it any more but use it from there), and with all comments so far addressed. I'm also spinning new try builds from this.

Created attachment 455598[details][diff][review]
part 5, v1.3: management UI
I'm not attaching a new version of part 6, as it didn't change at all. Removing the old stuff is the easy part, after all. ;-)

Hi Robert. I got a chance to test out the recent "try builds" mentioned on your blog and Places seem to work well (made comments on your blog page as EP). Only problem though minor is the "More" and "Show all tags" buttons which display like this "======" when selecting bookmarks in the places window [actually the buttons are shaped like a horizontal "pipeline"] which are somewhat ugly. In Firefox, those buttons in the Places window are shaped like a downward carrot or like the letter "V".
hopefully you can make a cosmetic change to some of the buttons in Places for Seamonkey. Other than that, everything else with Places is functional.

Thanks for testing, and please keep any comments that are not about review of the actual patches here to other places like my blog or the newsgroup, as this bug report is getting large enough with just the actual work and reviews alone. For any ugly buttons or such things, let's leave that to followups.

Comment on attachment 447991[details][diff][review]
part 6, v1.2: remove old, obsolete files
Finally, bug 580663 now covers the removal of old files obsoleted by this work.
I hope reviews can progress more easily with that split into smaller bugs where comments can care about one things only.
This bug here will continue to serve as the tracker for places bookmarks work.

Created attachment 461271[details]
diff-for-port script
I've written a small, quick-and-dirty shell script for doing diffs of the original files to the ported ones on our side.
I don't know where to put this script and its input file right now, so I'll put it on here, I'll attach its output for reference in the relevant bugs.
Given you put the script into the directory below a comm-central checkout with the patches applied, then this is how to invoke it:
> cd comm-central
> ../diff-for-port ../places-ports.txt
The output goes into places-core.diff and places-manage-ui.diff inside the comm-central dir. As I said, it's quick and dirty, but helpful.

The whole set of patches has been pushed, I'm waiting for it to stick now - and hope we'll still get the Modern changes in bug 584752 in for 2.1a3, I just finished and attached those before I checked in the rest.