Closed Bug 522066 Opened 10 years ago Closed 10 years ago

Awesomescreen unresponsive when "All bookmarks" loading

Categories

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

x86
macOS
defect

Tracking

(fennec1.0+)

VERIFIED FIXED
fennec1.0
Tracking Status
fennec 1.0+ ---

People

(Reporter: madhava, Assigned: Gavin)

Details

(Whiteboard: [polish])

Attachments

(1 file, 2 obsolete files)

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.
tracking-fennec: --- → ?
Whiteboard: [polish]
Priority: -- → P1
tracking-fennec: ? → 1.0+
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.
Firefox chunks the loading of bookmarks, fwiw. Ed or Shawn might have some suggestions, here. Gavin said he'd be able to do it.
Assignee: nobody → gavin.sharp
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).
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.
Attached patch patch (obsolete) — Splinter Review
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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #416833 - Attachment is obsolete: true
Attachment #417547 - Flags: review?(mark.finkle)
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
Attachment #417547 - Flags: review?(mark.finkle) → review+
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.
(In reply to comment #8)

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

Sounds like a good plan
Attached patch patchSplinter Review
With that change, and a comment.
Attachment #417547 - Attachment is obsolete: true
https://hg.mozilla.org/mobile-browser/rev/b59ae0b0351e
Status: NEW → RESOLVED
Closed: 10 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Post-B5
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
Status: RESOLVED → VERIFIED
Things shouldn't be any slower on first open. File a bug if they are?
OS: All → Mac OS X
Hardware: All → x86
Target Milestone: RC → ---
Target Milestone: --- → RC
Component: General → Bookmarks
You need to log in before you can comment on or make changes to this bug.