Bug 18548

Summary: Web Inspector freezes if a search fires with a small number of letters
Product: WebKit Reporter: Anthony Ricaud <rik>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, bjorn, mitz, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed patch
none
Proposed patch
timothy: review-
Patch with comments addressed timothy: review+

Anthony Ricaud
Reported 2008-04-17 11:07:34 PDT
If you type less than 3 letters, the search is performed and is really slow on a normal document. The search should wait for more letters or be activated by hitting the Enter button.
Attachments
Proposed patch (3.15 KB, patch)
2008-05-26 17:42 PDT, Anthony Ricaud 		no flags
DetailsFormatted DiffDiff
Proposed patch (3.14 KB, patch)
2008-05-26 18:02 PDT, Anthony Ricaud 		timothy: review-
DetailsFormatted DiffDiff
Patch with comments addressed (3.11 KB, patch)
2008-08-05 16:17 PDT, Anthony Ricaud 		timothy: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Anthony Ricaud
Comment 1 2008-05-20 16:51:59 PDT
Steps to reproduce 1. Go to http://digg.com 2. Open the inspector 3. Type "a" in the search field The inspector is now freezing while performing the search. 4. In the search results, select "<head>" The inspector is also freezing. I think the first freeze is normal (lots of things to retrieve) but the user should opt-in for this search. However, the freeze when selecting a node is not normal.
Anthony Ricaud
Comment 2 2008-05-26 17:42:30 PDT
Created attachment 21355 [details] Proposed patch This should solve the first problem (freeze on search). I'm not sure this patch follows code conventions.
Anthony Ricaud
Comment 3 2008-05-26 18:02:10 PDT
Created attachment 21356 [details] Proposed patch e becomes event and no braces for single lines if statements. Thanks pewtermoose.
mitz
Comment 4 2008-05-26 20:08:17 PDT
I think that in the long-term, it would be best to perform long searches without locking up the UI.
Adam Roben (:aroben)
Comment 5 2008-05-27 12:02:49 PDT
Comment on attachment 21356 [details] Proposed patch I don't think it's a good idea to have searching sometimes happen automatically and sometimes happen only when pressing Enter.
Anthony Ricaud
Comment 6 2008-05-27 12:51:28 PDT
In the ideal situation, the search will be performed without locking the UI. In this case, this patch is unnecessary. However, the actual behaviour is not providing enough feedback. The options are providing search only when pressing Enter (for a consistent behaviour) or the behaviour in the patch (easier to search for long strings without pressing Enter). For the second option, a feedback a la Spotlight could help informing the user about what's happening.
Timothy Hatcher
Comment 7 2008-05-28 09:18:21 PDT
I think we should change it to search only when pressing Enter. That should give a better experience while allowing all lengths of text. This would only require removing incremental="incremental" and using the "search" event.
Matt Lilek
Comment 8 2008-05-28 15:02:42 PDT
(In reply to comment #7) > I think we should change it to search only when pressing Enter. That should > give a better experience while allowing all lengths of text. > > This would only require removing incremental="incremental" and using the > "search" event. > Or perhaps only start the search after typing has paused for a certain amount of time?
Anthony Ricaud
Comment 9 2008-05-28 15:34:05 PDT
We should not forget about step 4. I see 3s for addRange() and focus() in "set currentFocusElement" There's 802 calls to WebInspector.ResourcesPanel.refreshResource done in 1s.
Bjorn Tipling
Comment 10 2008-07-02 13:38:23 PDT
This bug is a deal breaker for me. Hope it gets quashed. Good luck!
Eric Seidel (no email)
Comment 11 2008-08-01 15:01:10 PDT
Comment on attachment 21356 [details] Proposed patch Assigning to Tim to review. Adam also knows this code well.
Timothy Hatcher
Comment 12 2008-08-01 15:12:58 PDT
Comment on attachment 21356 [details] Proposed patch + var forceSearch = event.keyCode == 13; You should use event.keyIdentifier === "Enter" here. + if (!forceSearch && this.lastQuery && this.lastQuery == query) You should use === here. Otherwise I will r+ it.
Anthony Ricaud
Comment 13 2008-08-05 16:17:50 PDT
Created attachment 22664 [details] Patch with comments addressed
Timothy Hatcher
Comment 14 2008-08-07 20:56:44 PDT
Landed in r35632.
Note You need to log in before you can comment on or make changes to this bug.