Last Comment Bug 449116 - Findbar highlighting should not explicitly repaint selection
: Findbar highlighting should not explicitly repaint selection
Status: VERIFIED FIXED
: perf
Product: Toolkit
Classification: Components
Component: Find Toolbar (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla1.9.1b1
Assigned To: Graeme McCutcheon [:graememcc]
:
Mentors:
Depends on:
Blocks: 429732
  Show dependency treegraph
 
Reported: 2008-08-04 16:06 PDT by Graeme McCutcheon [:graememcc]
Modified: 2008-09-28 12:21 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (1.48 KB, patch)
2008-08-04 16:06 PDT, Graeme McCutcheon [:graememcc]
asaf: review+
Details | Diff | Splinter Review
Patch v2 (1.76 KB, patch)
2008-09-10 04:55 PDT, Graeme McCutcheon [:graememcc]
graeme.mccutcheon: review+
Details | Diff | Splinter Review

Description Graeme McCutcheon [:graememcc] 2008-08-04 16:06:47 PDT
Created attachment 332268 [details] [diff] [review]
Patch v1

Following on from bug 429732 comment 14:

The highlighting methods are performing too much work by explicitly calling the selection controller's RepaintSelection method. The very act of adding a range to the selection will ensure it's contents will get repainted.

No tests provided due to trivial nature of patch.
Comment 1 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-09-07 07:41:24 PDT
Comment on attachment 332268 [details] [diff] [review]
Patch v1

r=mano
Comment 2 Dão Gottwald [:dao] 2008-09-09 13:55:49 PDT
Comment on attachment 332268 [details] [diff] [review]
Patch v1

>           if (aHighlight)
>             findSelection.addRange(aRange);
>           if (isEditable) {
>             if (!aHighlight)
>               findSelection.removeAllRanges();
>-            controller.repaintSelection(this.nsISelectionController.SELECTION_FIND);
>           }

How about this:

>           if (aHighlight)
>             findSelection.addRange(aRange);
>           else if (isEditable)
>             findSelection.removeAllRanges();
Comment 3 Graeme McCutcheon [:graememcc] 2008-09-10 04:55:30 PDT
Created attachment 337854 [details] [diff] [review]
Patch v2

Yes, much tidier!

This doesn't change the logic, so am assuming this doesn't require re-review.
Comment 4 Dão Gottwald [:dao] 2008-09-10 05:08:10 PDT
http://hg.mozilla.org/mozilla-central/rev/0478578dd7a1
Comment 5 Henrik Skupin (:whimboo) 2008-09-28 12:21:19 PDT
No reported regression so far. Marking this bug as verified.

Note You need to log in before you can comment on or make changes to this bug.