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
Anthony Ricaud
2008-04-17 11:07:34 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. Created attachment 21355 [details]
Proposed patch
This should solve the first problem (freeze on search). I'm not sure this patch follows code conventions.
Created attachment 21356 [details]
Proposed patch
e becomes event and no braces for single lines if statements. Thanks pewtermoose.
I think that in the long-term, it would be best to perform long searches without locking up the UI. 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.
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. 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. (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? 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. This bug is a deal breaker for me. Hope it gets quashed. Good luck! Comment on attachment 21356 [details]
Proposed patch
Assigning to Tim to review. Adam also knows this code well.
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.
Created attachment 22664 [details]
Patch with comments addressed
Landed in r35632. |