Closed
Bug 522066
Opened 12 years ago
Closed 11 years ago
Awesomescreen unresponsive when "All bookmarks" loading
Categories
(Firefox for Android Graveyard :: Bookmarks, defect, P1)
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)
7.19 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•12 years ago
|
tracking-fennec: --- → ?
Whiteboard: [polish]
Reporter | ||
Updated•12 years ago
|
Priority: -- → P1
Updated•12 years ago
|
tracking-fennec: ? → 1.0+
Comment 1•12 years ago
|
||
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•11 years ago
|
||
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
Comment 3•11 years ago
|
||
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).
Assignee | ||
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
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.
Assignee | ||
Comment 6•11 years ago
|
||
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 7•11 years ago
|
||
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+
Assignee | ||
Comment 8•11 years ago
|
||
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•11 years ago
|
||
(In reply to comment #8) > Could bump the limit down to 80% just to be safe, though. Sounds like a good plan
Assignee | ||
Comment 10•11 years ago
|
||
With that change, and a comment.
Attachment #417547 -
Attachment is obsolete: true
Assignee | ||
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mobile-browser/rev/b59ae0b0351e
Status: NEW → RESOLVED
Closed: 11 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Post-B5
Comment 12•11 years ago
|
||
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
Assignee | ||
Comment 13•11 years ago
|
||
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 → ---
Assignee | ||
Updated•11 years ago
|
Target Milestone: --- → RC
Updated•11 years ago
|
Component: General → Bookmarks
You need to log in
before you can comment on or make changes to this bug.
Description
•