In Bug 1071513 we'll try to enable the new async Transactions in Nightly. Transactions are used by the UI to allow for undo/redo functionality in bookmarks, and so far they lived in PlacesUtils (see the various transaction objects). Moving towards async bookmarking, the old transactions can't work, since they are async, thus we have a new PlacesTransactions.jsm module that implements async transactions and uses the async bookmarking API internally. This requires fixing most of the ui code and tests to work asynchronously, and that's what we are doing in Bug 1071513. For Firefox 56, you could keep the browser.places.useAsyncTransactions pref to false, it will just keep working, though from 57 it's likely old APIs will start being removed.

async transactions are enabled in Nightly. if things work correctly, they may be enabled by default in 57, and after that point the old transactions code may go away. Also the old synchronous bookmarking API (or a good chunk of it) may go away since transactions were the biggest consumer, along with Sync.

Thanks for the heads up. With all the changes in Fx 57 we will need some time for fixing up SeaMonkey anyway so go ahead.

Not blocking the feature, but depends on it.

FYI, we plan to enable the feature in 58, and legacy code removal will likely happen in 59.

The patches are currently at 2.53 level and considered alpha quality. Part 1 add recentwindow.jsm to make things easier if you are only in the browser part.

Part 2 move over the bookmarks files to places.

Part 3 The funny part. Replaces the Bookmarks Manager with the Fx library. Only tested on Windows and 2.53 for now. Need to adapt to 2.57 and pull in the latest changes. Things to check/do: - private browsing: does privatebrowsingutils work in the browser part or should gPrivate be used. - Left in parts we don't have. - css check - Linux - OSX - Breaking up the patch into smaller parts. Ideas welcome.

Missed the new tag.png for osx

Minor cleanups

Some new fixes just to keep track. Lightly tested with async bookmarks transactions set in prefs. Works so far. Could probably be considered beta for Windows. I will check out Linux and osx over the weekend. All patches need Bug 1433962 in the tree and a minor rebase because I have some other patches in my local tree.

Tested with 2.53 under Windows Linux and osx with both sync and async paths. For 2.57 only async will remain in the code. Might need some minor rebasing because of other patches in my local tree.

Tests likely broken.

Example required l10n changes for de. Not sure how we should handle access keys in the final 2.57 patch. Keep current ones or align with Fx if this doesn't clash with existing keys.

Some small tweaks

Bleeding edge comm-central version part 1

Bleeding edge comm-central version part 2 I moved browser-places.js to browser. There is nothing in its final version suitable for mail or news.

Rebased patch from 2.53 / 56

Essentially the changes between 56 and 60 backported. I still miss 2 or 3 non essential ones but everything seems to work fine now. Without the sidebar and mail/news in 2.57 I was only able to test the browser part.

history icons fixed and css cleaned up. 2.53 version

"Final" versions for part 1 to 6. Lets discuss it as the meeting.

Port Bug 1423896 "Make the All Bookmarks folder for the left pane a virtual query".

Port Bug 1423896 "Make the All Bookmarks folder for the left pane a virtual query". Second part.

osx build fix. Missed filters.svg

Took out one location variable too many which broke smart bookmark recreation in nsSuiteGlue.js

Rebased after Bug 1436605 landed.

Rebased after Bug 1436605 landed.

Comment on attachment 8948123 [details] [diff] [review] 1378089-part1-recentwindow-253.patch LGTM r=me

Comment on attachment 8951973 [details] [diff] [review] 1378089-part1-recentwindow-257.patch LGTM r=me

Comment on attachment 8955773 [details] [diff] [review] 1378089-part2-bookmarks-257.patch LGTM r=me

Comment on attachment 8948124 [details] [diff] [review] 1378089-part2-bookmarks-253.patch LGTM r=me

Comment on attachment 8953460 [details] [diff] [review] 1378089-part3-bookmarks-253.patch f=me though we will need some fixes or follow-ups. When I start the manager I get the following error: Timestamp: 3/4/18, 8:07:16 PM GMT Warning: ReferenceError: reference to undefined property 0 Source File: chrome://communicator/content/places/treeView.js Line: 354 and when I start from Go -> History I get that and also: Timestamp: 3/4/18, 8:09:21 PM GMT Warning: ReferenceError: reference to undefined property 0 Source File: chrome://communicator/content/places/treeView.js Line: 276 The UI is missing the column selection widget (yes, right click on any column does give you that menu but would be nice to keep the widget) The column selection widget is missing keyword as an option but also the "Restore Column Order" option. In the History part, if you select multiple pages and then try to Bookmark them, nothing happens, no error either (doesn't work in Firefox either). The context menu for the History items, is missing some items so for a link from the site survey.example.com, I'd expect: Delete Delete History for survey.example.com Delete History for *.example.com Select All Instead I get: Delete Page Forget About This Site Do we want to add Delete History ones and Select All back in?

Thanks for the feedback. > When I start the manager I get the following error: If using 2.53 could you add in suite\browser\browser-prefs.js: // Whether useAsyncTransactions is enabled or not. // Currently we only enable them for nightly. #ifdef NIGHTLY_BUILD pref("browser.places.useAsyncTransactions", true); #else pref("browser.places.useAsyncTransactions", false); #endif Missed that. Both codepaths are tested and should work. sync codepatch and the pref is removed in 2.57. > The UI is missing the column selection widget (yes, right click on any column does give you that menu but would be nice to keep the widget) Will check it. > The context menu for the History items, is missing some items so for a link from the site survey.example.com, I'd expect: No longer works. Already severely broken in 2.49.2. See also Bug 1246424. > Select All back in? I don't miss it. We have clear browsing history in the Preferences. Also in clear private data. Will look at what can be done soon.

FWIW, the existing bookmarks manager in SeaMonkey is already a port of the Firefox library, but with a good amount of changes (and cutting out overlap with history). Of course that was a now pretty old version we based this on, from when places was still very young. That said, I'm all for reducing the diffs between SeaMonkey and Firefox (and Thunderbird), the amount of separate code that SeaMonkey has right now is unsustainable for a team as small as the SeaMonkey one.

(In reply to Robert Kaiser from comment #42) > FWIW, the existing bookmarks manager in SeaMonkey is already a port of the > Firefox library, but with a good amount of changes (and cutting out overlap > with history). Of course that was a now pretty old version we based this on, > from when places was still very young. That said, I'm all for reducing the > diffs between SeaMonkey and Firefox (and Thunderbird), the amount of > separate code that SeaMonkey has right now is unsustainable for a team as > small as the SeaMonkey one. As long as what you suggest does not result in losing functionality and reducing the user's ability to handle their own data as it used to be. FF seems to reduce as much choices from the user as they can, and SeaMonkey should not follow that path.

(In reply to Sanny from comment #43) > As long as what you suggest does not result in losing functionality and > reducing the user's ability to handle their own data as it used to be. FF > seems to reduce as much choices from the user as they can, and SeaMonkey > should not follow that path. I'm pretty sure the team would appreciate your help in this developing and porting work. If you can't contribute, the very small team will have to do with the resources they have and users will need to live with the necessary decision to reduce the work load.

browser-prefs.js: Added the missing prefs utilityOverlay.js: whereToOpenLink was duplicated and needed to be merged as per IRC openUILinkIn missed the private parameter so it was not possible to open a private browsing window from the Library. I will look at the column selection widget during the weekend. If it can be re-added I will put it in a patch on top. Reluctant to bring back the keyword column but will look at it too :)

2.57 version pref not needed. Found a new problem here. Doubleclicking a folder from the all bookmarks selection does not open it. Need to check if it is a bug or feature. Already missing a few subsequent patches. Works fine with any other item from the left sidebar including bookmarks menu and folders.

Rebased 2.57 patch. Unused /suite/themes/classic/communicator/bookmarks/bookmarksWindow.css removed. r+ from iann_bugzilla retained.

Rebased patch for latest changes in comm-central / comm-beta. This needs to go in because it blocks further development. Issues mentioned: - The UI is missing the column selection widget - keywords missing should be addressed in followup patches. The patches already need followups because of api changes. I will file additional bugs later.

rebased patch

2.53 only. r+ from IanN retained in case we do an interim release. Unused /suite/themes/classic/communicator/bookmarks/bookmarksWindow.css removed.

2.53 only. Minor fix for the async path and rebased.

Comment on attachment 8967948 [details] [diff] [review] 1378089-part3-bookmarks-257.patch >+++ b/suite/browser/browser-places.js >+ } catch (e) { >+ Cu..reportError(e); Double dot? > bookmarkCurrentPage: function PCH_bookmarkCurrentPage(aShowEditUI, aParent) { >- this.bookmarkPage(gBrowser.selectedBrowser, aParent, aShowEditUI); >+ this.bookmarkPage(gBrowser.selectedBrowser, aParent, aShowEditUI) >+ .catch(Cu..reportError); Double dot? > PlacesUtils.bookmarks.fetch({url: this._uri}, b => aItemGuids.push(b.guid)) >- .catch(Cu.reportError) >+ .catch(Cu..reportError) Double dot? >- Cu.reportError("BookmarkingUI failed adding a bookmarks observer: " + ex); >+ Cu..reportError("BookmarkingUI failed adding a bookmarks observer: " + ex); Double dot? This is a difficult patch to review, due to the number of indirect changes needed for syncing e.g. Cc, Ci, Cu but from what I can tell LGTM r=me

Comment on attachment 8951977 [details] [diff] [review] 1378089-part5-bookmarks-257.patch LGTM r=me

Comment on attachment 8951978 [details] [diff] [review] 1378089-part6-bookmarks-257.patch LGTM r=me

Comment on attachment 8967949 [details] [diff] [review] 1378089-part4-bookmarks-257.patch Looks like this patch does fix the previous Cu.. issues as a bonus. LGTM r=me

Comment on attachment 8951973 [details] [diff] [review] 1378089-part1-recentwindow-257.patch [Triage Comment] a=me for 2.57/c-b

Comment on attachment 8951977 [details] [diff] [review] 1378089-part5-bookmarks-257.patch [Triage Comment] a=me for 2.57/c-b

Comment on attachment 8951978 [details] [diff] [review] 1378089-part6-bookmarks-257.patch [Triage Comment] a=me for 2.57/c-b

Comment on attachment 8951985 [details] [diff] [review] 1378089-part7-bookmarks-257.patch [Triage Comment] a=me for 2.57/c-b

Comment on attachment 8967947 [details] [diff] [review] 1378089-part2-bookmarks-257.patch [Triage Comment] a=me for 2.57/c-b

Comment on attachment 8967948 [details] [diff] [review] 1378089-part3-bookmarks-257.patch [Triage Comment] a=me for 2.57/c-b

Comment on attachment 8967949 [details] [diff] [review] 1378089-part4-bookmarks-257.patch [Triage Comment] a=me for 2.57/c-b

Comment on attachment 8967951 [details] [diff] [review] 1378089-part3-bookmarks-253.patch r=me just in case...

Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/b9a5db2726f3 Part 1. Add recentwindow.jsm to SeaMonkey. r=IanN https://hg.mozilla.org/comm-central/rev/0c8c03f917f1 Part 2. Mass rename bookmarks files and dirs to corresponding Firefox ones. r=IanN https://hg.mozilla.org/comm-central/rev/f000c3a844b1 Part 3. Replace the Bookmarks Manager and the History viewer with the Firefox library. r=IanN https://hg.mozilla.org/comm-central/rev/fd9d6ed9952e Part 4. Replace the Bookmarks Manager and the History viewer with the Firefox library. r=IanN https://hg.mozilla.org/comm-central/rev/838853b3bbab Part 5. Make the All Bookmarks folder for the left pane of the Library a virtual query. r=IanN https://hg.mozilla.org/comm-central/rev/016955819613 Part 6. Make the new All Bookmarks folder query only update on the mobile folder status change for better performance. r=IanN https://hg.mozilla.org/comm-central/rev/865b73490b82 Part 7. Move osx filters.svg from bookmarks to places. r=IanN

Not really fixed because of later changes in 60 and 61. Stay tuned for the followup bugs.

Just a small patch for 2.53 already in the 2.57 patch. Fixes: Timestamp: 4/28/2018, 11:23:15 AM Error: TypeError: this._recentFolders is undefined Source File: chrome://communicator/content/places/editBookmarkOverlay.js Line: 1127

https://hg.mozilla.org/releases/comm-beta/rev/49d90f6c578a https://hg.mozilla.org/releases/comm-beta/rev/1073c50c7d8c https://hg.mozilla.org/releases/comm-beta/rev/2516f9f3631a https://hg.mozilla.org/releases/comm-beta/rev/551982a6e2bd https://hg.mozilla.org/releases/comm-beta/rev/d743fc6395f8 https://hg.mozilla.org/releases/comm-beta/rev/a414897db0d9 https://hg.mozilla.org/releases/comm-beta/rev/dc4acdaa5df1 This has l10n impact for comm-esr60. We will fix this on the SeaMonkey side during building by merging a different cset for the suite dir only.

Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/dc4ab600181b Follow-Up. Fix bad search placeholder names in sidebar panel. r=me