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

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect, P2)

Description

[Serge Gautherie (:sgautherie)]

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.)

Comment 1

[Marco Bonardo [:mak]]

the old API should still mostly work, some tests may not. I think porting tests changes before is feasible.

Comment 2

[Serge Gautherie (:sgautherie)]

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

Bug 730849

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

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)

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

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

Comment 5

[Marco Bonardo [:mak]]

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.

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

(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. ;-)

Comment 7

[Marco Bonardo [:mak]]

ops, sorry I got a bunch of bugmail and thought I was answering in the original bug :(

Comment 9

[[e-mail]]

Attachment: Code changes only

In particular, not including
* test changes
* theme changes
* variable renames
* subsequent bugfixes

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 624900 [details] [diff] [review]
Code changes only

Patch doesn't apply cleanly. Possibly bitrotted by bug 732027 which landed yesterday.

Comment 11

[[e-mail]]

Attachment: Updated for bitrot

Comment 12

[Ian Neal]

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?

Comment 13

[[e-mail]]

(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.

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

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 15

[Jens Hatlak (:InvisibleSmiley)]

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

Comment 16

[Robert Kaiser]

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.

Comment 17

[Ian Neal]

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.

Comment 18

[Philip Chee]

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

[Philip Chee]

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 ;)

Comment 20

[[e-mail]]

(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.

Comment 21

[[e-mail]]

(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.

Comment 22

[[e-mail]]

Attachment: Fixed code changes

Comment 23

[[e-mail]]

Attachment: Really fixed code changes [Checked in: Comment 24]

Comment 24

[Jens Hatlak (:InvisibleSmiley)]

FTR, Neil pushed this with the base bug's number:
http://hg.mozilla.org/comm-central/rev/a87aff0360cc

Comment 25

[[e-mail]]

Attachment: 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.

Comment 26

[Ian Neal]

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

Comment 27

[[e-mail]]

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.

Comment 28

[[e-mail]]

Pushed comm-central changeset 9ff708bf4069.

Anyone want to port the tests?

Comment 29

[[e-mail]]

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

Comment 30

[[e-mail]]

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

Comment 31

[Serge Gautherie (:sgautherie)]

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

(In reply to [e-mail] 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 [e-mail] from comment #28)
> Anyone want to port the tests?

I already did it in bug 730849.

Comment 32

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] 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?

Comment 33

[[e-mail]]

(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 [e-mail] 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!

Comment 34

[[e-mail]]

(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.

Comment 35

[Philip Chee]

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

a=me

Comment 36

[Philip Chee]

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

a=me

Comment 37

[[e-mail]]

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:

Comment 38

[[e-mail]]

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:

Comment 39

[[e-mail]]

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.

Comment 40

[Justin Wood (:Callek)]

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.

Comment 41

[Jens Hatlak (:InvisibleSmiley)]

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.]

Comment 42

[Justin Wood (:Callek)]

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.