Closed Bug 221361 Opened 17 years ago Closed 17 years ago

right click context speed is very slow with large selections

Categories

(SeaMonkey :: UI Design, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla, Assigned: bugzilla)

References

()

Details

(Keywords: perf)

Attachments

(1 file, 3 obsolete files)

go to http://brightonnewmedia.org/archives/2003-September.txt
press CTRL + A to select all text
right click on the page

it takes forever to the context menu to show.

20031003
When the context menu comes up, it executes the following code:

785         var selection = this.searchSelected();
...
792             if (searchSelectText.length > 15)
793                 searchSelectText = searchSelectText.substr(0,15) + "...";
...
797           searchSelectText = bundle.formatStringFromName("searchText",
798                                                          [searchSelectText], 1);
799           this.setItemAttr("context-searchselect", "label", searchSelectText);

where we have:

804     searchSelected : function() {
805         var focusedWindow = document.commandDispatcher.focusedWindow;
806         var searchStr =
focusedWindow.__proto__.getSelection.call(focusedWindow);
807         searchStr = searchStr.toString();
808         searchStr = searchStr.replace( /^\s+/, "" );
809         searchStr = searchStr.replace(/(\n|\r|\t)+/g, " ");
810         searchStr = searchStr.replace(/\s+$/,"");
811         return searchStr;
812     },

So the problem is that the entire selection is stringified, then linearly
search/replaced three times, before the first 15 chars are grabbed... This is
bound to be slow with a large selection.
Summary: right click context speed is very slow → right click context speed is very slow with large selections
1st and 3th regexp can be made as one at least in perl:

$str =~ s/^\s*(.*?)\s*$/$1/;
Keywords: perf
I can boil it down to two regexp:
searchStr = searchStr.replace(/(\n|\r|\t)+/g, " ");
searchStr = searchStr.replace(/\s*(.*?)\s*$/,"$1");

the one regexp that takes a long time is:
searchStr = searchStr.replace(/(\n|\r|\t)+/g, " ");

cant we just return if searchStr.lenght is larger than fx 150 chars? and then
just take the first 150 chars and run through the regexps?



Attachment #133404 - Flags: review?(bzbarsky)
Attached patch new patch using help from Neil. (obsolete) — Splinter Review
how it works:
1) find the first 16 important chars but get the whole string
2) remove any tabs/newslines
3) remove leading and trailing spaces
4) replace multi whitespace with single whitespace
Attachment #133404 - Attachment is obsolete: true
Attachment #133408 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 133404 [details] [diff] [review]
If the selection is larger than 150 chars cut if off for performance

(clearing review request on obsolete patch)
Attachment #133404 - Flags: review?(bzbarsky)
Attached patch new patch fixing problems (obsolete) — Splinter Review
added charlen so that max query to search engine is 2000 chars.
try fx to load http://www.mozilla.org/start/ and select all (CTRL+A) and press
"web search for ..." and you get "google error Bad Request"
Attachment #133408 - Attachment is obsolete: true
Attachment #133408 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #133415 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 133415 [details] [diff] [review]
new patch fixing problems

Why didn't you patch contentAreaContextMenu.xul to pass a length?

>         searchStr = searchStr.replace(/(\n|\r|\t)+/g, " ");
This line is superfluous, your other regexps take care of it.

Forwarding on the review to Jan because it affects Firebird.
Attachment #133415 - Flags: review?(neil.parkwaycc.co.uk) → review?(varga)
Comment on attachment 133415 [details] [diff] [review]
new patch fixing problems

looks good to me
r=varga with Neil's suggestions fixed
Attachment #133415 - Flags: review?(varga) → review+
Attachment #133415 - Attachment is obsolete: true
Attachment #133761 - Flags: review+
I'm looking for sr and checkin for the patch. already emailed
reviewer@mozilla.org twice! and nothing...
Attachment #133761 - Flags: superreview?(blake)
taken
Assignee: guifeatures → bugzilla
Attachment #133761 - Flags: superreview?(blake) → superreview?(alecf)
Comment on attachment 133761 [details] [diff] [review]
new patch with Neils fixes

neat, though kind of ugly that this function is in both browser.js and
nsContextMenu.js - sure would be nice to consolidate!
sr=alecf
Attachment #133761 - Flags: superreview?(alecf) → superreview+
Filed bug 223968 for alecf's unfork request.
Status: NEW → ASSIGNED
Alecf: the two routines do not belong to the same codebase. They are forked
between Firebird and the suite.
That's a statement of fact. The question is, why does it *need* to be forked?
Comment on attachment 133761 [details] [diff] [review]
new patch with Neils fixes

checked in
Fixed with checkin from Jan Varga.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Henrik: about searchStr = searchStr.replace(/\s*(.*?)\s*$/,"$1"):

- Why not anchor at the front?
- Why the ? for the .*?

/be

Product: Core → Mozilla Application Suite
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.