Closed Bug 521116 Opened 10 years ago Closed 10 years ago

showing "All bookmarks" from the awesomebar/autocomplete dropdown option can be very slow

Categories

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

defect

Tracking

(fennec1.0+)

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

People

(Reporter: Gavin, Assigned: vingtetun)

References

Details

(Whiteboard: [polish])

Attachments

(1 file, 3 obsolete files)

From Madhava's bug 514220 comment 7:

We get row-highlighting immediately now, which is helpful, but it still takes a
number of seconds to bring up the bookmarks list when you have a lot of
bookmarks (as you do after a weave sync with your desktop set).  I think that
we need something more than just immediate row-highlighting to give the user a
sense that something is actually happening.  There was an idea in the original
bug summary to replace the > indicator with a throbber when we're waiting for
the list to come up.

Also - is there any way to speed up bringing up the list?
Whiteboard: [polish]
tracking-fennec: --- → ?
(In reply to comment #0)
> From Madhava's bug 514220 comment 7:
> 
> We get row-highlighting immediately now, which is helpful, but it still takes a
> number of seconds to bring up the bookmarks list when you have a lot of
> bookmarks (as you do after a weave sync with your desktop set).  I think that
> we need something more than just immediate row-highlighting to give the user a
> sense that something is actually happening.  There was an idea in the original
> bug summary to replace the > indicator with a throbber when we're waiting for
> the list to come up.

That would be cool!

> Also - is there any way to speed up bringing up the list?

What I'm doing when I have a bunch of elements to quickly display is the following:
 * clone the container
 * set display:none to the clone
 * append the bunch of items into the clone
 * replace the container by the clone
 * remove display:none on the clone (aka the new container)

I'm doing that to display thousands of items in a custom filemanager in XUL and it works great.

I can do a wip to test that on our bookmarks and see how long it takes.
Attached patch wip (obsolete) — Splinter Review
Ok. The wip allow a small speedup but not as much as I've expected.
The icons loading kill us a bit and the complicated structure of the bookmarks
too.
We should also not make the screen 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.
(In reply to comment #3)
> We should also not make the screen 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.

That's an entirely different bug, which you should file.
Priority: -- → P1
tracking-fennec: ? → 1.0+
Duplicate of this bug: 526511
lets make sure that panning of bookmarks work good with the long list.
This patch did not adress all the bug part (the throbber one as example) but speed up a bit the display of the list:

This is the result I have on a n810:
bookmarks : 17

Current       : 669, 662, 662, 653, 660 (AVG: 661.2)
Patch applied : 376, 383, 385, 385, 376 (AVG: 381)

~ 42% gain

I guess the benefit will depends on the number of items
Attachment #405674 - Attachment is obsolete: true
Attachment #413488 - Flags: review?(mark.finkle)
Comment on attachment 413488 [details] [diff] [review]
patch for speeding up a bit the display of the list

>diff -r 882f33509828 chrome/content/bindings.xml

>-      <field name="_ioService" readonly="true">
>-        Components.classes["@mozilla.org/network/io-service;1"]
>-                  .getService(Components.interfaces.nsIIOService);
>-      </field>

We should keep the ioservice field. <field>s are lazily evaluated and are only evaluated once, not each time they are accessed. So it's faster than requesting the service each time, like you do below.

>+              // XXX don't forget to change that
>               // If the URI was updated change it in the bookmark, but don't
>               // allow a blank URI. Revert to previous URI if blank.

What does the XXX refer too?

>diff -r 882f33509828 themes/hildon/browser.css

>+placeitem[src=""] .bookmark-item-label > image {
>+  list-style-image: url(chrome://mozapps/skin/places/defaultFavicon.png);

We have our own default favicon at images/favicon-default-30.png

Hmm, I guess we need to make that 32px now that bookmarks have 32px images. File a bug since we use the default for the URLBar too. The current 30px image will just be stretched for now.

>diff -r 882f33509828 themes/wince/browser.css

>+placeitem[src=""] .bookmark-item-label > image {
>+  list-style-image: url(chrome://mozapps/skin/places/defaultFavicon.png);
>+}
>+

Move to browser-low (24px) and browser-high (32px like hildon)

Nice refactor. Removing the init method and it's use of the favicon service + the document fragment seemed to help a lot.

Should be ready to land soon!
Attachment #413488 - Flags: review?(mark.finkle) → review-
(In reply to comment #9)
> (From update of attachment 413488 [details] [diff] [review])
> >diff -r 882f33509828 chrome/content/bindings.xml
> 
> >+              // XXX don't forget to change that
> >               // If the URI was updated change it in the bookmark, but don't
> >               // allow a blank URI. Revert to previous URI if blank.
> 
> What does the XXX refer too?

It was related to the ioService changed but it was more for remind it to change it for myself.

> 
> >diff -r 882f33509828 themes/hildon/browser.css
> 
> >+placeitem[src=""] .bookmark-item-label > image {
> >+  list-style-image: url(chrome://mozapps/skin/places/defaultFavicon.png);
> 
> We have our own default favicon at images/favicon-default-30.png

I have used this one because that's the one places use for giving us the result in the awesomebar.
I've let the defaultFavicon.png in this patch - I can change if it is really needed.

 
> Hmm, I guess we need to make that 32px now that bookmarks have 32px images.
> File a bug since we use the default for the URLBar too. The current 30px image
> will just be stretched for now.

Filed bug 530062
Attached patch Patch v0.2 for display boost (obsolete) — Splinter Review
oups, i've forgot the patch!
Attachment #413488 - Attachment is obsolete: true
Comment on attachment 413598 [details] [diff] [review]
Patch v0.2 for display boost

>diff -r f305d4de5ece chrome/content/bindings.xml

>+      <property name="uri">

>+          if (!this._uri && this._getAttribute("uri")) {
>+            this._uri = this._ioService.newURI(this._uri, null, null);

This doesn't look right...
Attached patch PatchSplinter Review
(In reply to comment #12)
> (From update of attachment 413598 [details] [diff] [review])
> >diff -r f305d4de5ece chrome/content/bindings.xml
> 
> >+      <property name="uri">
> 
> >+          if (!this._uri && this._getAttribute("uri")) {
> >+            this._uri = this._ioService.newURI(this._uri, null, null);
> 
> This doesn't look right...

true...


I've updated the patch and made it pass all browser-chrome tests.

I've also change one line in the browser_bookmarks.js test, otherwise the click is not dispatched to the bookmark for some reason.

There is also a bug on the textbox.xml binding which cause some test to fail in some case (and throw an error in normal too), I've filed bug 530496 for that.
Attachment #413598 - Attachment is obsolete: true
Attachment #414020 - Flags: review?(mark.finkle)
Attachment #414020 - Flags: review?(mark.finkle) → review+
Assignee: nobody → 21
pushed:
https://hg.mozilla.org/mobile-browser/rev/597af8186228
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Post-B5
Component: General → Bookmarks
You need to log in before you can comment on or make changes to this bug.