14824 - [Startup Performance] Defer loading bookmarks until UI has finished loading

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect, P1)

Description

[Robert John Churchill]

Defer loading bookmarks.html until UI has finished loading. On the Mac, this
should shave off another second or so from the perceived startup time... the UI
will be displayed, and THEN we'll load in the bookmark file.

Comment 1

[Robert John Churchill]

Note: to get this to work, nsBookmarkService::Init() is changed to NOT call
ReadBookmarks(). Also, in navigator.js, at the end of the Startup() function, add
a SetTimeout() function for a half-second or so later (the timer will fire after
the UI has finished loading and laying out) which, when it fires, calls
ReadBookmarks(). Also had to make a small change to the bookmark's parsing code
to recurve on a node's children BEFORE adding the node itself into the graph so
that all "containment" issues are resolved properly.

Comment 2

[Suresh Duddi (gone)]

That would be good. Will this ensure that the personal toolbar will just fillin
as we readin the bookmarks...

Comment 3

[Robert John Churchill]

Yes.

Comment 4

[Robert John Churchill]

Marking this as WONTFIX.  A poll of various Mac-heads concluded that they prefer
having 100% of the chrome and its contents loaded in before the browser window
appears.

Comment 5

[Tom Everingham]

marking verified per engineer's last comments

Comment 6

[Paul Chen]

reopening and taking this bug

Comment 7

[Paul Chen]

taking

Comment 8

[Paul Chen]

Argh, no, really, taking it this time, sorry for the spam.

Comment 9

[Paul Chen]

Attachment: call ReadBookmarks() after timeout in Startup()

Comment 10

[Paul Chen]

Attachment: add protected initDatasource() method

Comment 11

[Paul Chen]

Attachment: nsBookmarksService::Init() now just calls initDatasource, protect against loading bookmarks file every call to ReadBookmarks, and reset loaded flag when profile has changed

Comment 12

[Paul Chen]

Attachment: load bookmarks from a timer setup in Startup()

Comment 13

[Paul Chen]

Attachment: add new protected members LoadBookmarks and initDatasource

Comment 14

[Paul Chen]

Attachment: don't load bookmarks in nsBookmarkService::Init but do initialize datasource, in ReadBookmarks check to see if bookmarks are already loaded

Comment 15

[Paul Chen]

showed diffs to rjc earlier, verbal r=rjc

Comment 16

[Blake Ross]

cool. sr=blake on those.

Comment 17

[Paul Chen]

checked into trunk

Comment 18

[Blake Ross]

question, wouldn't a timeout of 0 still have the same effect (forcing the 
window to show before loading the bookmarks)?

Comment 19

[Robert John Churchill]

Paul, just thought of something.  This change may have exposed a race condition
between when ReadBookmarks() is called via the timer firing from Startup(), and
the Bookmarks sidebar panel.  You might want to test... and potentially add a
ReadBookmarks()s call and ___.rebuild() into the bookmark sidebar panel's
onload() function.

I'll reopen this bug.

Comment 20

[David Hyatt]

Please change the 100ms timeout to a 0ms timeout.  That is sufficient to delay 
until after the window shows, and prevents us from basically sitting around for 
100ms doing nothing.

Comment 21

[David Hyatt]

Leaks are up following this checkin.

Comment 22

[rpotts (gone)]

yes... 

it appears that nsBookmarksService::initDataSource(...) is called multiple
times...  however, mInner is not released (if non-null) so previous instances
get leaked :-(

-- rick

Comment 23

[rpotts (gone)]

here's the line...
http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/src/nsBookmarksService.cpp#4141

Comment 24

[Chris Waterson]

We should change that to NS_IF_RELEASE(mInner).

Comment 25

[Paul Chen]

leak was fixed in bugzilla 108539, marking fixed

Comment 26

[Mike Jaques]

>Please change the 100ms timeout to a 0ms timeout.  That is sufficient to delay 
>until after the window shows, and prevents us from basically sitting around 
>for 100ms doing nothing.

New bug?