564885 - Bookmarks toolbar folders take ~1/2 second to open

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Daniel Holbert [:dholbert]]

STEPS TO REPRODUCE:
 1. Start Firefox [with a fresh profile, for simplicity]
 2(a). Click "most visited" or "latest headlines" pseudo-folders on your bookmarks toolbar.
    OR:
 2(b) Right-click bookmarks toolbar, and choose "New Folder", click "Add", and then try to open the folder.

ACTUAL RESULTS:
  Folder takes ~1 sec to open.

First bad build is:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a5pre) Gecko/20100506 Minefield/3.7a5pre

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=29bc8f25685d&tochange=014349e11696
in which this push looks most suspicious:
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=d7393e28fb2d

Comment 1

[Marco Bonardo [:mak]]

We special handle toolbar menus opening on Linux to allow drag&drop without a key modifier, could be something broke there in the bindings rewrite.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: screencast: broken behavior

Here's a screencast of the broken behavior, with the 20100506 nightly.  The menu item "blinks" when I click it -- note the delay from the "blink" to the menu actually opening.

(Maybe it's ~1/2 second rather than a full second, but it's still very tangible & slow from a user's perspective.)

Comment 3

[Marco Bonardo [:mak]]

Yeah, if it's half a second, is what i was talking about in comment 1, we delay menu opening to allow drag&drop, but as soon as you mouseup it should open instantly (and this is probably the part that broke)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: screencast: working behavior

and here's the working behavior, with the 20100502 nightly.  (I meant record the 20100505 nightly, but accidentally did this one -- I've verified that the 20100505 behavior matches this, though.)

Here, the menu opens before the "blink" even completes, I think.

Comment 5

[Marco Bonardo [:mak]]

yep there is a missing onMouseUp handler in
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/browserPlacesViews.js#982

Actually we should not even have those on other platforms, thus that probably needs some ifdef love too.

I'll take this for now.

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1.0

This should make it, but I have to test it

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 444874 [details] [diff] [review]
patch v1.0

ok, tested on ubuntu 64, looks good.
I guess a test for this timings would be hard or too much prone to randomness.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 444874 [details] [diff] [review]
patch v1.0

r=mano

Comment 9

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/b66869585d06