731274 - Refresh all livemarks on accessing one

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Girish Sharma [:Optimizer]]

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.

Comment 1

[jcpereira]

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

Comment 2

[Marco Bonardo [:mak]]

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?

Comment 3

[Girish Sharma [:Optimizer]]

(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

Comment 4

[jcpereira]

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.

Comment 5

[Marco Bonardo [:mak]]

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

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1.0

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/[e-mail]-694dbccf7a11/

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

Comment 7

[Girish Sharma [:Optimizer]]

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.

Comment 8

[Marco Bonardo [:mak]]

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

Comment 9

[jcpereira]

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.

Comment 10

[Marco Bonardo [:mak]]

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

Comment 11

[Girish Sharma [:Optimizer]]

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

Comment 12

[Marco Bonardo [:mak]]

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

Comment 13

[Girish Sharma [:Optimizer]]

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.

Comment 14

[jcpereira]

Yes. I saw this with the try build.

Comment 15

[Marco Bonardo [:mak]]

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

which OS?

Comment 16

[jcpereira]

(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

Comment 17

[jcpereira]

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

Comment 18

[Marco Bonardo [:mak]]

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

Comment 19

[Marco Bonardo [:mak]]

Attachment: patch v1.1

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.

Comment 20

[Dietrich Ayala (:dietrich)]

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

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

r=me

Comment 21

[Asaf Romano (gone)]

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.

Comment 22

[Marco Bonardo [:mak]]

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

Comment 23

[Marco Bonardo [:mak]]

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

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

Comment 24

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 25

[Marco Bonardo [:mak]]

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

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

(In reply to Gavin Sharp (use [e-mail] 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 :(

Comment 26

[Marco Bonardo [:mak]]

Attachment: patch on top v1.1

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.

Comment 27

[Marco Bonardo [:mak]]

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

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

Comment 28

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 29

[Marco Bonardo [:mak]]

(In reply to Gavin Sharp (use [e-mail] 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.

Comment 30

[Marco Bonardo [:mak]]

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

Comment 31

[jcpereira]

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?

Comment 32

[Girish Sharma [:Optimizer]]

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

Comment 33

[Girish Sharma [:Optimizer]]

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

Comment 34

[Marco Bonardo [:mak]]

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

Comment 35

[Marco Bonardo [:mak]]

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

Comment 36

[jcpereira]

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

Comment 37

[jcpereira]

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

Comment 38

[Marco Bonardo [:mak]]

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

Comment 39

[jcpereira]

(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

Comment 40

[Marco Bonardo [:mak]]

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