Bug 18548 - Web Inspector freezes if a search fires with a small number of letters
Summary: Web Inspector freezes if a search fires with a small number of letters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-04-17 11:07 PDT by Anthony Ricaud
Modified: 2008-08-07 20:56 PDT (History)
4 users (show)

See Also:


Attachments
Proposed patch (3.15 KB, patch)
2008-05-26 17:42 PDT, Anthony Ricaud
no flags Details | Formatted Diff | Diff
Proposed patch (3.14 KB, patch)
2008-05-26 18:02 PDT, Anthony Ricaud
timothy: review-
Details | Formatted Diff | Diff
Patch with comments addressed (3.11 KB, patch)
2008-08-05 16:17 PDT, Anthony Ricaud
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anthony Ricaud 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.
Comment 1 Anthony Ricaud 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.
Comment 2 Anthony Ricaud 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.
Comment 3 Anthony Ricaud 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.
Comment 4 mitz 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.
Comment 5 Adam Roben (:aroben) 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.
Comment 6 Anthony Ricaud 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.
Comment 7 Timothy Hatcher 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.
Comment 8 Matt Lilek 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?
Comment 9 Anthony Ricaud 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.
Comment 10 Bjorn Tipling 2008-07-02 13:38:23 PDT
This bug is a deal breaker for me. Hope it gets quashed. Good luck!
Comment 11 Eric Seidel (no email) 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.
Comment 12 Timothy Hatcher 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.
Comment 13 Anthony Ricaud 2008-08-05 16:17:50 PDT
Created attachment 22664 [details]
Patch with comments addressed
Comment 14 Timothy Hatcher 2008-08-07 20:56:44 PDT
Landed in r35632.