Summary: | Web Inspector: "Goto Function" filtering should be less restrictive. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | eustas.bug | ||||||||
Component: | Web Inspector (Deprecated) | Assignee: | eustas.bug | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, eustas.bug, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
eustas.bug
2012-05-04 00:12:35 PDT
Created attachment 140171 [details]
Patch
Comment on attachment 140171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140171&action=review > Source/WebCore/inspector/front-end/FilteredItemSelectionDialog.js:129 > + if (this._checkItemAt(i)) { To make things simple, I would structure the code as follows: var regex = this._createSearchRegex(this._promptElement.value); for (var i = ...) { if (regex.test(this._delegate.itemKeyAt(i)) ... } Then method below would not be needed. > Source/WebCore/inspector/front-end/FilteredItemSelectionDialog.js:195 > + if (!query) { The WebKit style for thise would be: if (!query) { delete this._checkItemAt; return; } rest of the code here. > Source/WebCore/inspector/front-end/FilteredItemSelectionDialog.js:204 > + this._checkItemAt = checker.bind(this); You should never overwrite methods on the prototype. > Source/WebCore/inspector/front-end/FilteredItemSelectionDialog.js:231 > + c = c + "\\"; Escaping via appending \\ looks strange. Having test cases suggesting the behavior pattern would help understanding the logic. Comment on attachment 140171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140171&action=review >> Source/WebCore/inspector/front-end/FilteredItemSelectionDialog.js:129 >> + if (this._checkItemAt(i)) { > > To make things simple, I would structure the code as follows: > > var regex = this._createSearchRegex(this._promptElement.value); > for (var i = ...) { > if (regex.test(this._delegate.itemKeyAt(i)) > ... > } > > Then method below would not be needed. Right you are, but i've tried to preserve original code structure. Fixed. >> Source/WebCore/inspector/front-end/FilteredItemSelectionDialog.js:195 >> + if (!query) { > > The WebKit style for thise would be: > > if (!query) { > delete this._checkItemAt; > return; > } > > rest of the code here. Thanks. >> Source/WebCore/inspector/front-end/FilteredItemSelectionDialog.js:204 >> + this._checkItemAt = checker.bind(this); > > You should never overwrite methods on the prototype. OK >> Source/WebCore/inspector/front-end/FilteredItemSelectionDialog.js:231 >> + c = c + "\\"; > > Escaping via appending \\ looks strange. Having test cases suggesting the behavior pattern would help understanding the logic. Oops. Fixed. Tests added Created attachment 140398 [details]
Patch
Created attachment 140402 [details]
Patch
Comment on attachment 140402 [details] Patch Clearing flags on attachment: 140402 Committed r116244: <http://trac.webkit.org/changeset/116244> All reviewed patches have been landed. Closing bug. |