Replace the Bookmark Manager with the Firefox Library in SeaMonkey

RESOLVED FIXED in seamonkey2.58

Status

RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: mak, Assigned: frg)

Tracking

(Blocks: 5 bugs)

unspecified
seamonkey2.58
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.53 affected, seamonkey2.57esr fixed, seamonkey2.58 fixed)

Details

Attachments

(13 attachments, 24 obsolete attachments)

4.36 KB, patch
iann_bugzilla
: review+
Details | Diff | Splinter Review
8.95 KB, patch
Details | Diff | Splinter Review
18.00 KB, patch
Details | Diff | Splinter Review
4.38 KB, patch
iann_bugzilla
: review+
iann_bugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
34.92 KB, patch
iann_bugzilla
: review+
iann_bugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
1.38 KB, patch
iann_bugzilla
: review+
iann_bugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
457 bytes, patch
iann_bugzilla
: review+
iann_bugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
108.91 KB, patch
frg
: review+
iann_bugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
768.70 KB, patch
iann_bugzilla
: review+
iann_bugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
245.87 KB, patch
iann_bugzilla
: review+
iann_bugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
109.70 KB, patch
frg
: review+
Details | Diff | Splinter Review
798.84 KB, patch
iann_bugzilla
: review+
Details | Diff | Splinter Review
3.23 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1379370
(Reporter)

Comment 1

2 years ago
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.
(Assignee)

Comment 2

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1387409
(Assignee)

Updated

2 years ago
Blocks: 1394144
(Reporter)

Comment 3

2 years ago
Not blocking the feature, but depends on it.
No longer blocks: 979280
Depends on: 979280
(Reporter)

Comment 4

2 years ago
FYI, we plan to enable the feature in 58, and legacy code removal will likely happen in 59.
(Assignee)

Updated

a year ago
Blocks: 1420718
(Assignee)

Updated

a year ago
Blocks: 1433370
(Assignee)

Updated

a year ago
Summary: Figure out what to do with bookmarks transactions → Replace the Bookmark Manager with the Firefox Library in SeaMonkey
(Assignee)

Comment 5

a year ago
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.
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
(Assignee)

Comment 6

a year ago
Part 2 move over the bookmarks files to places.
(Assignee)

Comment 7

a year ago
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.
(Assignee)

Updated

a year ago
Depends on: 1433962
(Assignee)

Comment 8

a year ago
Missed the new tag.png for osx
Attachment #8946354 - Attachment is obsolete: true
(Assignee)

Comment 9

a year ago
Minor cleanups
Attachment #8946358 - Attachment is obsolete: true
(Assignee)

Comment 10

a year ago
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.
(Assignee)

Comment 11

a year ago
Attachment #8946353 - Attachment is obsolete: true
(Assignee)

Comment 12

a year ago
Attachment #8946740 - Attachment is obsolete: true
(Assignee)

Comment 13

a year ago
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.
Attachment #8946742 - Attachment is obsolete: true
Attachment #8946747 - Attachment is obsolete: true
(Assignee)

Comment 14

a year ago
Tests likely broken.
(Assignee)

Comment 15

a year ago
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.
(Assignee)

Updated

a year ago
Depends on: 796994
(Assignee)

Comment 16

a year ago
Some small tweaks
Attachment #8948126 - Attachment is obsolete: true
(Assignee)

Comment 17

a year ago
Bleeding edge comm-central version part 1
(Assignee)

Comment 18

a year ago
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.
(Assignee)

Comment 19

a year ago
Rebased patch from 2.53 / 56
(Assignee)

Comment 20

a year ago
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.
(Assignee)

Updated

a year ago
Blocks: 1439220
(Assignee)

Comment 21

a year ago
history icons fixed and css cleaned up. 2.53 version
Attachment #8950690 - Attachment is obsolete: true
(Assignee)

Comment 22

a year ago
"Final" versions for part 1 to 6. Lets discuss it as the meeting.
Attachment #8950702 - Attachment is obsolete: true
Flags: needinfo?(iann_bugzilla)
(Assignee)

Comment 23

a year ago
Attachment #8950703 - Attachment is obsolete: true
(Assignee)

Comment 24

a year ago
Attachment #8950705 - Attachment is obsolete: true
(Assignee)

Comment 25

a year ago
Attachment #8950709 - Attachment is obsolete: true
(Assignee)

Comment 26

a year ago
Port Bug 1423896 "Make the All Bookmarks folder for the left pane a virtual query".
(Assignee)

Comment 27

a year ago
Port Bug 1423896 "Make the All Bookmarks folder for the left pane a virtual query". Second part.
(Assignee)

Comment 28

a year ago
osx build fix. Missed filters.svg
(Assignee)

Updated

a year ago
Duplicate of this bug: 560111
(Assignee)

Updated

a year ago
Duplicate of this bug: 1299352
(Assignee)

Comment 31

a year ago
Took out one location variable too many which broke smart bookmark recreation in nsSuiteGlue.js
Attachment #8951972 - Attachment is obsolete: true
(Assignee)

Comment 32

a year ago
Attachment #8951975 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Blocks: 942937
(Assignee)

Comment 33

a year ago
Rebased after Bug 1436605 landed.
Attachment #8951974 - Attachment is obsolete: true
(Assignee)

Comment 34

a year ago
Rebased after Bug 1436605 landed.
Attachment #8953461 - Attachment is obsolete: true
(Assignee)

Comment 35

a year ago
Attachment #8951976 - Attachment is obsolete: true

Comment 36

a year ago
Comment on attachment 8948123 [details] [diff] [review]
1378089-part1-recentwindow-253.patch

LGTM r=me
Attachment #8948123 - Flags: review+

Comment 37

a year ago
Comment on attachment 8951973 [details] [diff] [review]
1378089-part1-recentwindow-257.patch

LGTM r=me
Attachment #8951973 - Flags: review+

Comment 38

a year ago
Comment on attachment 8955773 [details] [diff] [review]
1378089-part2-bookmarks-257.patch

LGTM r=me
Attachment #8955773 - Flags: review+

Comment 39

a year ago
Comment on attachment 8948124 [details] [diff] [review]
1378089-part2-bookmarks-253.patch

LGTM r=me
Attachment #8948124 - Flags: review+

Comment 40

a year ago
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?
Flags: needinfo?(iann_bugzilla)
Attachment #8953460 - Flags: feedback+
(Assignee)

Comment 41

a year ago
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.

Comment 42

a year ago
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.

Comment 43

a year ago
(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.

Comment 44

a year ago
(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.
(Assignee)

Comment 45

a year ago
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 :)
Attachment #8953460 - Attachment is obsolete: true
(Assignee)

Comment 46

a year ago
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.
Attachment #8955774 - Attachment is obsolete: true
(Assignee)

Comment 47

11 months ago
Rebased 2.57 patch. Unused /suite/themes/classic/communicator/bookmarks/bookmarksWindow.css removed. r+ from iann_bugzilla retained.
Attachment #8955773 - Attachment is obsolete: true
Attachment #8967947 - Flags: review+
(Assignee)

Comment 48

11 months ago
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.
Attachment #8956238 - Attachment is obsolete: true
Attachment #8967948 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 49

11 months ago
rebased patch
Attachment #8955775 - Attachment is obsolete: true
(Assignee)

Updated

11 months ago
Attachment #8967949 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

11 months ago
Attachment #8956240 - Attachment is obsolete: true
(Assignee)

Comment 50

11 months ago
2.53 only. r+ from IanN retained in case we do an interim release.

Unused /suite/themes/classic/communicator/bookmarks/bookmarksWindow.css removed.
Attachment #8948124 - Attachment is obsolete: true
Attachment #8967950 - Flags: review+
(Assignee)

Comment 51

11 months ago
2.53 only. Minor fix for the async path and rebased.
(Assignee)

Updated

11 months ago
Attachment #8951977 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

11 months ago
Attachment #8951978 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

11 months ago
Attachment #8951985 - Flags: review?(iann_bugzilla)

Comment 52

11 months ago
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
Attachment #8967948 - Flags: review?(iann_bugzilla) → review+

Comment 53

11 months ago
Comment on attachment 8951977 [details] [diff] [review]
1378089-part5-bookmarks-257.patch

LGTM r=me
Attachment #8951977 - Flags: review?(iann_bugzilla) → review+

Comment 54

11 months ago
Comment on attachment 8951978 [details] [diff] [review]
1378089-part6-bookmarks-257.patch

LGTM r=me
Attachment #8951978 - Flags: review?(iann_bugzilla) → review+

Updated

11 months ago
Attachment #8951985 - Flags: review?(iann_bugzilla) → review+

Comment 55

11 months ago
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
Attachment #8967949 - Flags: review?(iann_bugzilla) → review+

Comment 56

11 months ago
Comment on attachment 8951973 [details] [diff] [review]
1378089-part1-recentwindow-257.patch

[Triage Comment]
a=me for 2.57/c-b
Attachment #8951973 - Flags: approval-comm-beta+

Comment 57

11 months ago
Comment on attachment 8951977 [details] [diff] [review]
1378089-part5-bookmarks-257.patch

[Triage Comment]

a=me for 2.57/c-b
Attachment #8951977 - Flags: approval-comm-beta+

Comment 58

11 months ago
Comment on attachment 8951978 [details] [diff] [review]
1378089-part6-bookmarks-257.patch

[Triage Comment]
a=me for 2.57/c-b
Attachment #8951978 - Flags: approval-comm-beta+

Comment 59

11 months ago
Comment on attachment 8951985 [details] [diff] [review]
1378089-part7-bookmarks-257.patch

[Triage Comment]
a=me for 2.57/c-b
Attachment #8951985 - Flags: approval-comm-beta+

Comment 60

11 months ago
Comment on attachment 8967947 [details] [diff] [review]
1378089-part2-bookmarks-257.patch

[Triage Comment]
a=me for 2.57/c-b
Attachment #8967947 - Flags: approval-comm-beta+

Comment 61

11 months ago
Comment on attachment 8967948 [details] [diff] [review]
1378089-part3-bookmarks-257.patch

[Triage Comment]
a=me for 2.57/c-b
Attachment #8967948 - Flags: approval-comm-beta+

Comment 62

11 months ago
Comment on attachment 8967949 [details] [diff] [review]
1378089-part4-bookmarks-257.patch

[Triage Comment]
a=me for 2.57/c-b
Attachment #8967949 - Flags: approval-comm-beta+

Comment 63

11 months ago
Comment on attachment 8967951 [details] [diff] [review]
1378089-part3-bookmarks-253.patch

r=me just in case...
Attachment #8967951 - Flags: review+

Comment 64

11 months ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
(Assignee)

Comment 65

11 months ago
Not really fixed because of later changes in 60 and 61. Stay tuned for the followup bugs.
status-seamonkey2.53: --- → affected
status-seamonkey2.57esr: --- → affected
status-seamonkey2.58: --- → fixed
Target Milestone: --- → Seamonkey2.58
(Assignee)

Updated

11 months ago
Blocks: 1456589
(Assignee)

Comment 66

11 months ago
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
(Assignee)

Updated

10 months ago
Duplicate of this bug: 1347652
(Assignee)

Updated

10 months ago
Duplicate of this bug: 1463667

Updated

9 months ago
Depends on: 1474125
(Assignee)

Updated

8 months ago
Blocks: 1476660
(Assignee)

Updated

8 months ago
Blocks: 1476662
(Assignee)

Updated

7 months ago
Blocks: 1489307
(Assignee)

Updated

2 months ago
Blocks: 1524791
You need to log in before you can comment on or make changes to this bug.