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.

I am in favor of this change as long as the look and feel of the bookmarks manager remains consistent with the seamonkey ui. Not just porting the firefox ui over unchanged

Matt, do you see a real differences between seamonkey and current firefox bookmark manager interface? As I see now firefox uses seamonkey view and functional.

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.

I've started a first look into that but bug 546942 did hold me off from getting data imported to use further. Once I have data, I probably can look into the UI.

As I've started some work on this, I better reassign it.

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.

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.

ps: imo transactions service should not even be a service, but a simple module in browser/components/places, used as an UI helper.

(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.

Comment on attachment 432429 [details] [diff] [review] Part 1, v1: add services uses by places bookmarks OK, obsoleting this for the moment, I filed bug 552444 for moving the microsummary service as Dietrich has agreed we should do that.

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

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.

Here's the patch for the glue, including import and backups.

This part 4 has the browser UI, I also could get bookmarks keywords to work now by copying Firefox' getShortcutOrURI function.

This part has the editing UI for bookmark based on places, including the new bookmarks manager.

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.

I did run those patches through the try server, here are the builds: http://s3.mozillamessaging.com/build/try-server/2010-03-19_06:10-kairo@kairo.at-places-bookmarks/kairo@kairo.at-places-bookmarks-suite-try-linux.tar.bz2 http://s3.mozillamessaging.com/build/try-server/2010-03-19_06:10-kairo@kairo.at-places-bookmarks/kairo@kairo.at-places-bookmarks-suite-try-win32.zip http://s3.mozillamessaging.com/build/try-server/2010-03-19_06:10-kairo@kairo.at-places-bookmarks/kairo@kairo.at-places-bookmarks-suite-try-win32.installer.exe (Mac failed because of a probably unrelated problem, hope it will work with the next iteration of the patches, once I get some review feedback.)

Oh, shoot, try builds are flawed as they don't have the patches for bug 546942 and bug 552444. I'll do another round next week, maybe there's even some review, then.

The dependencies have landed, so I started another try run. Builds are here: http://s3.mozillamessaging.com/build/try-server/2010-03-24_06:20-kairo@kairo.at-bug498596-places-bookmarks/kairo@kairo.at-bug498596-places-bookmarks-suite-try-linux.tar.bz2 http://s3.mozillamessaging.com/build/try-server/2010-03-24_06:20-kairo@kairo.at-bug498596-places-bookmarks/kairo@kairo.at-bug498596-places-bookmarks-suite-try-mac.dmg http://s3.mozillamessaging.com/build/try-server/2010-03-24_06:20-kairo@kairo.at-bug498596-places-bookmarks/kairo@kairo.at-bug498596-places-bookmarks-suite-try-win32.zip http://s3.mozillamessaging.com/build/try-server/2010-03-24_06:20-kairo@kairo.at-bug498596-places-bookmarks/kairo@kairo.at-bug498596-places-bookmarks-suite-try-win32.installer.exe

OK, those try builds did miss packaging for the JS services, so I did another round that should have that as well: http://s3.mozillamessaging.com/build/try-server/2010-03-24_12:40-kairo@kairo.at-bug498596-places-bookmarks/kairo@kairo.at-bug498596-places-bookmarks-suite-try-linux.tar.bz2 http://s3.mozillamessaging.com/build/try-server/2010-03-24_12:40-kairo@kairo.at-bug498596-places-bookmarks/kairo@kairo.at-bug498596-places-bookmarks-suite-try-mac.dmg http://s3.mozillamessaging.com/build/try-server/2010-03-24_12:40-kairo@kairo.at-bug498596-places-bookmarks/kairo@kairo.at-bug498596-places-bookmarks-suite-try-win32.zip http://s3.mozillamessaging.com/build/try-server/2010-03-24_12:40-kairo@kairo.at-bug498596-places-bookmarks/kairo@kairo.at-bug498596-places-bookmarks-suite-try-win32.installer.exe I hope those finally work.

I did another try build with a few more fixes, but I don't want to spam everyone with those long URLs here any more. See http://home.kairo.at/blog/2010-04/places_bookmarks_another_try for links.

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.

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.

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.

New try builds with this patch set are available now as well: http://home.kairo.at/blog/2010-04/new_patches_and_try_builds_for_places_bo

(In reply to comment #31) > New try builds with this patch set are available now as well: > http://home.kairo.at/blog/2010-04/new_patches_and_try_builds_for_places_bo Everything is fine, but when I run "Import HTML..." SeaMonkey isn't responding. OS - Windows 7.

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.

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.

As a side note, I should also have fixed most bugs that have been reported/seen in previous versions of this.

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?

Comment on attachment 447977 [details] [diff] [review] part 1, v1.2: module >+ var pref = Services.prefs; >+ var prompt = Services.prompt; This doesn't make sense. Why not just use Services everywhere? >+ return aUrlString.substr(0, aUrlString.indexOf(":")); [Or you can use replace or split or ... !]

Comment on attachment 447982 [details] [diff] [review] part 2, v1.2: places core >+ isCommandEnabled: function PC_isCommandEnabled(aCommand) { ... > supportsCommand: function PC_supportsCommand(aCommand) { ... >- isCommandEnabled: function PC_isCommandEnabled(aCommand) { isCommandEnabled got moved when it shouldn't have :-(

(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 #46) > Not in this bug. fair enough > http://home.kairo.at/blog/tags/places It's too scrambled/blog-like to figure out.

Perhaps the documentation at <https://developer.mozilla.org/en/Places> will help you.

Comment on attachment 447983 [details] [diff] [review] part 3, v1.2: glue >-pref("browser.chrome.image_icons.max_size", 1024); Removed by mistake? >+ _isIdleObserver: false, >+ _isPlacesInitObserver: false, >+ _isPlacesLockedObserver: false, >+ _isPlacesShutdownObserver: false, s/is/has/ >+ _isPlacesDatabaseLocked: false, [But not this one!] >- this._onProfileShutdown(); Why did this move? >+ * Initialize Places [Whoa, that's a lot of work... will skim later] >+ callback: function(aNotificationBar, aButton) { >+ browser.selectedTab = browser.addTab(url); This needs to open Help.

(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.

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.

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 455593 [details] [diff] [review] part 2, v1.3: places core To ease reviews, I'm splitting off the patches into their own, smaller bugs. The new core is now in bug 580656.

Comment on attachment 455596 [details] [diff] [review] part 3, v1.3: glue Bug 580658 covers glue now.

Comment on attachment 455597 [details] [diff] [review] part 4, v1.3: browser UI The browser UI changes are in bug 580660 now.

Comment on attachment 455598 [details] [diff] [review] part 5, v1.3: management UI Management UI moved over to bug 580662.

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.

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.

Just noticed that the input file did miss PlacesUIUtils.

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.

Marking fixed after Modern has landed, but I've just been informed that we missed something on Mac theming. There will be a followup on that.