Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1831903002:
[DevTools] Added keyboard search while in sources (Closed)
|
Created:
4 years, 9 months ago by allada 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
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,pfeldman
BUG=594654
Committed: https://crrev.com/391574fa0e749ba67d59c7b7bdb8eb33624fa355
Cr-Commit-Position: refs/heads/master@{#387079}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fixed a few minor code styles #
Total comments: 26
Patch Set 3 : Moved some code around #Patch Set 4 : Fixed a few code styling issues #
Total comments: 19
Patch Set 5 : Code review fixes #Patch Set 6 : Fixed codereview issues #
Total comments: 14
Patch Set 7 : Fixed minor changes requested #
Messages
Total messages: 24 (7 generated)
PTL (because of mixup with allada@google vs allada@chromium I had to create a new CL). Old cl: https://codereview.chromium.org/1803813002/ https://codereview.chromium.org/1831903002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/1831903002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:94: this._interactiveFilterEnabled = enable; >pfeldman 2016/03/22 00:00:33 >Should disabling perform the cleanup? Done. https://codereview.chromium.org/1831903002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:112: return new RegExp(filterString.escapeForRegExp(), "gi") >pfeldman 2016/03/22 00:00:34 >Do you need the () group? Done. https://codereview.chromium.org/1831903002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:127: if (!this._currentSelectionFilterString) >pfeldman 2016/03/22 00:00:33 >if (!this._currentSelectionFilterString) Done. https://codereview.chromium.org/1831903002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:131: if (!this.selectNext()) >pfeldman 2016/03/22 00:00:34 >statement per line please. >if (!selectNext) > selectPrevious(); Done. https://codereview.chromium.org/1831903002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:147: if (ranges.length) >pfeldman 2016/03/22 00:00:34 >if (ranges.length) Done. https://codereview.chromium.org/1831903002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:152: this._highlightChangedNodes.push(node); >pfeldman 2016/03/22 00:00:33 >Can't there only be one highlighted node? Only one node can be selected, but multiple nodes can be "selectable" (underlined) at once. This list contains a list of nodes that are underlined (they used to be highlighted yellow now they are underlined). https://codereview.chromium.org/1831903002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:165: return this._currentSelectionFilterString ? this._makeFilterRegExpFromString(this._currentSelectionFilterString).test(treeElement._titleElement.textContent) : true; >pfeldman 2016/03/22 00:00:33 >this._currentSelectionFilterString ? ... Done. https://codereview.chromium.org/1831903002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:195: if (!currentFilterString.length) >pfeldman 2016/03/22 00:00:34 >!length Done.
The CQ bit was checked by allada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1831903002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1831903002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice! https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:89: setInteractiveFilterable: function (enable) style: extra space after function keyword https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:91: if (!enable) let's do nothing if the state doesn't change https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:110: _makeFilterRegExpFromString: function (filterString) despite the JavaScript class RegExp, we always abbreviate regular expression as "regex". So it would be consistent to name this "_makeFilterRegexFromString" https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:120: var filterRegExp = this._makeFilterRegExpFromString(this._currentSelectionFilterString); let's move this closer to the point where it is actually used https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:125: this._highlightChanges = []; this seems to be not used https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:130: if (this.selectedTreeElement && !this.selectedTreeElement.selectable) { why do you need this? https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:137: var textContent = node._listItemNode.textContent; let's move this logic into TreeElement. The treeElement._setHighlightChanges method will not be needed, as well as treeOutline._checkFilter method E.g. TreeElement.prototype = { _setFilter: function(regex); } https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:190: switch (event.data) { I think this switch could be simplified if (key === "\r" || key === "\n" || (key === " " && !this._currentSelectionFilterString)) // Do nothing. else this._setCurrentSelectionFilterString(...) https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:196: break; we do not use fall-through as they are confusing. (unless it's a case A: case B: case C: syntax).
PTL https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:89: setInteractiveFilterable: function (enable) On 2016/03/25 17:15:59, lushnikov wrote: > style: extra space after function keyword Done. https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:91: if (!enable) On 2016/03/25 17:15:59, lushnikov wrote: > let's do nothing if the state doesn't change Done. https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:110: _makeFilterRegExpFromString: function (filterString) On 2016/03/25 17:15:59, lushnikov wrote: > despite the JavaScript class RegExp, we always abbreviate regular expression as > "regex". > So it would be consistent to name this "_makeFilterRegexFromString" Done. https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:120: var filterRegExp = this._makeFilterRegExpFromString(this._currentSelectionFilterString); On 2016/03/25 17:15:59, lushnikov wrote: > let's move this closer to the point where it is actually used Done. https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:125: this._highlightChanges = []; On 2016/03/25 17:15:59, lushnikov wrote: > this seems to be not used Done. https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:130: if (this.selectedTreeElement && !this.selectedTreeElement.selectable) { On 2016/03/25 17:15:59, lushnikov wrote: > why do you need this? Because this function is executed after a new filter is put on it. If there the currently selected item does meet the criteria for the new filter it should try and find the next item in the tree that meets the filter's criteria. If it fails on the next, it tries to go backwards looking for an item. (this is probably the most important part of the user interaction) https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:137: var textContent = node._listItemNode.textContent; On 2016/03/25 17:16:00, lushnikov wrote: > let's move this logic into TreeElement. > The treeElement._setHighlightChanges method will not be needed, as well as > treeOutline._checkFilter method > > E.g. TreeElement.prototype = { > _setFilter: function(regex); > } Done. https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:190: switch (event.data) { On 2016/03/25 17:15:59, lushnikov wrote: > I think this switch could be simplified > > if (key === "\r" || key === "\n" || (key === " " && > !this._currentSelectionFilterString)) > // Do nothing. > else > this._setCurrentSelectionFilterString(...) Done. https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:196: break; On 2016/03/25 17:16:00, lushnikov wrote: > we do not use fall-through as they are confusing. (unless it's a case A: case > B: case C: syntax). Done.
Almost there! https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:89: setInteractiveFilterable: function (enable) On 2016/04/05 00:16:22, allada wrote: > On 2016/03/25 17:15:59, lushnikov wrote: > > style: extra space after function keyword > > Done. This seems to be not done. https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:91: if (!enable) On 2016/04/05 00:16:22, allada wrote: > On 2016/03/25 17:15:59, lushnikov wrote: > > let's do nothing if the state doesn't change > > Done. This seems to be not done. https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:110: _makeFilterRegExpFromString: function (filterString) On 2016/04/05 00:16:22, allada wrote: > On 2016/03/25 17:15:59, lushnikov wrote: > > despite the JavaScript class RegExp, we always abbreviate regular expression > as > > "regex". > > So it would be consistent to name this "_makeFilterRegexFromString" > > Done. This seems to be not done. https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:190: switch (event.data) { On 2016/04/05 00:16:22, allada wrote: > On 2016/03/25 17:15:59, lushnikov wrote: > > I think this switch could be simplified > > > > if (key === "\r" || key === "\n" || (key === " " && > > !this._currentSelectionFilterString)) > > // Do nothing. > > else > > this._setCurrentSelectionFilterString(...) > > Done. This seems to be not done. https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:50: this._contentElement.addEventListener("keypress", this._handleKeyPressForHighlighting.bind(this), true); this.element.addEventListener... https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:51: this.element.addEventListener("blur", this._clearFilter.bind(this), true); is capturing actually needed in these three events? https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:115: _refreshHighlighting: function () nit: _refreshHighlight: https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:120: var filterRegExp = this._makeFilterRegExpFromString(this._currentSelectionFilterString); nit: filterRegex = https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:136: do { nit: no need for do-while; simple while will work https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:147: _checkFilter: function (treeElement) Let's move this method into TreeElement; treeElement already has a regex, we will not need to recreate it here https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:264: this.element.setAttribute("tabIndex", 0); this and below seems to be unnecessary https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:382: nit: stray line https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:396: if (this.selectedTreeElement.expanded) { Are there any changes in this if-else-if code? nit: if you put this branch before the "right" code, the "git diff" will be more descriptive in this case https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:564: if (this._highlightChanges.length) return !!this._highlightChanges.length;
Sorry, I think I stashed my changes instead of committing them when I sent this last CL in. PTL. https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:89: setInteractiveFilterable: function (enable) On 2016/04/06 00:22:36, lushnikov wrote: > On 2016/04/05 00:16:22, allada wrote: > > On 2016/03/25 17:15:59, lushnikov wrote: > > > style: extra space after function keyword > > > > Done. > > This seems to be not done. Sorry must have stashed and not committed my changes https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:91: if (!enable) On 2016/04/06 00:22:36, lushnikov wrote: > On 2016/04/05 00:16:22, allada wrote: > > On 2016/03/25 17:15:59, lushnikov wrote: > > > let's do nothing if the state doesn't change > > > > Done. > > This seems to be not done. Done. https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:110: _makeFilterRegExpFromString: function (filterString) On 2016/04/06 00:22:36, lushnikov wrote: > On 2016/04/05 00:16:22, allada wrote: > > On 2016/03/25 17:15:59, lushnikov wrote: > > > despite the JavaScript class RegExp, we always abbreviate regular expression > > as > > > "regex". > > > So it would be consistent to name this "_makeFilterRegexFromString" > > > > Done. > > This seems to be not done. Done. https://codereview.chromium.org/1831903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:190: switch (event.data) { On 2016/04/06 00:22:36, lushnikov wrote: > On 2016/04/05 00:16:22, allada wrote: > > On 2016/03/25 17:15:59, lushnikov wrote: > > > I think this switch could be simplified > > > > > > if (key === "\r" || key === "\n" || (key === " " && > > > !this._currentSelectionFilterString)) > > > // Do nothing. > > > else > > > this._setCurrentSelectionFilterString(...) > > > > Done. > > This seems to be not done. Done.
Thanks! Can we have the rest of the comments addressed?
PTL I think I got them all this time. https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:50: this._contentElement.addEventListener("keypress", this._handleKeyPressForHighlighting.bind(this), true); On 2016/04/06 00:22:36, lushnikov wrote: > this.element.addEventListener... Done. https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:115: _refreshHighlighting: function () On 2016/04/06 00:22:37, lushnikov wrote: > nit: _refreshHighlight: Done. https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:120: var filterRegExp = this._makeFilterRegExpFromString(this._currentSelectionFilterString); On 2016/04/06 00:22:36, lushnikov wrote: > nit: filterRegex = Done. https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:136: do { On 2016/04/06 00:22:36, lushnikov wrote: > nit: no need for do-while; simple while will work Done. https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:147: _checkFilter: function (treeElement) On 2016/04/06 00:22:36, lushnikov wrote: > Let's move this method into TreeElement; treeElement already has a regex, we > will not need to recreate it here Done. https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:264: this.element.setAttribute("tabIndex", 0); On 2016/04/06 00:22:36, lushnikov wrote: > this and below seems to be unnecessary This is needed to be able to capture keyboard events properly while in this focused in this frame. To preserve full reverse compatibility to work exactly how it was, a "nonFocusable" argument was added to the constructor. https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:382: On 2016/04/06 00:22:36, lushnikov wrote: > nit: stray line Done. https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:396: if (this.selectedTreeElement.expanded) { On 2016/04/06 00:22:36, lushnikov wrote: > Are there any changes in this if-else-if code? > > nit: if you put this branch before the "right" code, the "git diff" will be more > descriptive in this case Done. https://codereview.chromium.org/1831903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:564: if (this._highlightChanges.length) On 2016/04/06 00:22:36, lushnikov wrote: > return !!this._highlightChanges.length; Done.
thanks! LGTM given comments are addressed https://codereview.chromium.org/1831903002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/1831903002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:93: if (!enable) nit: do we need this if statement? https://codereview.chromium.org/1831903002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:111: _makeFilterRegExFromString: function(filterString) nit: _makeFilterRegexFromString https://codereview.chromium.org/1831903002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:134: var filterRegEx = this._makeFilterRegExFromString(this._currentSelectionFilterString); nit: filterRegex https://codereview.chromium.org/1831903002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:147: _checkFilter: function(treeElement) let's move this inside TreeElement. https://codereview.chromium.org/1831903002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:356: nit: stray line https://codereview.chromium.org/1831903002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:364: nit: stray line https://codereview.chromium.org/1831903002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:384: nit: stray line
done https://codereview.chromium.org/1831903002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/1831903002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:93: if (!enable) On 2016/04/13 17:57:41, lushnikov wrote: > nit: do we need this if statement? Yes, if setInteractiveFilter is called to turn it off then we want to make sure it clears the filter. If it is turned on it is already empty. https://codereview.chromium.org/1831903002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:111: _makeFilterRegExFromString: function(filterString) On 2016/04/13 17:57:41, lushnikov wrote: > nit: _makeFilterRegexFromString Done. https://codereview.chromium.org/1831903002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:134: var filterRegEx = this._makeFilterRegExFromString(this._currentSelectionFilterString); On 2016/04/13 17:57:41, lushnikov wrote: > nit: filterRegex Done. https://codereview.chromium.org/1831903002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:147: _checkFilter: function(treeElement) On 2016/04/13 17:57:41, lushnikov wrote: > let's move this inside TreeElement. Done. https://codereview.chromium.org/1831903002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:356: On 2016/04/13 17:57:41, lushnikov wrote: > nit: stray line Done. https://codereview.chromium.org/1831903002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:364: On 2016/04/13 17:57:41, lushnikov wrote: > nit: stray line Done. https://codereview.chromium.org/1831903002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:384: On 2016/04/13 17:57:41, lushnikov wrote: > nit: stray line Done.
The CQ bit was checked by allada@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/1831903002/#ps120001 (title: "Fixed minor changes requested")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1831903002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1831903002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by allada@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1831903002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1831903002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
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,pfeldman BUG=594654 ========== to ========== [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,pfeldman BUG=594654 Committed: https://crrev.com/391574fa0e749ba67d59c7b7bdb8eb33624fa355 Cr-Commit-Position: refs/heads/master@{#387079} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/391574fa0e749ba67d59c7b7bdb8eb33624fa355 Cr-Commit-Position: refs/heads/master@{#387079}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/1889933003/ by allada@chromium.org. The reason for reverting is: Screwed up timeline.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
