Refresh all livemarks on accessing one

RESOLVED FIXED in mozilla13

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Optimizer, Assigned: mak)

Tracking

13 Branch
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Component: Untriaged → Places
Product: Firefox → Toolkit
Blocks: livemarksIO
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: asynchronous load-on-demand livemarks should be put behind a preference with the preference on by default → asynchronous load-on-demand livemarks should be put behind a preference
I think also that should also be a preference for the refresh time.
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?
(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
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.
(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.
Assignee: nobody → mak77
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: asynchronous load-on-demand livemarks should be put behind a preference → Refresh all livemarks on accessing one
Posted patch patch v1.0 (obsolete) — Splinter Review
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.
Whiteboard: [needs SR]
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.
(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.
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.
(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?
Can it be pushed to central? , as the try builds are looking fine.
as soon as I can write a test and get a review, sure.
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.
Yes. I saw this with the try build.
(In reply to João Carlos from comment #14)
> Yes. I saw this with the try build.

which OS?
(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
(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.
(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.
Posted patch patch v1.1Splinter Review
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.
Attachment #602351 - Attachment is obsolete: true
Attachment #603482 - Flags: review?(dietrich)
Comment on attachment 603482 [details] [diff] [review]
patch v1.1

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

r=me
Attachment #603482 - Flags: review?(dietrich) → review+
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.
(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 on attachment 603482 [details] [diff] [review]
patch v1.1

need SR for the addition of the optional aForceUpdate param to the reloadLivemarks API.
Attachment #603482 - Flags: superreview?(gavin.sharp)
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 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 :(
Attachment #603482 - Flags: superreview?(gavin.sharp)
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.
Attachment #603822 - Flags: review?(gavin.sharp)
Comment on attachment 603482 [details] [diff] [review]
patch v1.1

restoring request, for this patch + the on-top ones.
Attachment #603482 - Flags: superreview?(gavin.sharp)
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.
Attachment #603482 - Flags: superreview?(gavin.sharp) → superreview+
Attachment #603822 - Flags: review?(gavin.sharp) → review+
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/91a9294ae936
Whiteboard: [needs SR]
Target Milestone: --- → mozilla13
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?
(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.
(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.
(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.
(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.
Depends on: 734073
No longer depends on: 734073
(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.
(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?
please keep the discussion in the new bug filed for the pref, this is completely off-topic here.
(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
https://hg.mozilla.org/mozilla-central/rev/91a9294ae936
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
QA Contact: untriaged → places
You need to log in before you can comment on or make changes to this bug.