Closed Bug 473058 Opened 11 years ago Closed 11 years ago

RSS feeds automatically close once done updating

Categories

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

defect

Tracking

()

RESOLVED INVALID
Firefox 3.6a1

People

(Reporter: stifu, Assigned: dietrich)

References

Details

(Keywords: regression)

Attachments

(2 files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090110 Shiretoko/3.1b3pre (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090110 Shiretoko/3.1b3pre (.NET CLR 3.5.30729)

See title. This behavior is new (introduced a few days ago, it seems), and I don't think it's expected.
Always happens, on browser start up, too.

If you opened a feed while it was updating, then the article list will close before your eyes once it's done reloading. To reproduce the problem, right click a feed to force the reload, then quickly open the feed before it's done reloading.

Reproducible: Always
Summary: RSS feeds automatically close once done updating → RSS feeds automatically close once done updated
Summary: RSS feeds automatically close once done updated → RSS feeds automatically close once done updating
Component: RSS Discovery and Preview → Bookmarks & History
QA Contact: rss.preview → bookmarks
Yeah, I noticed this too. I have a folder on my toolbar with folders, bookmarks and one feed in it. When I reload the feed the whole folder collapses. This was not the case in Firefox 3.
This regressed on 2 Jan 2009: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-01-02+03%3A00%3A00&enddate=2009-01-03+04%3A00%3A00
so probably from bug 432706.
Blocks: 432706
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox3.1?
Keywords: regression
Version: unspecified → 3.1 Branch
I have also noticed this on Shiretoko since early January.  When I start up Firefox, I have to wait a few minutes to click on my RSS Feeds folder in the Bookmarks Toolbar.  This is when the RSS Feeds are reloading.

I have also noticed that when I manually do a Reload Live Bookmark, I will see the greyed out "Live Bookmark Loading" and then it closes the whole folder.  Before, it would not close, I would just see it.

It is a bit annoying, but worst at the beginning of a new session when the RSS Feeds folder is unavailable for a few minutes.
Attached image screenshot
I see also movements on the toolbar when the livemarks are loading. The first bookmark in the overflow menu covers shortly the arrow. This happens 11 times, and indeed, I have 11 livemarks :)
blocking, p2.
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Priority: -- → P2
Assignee: nobody → dietrich
OS: Windows XP → All
Hardware: x86 → All
Target Milestone: --- → Firefox 3.2a1
Version: 3.1 Branch → Trunk
Duplicate of this bug: 473301
hm, first - in the toolbar binding, if the menu is open during a rebuild, shouldn't close it, just clear and repopulate it's contents... if it's possible to do without excessive jumpiness. another option would be to just remove-and-replace each node, one at a time (and remove leftovers).

second - i'm seeing invalidateContainer() called for both the affected node, as well as the root. a log:

*** invalidateContainer(): begin: bookmarksBarContent
*** invalidateContainer(): rebuilding a sub-container
*** invalidateContainer(): and live-updating it
*** invalidateContainer(): done
*** invalidateContainer(): begin: bookmarksBarContent
*** invalidateContainer(): rebuilding at root
*** invalidateContainer(): returning early
For completeness, not only updating feeds are causing this problem; also deleting a bookmark in a toolbar folder.
every operation done in batch will cause that since we are noi refreshing and the toolbar is a container always open and observing.
Attached patch v1Splinter Review
not optimal, but works. needs tests still, but want some input on the approach first.

- the popup flashes on rebuild. this is due to the fact that the root container does a full rebuild when invalidateContainer() is called, causing the popup to be completely deleted and recreated. when rebuilding, instead of "removing all, then add all", could maybe try a "replace, then delete/add as necessary" approach. not sure if this would make things more jumpy or less...

- invalidateContainer() is called once for *each* container, which means lots of unnecessary rebuilds of subcontainers on endUpdateBatch. i tried modifying nsNavHistoryFolderResultNode to only call invalidateContainer() on the root node, and it seemed to be ok. however, there are might be scenarios where that's not ok, and so haven't included that change yet, need to think more on it.
Attachment #359578 - Flags: review?(mak77)
the patch doesn't quiet work for me, if you're browsing through a bookmark and theres an update on a page or an rss feed, it seems to reset it back to the main bookmark menu
Comment on attachment 359578 [details] [diff] [review]
v1

>diff --git a/browser/components/places/content/toolbar.xml b/browser/components/places/content/toolbar.xml
>
>+            if (this.firstChild.firstChild &&
>
>+                this.firstChild.firstChild.tagName == "menupopup" &&
>
>+                this.firstChild.firstChild.state == "open" &&

you could probably check if firstChild's attribute "type" is "menu" and its open property

mh, there's another problem with this approach, it is only considering first level, for example i have an RSS folder in the toolbar containing 6/7 live bookmarks, and that's jumping badly. This is more a workaround for the first level folders on the toolbar, still causing some kumpiness by the fact the menu is dismissed and reopened immediately :\
We should walk all the nodes to annotate opened ones, i fear that could cause some ui slowdown/locking, not sure this is a clean way to go...

Instead the second purpose (try to reduce number of calls to invalidate container) could be a good workaround for observers paths, i mean, we could backout observers patch and try apply this method to reduce number of views updates. Not sure will work but could be worth trying.
Attachment #359578 - Flags: review?(mak77)
the incremental updade way could work better... not sure how much it would be complex to implement though, we don't know what happened during the batch
(In reply to comment #10)
> the patch doesn't quiet work for me, if you're browsing through a bookmark and
> theres an update on a page or an rss feed, it seems to reset it back to the
> main bookmark menu

the patch only works for the toolbar at the moment.
(In reply to comment #11)
> Instead the second purpose (try to reduce number of calls to invalidate
> container) could be a good workaround for observers paths, i mean, we could
> backout observers patch and try apply this method to reduce number of views
> updates. Not sure will work but could be worth trying.

Hm, I don't understand what you mean. Before the observers patch, folderObserver.onEndUpdateBatch() didn't call viewer.invalidateContainer(). If we revert that patch, we go back to live-update all the time.
Bug 432706 got backed out, so this is now INVALID.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.