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

Status: RESOLVED FIXED

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

Description

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

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?

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip

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.

Comment 3

[Madhava Enros [:madhava]]

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.

Comment 4

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

(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.

Comment 5

[Madhava Enros [:madhava]]

opened as Bug 522066

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

lets make sure that panning of bookmarks work good with the long list.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: patch for speeding up a bit the display of the 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

Comment 9

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

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!

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2 for display boost

oups, i've forgot the patch!

Comment 12

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

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...

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

(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.

Comment 14

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

pushed:
https://hg.mozilla.org/mobile-browser/rev/597af8186228