Bug 498596 (SMPlacesBMarks)

Switch SeaMonkey bookmarks to places backend

RESOLVED FIXED in seamonkey2.1a3

Status

defect
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: kairo, Assigned: kairo)

Tracking

(Blocks 4 bugs)

SeaMonkey Tracking Flags

(seamonkey2.1 wanted)

Details

Attachments

(2 attachments, 25 obsolete attachments)

1.06 KB, text/plain
Details
1.95 KB, text/plain
Details
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.
Flags: wanted-seamonkey2.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. 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.
Depends on: 546942
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.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
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.
Attachment #412621 - Attachment is obsolete: true
Attachment #432429 - Flags: review?(neil)
Attachment #432429 - Flags: feedback?(mak77)
(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.
Attachment #432429 - Flags: feedback?(mak77)
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.
Depends on: 552444
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.
Attachment #432429 - Attachment is obsolete: true
Attachment #432429 - Flags: review?(neil)
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
Depends on: 553459
Blocks: 553459
No longer depends on: 553459
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.
Attachment #433474 - Flags: review?(neil)
Posted patch part 3, v1: glue (obsolete) — Splinter Review
Here's the patch for the glue, including import and backups.
Attachment #433475 - Flags: review?(neil)
Posted patch part 4, v1: browser UI (obsolete) — Splinter Review
This part 4 has the browser UI, I also could get bookmarks keywords to work now by copying Firefox' getShortcutOrURI function.
Attachment #433477 - Flags: review?(neil)
Posted patch part 5, v1: management UI (obsolete) — Splinter Review
This part has the editing UI for bookmark based on places, including the new bookmarks manager.
Attachment #433478 - Flags: review?(neil)
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.
Attachment #433480 - Flags: review?(neil)
Attachment #433474 - Attachment description: parts 1&2: places core files → parts 1&2, v1: places core files
Attachment #433475 - Attachment description: part 3: glue → part 3, v1: glue
Attachment #433477 - Attachment description: part 4: browser UI → part 4, v1: browser UI
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.
Blocks: 479945
Depends on: 554855
Blocks: 554908
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.
Blocks: 415373
Blocks: 557496
Blocks: 557504
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.
Alias: SMPlacesBMarks
Blocks: 558189
Blocks: 560111
Posted patch part 1, v1.1: service/module (obsolete) — Splinter Review
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.
Attachment #433474 - Attachment is obsolete: true
Attachment #439780 - Flags: review?(neil)
Attachment #433474 - Flags: review?(neil)
Posted patch part 2, v1.1: places core (obsolete) — Splinter Review
Attachment #439781 - Flags: review?(neil)
Posted patch part 3, v1.1: glue (obsolete) — Splinter Review
Attachment #433475 - Attachment is obsolete: true
Attachment #439782 - Flags: review?(neil)
Attachment #433475 - Flags: review?(neil)
Posted patch part 4, v1.1: browser UI (obsolete) — Splinter Review
Attachment #439783 - Flags: review?(neil)
Posted patch part 5, v1.1: management UI (obsolete) — Splinter Review
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.
Attachment #433477 - Attachment is obsolete: true
Attachment #433478 - Attachment is obsolete: true
Attachment #439784 - Flags: review?(neil)
Attachment #433477 - Flags: review?(neil)
Attachment #433478 - Flags: review?(neil)
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.
Blocks: 559733
Flags: wanted-seamonkey2.1+
Blocks: 481843
Depends on: 562339
Depends on: 528884, 553467
Depends on: 547815
Posted patch part 1, v1.2: module (obsolete) — Splinter Review
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.
Attachment #439780 - Attachment is obsolete: true
Attachment #447977 - Flags: review?(neil)
Attachment #439780 - Flags: review?(neil)
Posted patch part 2, v1.2: places core (obsolete) — Splinter Review
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.
Attachment #439781 - Attachment is obsolete: true
Attachment #447982 - Flags: review?(neil)
Attachment #439781 - Flags: review?(neil)
Posted patch part 3, v1.2: glue (obsolete) — Splinter Review
Attachment #439782 - Attachment is obsolete: true
Attachment #447983 - Flags: review?(neil)
Attachment #439782 - Flags: review?(neil)
Posted patch part 4, v1.2: browser UI (obsolete) — Splinter Review
Attachment #439783 - Attachment is obsolete: true
Attachment #447986 - Flags: review?(neil)
Attachment #439783 - Flags: review?(neil)
Posted patch part 5, v1.2: management UI (obsolete) — Splinter Review
As a side note, I should also have fixed most bugs that have been reported/seen in previous versions of this.
Attachment #439784 - Attachment is obsolete: true
Attachment #447987 - Flags: review?(neil)
Attachment #439784 - Flags: review?(neil)
The removal stuff hasn't changed much since the beginning, and I guess is the easiest to review despite its large size ;-)
Attachment #433480 - Attachment is obsolete: true
Attachment #447991 - Flags: review?(neil)
Attachment #433480 - Flags: review?(neil)
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 :-(
Blocks: 570788
(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?
Blocks: 200136
(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.
Blocks: 573353
(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.
Depends on: 573384
Posted patch part 1, v1.3: module (obsolete) — Splinter Review
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.
Attachment #447977 - Attachment is obsolete: true
Attachment #447978 - Attachment is obsolete: true
Attachment #455592 - Flags: review?(neil)
Attachment #447977 - Flags: review?(neil)
Posted patch part 2, v1.3: places core (obsolete) — Splinter Review
Attachment #447982 - Attachment is obsolete: true
Attachment #455593 - Flags: review?(neil)
Attachment #447982 - Flags: review?(neil)
Posted patch part 3, v1.3: glue (obsolete) — Splinter Review
Attachment #455596 - Flags: review?(neil)
Posted patch part 4, v1.3: browser UI (obsolete) — Splinter Review
Attachment #447983 - Attachment is obsolete: true
Attachment #447986 - Attachment is obsolete: true
Attachment #455597 - Flags: review?(neil)
Attachment #447983 - Flags: review?(neil)
Attachment #447986 - Flags: review?(neil)
Posted patch part 5, v1.3: management UI (obsolete) — Splinter Review
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. ;-)
Attachment #447987 - Attachment is obsolete: true
Attachment #455598 - Flags: review?(neil)
Attachment #447987 - Flags: review?(neil)
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.
Blocks: 573857
Depends on: 580656
Depends on: 580658
Depends on: 580660
Depends on: 580662
Depends on: 580663
Attachment #455592 - Attachment is obsolete: true
Attachment #455592 - Flags: review?(neil)
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.
Attachment #455593 - Attachment is obsolete: true
Attachment #455593 - Flags: review?(neil)
No longer blocks: 560111
No longer blocks: 557496
Depends on: 557496
Comment on attachment 455596 [details] [diff] [review]
part 3, v1.3: glue

Bug 580658 covers glue now.
Attachment #455596 - Attachment is obsolete: true
Attachment #455596 - Flags: review?(neil)
Comment on attachment 455597 [details] [diff] [review]
part 4, v1.3: browser UI

The browser UI changes are in bug 580660 now.
Attachment #455597 - Attachment is obsolete: true
Attachment #455597 - Flags: review?(neil)
Comment on attachment 455598 [details] [diff] [review]
part 5, v1.3: management UI

Management UI moved over to bug 580662.
Attachment #455598 - Attachment is obsolete: true
Attachment #455598 - Flags: review?(neil)
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.
Attachment #447991 - Attachment is obsolete: true
Attachment #447991 - Flags: review?(neil)
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.
Attachment #461272 - Attachment is obsolete: true
Depends on: 583603
Depends on: 584752
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.
Target Milestone: --- → seamonkey2.1a3
Blocks: 585500
Blocks: 585601
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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 586026
Depends on: 586363
Depends on: 586408
Depends on: 586415
No longer depends on: 586408
Blocks: 586840
Blocks: 226720
Blocks: 505121
Blocks: 589592
Blocks: 589601
Blocks: 586947
Depends on: 590714
Depends on: 591807
Blocks: 593728
Depends on: 594127
Duplicate of this bug: 599620
Blocks: 174778
Blocks: 618268
Duplicate of this bug: 46078
You need to log in before you can comment on or make changes to this bug.