Bug 730837 (SM-livemarksIO)

Port |Bug 613588 - Replace livemarks with asynchronous load-on-demand livemarks (was: Livemarks cause synchronous I/O during txul)| to SeaMonkey, code

RESOLVED FIXED in seamonkey2.12

Status

SeaMonkey
Bookmarks & History
P2
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: neil@parkwaycc.co.uk)

Tracking

({regression})

Trunk
seamonkey2.12
regression
Dependency tree / graph
Bug Flags:
in-testsuite -

SeaMonkey Tracking Flags

(seamonkey2.10? fixed, seamonkey2.11 fixed, seamonkey2.12 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
New SeaMonkey failures on mochitest-browser-chrome:
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/places/tests/browser_457473_no_copy_guid.js | Exception thrown - [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsITransactionManager.undoTransaction]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource:///modules/PlacesUIUtils.jsm :: <TOP_LEVEL> :: line 1396"  data: no]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/places/tests/browser_library_views_liveupdate.js | Node is recognized as a livemark
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/places/tests/browser_library_views_liveupdate.js | Node is recognized as a livemark
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/places/tests/browser_library_views_liveupdate.js | Node is recognized as a livemark
}

There may be more on other suites.

***

Need to port:
*Code : https://hg.mozilla.org/mozilla-central/rev/08a7bb3f5c92
*Tests: https://hg.mozilla.org/mozilla-central/rev/4814a8d64b8d

***

Marco, do you think I could port tests even before code is updated, as this seems to be mostly livemark check removals? (And that tests would (mostly) pass.)
the old API should still mostly work, some tests may not. I think porting tests changes before is feasible.
(Reporter)

Updated

5 years ago
Blocks: 730849
(Reporter)

Updated

5 years ago
No longer blocks: 452942
(Reporter)

Comment 2

5 years ago
(In reply to Marco Bonardo [:mak] from comment #1)
> I think porting tests changes before is feasible.

Bug 730849
Severity: major → normal
Summary: Port |Bug 613588 - (livemarksIO) Replace livemarks with asynchronous load-on-demand livemarks (was: Livemarks cause synchronous I/O during txul)| to SeaMonkey → Port |Bug 613588 - (livemarksIO) Replace livemarks with asynchronous load-on-demand livemarks (was: Livemarks cause synchronous I/O during txul)| to SeaMonkey, code

Updated

5 years ago
Blocks: 730752
So I guess this should fix those:

"start is deprecated and will be removed in a future release. Check the nsILivemarkService interface."
(.../components/nsLivemarkService.js line 201)

"isLivemark is deprecated and will be removed in a future release. Check the nsILivemarkService interface.""
(.../components/nsLivemarkService.js line 201)
Also:

Error: Synchronous livemarks methods and PlacesUtils livemarks utils (itemIsLivemark, nodeIsLivemarkContainer, nodeIsLivemarkItem) are deprecated and will be removed in a future release.
Source File: resource://gre/modules/PlacesUtils.jsm
Line: 497
Tookit and Firefox code doesn't use those methods anymore, if you mean Seamonkey code, it will have to port the frontend part from this bug.
(In reply to Marco Bonardo [:mak] from comment #5)
> Tookit and Firefox code doesn't use those methods anymore, if you mean
> Seamonkey code, it will have to port the frontend part from this bug.

Well yes, this is a SM porting action bug so I thought that would be obvious. ;-)
ops, sorry I got a bunch of bugmail and thought I was answering in the original bug :(
(Reporter)

Updated

5 years ago
Duplicate of this bug: 735683
(Reporter)

Updated

5 years ago
Severity: normal → major
status-seamonkey2.10: --- → affected
Keywords: helpwanted, regression
Whiteboard: [lang=js][mentor=Neil]
(Reporter)

Updated

5 years ago
Target Milestone: seamonkey2.10 → seamonkey2.11
(Reporter)

Updated

5 years ago
Blocks: 743677
(Assignee)

Comment 9

5 years ago
Created attachment 624900 [details] [diff] [review]
Code changes only

In particular, not including
* test changes
* theme changes
* variable renames
* subsequent bugfixes
Attachment #624900 - Flags: feedback?(philip.chee)
Attachment #624900 - Flags: feedback?(kairo)
Attachment #624900 - Flags: feedback?(jh)
Attachment #624900 - Flags: feedback?(iann_bugzilla)
(Assignee)

Updated

5 years ago
status-seamonkey2.11: --- → affected
status-seamonkey2.12: --- → affected
status-seamonkey2.9: --- → unaffected
tracking-seamonkey2.11: --- → ?
tracking-seamonkey2.12: --- → ?
Comment on attachment 624900 [details] [diff] [review]
Code changes only

Patch doesn't apply cleanly. Possibly bitrotted by bug 732027 which landed yesterday.
Attachment #624900 - Flags: feedback?(jh) → feedback-
(Assignee)

Comment 11

5 years ago
Created attachment 625299 [details] [diff] [review]
Updated for bitrot
Attachment #624900 - Attachment is obsolete: true
Attachment #625299 - Flags: feedback?(philip.chee)
Attachment #625299 - Flags: feedback?(kairo)
Attachment #625299 - Flags: feedback?(jh)
Attachment #625299 - Flags: feedback?(iann_bugzilla)
Attachment #624900 - Flags: feedback?(philip.chee)
Attachment #624900 - Flags: feedback?(kairo)
Attachment #624900 - Flags: feedback?(iann_bugzilla)

Comment 12

5 years ago
Comment on attachment 625299 [details] [diff] [review]
Updated for bitrot

I notice you have used a callback onCompletion, is this more efficient than the way Firefox does it?
Attachment #625299 - Flags: feedback?(iann_bugzilla) → feedback+
(Assignee)

Comment 13

5 years ago
(In reply to Ian Neal from comment #12)
> I notice you have used a callback onCompletion, is this more efficient than
> the way Firefox does it?
Well, this is a function interface, so you have a range of options:
1. Global function (1)
2. Global function bound to this (1NT)
3. Local function (NS)
4. Local function bound to this (NNST)
5. Global object (1)
6. Local object (NV)
7. this object (T)
Variables: S - scope variables T - this V - selected values
Instances: 1 - single N - instance per call NN - two instances per call
So using this as callback requires no extra objects and gives you this access.

BTW I've just noticed that I've named my callback functions onComplete instead of onCompletion, that's obviously a typo.
(Reporter)

Updated

5 years ago
No longer blocks: 730752
(Reporter)

Updated

5 years ago
Blocks: 730752
Comment on attachment 625299 [details] [diff] [review]
Updated for bitrot

I'm having a hard time testing this (see bug 732027 comment 19). At least the Toolkit-triggered error messages are gone. ;-)
Comment on attachment 625299 [details] [diff] [review]
Updated for bitrot

I'll spare you the unbitrotting this time since it was easy enough to fix. ;-)

Done:
- Open BM (checked folder panes and edit area)
- File Bookmark (checked drop-down and folder tree)
- Open a website feed, use Live Bookmarks to subscribe, open new LB folder and let it populate, click one of the entries to see target page load
- Check Error Console (no entries whatsoever with mode = Error)

Not done:
- Code review of any kind
Attachment #625299 - Flags: feedback?(jh) → feedback+

Comment 16

5 years ago
Comment on attachment 625299 [details] [diff] [review]
Updated for bitrot

I don't pretend to really know or understand this code, but I don't spot anything glaringly wrong.

BTW, if you end up improving on Firefox code (and I know you usually do that), can you file a patch to backport those improvements? Thanks.
Attachment #625299 - Flags: feedback?(kairo) → feedback+

Comment 17

5 years ago
Yes, it would be good to have an icon to indicate unvisited ones, I'd be happy for the "bookmark updated" icon to be used for that.
No longer blocks: 730849, 743677
No longer depends on: 613588

Updated

5 years ago
Blocks: 730849, 743677
Depends on: 613588

Comment 18

5 years ago
Comment on attachment 625299 [details] [diff] [review]
Updated for bitrot

Phase 1: Nitpicking and Flyspecking

Please remove dump()s from BrowserPlacesViews.js before checkin.

> +  _populateLivemarkContainer: function PTV__populateLivemarkContainer(aNode) {
> +    PlacesUtils.livemarks.getLivemark({ id: aNode.itemId },
> +      function (aStatus, aLivemark) {
Anonymous function here.

>    nodeAnnotationChanged: function PTV_nodeAnnotationChanged(aNode, aAnno) {
......
> +      PlacesUtils.livemarks.getLivemark({ id: aNode.itemId },
> +        function (aStatus, aLivemark) {
Anonymous function here.

>    function PTV_containerStateChanged(aNode, aOldState, aNewState) {
......
> +      PlacesUtils.livemarks.getLivemark({ id: aNode.itemId },
> +        function (aStatus, aLivemark) {
Anonymous function here.

(At least one more anonymous function somewhere)

> +++ b/browser/fuel/src/fuelApplication.js
.......
>    get livemarks() {
>      let livemarks = Cc["@mozilla.org/browser/livemark-service;2"].
> -                    getService(Ci.nsILivemarkService);
> +                    getService[Ci.mozIAsyncLivemarks].
> +                    QueryInterface(Ci.nsILivemarkService);
Don't we need this change too?

Comment 19

5 years ago
Comment on attachment 625299 [details] [diff] [review]
Updated for bitrot

> - Open BM (checked folder panes and edit area)
Error: TypeError: gEditItemOverlay.onLocationFieldBlur is not a function
Source file: chrome://communicator/content/bookmarks/bookmarksManager.xul
Line: 1

> - File Bookmark (checked drop-down and folder tree)
Error: An error occurred updating the cmd_selectAll command: [Exception... "'[JavaScript Error: "this._view.result is null" {file: "chrome://communicator/content/places/controller.js" line: 180}]' when calling method: [nsIController::isCommandEnabled]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: goUpdateCommand :: line 82"  data: yes]
Source file: chrome://global/content/globalOverlay.js
Line: 88

> - Open a website feed,
Error: An error occurred updating the cmd_selectAll command: [Exception... "'[JavaScript Error: "this._view.result is null" {file: "chrome://communicator/content/places/controller.js" line: 180}]' when calling method: [nsIController::isCommandEnabled]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: goUpdateCommand :: line 82"  data: yes]
Source file: chrome://global/content/globalOverlay.js
Line: 88

>  use Live Bookmarks to subscribe,
OK.

> open new LB folder and let it populate,
OK.

> click one of the entries to see target page load
OK.

Click on [Open "Slashdot"]
OK.
Note: no "Open all in tabs" menu item.
From the Bookmarks Manager, the "Open all in tabs" item is greyed out, but I can select all and then right click and then open all in tabs.

> - Check Error Console (no entries whatsoever with mode = Error)
Your Error Console is broken ;)
Attachment #625299 - Flags: feedback?(philip.chee) → feedback-
(Assignee)

Comment 20

5 years ago
(In reply to Philip Chee from comment #18)
> Please remove dump()s from BrowserPlacesViews.js before checkin.
Yeah, I was debugging something this turns out to be a known places bug.

> > +    PlacesUtils.livemarks.getLivemark({ id: aNode.itemId },
> > +      function (aStatus, aLivemark) {
> Anonymous function here.
Yeah, I noticed that the ones I had named were wrong anyway.

> >    get livemarks() {
> >      let livemarks = Cc["@mozilla.org/browser/livemark-service;2"].
> > -                    getService(Ci.nsILivemarkService);
> > +                    getService[Ci.mozIAsyncLivemarks].
> > +                    QueryInterface(Ci.nsILivemarkService);
> Don't we need this change too?
Fixed locally but accidentally excluded from the patch.
(Assignee)

Comment 21

5 years ago
(In reply to Philip Chee from comment #19)
> Error: TypeError: gEditItemOverlay.onLocationFieldBlur is not a function
Oops, I shouldn't have removed this, we didn't actually have the function that Firefox removed in the first place.

> Error: An error occurred updating the cmd_selectAll command: [Exception...
Hmm, are you sure this is due to this patch? ;-)
Do you have some better STR, I'm not sure what you did here.

> Note: no "Open all in tabs" menu item.
Yes, this is "by design".

> From the Bookmarks Manager, the "Open all in tabs" item is greyed out, but I
> can select all and then right click and then open all in tabs.
Again, by design, the bookmarks manager will show "Open all in tabs" whenever there isn't exactly one item selected, whatever sort of items they are.
(Assignee)

Comment 22

5 years ago
Created attachment 626783 [details] [diff] [review]
Fixed code changes
Assignee: nobody → neil
Attachment #625299 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #626783 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 23

5 years ago
Created attachment 626843 [details] [diff] [review]
Really fixed code changes
[Checked in: Comment 24]
Attachment #626783 - Attachment is obsolete: true
Attachment #626783 - Flags: review?(iann_bugzilla)
Attachment #626843 - Flags: review?(iann_bugzilla)

Updated

5 years ago
Attachment #626843 - Flags: review?(iann_bugzilla) → review+
FTR, Neil pushed this with the base bug's number:
http://hg.mozilla.org/comm-central/rev/a87aff0360cc
(Assignee)

Comment 25

5 years ago
Created attachment 627932 [details] [diff] [review]
Show visited livemarks
[Checked in: Comment 28]

I thought it would be easiest to resurrect the old bookmark-item-updated images that the old 2.0 bookmarks system used, although they didn't work properly because they were only 14x14 so I've had to pad them to 16x16; I haven't optimised them yet though, as I only have optipng on my check in VM.
Attachment #627932 - Flags: review?(iann_bugzilla)
(Reporter)

Updated

5 years ago
Attachment #626843 - Attachment description: Really fixed code changes → Really fixed code changes [Checked in: Comment 24]
(Reporter)

Updated

5 years ago
Keywords: helpwanted
Whiteboard: [lang=js][mentor=Neil]
Target Milestone: seamonkey2.11 → seamonkey2.12

Comment 26

5 years ago
Comment on attachment 627932 [details] [diff] [review]
Show visited livemarks
[Checked in: Comment 28]

>+++ b/suite/themes/modern/communicator/bookmarks/bookmarks.css

>+.bookmark-item[container][livemark] .bookmark-item[visited],
>+treechildren::-moz-tree-image(title, livemarkItem, visited) {
>+  list-style-image: url("chrome://communicator/skin/bookmarks/bookmark-item.gif");
Should we be using the png version of this icon, at the moment it is only used by help and places for the default favicon?

r=me with that answered
Attachment #627932 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 27

5 years ago
I'm not on a mission to convert swathes of Modern from GIF to PNG, especially when I want these patches to land on the branches for 2.10! I think the PNG only exists because it's hardcoded somewhere (sadface), rather than using CSS.
(Assignee)

Comment 28

5 years ago
Pushed comm-central changeset 9ff708bf4069.

Anyone want to port the tests?
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 29

5 years ago
Comment on attachment 626843 [details] [diff] [review]
Really fixed code changes
[Checked in: Comment 24]

[Approval Request Comment]
Regression caused by (bug #): 613588
User impact if declined: Livemarks do not function
Testing completed (on m-c, etc.): Baked for a week, tested by >1 user
Risk to taking this patch (and alternatives if risky): medium
String changes made by this patch: none
Attachment #626843 - Flags: approval-comm-beta?
Attachment #626843 - Flags: approval-comm-aurora?
(Assignee)

Comment 30

5 years ago
Comment on attachment 627932 [details] [diff] [review]
Show visited livemarks
[Checked in: Comment 28]

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: Incomplete port of bug 613588
Testing completed (on m-c, etc.): Straight port of m-c patch in bug 613588
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: none
Attachment #627932 - Flags: approval-comm-beta?
Attachment #627932 - Flags: approval-comm-aurora?
(Reporter)

Updated

5 years ago
Attachment #627932 - Attachment description: Show visited livemarks → Show visited livemarks [Checked in: Comment 28]
(Reporter)

Comment 31

5 years ago
(In reply to Marco Bonardo [:mak] from comment #1)
> the old API should still mostly work

(In reply to neil@parkwaycc.co.uk from comment #29)
> User impact if declined: Livemarks do not function

(I'm all for landing this in SM 2.10, but)
Ftr, could you give an example of current breakage?

*****

(In reply to neil@parkwaycc.co.uk from comment #28)
> Anyone want to port the tests?

I already did it in bug 730849.
status-seamonkey2.12: affected → ---
status-seamonkey2.9: unaffected → ---
tracking-seamonkey2.12: ? → ---
Flags: in-testsuite-
(Reporter)

Comment 32

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #27)
> I think the PNG only exists because it's hardcoded somewhere (sadface),
> rather than using CSS.

http://mxr.mozilla.org/comm-central/find?text=&string=bookmark-item
{
/suite/themes/modern/communicator/bookmarks/bookmark-item.gif
/suite/themes/modern/communicator/bookmarks/bookmark-item.png
}

http://mxr.mozilla.org/comm-central/search?string=bookmark-item.&case=on&find=%2Fsuite%2F

Is this unduplication covered by bug 249744, or should a specific bug be filed?
Alias: SM-livemarksIO
Summary: Port |Bug 613588 - (livemarksIO) Replace livemarks with asynchronous load-on-demand livemarks (was: Livemarks cause synchronous I/O during txul)| to SeaMonkey, code → Port |Bug 613588 - Replace livemarks with asynchronous load-on-demand livemarks (was: Livemarks cause synchronous I/O during txul)| to SeaMonkey, code
(Assignee)

Comment 33

5 years ago
(In reply to Serge Gautherie from comment #31)
> (In reply to comment #29)
> > User impact if declined: Livemarks do not function
> Ftr, could you give an example of current breakage?
Sure. Just open a livemark and notice that there are no entries.

> (In reply to neil@parkwaycc.co.uk from comment #28)
> > Anyone want to port the tests?
> I already did it in bug 730849.
D'oh, I should have read comment #2. Sorry about that. Thanks for the work!
(Assignee)

Comment 34

5 years ago
(In reply to Serge Gautherie from comment #32)
> (In reply to comment #27)
> > I think the PNG only exists because it's hardcoded somewhere (sadface),
> > rather than using CSS.
> Is this unduplication covered by bug 249744, or should a specific bug be
> filed?
No, there's also a second place where we override defaultFavicon.png, and you can't override on a per-theme basis, so the file has to exist in both themes.

Updated

5 years ago
Blocks: 761038

Comment 35

5 years ago
Comment on attachment 626843 [details] [diff] [review]
Really fixed code changes
[Checked in: Comment 24]

a=me
Attachment #626843 - Flags: approval-comm-beta?
Attachment #626843 - Flags: approval-comm-beta+
Attachment #626843 - Flags: approval-comm-aurora?
Attachment #626843 - Flags: approval-comm-aurora+

Comment 36

5 years ago
Comment on attachment 627932 [details] [diff] [review]
Show visited livemarks
[Checked in: Comment 28]

a=me
Attachment #627932 - Flags: approval-comm-beta?
Attachment #627932 - Flags: approval-comm-beta+
Attachment #627932 - Flags: approval-comm-aurora?
Attachment #627932 - Flags: approval-comm-aurora+
(Assignee)

Comment 37

5 years ago
Comment on attachment 626843 [details] [diff] [review]
Really fixed code changes
[Checked in: Comment 24]

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
String changes made by this patch:
Attachment #626843 - Flags: approval-comm-release?
(Assignee)

Comment 38

5 years ago
Comment on attachment 627932 [details] [diff] [review]
Show visited livemarks
[Checked in: Comment 28]

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
String changes made by this patch:
Attachment #627932 - Flags: approval-comm-release?

Updated

5 years ago
Attachment #626843 - Flags: approval-comm-release? → approval-comm-release+

Updated

5 years ago
Attachment #627932 - Flags: approval-comm-release? → approval-comm-release+
(Assignee)

Comment 39

5 years ago
Pushed comm-aurora changeset 55bd2112f6a4, comm-beta changeset df0e75a9f2c1 and comm-release changeset 0eeebd728b9f. Unfortunately this was my first attempt at hg transplant and it ended badly; I was able to recover but only after folding the changesets together. I did at least get the bug number right.
status-seamonkey2.10: affected → fixed
status-seamonkey2.11: affected → fixed

Updated

5 years ago
status-seamonkey2.12: --- → fixed
FWIW this is *not* fixed for SM2.10, even though it is in repo, after IRC discussion, me Neil and Jens decided to relnote it, due to lack of total testing of this patch.
I'd like to note that if we need to spin a 2.10.1 for some other reason we should be careful whether to include this fix, too, since so far we have zero 2.10 builds with the fix (branch migration happened before an 2.10-Aurora build including this fix could be produced). Maybe we should consider creating a 2.10-Release test build including the fix. Callek, is this feasible releng-wise? [If not I could at least create a Windows test build locally.]
tracking-seamonkey2.11: ? → ---
Keywords: relnote
After our first 2.11Beta spins I am ok spinning a 2.10.1-candidate build1 (with an explicit note that we are NOT releasing that one, unless tehre is a big reason) so we can test this.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.