Girish Sharma [:Optimizer] Reporter
	Description • 13 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120228 Firefox/13.0a1
Build ID: 20120228031102

Steps to reproduce:

Since bug 613588 landed, the livemarks are now load on demand. While it brings snappiness to firefox, it can be really tiresome for some users who really read livemarks frequently. They have to click on the livemark each time and wait till its loading, for each of the livemark subscribed.




Expected results:

There should be a pref, something like browser.livemarks.load_on_demand or like browser.bookmarks.load_livemarks_on_demand which should have a default as true, but a user if he wants should be able to switch the behavior back to automatic laoding of livemarks.
Although the async loading should remain as it is.

	jcpereira
	Comment 1 • 13 years ago

I think also that should also be a preference for the refresh time.

	Marco Bonardo [:mak] Assignee
	Comment 2 • 13 years ago

I'm rather thinking to make all livemarks refresh when one does.

I don't want to reintroduce pure automatic loading, since vast majority of users don't use livemarks and should not pay their price, and adding preferences is usually a poor solution, it works only if you know about it.

Doing the former would basically update livemarks when the user is more likely to use them, he would just pay the waiting time once. And doing that we could also reduce the current refresh time.
May that work for you?

	Girish Sharma [:Optimizer] Reporter
	Comment 3 • 13 years ago

(In reply to Marco Bonardo [:mak] from comment #2)
> May that work for you?

I have around 40 livemarks, so, it will take more than a couple of minutes for all of them to load.
Can you do like this: Load all the live marks in a cyclic order starting from the one the user opened first. And since the livemarks are Async, it would not hang the Firefox, and incrementally user will get the loadwed Livemark as he proceeds to visit them in order (if he does in order).

I still think having a pref would benefit, since normally you will keep this feature off, and only power users will enable it. Come to think of it, this feature landed in Nightly, so user using Nightly will have the knowledge of preferences entries in about:config

	jcpereira
	Comment 4 • 13 years ago

I think that updating all the live bookmarks when we visit one it´s the best solution that guarantees that they are always up to date and ensures that no resource has been wasted updating them when we don´t use them. 

In that case no preference is needed.

	Marco Bonardo [:mak] Assignee
	Comment 5 • 13 years ago

(In reply to Girish Sharma from comment #3)
> I have around 40 livemarks, so, it will take more than a couple of minutes
> for all of them to load.
> Can you do like this: Load all the live marks in a cyclic order starting
> from the one the user opened first.

yes I can enforce the one opened as being the first one.

And since the livemarks are Async, it
> would not hang the Firefox, and incrementally user will get the loadwed
> Livemark as he proceeds to visit them in order (if he does in order).

well, it's mostly async, though the feedprocessor is quite slow (bug 725964), so it actually may create some slowdowns. Indeed one of the things to figure is how much time delay each livemark from the previous one.

> I still think having a pref would benefit, since normally you will keep this
> feature off, and only power users will enable it. Come to think of it, this
> feature landed in Nightly, so user using Nightly will have the knowledge of
> preferences entries in about:config

This could be done really easily with a restartless add-on, the API can do that, just matter of creating a timer and asking the api to reload livemarks.

	Marco Bonardo [:mak] Assignee
	Comment 6 • 13 years ago

the patch is pretty simple, though reload is hard to test since we don't want long tests taking multiple seconds. Will check what I can do.

Later you should be able to find some trybuilds here:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mak77@bonardo.net-694dbccf7a11/

Please let me know if it's satisfying, or slow. We can tweak some timing.

	Girish Sharma [:Optimizer] Reporter
	Comment 7 • 13 years ago

It works fine, and the currently opened livemark is loaded first, but randomly some of the livemarks did not load up, r may be it is due to my network connection.

	Marco Bonardo [:mak] Assignee
	Comment 8 • 13 years ago

(In reply to Girish Sharma from comment #7)
> It works fine, and the currently opened livemark is loaded first, but
> randomly some of the livemarks did not load up, r may be it is due to my
> network connection.

it may depend on how many you have and the order they load, it's possible it was not yet its turn for refreshing.

	jcpereira
	Comment 9 • 13 years ago

What´s the interval between the livemarks refresh?

I notice that if i went back and forward between two of them, Firefox loses 1 news item until it shows none. Then the next time it shows all of them again.

	Marco Bonardo [:mak] Assignee
	Comment 10 • 13 years ago

(In reply to João Carlos from comment #9)
> What´s the interval between the livemarks refresh?
> 
> I notice that if i went back and forward between two of them, Firefox loses
> 1 news item until it shows none. Then the next time it shows all of them
> again.

this is a bug in current version, but should be fixed in the try build in comment 6, did you see this problem there?

	Girish Sharma [:Optimizer] Reporter
	Comment 11 • 13 years ago

Can it be pushed to central? , as the try builds are looking fine.

	Marco Bonardo [:mak] Assignee
	Comment 12 • 13 years ago

as soon as I can write a test and get a review, sure.

	Girish Sharma [:Optimizer] Reporter
	Comment 13 • 13 years ago

I know you might be busy in other stuff too, but I am held back on using try builds till this bugs is fixed in mc.

	jcpereira
	Comment 14 • 13 years ago

Yes. I saw this with the try build.

	Marco Bonardo [:mak] Assignee
	Comment 15 • 13 years ago

(In reply to João Carlos from comment #14)
> Yes. I saw this with the try build.

which OS?

	jcpereira
	Comment 16 • 13 years ago

(In reply to Marco Bonardo [:mak] from comment #15)
> (In reply to João Carlos from comment #14)
> > Yes. I saw this with the try build.
> 
> which OS?

Windows XP SP3

	jcpereira
	Comment 17 • 13 years ago

(In reply to João Carlos from comment #16)
> (In reply to Marco Bonardo [:mak] from comment #15)
> > (In reply to João Carlos from comment #14)
> > > Yes. I saw this with the try build.
> > 
> > which OS?
> 
> Windows XP SP3

With the latest Nightly this problem doesn´t exist.

	Marco Bonardo [:mak] Assignee
	Comment 18 • 13 years ago

(In reply to João Carlos from comment #9)
> I notice that if i went back and forward between two of them, Firefox loses
> 1 news item until it shows none. Then the next time it shows all of them
> again.

I fixed this in bug 731563.

	Marco Bonardo [:mak] Assignee
	Comment 19 • 13 years ago

this is likely the last needed patch for stabilization of the recent livemarks changes, unless there is something really really broken.

What the patch does:
- when accessing one livemark, first ask to refresh that livemark, then ask to refresh the other ones. This is most cases a no-op (the service checks expiration before starting any fetching work, multipel requests clear the previous timer and restart).
- fix a leak that may happen if someone removes a livemark during its invalidation.
- avoid passing a bogus id to the livemark contructor

The 2 tests helped me finding these, and test correct behavior.

	Dietrich Ayala (:dietrich)
	Comment 20 • 13 years ago

Comment on attachment 603482 [details] [diff] [review]
patch v1.1

Review of attachment 603482 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

	Asaf Romano (gone)
	Comment 21 • 13 years ago

Another proposal: start (re)loading a livemark when it's accessible. For example:
1. If your bookmarks toolbar is visible and contains livemarks, reload those livemarks, (re)load the livemark when the window opens
2. If you have a livemark(s) in some bookmark folder, (re)load the livemarks when the folder is opened.

This may be good-enough now that we don't trash the livemarks contents on-popup-showing.

	Marco Bonardo [:mak] Assignee
	Comment 22 • 13 years ago

(In reply to Mano from comment #21)
> 1. If your bookmarks toolbar is visible and contains livemarks, reload those
> livemarks, (re)load the livemark when the window opens

This is exactly one of the things we want to avoid, cause we used to ship a livemark in the toolbar by default, and users may never use it but have the toolbar visible. They should not pay for that.
Without going into the special case of a user closing the browser with the bookmarks sidebar open.

> 2. If you have a livemark(s) in some bookmark folder, (re)load the livemarks
> when the folder is opened.

I thought about this, but seeing the issue in 1. We should special case views and such that is lots of complication I don't want to even think of.

	Marco Bonardo [:mak] Assignee
	Comment 23 • 13 years ago

Comment on attachment 603482 [details] [diff] [review]
patch v1.1

need SR for the addition of the optional aForceUpdate param to the reloadLivemarks API.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 24 • 13 years ago

With that patch applied, if I call:
liveMarksService.reloadLivemarks(true);
liveMarksService.reloadLivemarks();

Won't the setting of _forceUpdate = false from the second call potentially impact the async processing of the first call?

	Marco Bonardo [:mak] Assignee
	Comment 25 • 13 years ago

Comment on attachment 603482 [details] [diff] [review]
patch v1.1

Review of attachment 603482 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #24)
> Won't the setting of _forceUpdate = false from the second call potentially
> impact the async processing of the first call?

hm, yes, for our usage that would be fine (and cheaper), though may impact multiple consumers.
Fixing this will require quite some additional change :(

	Marco Bonardo [:mak] Assignee
	Comment 26 • 13 years ago

Addressed gavin's concern.
I also found a missing change in the deprecated api (reloadAllLivemarks should do a forced reload).
The test ensures this works, by timing out if it doesn't. Doing it otherwise would be quite complex since I should take counts all around on the number of calls and such.

	Marco Bonardo [:mak] Assignee
	Comment 27 • 13 years ago

Comment on attachment 603482 [details] [diff] [review]
patch v1.1

restoring request, for this patch + the on-top ones.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 28 • 13 years ago

Comment on attachment 603482 [details] [diff] [review]
patch v1.1

>diff --git a/toolkit/components/places/mozIAsyncLivemarks.idl b/toolkit/components/places/mozIAsyncLivemarks.idl

> interface mozIAsyncLivemarks : nsISupports

>-   * Forces a reload of all livemarks, whether or not they've expired.

This comment seems like it was incorrect, right? The existing code seems to obey expiry times.

	Marco Bonardo [:mak] Assignee
	Comment 29 • 13 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #28)
> >-   * Forces a reload of all livemarks, whether or not they've expired.
> 
> This comment seems like it was incorrect, right? The existing code seems to
> obey expiry times.

yes, it was plain wrong unfortunately.

	Marco Bonardo [:mak] Assignee
	Comment 30 • 13 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/91a9294ae936

	jcpereira
	Comment 31 • 13 years ago

The only way to refresh all livemarks is to close Firefox and open it again?

Not very practical. Can we define the refresh interval like in the old ones?

I had browser.bookmarks.livemark_refresh_seconds set to my preference. 

In this one is there a preference to set?

	Girish Sharma [:Optimizer] Reporter
	Comment 32 • 13 years ago

(In reply to João Carlos from comment #31)
> The only way to refresh all livemarks is to close Firefox and open it again?
> 
> Not very practical. Can we define the refresh interval like in the old ones?
> 
> I had browser.bookmarks.livemark_refresh_seconds set to my preference. 
> 
> In this one is there a preference to set?

Atleast it is better than having to open every livemark to view the feed. Though I also wish that there were a preference to set time interval.

	Girish Sharma [:Optimizer] Reporter
	Comment 33 • 13 years ago

(In reply to João Carlos from comment #31)
> The only way to refresh all livemarks is to close Firefox and open it again?

The name even suggests that this is not the case. When you access any one of them, all gets refreshed one by one.

	Marco Bonardo [:mak] Assignee
	Comment 34 • 13 years ago

(In reply to João Carlos from comment #31)
> The only way to refresh all livemarks is to close Firefox and open it again?

doing this would be madness, clearly. comment 33 got it right.

	Marco Bonardo [:mak] Assignee
	Comment 35 • 13 years ago

(In reply to João Carlos from comment #31)
> In this one is there a preference to set?

No, it's hardcoded to 1 hour, we may evaluate to reduce it, but it's probably not worth it. Btw, you are free to file a bug for that.

	jcpereira
	Comment 36 • 13 years ago

(In reply to Girish Sharma from comment #33)
> (In reply to João Carlos from comment #31)
> > The only way to refresh all livemarks is to close Firefox and open it again?
> 
> The name even suggests that this is not the case. When you access any one of
> them, all gets refreshed one by one.

That only work when we open firefox and access one livemark for the first time. 

If we keep Firefox open the entire day they only refresh in one hour interval. I have to refresh manually one by one to get the most up to date news. Right now i have to close firefox and open it again to refresh all of them.

The title of this bug says "Refresh all livemarks on accessing one", but firefox only does this the first time we access one. After that it doesn´t anymore. Only if it was passed one hour after the last refresh.


If i open Firefox and check the news it update all of them. Half hour later if i check the news again it shows the same news. That is what i think it´s wrong. It should update all of them again when i access one.

	jcpereira
	Comment 37 • 13 years ago

(In reply to Marco Bonardo [:mak] from comment #35)
> (In reply to João Carlos from comment #31)
> > In this one is there a preference to set?
> 
> No, it's hardcoded to 1 hour, we may evaluate to reduce it, but it's
> probably not worth it. Btw, you are free to file a bug for that.

Why don´t you give the users the liberty to set this?

	Marco Bonardo [:mak] Assignee
	Comment 38 • 13 years ago

please keep the discussion in the new bug filed for the pref, this is completely off-topic here.

	jcpereira
	Comment 39 • 13 years ago

(In reply to Marco Bonardo [:mak] from comment #38)
> please keep the discussion in the new bug filed for the pref, this is
> completely off-topic here.

Filed Bug 734265

	Marco Bonardo [:mak] Assignee
	Comment 40 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/91a9294ae936