522066 - Awesomescreen unresponsive when "All bookmarks" loading

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Bookmarks, defect, P1)

Description

[Madhava Enros [:madhava]]

related to Bug 521116
	
The awesomescreen is unresponsive when the user is waiting for the bookmarks list to come up.  Tapping on another row should go to the tapped site, and hitting the "back out" button in the upper right should back out of the screen.

Comment 1

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

I don't see how we can be responsive _and_ load the bookmarks quickly. We'd have to yield or chunk the process of loading the bookmarks, which makes it slower.

Minimizing the amount of bookmarks in the initial screen is a cheap way to speed everything up and is one of our goals.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Firefox chunks the loading of bookmarks, fwiw. Ed or Shawn might have some suggestions, here. Gavin said he'd be able to do it.

Comment 3

[Shawn Wilsher :sdwilsh]

Can somebody fill me in a bit more on the architecture of how this works on Fennec please?  Trying to understand the problem better, and comment 0 isn't doing a great job for me (being outside of Fennec development).

Comment 4

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

http://mxr.mozilla.org/mobile-browser/source/chrome/content/bindings.xml#721 (_getChildren) is how we query for bookmarks. http://mxr.mozilla.org/mobile-browser/source/chrome/content/bindings.xml#791 (openFolder) is how we add them to the list. Both of these steps together is slow with many bookmarks (though I'm not sure what proportion of the time is spent in each).

Dietrich says there are no APIs for requesting "sets" or pages of results from the history service, so I don't think it's possible to batch those queries, but we certainly can just keep the results in memory and only display a certain number at a time.

Comment 5

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

Attachment: patch

This avoids adding more than 25 bookmarks to the list unless you pan near the end (i.e. we add the items shortly before you'd reach them). Doing stuff onscroll isn't ideal, since it's called often while panning, but I'm not sure there are any better ways to get the scroll-triggered behavior.

Comment 6

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

Attachment: patch

Implements dynamic addition of items in batches, based on scroll position. Also adjusted the code to only have the scroll listener added when it was needed, and attempted to minimize the amount of work it does to reduce it's impact on panning perf.

Comment 7

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

Comment on attachment 417547 [details] [diff] [review]
patch


>-      <property name="scrollBoxObject" readonly="true" onget="return this._children.scrollBoxObject;"/>
>+      <field name="scrollBoxObject">this._children.scrollBoxObject</field>
>+      <field name="_childrenHeight">this.scrollBoxObject.height;</field>

window resizes while the manager is open would not see the new height, right? since the field is interpreted once.

r+, but let me know what you think about the prop vs field thing

Comment 8

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

I'd rather avoid getting the height on each scroll event, and invalidating only when the window resizes is a pain. I think what we can do instead is just not care about height changes, since we don't really need an exact measure of the scroll position - we just need to know roughly when the user's near the end of the list. So for the window shrinking case, just means we'll add new bookmarks sooner, and for the window growing case, means we'll potentially show them a bit later. Only concern is that we'd end up showing them so much later that all items are already on screen and the user can notice it, but since the scrollbox height is small in comparison to the scroll height, that's pretty unlikely. Could bump the limit down to 80% just to be safe, though.

Comment 9

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

(In reply to comment #8)

> Could bump the limit down to 80% just to be safe, though.

Sounds like a good plan

Comment 10

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

Attachment: patch

With that change, and a comment.

Comment 11

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

https://hg.mozilla.org/mobile-browser/rev/b59ae0b0351e

Comment 12

[Aakash Desai [:aakashd]]

This is really slow on first load of the bookmarks manager after installing and syncing Weave. Afterwards, this works great. 

verified FIXED on builds:
Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b5pre) Gecko/20091216 Firefox/3.6b5pre Fennec/1.0b6pre

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091216 Firefox/3.7a1pre Fennec/1.0b5

Comment 13

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

Things shouldn't be any slower on first open. File a bug if they are?