Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2522783003:
MD Settings: Fix CrScrollableBehavior (Closed)
|
Created:
4 years ago by stevenjb Modified:
4 years ago Reviewers:
dpapad CC:
chromium-reviews, dbeam+watch-elements_chromium.org, michaelpg+watch-elements_chromium.org, oshima+watch_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Fix CrScrollableBehavior
Currently if updateScrollableContents gets called more than once
before the container height resolves, setInterval will be called
multiplie times but only cleared once. This is very bad and is
responsible for strange behavior and (likely) performance issues.
BUG=665361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/d5abef8fb532c2f2d647463052c1d5d9bd99c794
Cr-Commit-Position: refs/heads/master@{#434235}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix logic and use null #
Total comments: 4
Patch Set 3 : Feedback #Messages
Total messages: 16 (6 generated)
Description was changed from ========== MD Settings: Fix CrScrollableBehavior Currently if updateScrollableContents gets called more than once before the container height resolves, setInterval will be called multiplie times but only cleared once. This is very bad and is responsible for strange behavior and (likely) performance issues. BUG=665631 ========== to ========== MD Settings: Fix CrScrollableBehavior Currently if updateScrollableContents gets called more than once before the container height resolves, setInterval will be called multiplie times but only cleared once. This is very bad and is responsible for strange behavior and (likely) performance issues. BUG=665631 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
stevenjb@chromium.org changed reviewers: + dpapad@chromium.org
https://codereview.chromium.org/2522783003/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_scrollable_behavior.js (right): https://codereview.chromium.org/2522783003/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_scrollable_behavior.js:71: if (this.intervalId_ === undefined) Shouldn't this be the opposite? if (this.intervalId_ !== undefined) return; such that we return early if this method has already been called? https://codereview.chromium.org/2522783003/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_scrollable_behavior.js:85: this.intervalId_ = undefined; Setting something to |undefined| is counter intuitive. |undefined| in JS is the default value of something that has not been defined yet like "var a;". Can we use null instead and initialize intervalId_ to null at line 41? |null| signifies more explicitly that a value has been assigned, unlike |undefined| which can be the result of various errors (typos for example).
PTAL https://codereview.chromium.org/2522783003/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_scrollable_behavior.js (right): https://codereview.chromium.org/2522783003/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_scrollable_behavior.js:71: if (this.intervalId_ === undefined) On 2016/11/23 01:12:51, dpapad wrote: > Shouldn't this be the opposite? > > if (this.intervalId_ !== undefined) > return; > > such that we return early if this method has already been called? Er, ugh, yes. I must already be in vacation mode. I guess this "fixed" the problem by never running. I'll have to make sure to more carefully test the cases where we need this logic. Done. https://codereview.chromium.org/2522783003/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_scrollable_behavior.js:85: this.intervalId_ = undefined; On 2016/11/23 01:12:51, dpapad wrote: > Setting something to |undefined| is counter intuitive. |undefined| in JS is the > default value of something that has not been defined yet like "var a;". > > Can we use null instead and initialize intervalId_ to null at line 41? |null| > signifies more explicitly that a value has been assigned, unlike |undefined| > which can be the result of various errors (typos for example). It seems odd to me to set a Number to null, but I guess it isn't really different than setting it to undefined (and I have to pacify Closure either way). Done.
On 2016/11/23 18:07:01, stevenjb wrote: > PTAL > > https://codereview.chromium.org/2522783003/diff/1/ui/webui/resources/cr_eleme... > File ui/webui/resources/cr_elements/cr_scrollable_behavior.js (right): > > https://codereview.chromium.org/2522783003/diff/1/ui/webui/resources/cr_eleme... > ui/webui/resources/cr_elements/cr_scrollable_behavior.js:71: if > (this.intervalId_ === undefined) > On 2016/11/23 01:12:51, dpapad wrote: > > Shouldn't this be the opposite? > > > > if (this.intervalId_ !== undefined) > > return; > > > > such that we return early if this method has already been called? > > Er, ugh, yes. I must already be in vacation mode. I guess this "fixed" the > problem by never running. I'll have to make sure to more carefully test the > cases where we need this logic. > Done. > > https://codereview.chromium.org/2522783003/diff/1/ui/webui/resources/cr_eleme... > ui/webui/resources/cr_elements/cr_scrollable_behavior.js:85: this.intervalId_ = > undefined; > On 2016/11/23 01:12:51, dpapad wrote: > > Setting something to |undefined| is counter intuitive. |undefined| in JS is > the > > default value of something that has not been defined yet like "var a;". > > > > Can we use null instead and initialize intervalId_ to null at line 41? |null| > > signifies more explicitly that a value has been assigned, unlike |undefined| > > which can be the result of various errors (typos for example). > > It seems odd to me to set a Number to null, but I guess it isn't really > different than setting it to undefined (and I have to pacify Closure either > way). Done. P.S. I was able to verify that a) we still need this logic and b) this fix actually works now.
https://codereview.chromium.org/2522783003/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_scrollable_behavior.js (right): https://codereview.chromium.org/2522783003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_scrollable_behavior.js:71: updateScrollableContents() { updateScrollableContents: function() { https://codereview.chromium.org/2522783003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_scrollable_behavior.js:74: if (this.intervalId_ !== null) Can you move this before the querySelectorAll? So that we don't do any unnecessary work if we are going to return early?
PTAL https://codereview.chromium.org/2522783003/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_scrollable_behavior.js (right): https://codereview.chromium.org/2522783003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_scrollable_behavior.js:71: updateScrollableContents() { On 2016/11/23 18:30:21, dpapad wrote: > updateScrollableContents: function() { Sigh. Still a C++ developer at heart. Done. https://codereview.chromium.org/2522783003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_scrollable_behavior.js:74: if (this.intervalId_ !== null) On 2016/11/23 18:30:21, dpapad wrote: > Can you move this before the querySelectorAll? So that we don't do any > unnecessary work if we are going to return early? Good catch. Done.
lgtm
The CQ bit was checked by stevenjb@chromium.org
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": 1479926686846130,
"parent_rev": "d67eacefefaa0df6d08d6b8cb5ddf8718fa0609a", "commit_rev":
"80578175da4ad98bdd5a1b616bde5dd3785f4cf1"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Fix CrScrollableBehavior Currently if updateScrollableContents gets called more than once before the container height resolves, setInterval will be called multiplie times but only cleared once. This is very bad and is responsible for strange behavior and (likely) performance issues. BUG=665631 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Fix CrScrollableBehavior Currently if updateScrollableContents gets called more than once before the container height resolves, setInterval will be called multiplie times but only cleared once. This is very bad and is responsible for strange behavior and (likely) performance issues. BUG=665631 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/d5abef8fb532c2f2d647463052c1d5d9bd99c794 Cr-Commit-Position: refs/heads/master@{#434235} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d5abef8fb532c2f2d647463052c1d5d9bd99c794 Cr-Commit-Position: refs/heads/master@{#434235}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Fix CrScrollableBehavior Currently if updateScrollableContents gets called more than once before the container height resolves, setInterval will be called multiplie times but only cleared once. This is very bad and is responsible for strange behavior and (likely) performance issues. BUG=665631 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/d5abef8fb532c2f2d647463052c1d5d9bd99c794 Cr-Commit-Position: refs/heads/master@{#434235} ========== to ========== MD Settings: Fix CrScrollableBehavior Currently if updateScrollableContents gets called more than once before the container height resolves, setInterval will be called multiplie times but only cleared once. This is very bad and is responsible for strange behavior and (likely) performance issues. BUG=665361 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/d5abef8fb532c2f2d647463052c1d5d9bd99c794 Cr-Commit-Position: refs/heads/master@{#434235} ========== |
