Last Comment Bug 498596 - (SMPlacesBMarks) Switch SeaMonkey bookmarks to places backend
(SMPlacesBMarks)
: Switch SeaMonkey bookmarks to places backend
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- normal with 7 votes (vote)
: seamonkey2.1a3
Assigned To: Robert Kaiser
:
Mentors:
: 46078 599620 (view as bug list)
Depends on: 382187 528884 546942 547815 552444 553467 554855 557496 562339 573384 580656 580658 580660 580662 580663 583603 584752 586026 586363 586415 590714 591807 594127
Blocks: 479945 557504 573353 586840 141619 174778 200136 212605 226720 415373 481843 498111 505121 553459 554908 558189 559733 570788 573857 585500 585601 586050 586056 586947 588807 589592 589601 593728 618268
  Show dependency treegraph
 
Reported: 2009-06-16 03:42 PDT by Robert Kaiser
Modified: 2011-11-19 08:11 PST (History)
22 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wanted


Attachments
Screenshot of Seamonkey Bookmark Manager and Firefox Places (218.90 KB, image/png)
2009-11-16 09:26 PST, Matt A. Tobin [:BinOC]
no flags Details
Part 1, v1: add services uses by places bookmarks (157.46 KB, patch)
2010-03-14 09:57 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
parts 1&2, v1: places core files (366.28 KB, patch)
2010-03-18 18:16 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 3, v1: glue (32.93 KB, patch)
2010-03-18 18:18 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 4, v1: browser UI (143.72 KB, patch)
2010-03-18 18:19 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 5, v1: management UI (245.57 KB, patch)
2010-03-18 18:21 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 6, v1: remove old, obsolete files (419.86 KB, patch)
2010-03-18 18:26 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 1, v1.1: service/module (118.10 KB, patch)
2010-04-18 08:34 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 2, v1.1: places core (250.10 KB, patch)
2010-04-18 08:35 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 3, v1.1: glue (34.68 KB, patch)
2010-04-18 08:36 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 4, v1.1: browser UI (148.11 KB, patch)
2010-04-18 08:37 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 5, v1.1: management UI (245.73 KB, patch)
2010-04-18 08:39 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 1, v1.2: module (56.45 KB, patch)
2010-05-28 05:57 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 1x, v1.2: transaction service (not for review, will be obsoleted) (59.60 KB, patch)
2010-05-28 05:58 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 2, v1.2: places core (241.98 KB, patch)
2010-05-28 06:03 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 3, v1.2: glue (35.28 KB, patch)
2010-05-28 06:04 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 4, v1.2: browser UI (158.74 KB, patch)
2010-05-28 06:10 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 5, v1.2: management UI (249.38 KB, patch)
2010-05-28 06:13 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 6, v1.2: remove old, obsolete files (423.96 KB, patch)
2010-05-28 06:28 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 1, v1.3: module (62.05 KB, patch)
2010-07-01 16:56 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 2, v1.3: places core (243.61 KB, patch)
2010-07-01 16:57 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 3, v1.3: glue (35.72 KB, patch)
2010-07-01 16:58 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 4, v1.3: browser UI (160.39 KB, patch)
2010-07-01 16:59 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
part 5, v1.3: management UI (251.43 KB, patch)
2010-07-01 17:00 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
diff-for-port script (1.06 KB, text/plain)
2010-07-29 11:09 PDT, Robert Kaiser
no flags Details
places-ports input file for diff-for-port (1.86 KB, text/plain)
2010-07-29 11:11 PDT, Robert Kaiser
no flags Details
places-ports input file for diff-for-port (1.95 KB, text/plain)
2010-07-29 11:15 PDT, Robert Kaiser
no flags Details

Description Robert Kaiser 2009-06-16 03:42:10 PDT
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.
Comment 1 Matt A. Tobin [:BinOC] 2009-11-16 07:34:08 PST
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
Comment 2 Igor Velkov 2009-11-16 08:56:21 PST
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.
Comment 3 Matt A. Tobin [:BinOC] 2009-11-16 09:26:35 PST
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.
Comment 4 Robert Kaiser 2009-11-30 09:38:32 PST
(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.
Comment 5 Robert Kaiser 2010-02-18 07:54:00 PST
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.
Comment 6 Robert Kaiser 2010-02-24 16:30:18 PST
As I've started some work on this, I better reassign it.
Comment 7 Robert Kaiser 2010-03-11 18:15:30 PST
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.
Comment 8 Robert Kaiser 2010-03-12 07:50:43 PST
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.
Comment 9 Robert Kaiser 2010-03-14 09:57:26 PDT
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.
Comment 10 Marco Bonardo [::mak] 2010-03-15 06:19:12 PDT
(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.
Comment 11 Marco Bonardo [::mak] 2010-03-15 06:30:25 PDT
ps: imo transactions service should not even be a service, but a simple module in browser/components/places, used as an UI helper.
Comment 12 Robert Kaiser 2010-03-15 06:52:26 PDT
(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 13 Robert Kaiser 2010-03-15 10:19:03 PDT
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.
Comment 14 Robert Kaiser 2010-03-18 14:56:03 PDT
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
Comment 15 Robert Kaiser 2010-03-18 18:16:36 PDT
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.
Comment 16 Robert Kaiser 2010-03-18 18:18:21 PDT
Created attachment 433475 [details] [diff] [review]
part 3, v1: glue

Here's the patch for the glue, including import and backups.
Comment 17 Robert Kaiser 2010-03-18 18:19:55 PDT
Created attachment 433477 [details] [diff] [review]
part 4, v1: browser UI

This part 4 has the browser UI, I also could get bookmarks keywords to work now by copying Firefox' getShortcutOrURI function.
Comment 18 Robert Kaiser 2010-03-18 18:21:35 PDT
Created attachment 433478 [details] [diff] [review]
part 5, v1: management UI

This part has the editing UI for bookmark based on places, including the new bookmarks manager.
Comment 19 Robert Kaiser 2010-03-18 18:26:43 PDT
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.
Comment 21 Robert Kaiser 2010-03-19 10:55:54 PDT
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.
Comment 24 Robert Kaiser 2010-04-02 18:24:07 PDT
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.
Comment 25 Igor Velkov 2010-04-06 22:37:04 PDT
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.
Comment 26 Robert Kaiser 2010-04-18 08:34:41 PDT
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.
Comment 27 Robert Kaiser 2010-04-18 08:35:50 PDT
Created attachment 439781 [details] [diff] [review]
part 2, v1.1: places core
Comment 28 Robert Kaiser 2010-04-18 08:36:28 PDT
Created attachment 439782 [details] [diff] [review]
part 3, v1.1: glue
Comment 29 Robert Kaiser 2010-04-18 08:37:09 PDT
Created attachment 439783 [details] [diff] [review]
part 4, v1.1: browser UI
Comment 30 Robert Kaiser 2010-04-18 08:39:46 PDT
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.
Comment 31 Robert Kaiser 2010-04-18 13:58:39 PDT
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
Comment 32 tymerkaev 2010-04-19 00:15:05 PDT
(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.
Comment 33 Robert Kaiser 2010-05-28 05:57:18 PDT
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.
Comment 34 Robert Kaiser 2010-05-28 05:58:33 PDT
Created attachment 447978 [details] [diff] [review]
part 1x, v1.2: transaction service (not for review, will be obsoleted)
Comment 35 Robert Kaiser 2010-05-28 06:03:24 PDT
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.
Comment 36 Robert Kaiser 2010-05-28 06:04:13 PDT
Created attachment 447983 [details] [diff] [review]
part 3, v1.2: glue
Comment 37 Robert Kaiser 2010-05-28 06:10:29 PDT
Created attachment 447986 [details] [diff] [review]
part 4, v1.2: browser UI
Comment 38 Robert Kaiser 2010-05-28 06:13:02 PDT
Created attachment 447987 [details] [diff] [review]
part 5, v1.2: management UI

As a side note, I should also have fixed most bugs that have been reported/seen in previous versions of this.
Comment 39 Robert Kaiser 2010-05-28 06:28:19 PDT
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 ;-)
Comment 40 Robert Kaiser 2010-05-28 06:51:45 PDT
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 41 neil@parkwaycc.co.uk 2010-06-02 14:01:40 PDT
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 42 neil@parkwaycc.co.uk 2010-06-02 15:54:51 PDT
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 43 neil@parkwaycc.co.uk 2010-06-02 16:37:53 PDT
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 :-(
Comment 44 Robert Kaiser 2010-06-08 13:01:55 PDT
(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?
Comment 45 Tom Schneider 2010-06-15 15:05:11 PDT
(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!
Comment 46 Robert Kaiser 2010-06-15 18:21:19 PDT
(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.
Comment 47 Tom Schneider 2010-06-15 20:09:15 PDT
(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.
Comment 48 :Ms2ger (⌚ UTC+1/+2) 2010-06-16 05:02:43 PDT
Perhaps the documentation at <https://developer.mozilla.org/en/Places> will help you.
Comment 49 neil@parkwaycc.co.uk 2010-06-19 16:44:51 PDT
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.
Comment 50 Robert Kaiser 2010-06-20 07:17:56 PDT
(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.
Comment 51 Robert Kaiser 2010-06-20 07:53:20 PDT
(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.
Comment 52 Tom Schneider 2010-06-20 08:42:15 PDT
(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.)
Comment 53 Robert Kaiser 2010-06-20 08:47:32 PDT
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.
Comment 54 Robert Kaiser 2010-07-01 16:56:41 PDT
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.
Comment 55 Robert Kaiser 2010-07-01 16:57:37 PDT
Created attachment 455593 [details] [diff] [review]
part 2, v1.3: places core
Comment 56 Robert Kaiser 2010-07-01 16:58:16 PDT
Created attachment 455596 [details] [diff] [review]
part 3, v1.3: glue
Comment 57 Robert Kaiser 2010-07-01 16:59:02 PDT
Created attachment 455597 [details] [diff] [review]
part 4, v1.3: browser UI
Comment 58 Robert Kaiser 2010-07-01 17:00:35 PDT
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. ;-)
Comment 59 erpman1 2010-07-09 10:49:16 PDT
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.
Comment 60 Robert Kaiser 2010-07-09 11:24:11 PDT
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 61 Robert Kaiser 2010-07-21 09:32:00 PDT
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 62 Robert Kaiser 2010-07-21 09:37:59 PDT
Comment on attachment 455596 [details] [diff] [review]
part 3, v1.3: glue

Bug 580658 covers glue now.
Comment 63 Robert Kaiser 2010-07-21 09:41:42 PDT
Comment on attachment 455597 [details] [diff] [review]
part 4, v1.3: browser UI

The browser UI changes are in bug 580660 now.
Comment 64 Robert Kaiser 2010-07-21 09:46:34 PDT
Comment on attachment 455598 [details] [diff] [review]
part 5, v1.3: management UI

Management UI moved over to bug 580662.
Comment 65 Robert Kaiser 2010-07-21 09:49:29 PDT
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.
Comment 66 Robert Kaiser 2010-07-29 11:09:51 PDT
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.
Comment 67 Robert Kaiser 2010-07-29 11:11:18 PDT
Created attachment 461272 [details]
places-ports input file for diff-for-port
Comment 68 Robert Kaiser 2010-07-29 11:15:34 PDT
Created attachment 461274 [details]
places-ports input file for diff-for-port

Just noticed that the input file did miss PlacesUIUtils.
Comment 69 Robert Kaiser 2010-08-08 13:15:27 PDT
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.
Comment 70 Robert Kaiser 2010-08-10 12:19:03 PDT
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.
Comment 71 Bruno 'Aqualon' Escherl 2010-09-25 11:36:01 PDT
*** Bug 599620 has been marked as a duplicate of this bug. ***
Comment 72 Philip Chee 2011-11-19 08:11:42 PST
*** Bug 46078 has been marked as a duplicate of this bug. ***

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