Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1803813002:
[DevTools] Added keyboard search while in sources (Closed)
|
Created:
4 years, 9 months ago by Allada-Google Modified:
4 years, 8 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, sergeyv+blink_chromium.org, pfeldman, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Added keyboard search while in sources
Issue moves, see: https://codereview.chromium.org/1831903002/#
Patch Set 1 #
Total comments: 11
Patch Set 2 : Code cleanup from last push #Patch Set 3 : More code cleanup #Patch Set 4 : Final code cleanup #
Total comments: 47
Patch Set 5 : Code review input implemented/fixed #
Total comments: 8
Messages
Total messages: 12 (4 generated)
PTL
Just style comments https://codereview.chromium.org/1803813002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sources/SourcesNavigator.js (right): https://codereview.chromium.org/1803813002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sources/SourcesNavigator.js:169: // remove filter we usually don't put comments in the code unless its *absolutely* needed. In an ideal world, the code is self-explanatory. And comments are usually overlooked during refactoring/working with code, thus becoming stale pretty fast. https://codereview.chromium.org/1803813002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sources/SourcesNavigator.js:182: * Does the highlighting for for when the filter changes ditto https://codereview.chromium.org/1803813002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sources/SourcesNavigator.js:185: _handleFilterChange: function (event){ style: no space after "function" style: the { should be on a new line. https://codereview.chromium.org/1803813002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sources/SourcesNavigator.js:188: if (node){ style: space before { https://codereview.chromium.org/1803813002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sources/SourcesNavigator.js:191: treeOutline.selectNext() || treeOutline.selectPrevious(); Style: we don't use {} for a one-line statements. https://www.chromium.org/blink/coding-style https://codereview.chromium.org/1803813002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sources/SourcesNavigator.js:196: let titleText = node.titleText; we haven't been using LET and don't have a policy for it yet. Let's not use it until everybody's agreed on it. https://codereview.chromium.org/1803813002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sources/SourcesNavigator.js:200: if (event.data.changeTo === null) { let's extract event.data and typecast it to the correct type. Otherwise the Closure has no idea what's going on here https://codereview.chromium.org/1803813002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sources/navigatorView.css (right): https://codereview.chromium.org/1803813002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sources/navigatorView.css:111: color: #00A !important; can we increase selector specificity instead of using !important? https://codereview.chromium.org/1803813002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/1803813002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:70: this._currentSelectionFilterString = ''; style: double quotes for strings https://codereview.chromium.org/1803813002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:100: style: stray line https://codereview.chromium.org/1803813002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:295: nextSelectedElement, style: we don't wrap variable declaration. If there's declaration with assignment, then we put a seperate var for each declaration+assignment pair. If there's just declaration, then we put multiple declarations in a single line.
PTL
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.css (right): https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.css:130: text-decoration:underline; text-decoration:<space>underline; https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:72: this._currentSelectionFilterString = ''; Please only use double quotes. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:73: this._currentSelectionFilter = null; You should declare a type here using JSDoc. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:87: * This will also set/override the RegExp to filter on (ie: setCurrentSelectionFilter()) We don't explain why (in Blink)... https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:88: * @param {string} filterString String to filter text on. or what... Just the types. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:90: setCurrentSelectionFilterString: function (filterString) Seems to be private (starts with _) https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:93: if (this._currentSelectionFilterString === '') "" https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:102: currentSelectionFilterString: function () lets not expose until required. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:108: setInteractiveFilterable: function (enable) JSDoc for enable type. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:110: this._interactiveFilterEnabled = !!enable; Declare it as a boolean and there would be no need for !!. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:121: if (this._highlightChanges && this._highlightChanges.length > 0) (this._highlightChanges && this._highlightChanges.length) historically Blink omits 0 in comparisons. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:122: WebInspector.revertDomChanges(this._highlightChanges); We are now vulnerable to the changes made to the element from outside. It seems like storing the highlight changes on the element itself and resetting it upon setTitle would be safer. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:125: if (filterRegExp && filterRegExp !== currentFilter) { We prefer early returns to nested conditions. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:127: if (this._rootElement) { ditto https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:128: var textNode = this._rootElement.firstChild(); textNode is not really a text node, it is a first TreeElement child in the root. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:129: var childTextNodes = this.element.childTextNodes(); unused? https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:130: var newExp = new RegExp(filterRegExp.source, 'gi'); "gi", also, why new regex? https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:139: while ((match = newExp.exec(textContent)) !== null) { drop !== null, extract match from comparison into a separate statement above. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:154: currentSelectionFilter: function () do not expose https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:275: checkFilter: function (treeElement) ditto https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:318: this.setCurrentSelectionFilterString(''); "" https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:349: if (currentFilterString.length !== 0) drop !== 0 https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:364: case WebInspector.KeyboardShortcut.Keys.Right.code: Group these and use case fallthrough to handle the ones that just reset the mode. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:443: if (this._interactiveFilterEnabled === false) !this._interactiveFilterEnabled https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:541: this.titleText = null; Lets compute this dynamically off element's text content when checking filter.
PTL https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.css (right): https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.css:130: text-decoration:underline; On 2016/03/14 23:51:34, pfeldman wrote: > text-decoration:<space>underline; Acknowledged. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:72: this._currentSelectionFilterString = ''; On 2016/03/14 23:51:35, pfeldman wrote: > Please only use double quotes. Done. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:73: this._currentSelectionFilter = null; On 2016/03/14 23:51:34, pfeldman wrote: > You should declare a type here using JSDoc. Done. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:87: * This will also set/override the RegExp to filter on (ie: setCurrentSelectionFilter()) On 2016/03/14 23:51:34, pfeldman wrote: > We don't explain why (in Blink)... Done. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:88: * @param {string} filterString String to filter text on. On 2016/03/14 23:51:35, pfeldman wrote: > or what... Just the types. Done. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:90: setCurrentSelectionFilterString: function (filterString) On 2016/03/14 23:51:35, pfeldman wrote: > Seems to be private (starts with _) Done. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:93: if (this._currentSelectionFilterString === '') On 2016/03/14 23:51:34, pfeldman wrote: > "" Done. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:102: currentSelectionFilterString: function () On 2016/03/14 23:51:34, pfeldman wrote: > lets not expose until required. Done. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:108: setInteractiveFilterable: function (enable) On 2016/03/14 23:51:34, pfeldman wrote: > JSDoc for enable type. Done. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:110: this._interactiveFilterEnabled = !!enable; On 2016/03/14 23:51:34, pfeldman wrote: > Declare it as a boolean and there would be no need for !!. Done. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:121: if (this._highlightChanges && this._highlightChanges.length > 0) On 2016/03/14 23:51:34, pfeldman wrote: > (this._highlightChanges && this._highlightChanges.length) > > historically Blink omits 0 in comparisons. Done. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:122: WebInspector.revertDomChanges(this._highlightChanges); On 2016/03/14 23:51:34, pfeldman wrote: > We are now vulnerable to the changes made to the element from outside. It seems > like storing the highlight changes on the element itself and resetting it upon > setTitle would be safer. Done. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:125: if (filterRegExp && filterRegExp !== currentFilter) { On 2016/03/14 23:51:34, pfeldman wrote: > We prefer early returns to nested conditions. Done. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:127: if (this._rootElement) { On 2016/03/14 23:51:34, pfeldman wrote: > ditto Done. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:130: var newExp = new RegExp(filterRegExp.source, 'gi'); On 2016/03/14 23:51:34, pfeldman wrote: > "gi", also, why new regex? Done. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:139: while ((match = newExp.exec(textContent)) !== null) { On 2016/03/14 23:51:35, pfeldman wrote: > drop !== null, extract match from comparison into a separate statement above. Done. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:154: currentSelectionFilter: function () On 2016/03/14 23:51:34, pfeldman wrote: > do not expose Done. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:275: checkFilter: function (treeElement) On 2016/03/14 23:51:34, pfeldman wrote: > ditto Done. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:318: this.setCurrentSelectionFilterString(''); On 2016/03/14 23:51:34, pfeldman wrote: > "" Done. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:349: if (currentFilterString.length !== 0) On 2016/03/14 23:51:35, pfeldman wrote: > drop !== 0 Done. https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:364: case WebInspector.KeyboardShortcut.Keys.Right.code: On 2016/03/14 23:51:34, pfeldman wrote: > Group these and use case fallthrough to handle the ones that just reset the > mode. I optimized the code I added here as much as I found needed, but I decided not to touch any of the code that was here before. The only major thing I did was: refactor it out of a bunch of "if-else-if" statements. Should I try and go through this code and see if I can make it more "readable"? https://codereview.chromium.org/1803813002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:443: if (this._interactiveFilterEnabled === false) On 2016/03/14 23:51:34, pfeldman wrote: > !this._interactiveFilterEnabled Done.
The CQ bit was checked by allada@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1803813002/70001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1803813002/70001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/1803813002/diff/70001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/1803813002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:91: this._interactiveFilterEnabled = enable; Should disabling perform the cleanup? https://codereview.chromium.org/1803813002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:109: return new RegExp(["(", filterString.escapeForRegExp(), ")"].join(""), "gi") Do you need the () group? https://codereview.chromium.org/1803813002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:124: if (this._currentSelectionFilterString === "") if (!this._currentSelectionFilterString) https://codereview.chromium.org/1803813002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:128: this.selectNext() || this.selectPrevious(); statement per line please. if (!selectNext) selectPrevious(); https://codereview.chromium.org/1803813002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:142: if (ranges.length > 0) if (ranges.length) https://codereview.chromium.org/1803813002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:147: this._highlightChangedNodes.push(node); Can't there only be one highlighted node? https://codereview.chromium.org/1803813002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:160: return this._currentSelectionFilterString !== "" ? this._makeFilterRegExpFromString(this._currentSelectionFilterString).test(treeElement._titleElement.textContent) : true; this._currentSelectionFilterString ? ... https://codereview.chromium.org/1803813002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:190: if (currentFilterString.length === 0) { !length
Description was changed from ========== [DevTools] Added keyboard search while in sources While in sources file explorer gives the ability to search/filter files by typing while panel is not blured. On blur event, click, esc or enter the filtering will cancel. The display underlines the matches found. If the user then uses arrow keys to navigate up and down only the filtered items should be selectable. R=lushnikov BUG=594654 ========== to ========== [DevTools] Added keyboard search while in sources Issue moves, see: https://codereview.chromium.org/1831903002/# ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
