363634 - Startup event for livemark feed parsing

Status: RESOLVED DUPLICATE of bug 382711

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

Description

[Robert Sayre]

Right now, the initial import of the "Latest Headline" feed will trigger a folder creation, but not populate it on startup (avoiding HTTP traffic during startup). Need to listen for an event to kick off refreshes.

Comment 1

[Dietrich Ayala (:dietrich)]

rob, what is needed here? an event after startup is complete, saying that it's ok to kickoff HTTP traffic, instead of waiting on the livemark service timer?

Comment 2

[Robert Sayre]

(In reply to comment #1)
> rob, what is needed here? an event after startup is complete, saying that it's
> ok to kickoff HTTP traffic, instead of waiting on the livemark service timer?

Yeah, we want to kick off this traffic after the first window is up. I gather that "delayedStartup" is too soon.

Comment 3

[Dan Mills [:thunder]]

Due to the fix in bug 380999, the places import-to-folder tests now hit this bug.  They are commented out atm, will need to be re-enabled when this is fixed.

Comment 4

[Steve Won (stevewon on irc)]

Can we load all the feeds when the folder is created?

Comment 5

[(not reading, please use [e-mail] instead)]

steve / dietrich, see also bug #364304

Comment 6

[Steve Won (stevewon on irc)]

Attachment: Proposing a patch

When the first window is open, notifyObservers is called with "first-browser-window-after-show"... maybe a better naming?

Would there be a more efficient way to figure out how many windows are there without iterating through the nsIWindowMediator's enumerator?

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 268302 [details] [diff] [review]
Proposing a patch


>Index: browser/base/content/browser.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser.js,v
>retrieving revision 1.796
>diff -u -8 -p -r1.796 browser.js
>--- browser/base/content/browser.js	6 Jun 2007 20:17:26 -0000	1.796
>+++ browser/base/content/browser.js	14 Jun 2007 01:09:44 -0000
>@@ -1154,16 +1154,30 @@ function delayedStartup()
>       var ss = Cc["@mozilla.org/browser/sessionstore;1"].
>                getService(Ci.nsISessionStore);
>       ss.init(window);
>     } catch(ex) {
>       dump("nsSessionStore could not be initialized: " + ex + "\n");
>     }
>   }
> 
>+  // Checking to see if this is the first window loaded.
>+  var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
>+                     .getService(Components.interfaces.nsIWindowMediator);
>+  var enumerator = wm.getEnumerator(null);
>+  var i = 0;
>+  while (enumerator.hasMoreElements()) {
>+    i++;
>+    if (i>1)
>+	  break;
>+    enumerator.getNext();
>+  }
>+  if (i == 1)
>+    os.notifyObservers(null, "first-browser-window-after-show", "");
>+

you could probably simplify this to something like:

enumerator.getNext();
if (enumerator.hasMoreElements())
  os.notifyObservers(...);

>+  observe: function(aSubject, aTopic, aData) {
>+    switch (aTopic) {
>+      case "first-browser-window-after-show":
>+	    for (var i=0; i < this._livemarks.length; ++i) {
>+	      this._updateLivemarkChildren(i, false);
>+	    }
>+	    break;
>+    }
>+  },
>+  

the fix needs to *prevent* livemark updates from occurring before the first window comes up.

Comment 8

[Steve Won (stevewon on irc)]

Attachment: Patch v2

In this patch, the observer for first-browser-window-after-show is removed when the event is caut, and timer for livemarks update is set here.

Comment 9

[Steve Won (stevewon on irc)]

Attachment: Spacing fixed

Spacing fixed

Comment 10

[Dietrich Ayala (:dietrich)]

retargeting bugs that don't meet the alpha release-blocker criteria at http://wiki.mozilla.org/Firefox3/Schedule.

Comment 11

[Steve Won (stevewon on irc)]

I was talking to Gavin and he brought this up.
Mac can have the app open but the window closed. So closing + reopening a window will cause first-browser-window-after-show to be fired multiple times. Will that cause problems for anything?

As for the livemark feed parsing, the timer (which fires every 15 sec) to update the livemarks would be called every time... so one way would be to check if the timer has been initiated already whenever first-browser-window-after-show is caught?

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

I am interested in how the notification in the patch relates to the same problem in bug 364304 and bug 386605. The issue with this notification being a hack to get around Ts regressions also comes to mind.

Does livemark feed parsing need to wait for the window to show because of technical reasons?

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

bug 386605 also mentions the desire to make this notification originate from somewhere in toolkit and not browser.js as this is not only a browser issue.

Also, if the intent is to provide an event/notification that signals it is safe for services to do work without effecting Ts, "first-browser-window-after-show" should be changed to something else. How about "final-ui-ready" to match with "final-ui-startup" (fired right before the first window is shown)

Comment 14

[Robert Sayre]

(In reply to comment #12)
> I am interested in how the notification in the patch relates to the same
> problem in bug 364304 and bug 386605. The issue with this notification being a
> hack to get around Ts regressions also comes to mind.

Well, the other day I was accused of "covering up a leak" with a patch that freed memory. ;) 

Ts is how we measure responsiveness on startup for users. One way to improve this responsiveness  is to put off things that aren't essential until the first window is up. A bad hack would be a change that evades Ts, but still delays the first window.

> Does livemark feed parsing need to wait for the window to show because of
> technical reasons?

Yes, service instantiation is not free, and livemark HTTP traffic is not free. It is best to avoid doing this during startup. What this means is that there will be possibly stale RSS content in the live bookmark for a sec after startup. I think that's the right way to do it.

Comment 15

[Steve Won (stevewon on irc)]

(In reply to comment #13)
> bug 386605 also mentions the desire to make this notification originate from
> somewhere in toolkit and not browser.js as this is not only a browser issue.
> 
So, should this bug wait on 386605? Or can this patch be a temporary solution until 386605 is resolved?

> Also, if the intent is to provide an event/notification that signals it is safe
> for services to do work without effecting Ts, "first-browser-window-after-show"
> should be changed to something else. How about "final-ui-ready" to match with
> "final-ui-startup" (fired right before the first window is shown)
> 

Sound good. I am bad at naming variables.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Is it possible to test something? We fire a notification ("final-ui-starup") here:
http://mxr.mozilla.org/seamonkey/source/toolkit/xre/nsAppRunner.cpp#3038

Why not fire a new notification ("final-ui-ready" for lack of a better name) here:
http://mxr.mozilla.org/seamonkey/source/toolkit/xre/nsAppRunner.cpp#3042

if that is too soon, keep pushing it further down XRE_main

Comment 17

[Shawn Wilsher :sdwilsh]

Mark, I think what you are suggesting is what Bug 386605 is looking for.

Comment 19

[Mike Connor [:mconnor]]

Florian, you should be able, at the worst, to just kick off the livemark parsing from delayedStartup as in bug 384810...

Comment 20

[Florian Quèze [:florian]]

I think the original issue here was fixed another way in bug 382711. Can we WONTFIX this?

Comment 21

[Florian Quèze [:florian]]

mconnor suggested I mark it as a dupe instead of WONTFIX'ing.

Comment 22

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv