If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Context menu slow to appear over image

VERIFIED FIXED in mozilla1.0.1

Status

SeaMonkey
UI Design
VERIFIED FIXED
16 years ago
9 years ago

People

(Reporter: Christopher Aillon (sabbatical, not receiving bugmail), Assigned: Stephen P. Morse)

Tracking

({perf})

Trunk
mozilla1.0.1
x86
All

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

1.71 KB, patch
Christopher Aillon (sabbatical, not receiving bugmail)
: review+
Chris Waterson
: superreview+
jesup
: approval+
Details | Diff | Splinter Review
Right clicking on an image takes approximately 1-2 seconds for the context menu
to appear whereas right clicking anywhere else loads the context menu
instantaneously.

I've seen this problem for a few months and couldn't find anything in Bugzilla
about it.

Currently using Linux 2001121708.

Comment 1

16 years ago
I see no speed difference, caillon.  Tried a fresh profile?  Linux 2001121606

Comment 2

16 years ago
Then again, it does appear somewhat slower over images, though not as much as
you say.  I have a K6-2 500 512MB RAM, what do you run on?

Comment 3

16 years ago
Could this be due to the large number of items in the context menu?
This is not blake's
Assignee: hyatt → blakeross

Comment 5

16 years ago
changing OS -> All
I've also seen this behaviour in win98 for some time (maybe around 2 months) and
I thought that it was already reported.
In my P2-350Mh 128Mb it can take easily more that 3 or 4 seconds to show the
context menu on images, so I don't use it anymore.
Build 2001-12-17-03, win98
OS: Linux → All

Updated

16 years ago
Component: XP Toolkit/Widgets: Menus → XP Apps: GUI Features
QA Contact: jrgm → sairuh
Target Milestone: --- → mozilla1.1

Comment 6

16 years ago
-> law
Assignee: blaker → law
Keywords: perf
Does this happen on all images?  Only some images?
Over every image.  I am beginning to speculate that this is related to the
number of cookies in the Allow/Deny list... I have cookies set to ask me when a
site requests to set a cookie.  I recently cleared my whole list and noticed a
speed improvement to match what happens when right clicking elsewhere.  It's
slowly deteriorated again as I re-add more cookie sites to the list.  I don't
have much proof of this yet since my list is still considerably small and the
pause is not as great as when I initially reported this....

Comment 9

16 years ago
One more reason to finally implement a good cookie management which allows you
to allow a few sites manually and block the rest...  Go place some votes on bug
75915.
That is in fact almost certainly it.  See the code at

http://lxr.mozilla.org/seamonkey/source/extensions/cookie/resources/content/cookieContextOverlay.xul#43

Comment 11

16 years ago
Sending to Steve.

Maybe we could make the image-block items hang off a submenu and then only pay
the price when the user wants to.  I know cascaded context menus are probably
heretical, though.
Assignee: law → morse
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Maybe we should fix the API to allow asking for image permissions _only_?
*** Bug 132648 has been marked as a duplicate of this bug. ***

Comment 14

16 years ago
I think that dividing image and cookie permission apis would only make this
faster by some small constant factor because someone could easily have many
image permissions in addition to many cookie blocking permissions.

In my opinion, the underlying problem is that we are getting an enumeration of
all the permissions and iterating through them in js. To speed this up, couldn't
we just use nsIPermissionManager::TestForBlocking() instead? TestForBlocking()
is implemented in nsPermissions through a similar O(n) lookup but at least it's
in native code. Furthermore, if we really wanted to make this fast, we could
reimplement TestForBlocking using a hashtable.

Comment 15

16 years ago
Created attachment 79775 [details] [diff] [review]
patch to switch to TestForBlocking()

Switching to TestForBlocking() seems to make a big difference on its own.

Comment 16

16 years ago
Can someone take a look at my minimal patch? It seems to be something that could
be added for 1.0 without much risk.
The patch looks reasonable to me, but Stephen Morse is the only one who can
really review it usefully.   Have you tried sending him email (not bugmail)
asking for a review?
Keywords: mozilla1.0, patch
(Assignee)

Comment 18

16 years ago
Look reasonable to me too.
r=morse.
Comment on attachment 79775 [details] [diff] [review]
patch to switch to TestForBlocking()

adding r=morse from comment 18.
Attachment #79775 - Flags: review+
Blake, jag, can either of you have a look at sr=ing this diff -uw for improving
perf of the image context menu?

Comment 21

16 years ago
This is a perfect patch that's been available for over a month now.
It still works flawlessly in RC3, so there's not even a regression.

Why hasn't this patch already been checked in?
This was checked in on the trunk at 05/24/2002 16:49.  See the checkin log.
(Assignee)

Comment 23

16 years ago
Actually what was checked in was the patch in bug 146048, which I now realize is 
a dup of this bug.  And, in fact, the patches are almost identical, except for 
some added bulletproofing and a more efficient return in this one.  So I'd like 
to change what was checked in to be this patch instead.  If I can get an sr on 
it, I'll do that.

Comment 24

16 years ago
Comment on attachment 79775 [details] [diff] [review]
patch to switch to TestForBlocking()

sr=waterson
Attachment #79775 - Flags: superreview+
(Assignee)

Comment 25

16 years ago
*** Bug 146048 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 26

16 years ago
Patch for bug 146048 has been backed out of the trunk and this patch was just 
checked in instead.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Assignee)

Updated

16 years ago
Target Milestone: mozilla1.1alpha → mozilla1.0.1
Comment on attachment 79775 [details] [diff] [review]
patch to switch to TestForBlocking()

Approved for 1.0.1 branch.

Please check into the 1.0.1 branch ASAP. once landed remove the
mozilla1.0.1+ keyword and add the fixed1.0.1 keyword
Attachment #79775 - Flags: approval+

Updated

16 years ago
Keywords: mozilla1.0 → mozilla1.0.1+
(Assignee)

Updated

16 years ago
Keywords: mozilla1.0.1+ → fixed1.0.1

Comment 28

16 years ago
*** Bug 150836 has been marked as a duplicate of this bug. ***

Comment 29

16 years ago
Just wanted to say that on HP-UX 11.0 the time difference between opening an
image context menu and opening a 'normal' context menu was about 10(!) seconds.

Build 2002060922 is fine.
vrfy'd fixed using 2002.06.17.0x comm branch bits on linux rh7.2, win2k and mac
10.1.5.
Status: RESOLVED → VERIFIED
Keywords: verified1.0.1
Keywords: fixed1.0.1
*** Bug 156954 has been marked as a duplicate of this bug. ***
Product: Core → Mozilla Application Suite

Updated

9 years ago
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.