Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2742653003:
[DevTools] chaotic green dots displacement fix (Closed)
|
Created:
3 years, 9 months ago by kozy Modified:
3 years, 9 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/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] chaotic green dots displacement fix
With the experiment, we see the markers for a brief second before they are
immediately removed without any user intervention. This CL fixes the race.
BUG=chromium:695236
R=luoe@chromium.org,lushnikov@chromium.org
Review-Url: https://codereview.chromium.org/2742653003
Cr-Commit-Position: refs/heads/master@{#456174}
Committed: https://chromium.googlesource.com/chromium/src/+/921d5298e10e7010e3609f5608e060cfac761ef7
Patch Set 1 #
Total comments: 2
Patch Set 2 : git branch #
Total comments: 2
Patch Set 3 : addressed comments #Messages
Total messages: 23 (13 generated)
Erik, please take a look.
The CQ bit was checked by kozyatinskiy@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...
lgtm. Can we update the description? For reference, the repro is: 1 pause on breakpoint 2 step into function With the experiment, we see the markers for a brief second before they are immediately removed without any user intervention. This CL fixes the race.
https://codereview.chromium.org/2742653003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js (right): https://codereview.chromium.org/2742653003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:807: this._clearValueWidgetsTimer = setTimeout(this._clearValueWidgets.bind(this), 1000); don't you want the same for inline values? https://codereview.chromium.org/2742653003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:810: clearTimeout(this._clearContinueToLocationsTimer); let's rather fix the original root of the problem!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thanks! all done! currently aligned with inline scope variables.
The CQ bit was checked by kozyatinskiy@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...
Description was changed from ========== [DevTools] always cleanup clear timeout before scheduling new one.. ..otherwise green dots are disappearing. BUG=chromium:695236 R=luoe@chromium.org,lushnikov@chromium.org ========== to ========== [DevTools] chaotic green dots displacement fix With the experiment, we see the markers for a brief second before they are immediately removed without any user intervention. This CL fixes the race. BUG=chromium:695236 R=luoe@chromium.org,lushnikov@chromium.org ==========
https://codereview.chromium.org/2742653003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js (right): https://codereview.chromium.org/2742653003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:601: } Why don't we want greendots to be removed in case of absence of localScope? Also, how is this fixing the race? Green dots will or will not be removed depending on the timegap between _showContinueToLocations invocation.
https://codereview.chromium.org/2742653003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js (right): https://codereview.chromium.org/2742653003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:601: } On 2017/03/10 04:51:57, lushnikov wrote: > Why don't we want greendots to be removed in case of absence of localScope? > > Also, how is this fixing the race? Green dots will or will not be removed > depending on the timegap between _showContinueToLocations invocation. Let me explain. When we clearExecutionLine we schedule green dots removing in 1 sec, if new execution line arrive before 1 second - we clear this timeout in this function and show correct green dots, otherwise we cleanup green dots. Issue was in the fact that we don't cancel green dots removing in case when localScope is empty - e.g. scope when top level frame just inside of the script and script doesn't have local variables by itself - but we should, otherwise next time when we call show green dots - we override clear timeout with new one and just never clear original one.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I couldn't reproduce case when we go from place with localScope to place without localScope but let be ready. Please take another look!
lgtm
The CQ bit was checked by kozyatinskiy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from luoe@chromium.org Link to the patchset: https://codereview.chromium.org/2742653003/#ps40001 (title: "addressed comments")
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": 1489174436541810,
"parent_rev": "adcdf2d7c58a495e0f917a50148c2f041a1a6e85", "commit_rev":
"921d5298e10e7010e3609f5608e060cfac761ef7"}
Message was sent while issue was closed.
Description was changed from ========== [DevTools] chaotic green dots displacement fix With the experiment, we see the markers for a brief second before they are immediately removed without any user intervention. This CL fixes the race. BUG=chromium:695236 R=luoe@chromium.org,lushnikov@chromium.org ========== to ========== [DevTools] chaotic green dots displacement fix With the experiment, we see the markers for a brief second before they are immediately removed without any user intervention. This CL fixes the race. BUG=chromium:695236 R=luoe@chromium.org,lushnikov@chromium.org Review-Url: https://codereview.chromium.org/2742653003 Cr-Commit-Position: refs/heads/master@{#456174} Committed: https://chromium.googlesource.com/chromium/src/+/921d5298e10e7010e3609f5608e0... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/921d5298e10e7010e3609f5608e0... |
