Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2371393002:
DevTools: Add lightweight StaticViewportControl (Closed)
|
Created:
4 years, 2 months ago by einbinder Modified:
4 years, 2 months ago 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: Add lightweight StaticViewportControl
BUG=none
Committed: https://crrev.com/ac8725368543179ac75b3ada6125f5867e790603
Cr-Commit-Position: refs/heads/master@{#422571}
Patch Set 1 #
Total comments: 22
Patch Set 2 : invalidateContent #
Total comments: 6
Patch Set 3 : Merge invalidate and refresh #
Total comments: 8
Patch Set 4 : Check if parent is innerElement #
Total comments: 5
Patch Set 5 : _indexSymbol #
Messages
Total messages: 21 (8 generated)
einbinder@chromium.org changed reviewers: + lushnikov@chromium.org
ptal
dgozman@chromium.org changed reviewers: + dgozman@chromium.org
https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js (right): https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:33: refresh: function() What's the difference from invalidate? Let's have more precise names. https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:41: this._cumulativeHeights[i] = height += this._provider.fastHeight(i); style: this should be two lines https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:50: return this.refresh(); refresh does not return anything https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:61: element.remove(); Remove/nullify element.__insertedAt https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:81: element.__insertedAt = index; Use symbol. https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:91: return Math.max(firstVisibleIndex, this._firstActiveIndex); I don't get why we use this._firstActiveIndex. https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:100: return Math.min(lastVisibleIndex, this._lastActiveIndex); ditto https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:144: return this.element.offsetHeight; Maybe inline? https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui_lazy/FilteredListWidget.js (right): https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui_lazy/FilteredListWidget.js:292: _rowsPerViewport: function() Should this be a function of ViewportControl?
https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js (right): https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:33: refresh: function() On 2016/09/28 at 16:50:06, dgozman wrote: > What's the difference from invalidate? Let's have more precise names. invalidate rebuilds the contents inside the viewport, whereas refresh rebuilds the size and position of the content. I renamed invalidate -> invalidateContent. https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:41: this._cumulativeHeights[i] = height += this._provider.fastHeight(i); On 2016/09/28 at 16:50:05, dgozman wrote: > style: this should be two lines Done. https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:50: return this.refresh(); On 2016/09/28 at 16:50:05, dgozman wrote: > refresh does not return anything Done. https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:61: element.remove(); On 2016/09/28 at 16:50:06, dgozman wrote: > Remove/nullify element.__insertedAt I'd have to do that above where I call this._innerElement.removeChildren(). Feels strange to add a lot of logic to set this value to null when I never handle a case where the value is null. https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:81: element.__insertedAt = index; On 2016/09/28 at 16:50:06, dgozman wrote: > Use symbol. Done. https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:100: return Math.min(lastVisibleIndex, this._lastActiveIndex); On 2016/09/28 at 16:50:05, dgozman wrote: > ditto If the viewport has very few items, lastVisibleIndex will be larger than the number of items in the viewport. I could do Math.min(lastVisibleIndex, this._itemCount) but it feels safer to lock both of these values to the active indices. https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:144: return this.element.offsetHeight; On 2016/09/28 at 16:50:06, dgozman wrote: > Maybe inline? ViewportControl has a comment here about not using clientHeight because of a padding bug. The choice of which height to use has a lot of subtle changes, so I'd rather people use _visibleHeight instead of maybe using the wrong height.
https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js (right): https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:33: refresh: function() On 2016/09/28 19:19:42, einbinder wrote: > On 2016/09/28 at 16:50:06, dgozman wrote: > > What's the difference from invalidate? Let's have more precise names. > > invalidate rebuilds the contents inside the viewport, whereas refresh rebuilds > the size and position of the content. I renamed invalidate -> > invalidateContent. As a client of viewport, I still don't understand which one should I use. Can we describe these in terms of client, not viewport itself? https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:61: element.remove(); On 2016/09/28 19:19:42, einbinder wrote: > On 2016/09/28 at 16:50:06, dgozman wrote: > > Remove/nullify element.__insertedAt > > I'd have to do that above where I call this._innerElement.removeChildren(). > Feels strange to add a lot of logic to set this value to null when I never > handle a case where the value is null. Nothing prevents me from moving my element to another viewport control, which may be confused by this property being set. https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:100: return Math.min(lastVisibleIndex, this._lastActiveIndex); On 2016/09/28 19:19:42, einbinder wrote: > On 2016/09/28 at 16:50:05, dgozman wrote: > > ditto > > If the viewport has very few items, lastVisibleIndex will be larger than the > number of items in the viewport. I could do Math.min(lastVisibleIndex, > this._itemCount) but it feels safer to lock both of these values to the active > indices. Thanks for explaining. This is the only place we use active indices, so I wonder whether we should not store them at all and change to itemCount here. https://codereview.chromium.org/2371393002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js (right): https://codereview.chromium.org/2371393002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:18: this.element.addEventListener("scroll", this.refresh.bind(this), false); Should this be innerElement? https://codereview.chromium.org/2371393002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:25: WebInspector.StaticViewportControl.prototype = { nit: blank lines, please. https://codereview.chromium.org/2371393002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:164: fastHeight: function(index) { return 0; }, itemHeight?
https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js (right): https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:33: refresh: function() On 2016/09/28 at 20:56:18, dgozman wrote: > On 2016/09/28 19:19:42, einbinder wrote: > > On 2016/09/28 at 16:50:06, dgozman wrote: > > > What's the difference from invalidate? Let's have more precise names. > > > > invalidate rebuilds the contents inside the viewport, whereas refresh rebuilds > > the size and position of the content. I renamed invalidate -> > > invalidateContent. > > As a client of viewport, I still don't understand which one should I use. Can we describe these in terms of client, not viewport itself? Merged invalidate into refresh. https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:61: element.remove(); On 2016/09/28 at 20:56:18, dgozman wrote: > On 2016/09/28 19:19:42, einbinder wrote: > > On 2016/09/28 at 16:50:06, dgozman wrote: > > > Remove/nullify element.__insertedAt > > > > I'd have to do that above where I call this._innerElement.removeChildren(). > > Feels strange to add a lot of logic to set this value to null when I never > > handle a case where the value is null. > > Nothing prevents me from moving my element to another viewport control, which may be confused by this property being set. Gave each instance of StaticViewportControl its own symbol. https://codereview.chromium.org/2371393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:100: return Math.min(lastVisibleIndex, this._lastActiveIndex); On 2016/09/28 at 20:56:18, dgozman wrote: > On 2016/09/28 19:19:42, einbinder wrote: > > On 2016/09/28 at 16:50:05, dgozman wrote: > > > ditto > > > > If the viewport has very few items, lastVisibleIndex will be larger than the > > number of items in the viewport. I could do Math.min(lastVisibleIndex, > > this._itemCount) but it feels safer to lock both of these values to the active > > indices. > > Thanks for explaining. > This is the only place we use active indices, so I wonder whether we should not store them at all and change to itemCount here. Done. https://codereview.chromium.org/2371393002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js (right): https://codereview.chromium.org/2371393002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:18: this.element.addEventListener("scroll", this.refresh.bind(this), false); On 2016/09/28 at 20:56:18, dgozman wrote: > Should this be innerElement? The .element scrolls, the ._innerElement is just very tall. But there is a mistake here, should be _update. https://codereview.chromium.org/2371393002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:25: WebInspector.StaticViewportControl.prototype = { On 2016/09/28 at 20:56:18, dgozman wrote: > nit: blank lines, please. Done. https://codereview.chromium.org/2371393002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:164: fastHeight: function(index) { return 0; }, On 2016/09/28 at 20:56:18, dgozman wrote: > itemHeight? fastItemHeight
Looks pretty good, but I'd like Andrey to take a look as well. https://codereview.chromium.org/2371393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js (right): https://codereview.chromium.org/2371393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:19: this.element.addEventListener("scroll", this._update.bind(this), false); Did you look into making this passive for performance? We can compensate with additional elements prepopulated above and below the viewport. https://codereview.chromium.org/2371393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:51: var firstActiveIndex = Math.max(Array.prototype.lowerBound.call(this._cumulativeHeights, visibleFrom + 1 - (activeHeight - visibleHeight) / 2), 0); I feel like inlining activeHeight will make this more understandable. https://codereview.chromium.org/2371393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:54: for (var i = this._innerElement.children.length - 1; i >= 0; --i) { I think that saving children into local variable might speed things up. https://codereview.chromium.org/2371393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:70: if (!element || element.parentElement) Let's maybe check that parentElement === this._innerElement ? This will be more robust for the case I move elements between containers.
https://codereview.chromium.org/2371393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js (right): https://codereview.chromium.org/2371393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:19: this.element.addEventListener("scroll", this._update.bind(this), false); On 2016/09/29 at 20:35:50, dgozman wrote: > Did you look into making this passive for performance? We can compensate with additional elements prepopulated above and below the viewport. We are already prepopulating elements. I'd rather add passive in another CL. Passive scroll listeners are still quite new, they might have strange side effects. https://codereview.chromium.org/2371393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:51: var firstActiveIndex = Math.max(Array.prototype.lowerBound.call(this._cumulativeHeights, visibleFrom + 1 - (activeHeight - visibleHeight) / 2), 0); On 2016/09/29 at 20:35:50, dgozman wrote: > I feel like inlining activeHeight will make this more understandable. The value of activeHeight here is somewhat arbitrary. I want to isolate it from the rest of the code. https://codereview.chromium.org/2371393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:54: for (var i = this._innerElement.children.length - 1; i >= 0; --i) { On 2016/09/29 at 20:35:50, dgozman wrote: > I think that saving children into local variable might speed things up. Done. https://codereview.chromium.org/2371393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:70: if (!element || element.parentElement) On 2016/09/29 at 20:35:50, dgozman wrote: > Let's maybe check that parentElement === this._innerElement ? This will be more robust for the case I move elements between containers. Done.
lgtm, good stuff https://codereview.chromium.org/2371393002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js (right): https://codereview.chromium.org/2371393002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:21: this._insertedAtSymbol = Symbol("WebInspector.StaticViewportControl._insertedAtSymbol"); _indexSymbol https://codereview.chromium.org/2371393002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:95: var lastVisibleIndex = Math.max(Array.prototype.lowerBound.call(this._cumulativeHeights, this.element.scrollTop + this._visibleHeight()), 0); you can drop Math.max here https://codereview.chromium.org/2371393002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui_lazy/FilteredListWidget.js (right): https://codereview.chromium.org/2371393002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui_lazy/FilteredListWidget.js:386: this._rowHeight = WebInspector.measurePreferredSize(element, this._itemElementsContainer).height; technically, you can make this._viewportControl.terriblySlowlyMeasureElement() method, but it should not be much different
The CQ bit was checked by einbinder@chromium.org
https://codereview.chromium.org/2371393002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js (right): https://codereview.chromium.org/2371393002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:21: this._insertedAtSymbol = Symbol("WebInspector.StaticViewportControl._insertedAtSymbol"); On 2016/10/01 at 01:16:54, lushnikov wrote: > _indexSymbol Done. https://codereview.chromium.org/2371393002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/StaticViewportControl.js:95: var lastVisibleIndex = Math.max(Array.prototype.lowerBound.call(this._cumulativeHeights, this.element.scrollTop + this._visibleHeight()), 0); On 2016/10/01 at 01:16:54, lushnikov wrote: > you can drop Math.max here 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/2371393002/#ps80001 (title: "_indexSymbol")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by einbinder@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/2371393002/#ps80001 (title: "_indexSymbol")
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: Add lightweight StaticViewportControl BUG=none ========== to ========== DevTools: Add lightweight StaticViewportControl BUG=none Committed: https://crrev.com/ac8725368543179ac75b3ada6125f5867e790603 Cr-Commit-Position: refs/heads/master@{#422571} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ac8725368543179ac75b3ada6125f5867e790603 Cr-Commit-Position: refs/heads/master@{#422571} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
