Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2799043006:
DevTools: make console viewport less error prone by always defining cumulativeHeights (Closed)
|
Created:
3 years, 8 months ago by luoe Modified:
3 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, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: make console viewport less error prone by always defining cumulativeHeights
ConsoleViewport's firstVisibleIndex() may throw an error if it calls
'lowerBound' while _cumulativeHeights is not defined. This scenario happens when
loading console after invalidating an empty viewport. Invalidate() deletes
_cumulativeHeights, the prompt text is changed, then firstVisibleIndex() is
called.
To make viewport's public functions less error prone, we can ensure that
_cumulativeHeights is always defined, using an empty typed array by default.
BUG=708845
Review-Url: https://codereview.chromium.org/2799043006
Cr-Commit-Position: refs/heads/master@{#463333}
Committed: https://chromium.googlesource.com/chromium/src/+/2d95267d052ef6af703663efd9fde7c3bb1730ba
Patch Set 1 #
Total comments: 6
Patch Set 2 : ac #Patch Set 3 : remove unrelated changes #Messages
Total messages: 19 (12 generated)
Description was changed from ========== DevTools: fix invalidate with restoring cumulative heights BUG=708845 ========== to ========== DevTools: fix invalidate with restoring cumulative heights ConsoleViewport's firstVisibleIndex() may throw an error if it calls 'lowerBound' while _cumulativeHeights is not defined. This scenario happens when loading console after invalidating an empty viewport. Invalidate() deletes _cumulativeHeights, the prompt text is changed, then firstVisibleIndex() is called. To make viewport's public functions less error prone, we can ensure that _cumulativeHeights is always defined, using an empty typed array by default. BUG=708845 ==========
Description was changed from ========== DevTools: fix invalidate with restoring cumulative heights ConsoleViewport's firstVisibleIndex() may throw an error if it calls 'lowerBound' while _cumulativeHeights is not defined. This scenario happens when loading console after invalidating an empty viewport. Invalidate() deletes _cumulativeHeights, the prompt text is changed, then firstVisibleIndex() is called. To make viewport's public functions less error prone, we can ensure that _cumulativeHeights is always defined, using an empty typed array by default. BUG=708845 ========== to ========== DevTools: make console viewport less error prone by always defining cumulativeHeights ConsoleViewport's firstVisibleIndex() may throw an error if it calls 'lowerBound' while _cumulativeHeights is not defined. This scenario happens when loading console after invalidating an empty viewport. Invalidate() deletes _cumulativeHeights, the prompt text is changed, then firstVisibleIndex() is called. To make viewport's public functions less error prone, we can ensure that _cumulativeHeights is always defined, using an empty typed array by default. BUG=708845 ==========
luoe@chromium.org changed reviewers: + dgozman@chromium.org, einbinder@chromium.org
Please take a look https://codereview.chromium.org/2799043006/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (left): https://codereview.chromium.org/2799043006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:182: this._viewport.invalidate(); drive by: Orthogonal to the root problem, but we don't need to invalidate until the console wasShown() https://codereview.chromium.org/2799043006/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (left): https://codereview.chromium.org/2799043006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:541: this._rebuildCumulativeHeightsIfNeeded(); By default, CH is an empty typed array if there are no items. CH only changes in invalidate() and innerRefresh() so we should only need to rebuild there.
dgozman@chromium.org changed reviewers: + lushnikov@chromium.org
Seems legit, but lushnikov@ knows better. https://codereview.chromium.org/2799043006/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2799043006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:304: this._viewport.invalidate(); Why this change? https://codereview.chromium.org/2799043006/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2799043006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:146: if (!this._itemCount) { I think this special-casing is not needed anymore.
Ready for review https://codereview.chromium.org/2799043006/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2799043006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:304: this._viewport.invalidate(); I actually think we don't need this at all! Maybe we can get rid of it? It was introduced in the original viewport impl so perhaps lushnikov@ knows. https://codereview.chromium.org/2799043006/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2799043006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:146: if (!this._itemCount) { On 2017/04/07 20:31:44, dgozman wrote: > I think this special-casing is not needed anymore. Done.
viewport lgtm! Let's have consoleview changes separately
Done, I'll make a separate patch for the unrelated stuff. Thanks for the quick review
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: This issue passed the CQ dry run.
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/2799043006/#ps40001 (title: "remove unrelated changes")
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": 40001, "attempt_start_ts": 1491842575267000,
"parent_rev": "39c7bd71dc2f9c9f692dd346e4e4e525af25ede1", "commit_rev":
"2d95267d052ef6af703663efd9fde7c3bb1730ba"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: make console viewport less error prone by always defining cumulativeHeights ConsoleViewport's firstVisibleIndex() may throw an error if it calls 'lowerBound' while _cumulativeHeights is not defined. This scenario happens when loading console after invalidating an empty viewport. Invalidate() deletes _cumulativeHeights, the prompt text is changed, then firstVisibleIndex() is called. To make viewport's public functions less error prone, we can ensure that _cumulativeHeights is always defined, using an empty typed array by default. BUG=708845 ========== to ========== DevTools: make console viewport less error prone by always defining cumulativeHeights ConsoleViewport's firstVisibleIndex() may throw an error if it calls 'lowerBound' while _cumulativeHeights is not defined. This scenario happens when loading console after invalidating an empty viewport. Invalidate() deletes _cumulativeHeights, the prompt text is changed, then firstVisibleIndex() is called. To make viewport's public functions less error prone, we can ensure that _cumulativeHeights is always defined, using an empty typed array by default. BUG=708845 Review-Url: https://codereview.chromium.org/2799043006 Cr-Commit-Position: refs/heads/master@{#463333} Committed: https://chromium.googlesource.com/chromium/src/+/2d95267d052ef6af703663efd9fd... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/2d95267d052ef6af703663efd9fd... |
