221361 - right click context speed is very slow with large selections

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Henrik Gemal]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Henrik Gemal]

1st and 3th regexp can be made as one at least in perl:

$str =~ s/^\s*(.*?)\s*$/$1/;

Comment 3

[Henrik Gemal]

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?

Comment 4

[Henrik Gemal]

Attachment: If the selection is larger than 150 chars cut if off for performance

Comment 5

[Henrik Gemal]

Attachment: new patch using help from Neil.

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

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

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)

Comment 7

[Henrik Gemal]

Attachment: new patch fixing problems

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"

Comment 8

[[e-mail]]

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.

Comment 9

[Jan Varga [:janv]]

Comment on attachment 133415 [details] [diff] [review]
new patch fixing problems

looks good to me
r=varga with Neil's suggestions fixed

Comment 10

[Henrik Gemal]

Attachment: new patch with Neils fixes

Comment 11

[Henrik Gemal]

I'm looking for sr and checkin for the patch. already emailed
[e-mail] twice! and nothing...

Comment 12

[Henrik Gemal]

taken

Comment 13

[Alec Flett]

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

Comment 14

[Christian Reis]

Filed bug 223968 for alecf's unfork request.

Comment 15

[Pierre Chanial]

Alecf: the two routines do not belong to the same codebase. They are forked
between Firebird and the suite.

Comment 16

[Christian Reis]

That's a statement of fact. The question is, why does it *need* to be forked?

Comment 17

[Jan Varga [:janv]]

Comment on attachment 133761 [details] [diff] [review]
new patch with Neils fixes

checked in

Comment 18

[Simon Paquet [:sipaq]]

Fixed with checkin from Jan Varga.

Comment 19

[Brendan Eich [:brendan]]

Henrik: about searchStr = searchStr.replace(/\s*(.*?)\s*$/,"$1"):

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

/be