Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2688803002:
DevTools: always do partial viewport update in console (Closed)
|
Created:
3 years, 10 months ago by luoe Modified:
3 years, 10 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 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: always do partial viewport update in console
Previously, all console viewport updates besides scrolling required a viewport
invalidation that removed all in-DOM elements and called wasShown/wasHide
unnecessarily. Now, viewport will always do a partial update.
BUG=none
Review-Url: https://codereview.chromium.org/2688803002
Cr-Commit-Position: refs/heads/master@{#451052}
Committed: https://chromium.googlesource.com/chromium/src/+/77837664dc6bab97b40da3585d86728d1bd79bec
Patch Set 1 #Patch Set 2 : focused #Patch Set 3 : more focused cl without driveby #
Total comments: 6
Patch Set 4 : ac #
Total comments: 6
Patch Set 5 : ac #
Messages
Total messages: 24 (10 generated)
Description was changed from ========== DevTools: always do partial viewport update in console BUG=none ========== to ========== DevTools: always do partial viewport update in console Previously, all console viewport updates besides scrolling required a viewport invalidation that removed all in-DOM elements and called wasShown/wasHide unnecessarily. Now, viewport will always do a partial update. BUG=none ==========
luoe@chromium.org changed reviewers: + einbinder@chromium.org, lushnikov@chromium.org, pfeldman@chromium.org
This CL is intended to be a self-contained prerequisite to another CL where rebuilding decoration icons will occur in ConsoleViewMessage.wasShown(). We don't want wasShown() to be called unnecessarily for performance reasons. I plan on adding tests soon, but you can take an early look!
Looks scary (hard to review) to me. Is there an incremental way?
The first patch set had some drive-by cleanup that I've removed in the 2nd patch. If it helps, the only changes are - Removes isInvalidating, fullViewportUpdate - partialViewportUpdate > updateViewport - updateViewport calls wasShown / willHide on a smaller set - prepare() used to be called after willHhide(), now it is called before ptal
On 2017/02/11 04:06:18, luoe wrote: > The first patch set had some drive-by cleanup that I've removed in the 2nd > patch. > > If it helps, the only changes are > - Removes isInvalidating, fullViewportUpdate > - partialViewportUpdate > updateViewport > - updateViewport calls wasShown / willHide on a smaller set > - prepare() used to be called after willHhide(), now it is called before > > ptal Really, the third point is the only one that's a dependency for the other icon CL so we could start with that.
ptal
https://codereview.chromium.org/2688803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2688803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:394: if (element !== anchor) { It looks like wasShown can be called on things that weren't hidden in the first place. Is that intentional?
https://codereview.chromium.org/2688803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2688803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:382: var willBeHidden = this._renderedItems.filter(item => !orderedItemsToRender.includes(item)); this will be faster with a Set() https://codereview.chromium.org/2688803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:394: if (element !== anchor) { On 2017/02/14 19:08:46, einbinder wrote: > It looks like wasShown can be called on things that weren't hidden in the first > place. Is that intentional? That's a good call. We probably want calls willHide() and wasShown() to be symmetrical
Test updated with reordering two messages. Ptal! https://codereview.chromium.org/2688803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2688803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:382: var willBeHidden = this._renderedItems.filter(item => !orderedItemsToRender.includes(item)); On 2017/02/14 21:37:51, lushnikov wrote: > this will be faster with a Set() Done. https://codereview.chromium.org/2688803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:394: if (element !== anchor) { Good point, there's no need for wasShown if the element was already inserted! https://codereview.chromium.org/2688803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:394: if (element !== anchor) { On 2017/02/14 21:37:51, lushnikov wrote: > On 2017/02/14 19:08:46, einbinder wrote: > > It looks like wasShown can be called on things that weren't hidden in the > first > > place. Is that intentional? > > That's a good call. We probably want calls willHide() and wasShown() to be > symmetrical Done.
lgtm https://codereview.chromium.org/2688803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2688803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:394: var wasReordered = element.parentElement === this._contentElement; can we check for element.parentElement being null? var shouldCallWasShown = !element.parentElement; if (shouldCallWasShown) wasShown.push(viewportElement); this._contentElement...
https://codereview.chromium.org/2688803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2688803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:397: this._contentElement.insertBefore(element, anchor); You are breaking the console.table (wasShown contract), aren't you?
https://codereview.chromium.org/2688803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2688803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:397: this._contentElement.insertBefore(element, anchor); On 2017/02/15 22:06:10, pfeldman wrote: > You are breaking the console.table (wasShown contract), aren't you? Element.prototype.insertBefore override seems to allow this case.
https://codereview.chromium.org/2688803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2688803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:394: var wasReordered = element.parentElement === this._contentElement; On 2017/02/15 21:34:14, lushnikov wrote: > can we check for element.parentElement being null? > > var shouldCallWasShown = !element.parentElement; > if (shouldCallWasShown) > wasShown.push(viewportElement); > this._contentElement... Done. https://codereview.chromium.org/2688803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:397: this._contentElement.insertBefore(element, anchor); On 2017/02/15 22:25:58, pfeldman wrote: > On 2017/02/15 22:06:10, pfeldman wrote: > > You are breaking the console.table (wasShown contract), aren't you? > > Element.prototype.insertBefore override seems to allow this case. Acknowledged. https://codereview.chromium.org/2688803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:397: this._contentElement.insertBefore(element, anchor); On 2017/02/15 22:06:10, pfeldman wrote: > You are breaking the console.table (wasShown contract), aren't you? Acknowledged.
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by luoe@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/2688803002/#ps80001 (title: "ac")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487268976592630,
"parent_rev": "0ac08e177bbcec4e6d2213da7a46a541633841b4", "commit_rev":
"77837664dc6bab97b40da3585d86728d1bd79bec"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: always do partial viewport update in console Previously, all console viewport updates besides scrolling required a viewport invalidation that removed all in-DOM elements and called wasShown/wasHide unnecessarily. Now, viewport will always do a partial update. BUG=none ========== to ========== DevTools: always do partial viewport update in console Previously, all console viewport updates besides scrolling required a viewport invalidation that removed all in-DOM elements and called wasShown/wasHide unnecessarily. Now, viewport will always do a partial update. BUG=none Review-Url: https://codereview.chromium.org/2688803002 Cr-Commit-Position: refs/heads/master@{#451052} Committed: https://chromium.googlesource.com/chromium/src/+/77837664dc6bab97b40da3585d86... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/77837664dc6bab97b40da3585d86... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
