Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2392963004:
DevTools: Display SuggestBox with a Viewport control (Closed)
|
Created:
4 years, 2 months ago by einbinder Modified:
4 years, 2 months ago Reviewers:
lushnikov CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Display SuggestBox with a Viewport control
This gives a large speedup when there are many elements in a SuggestBox, like when completing "window."
BUG=none
Committed: https://crrev.com/d07ec0748f7e29857bf61a19e1e452aa89daa7c1
Cr-Commit-Position: refs/heads/master@{#425530}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Braces #
Total comments: 14
Patch Set 3 : this._hasScrollBars #
Total comments: 5
Patch Set 4 : Lazy create elements #
Total comments: 5
Patch Set 5 : _hasVerticalScroll #
Messages
Total messages: 15 (4 generated)
einbinder@chromium.org changed reviewers: + lushnikov@chromium.org
SuggestBox with a viewport, attempt #2 ptal
https://codereview.chromium.org/2392963004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js (right): https://codereview.chromium.org/2392963004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:140: _updateWidth: function() This is unfortunate, but I think it's necessary.
https://codereview.chromium.org/2392963004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js (right): https://codereview.chromium.org/2392963004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:113: if (this._lastAnchorBox && this._lastAnchorBox.equals(anchorBox) && this._lastItemCount === this.itemCount()) this is not correct! https://codereview.chromium.org/2392963004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:136: var hasScrollBars = height > maxHeight; not used https://codereview.chromium.org/2392963004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:142: if (this._element.offsetHeight + this._rowHeight <= this._rowHeight * this._elementList.length) { this._element.offsetHeight will trigger sync layout, since you call this function after _updateBoxPosition https://codereview.chromium.org/2392963004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:142: if (this._element.offsetHeight + this._rowHeight <= this._rowHeight * this._elementList.length) { "I can store hasScrollBars!" (Joel) https://codereview.chromium.org/2392963004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:148: for (var i = 0; i < this._elementList.length; i++) // Perform N sync layouts inside a layout boundary of a small size. https://codereview.chromium.org/2392963004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:183: var measuringElement = this._createItemElement("1", "12"); WI.measurePrefferedSize? https://codereview.chromium.org/2392963004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:183: var measuringElement = this._createItemElement("1", "12"); s/1/m/ https://codereview.chromium.org/2392963004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:286: * @param {string=} className @return
ptal https://codereview.chromium.org/2392963004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js (right): https://codereview.chromium.org/2392963004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:113: if (this._lastAnchorBox && this._lastAnchorBox.equals(anchorBox) && this._lastItemCount === this.itemCount()) On 2016/10/08 at 00:37:16, lushnikov wrote: > this is not correct! No, it is correct! If the item count changes then hasScrollBars might change. https://codereview.chromium.org/2392963004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:136: var hasScrollBars = height > maxHeight; On 2016/10/08 at 00:37:16, lushnikov wrote: > not used this._hasScrollBars https://codereview.chromium.org/2392963004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:142: if (this._element.offsetHeight + this._rowHeight <= this._rowHeight * this._elementList.length) { On 2016/10/08 at 00:37:16, lushnikov wrote: > "I can store hasScrollBars!" (Joel) Done. https://codereview.chromium.org/2392963004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:148: for (var i = 0; i < this._elementList.length; i++) On 2016/10/08 at 00:37:16, lushnikov wrote: > // Perform N sync layouts inside a layout boundary of a small size. Done. https://codereview.chromium.org/2392963004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:183: var measuringElement = this._createItemElement("1", "12"); On 2016/10/08 at 00:37:17, lushnikov wrote: > WI.measurePrefferedSize? Can't, measurePreferredSize uses offsetHeight, and we need the floating point getBoundingClientRect().height https://codereview.chromium.org/2392963004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:286: * @param {string=} className On 2016/10/08 at 00:37:16, lushnikov wrote: > @return Done.
https://codereview.chromium.org/2392963004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js (right): https://codereview.chromium.org/2392963004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:150: for (var i = 0; i < this._elementList.length; i++) "I can find the largest size one and measure it instead!" (Joel) https://codereview.chromium.org/2392963004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:303: element.addEventListener("mousedown", this._onItemMouseDown.bind(this), false); one event listener on the container should be more performant/memory efficient FYI: enclosingNodeOrSelfWithClass https://codereview.chromium.org/2392963004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:321: this._elementList.push(this._createItemElement(userEnteredText, items[i].title, items[i].className)); lazy creation should work faster
ptal https://codereview.chromium.org/2392963004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js (right): https://codereview.chromium.org/2392963004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:150: for (var i = 0; i < this._elementList.length; i++) On 2016/10/10 at 20:49:24, lushnikov wrote: > "I can find the largest size one and measure it instead!" (Joel) Done. https://codereview.chromium.org/2392963004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:321: this._elementList.push(this._createItemElement(userEnteredText, items[i].title, items[i].className)); On 2016/10/10 at 20:49:24, lushnikov wrote: > lazy creation should work faster Done. https://codereview.chromium.org/2392963004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js (right): https://codereview.chromium.org/2392963004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:151: var maxIndex = 0; This might be the wrong element if some have history markers. But there's padding and that would be rare, so it shouldn't be noticeable.
works like a charm 🎆 lgtm! https://codereview.chromium.org/2392963004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js (right): https://codereview.chromium.org/2392963004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:140: this._hasScrollBars = height > maxHeight; _hasVerticalScroll https://codereview.chromium.org/2392963004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:156: var element = this.itemElement(maxIndex); /** @type {!Element} */
The CQ bit was checked by einbinder@chromium.org
https://codereview.chromium.org/2392963004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js (right): https://codereview.chromium.org/2392963004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:140: this._hasScrollBars = height > maxHeight; On 2016/10/13 at 23:45:53, lushnikov wrote: > _hasVerticalScroll Done. https://codereview.chromium.org/2392963004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:156: var element = this.itemElement(maxIndex); On 2016/10/13 at 23:45:53, lushnikov wrote: > /** @type {!Element} */ Done.
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/2392963004/#ps80001 (title: "_hasVerticalScroll")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: Display SuggestBox with a Viewport control This gives a large speedup when there are many elements in a SuggestBox, like when completing "window." BUG=none ========== to ========== DevTools: Display SuggestBox with a Viewport control This gives a large speedup when there are many elements in a SuggestBox, like when completing "window." BUG=none Committed: https://crrev.com/d07ec0748f7e29857bf61a19e1e452aa89daa7c1 Cr-Commit-Position: refs/heads/master@{#425530} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d07ec0748f7e29857bf61a19e1e452aa89daa7c1 Cr-Commit-Position: refs/heads/master@{#425530} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
