Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2694743004:
Reset the nesting level for setInterval after completion. (Closed)
|
Created:
3 years, 10 months ago by Dan Elphick Modified:
3 years, 10 months ago CC:
altimin, blink-reviews, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReset the nesting level for setInterval after completion.
Previously after calling setInterval the nesting level would not be reset until
after calling setTimeout, resulting in the 4ms clamping to apply even to
subsequent setIntervals.
BUG=331912
Review-Url: https://codereview.chromium.org/2694743004
Cr-Commit-Position: refs/heads/master@{#450756}
Committed: https://chromium.googlesource.com/chromium/src/+/81f013144ccfc759101193455919b4c1d04c1715
Patch Set 1 #
Total comments: 10
Patch Set 2 : Apply review comments #
Total comments: 4
Patch Set 3 : remove use of gin #
Total comments: 2
Patch Set 4 : remove redundant copy of ExecutionContext #
Messages
Total messages: 25 (12 generated)
The CQ bit was checked by delphick@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...
alexclarke@chromium.org changed reviewers: + alexclarke@chromium.org
Seems pretty good, thanks! Just some nits. https://codereview.chromium.org/2694743004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/DOMTimerTest.cpp (right): https://codereview.chromium.org/2694743004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/DOMTimerTest.cpp:28: platform_; Sadly this is in blink, please rename to m_platform. NB one day blink will be reformatted fully to chromium style. https://codereview.chromium.org/2694743004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/DOMTimerTest.cpp:30: gin::Converter<std::vector<double>> converter; please rename to m_converter https://codereview.chromium.org/2694743004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/DOMTimerTest.cpp:55: const char* kSetTimeoutScriptText = We are allowed to use C++11 string literals now which should make this more readable: const char* kSetTimeoutScriptText = R(" var id; var last = performance.now(); ... getTimeout(nestSetTimeouts, 1);"); https://codereview.chromium.org/2694743004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/DOMTimerTest.cpp:73: const std::vector<Matcher<double>> kExpectedTimings = { AFAIK the style guide does not allow globals that dynamically allocate memory. I think a plain C style array should work too. https://codereview.chromium.org/2694743004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/DOMTimerTest.cpp:91: "var last = performance.now();" Ditto
https://codereview.chromium.org/2694743004/diff/1/third_party/WebKit/Source/DEPS File third_party/WebKit/Source/DEPS (right): https://codereview.chromium.org/2694743004/diff/1/third_party/WebKit/Source/D... third_party/WebKit/Source/DEPS:4: "+gin", Maybe add a test specific one specific_include_rules = { ".*test\.cpp": [ "+gin", ], }
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2694743004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/DOMTimerTest.cpp (right): https://codereview.chromium.org/2694743004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/DOMTimerTest.cpp:28: platform_; On 2017/02/14 16:22:18, alex clarke wrote: > Sadly this is in blink, please rename to m_platform. NB one day blink will be > reformatted fully to chromium style. Done. https://codereview.chromium.org/2694743004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/DOMTimerTest.cpp:30: gin::Converter<std::vector<double>> converter; On 2017/02/14 16:22:18, alex clarke wrote: > please rename to m_converter Done. https://codereview.chromium.org/2694743004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/DOMTimerTest.cpp:55: const char* kSetTimeoutScriptText = On 2017/02/14 16:22:18, alex clarke wrote: > We are allowed to use C++11 string literals now which should make this more > readable: > > const char* kSetTimeoutScriptText = R(" > var id; > var last = performance.now(); > ... > getTimeout(nestSetTimeouts, 1);"); Looks like check-webkit-style hasn't got the memo:( check-webkit-style failed third_party/WebKit/Source/core/frame/DOMTimerTest.cpp:64: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] https://codereview.chromium.org/2694743004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/DOMTimerTest.cpp:91: "var last = performance.now();" On 2017/02/14 16:22:18, alex clarke wrote: > Ditto As above.
Description was changed from ========== Reset the nesting level for setInterval after completion. Previously after calling setInterval the nesting level would not be reset until after calling setTimeout, resulting in the 4ms clamping to apply even to subsequent setIntervals. R=alexclarke BUG=331912 ========== to ========== Reset the nesting level for setInterval after completion. Previously after calling setInterval the nesting level would not be reset until after calling setTimeout, resulting in the 4ms clamping to apply even to subsequent setIntervals. R=alexclarke BUG=331912 ==========
delphick@chromium.org changed reviewers: + jochen@chromium.org - alexclarke@google.com
Description was changed from ========== Reset the nesting level for setInterval after completion. Previously after calling setInterval the nesting level would not be reset until after calling setTimeout, resulting in the 4ms clamping to apply even to subsequent setIntervals. R=alexclarke BUG=331912 ========== to ========== Reset the nesting level for setInterval after completion. Previously after calling setInterval the nesting level would not be reset until after calling setTimeout, resulting in the 4ms clamping to apply even to subsequent setIntervals. BUG=331912 ==========
Non owner LGTM
https://codereview.chromium.org/2694743004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/DEPS (right): https://codereview.chromium.org/2694743004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/DEPS:4: "+gin", please use macros from Source/bindings/core/v8 instead https://codereview.chromium.org/2694743004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/DOMTimer.cpp (right): https://codereview.chromium.org/2694743004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/DOMTimer.cpp:167: if (!executionContext) I don't think that's possible?
Thanks https://codereview.chromium.org/2694743004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/DEPS (right): https://codereview.chromium.org/2694743004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/DEPS:4: "+gin", On 2017/02/15 10:35:12, jochen wrote: > please use macros from Source/bindings/core/v8 instead I've changed it to use toImplArray from that directory. https://codereview.chromium.org/2694743004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/DOMTimer.cpp (right): https://codereview.chromium.org/2694743004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/DOMTimer.cpp:167: if (!executionContext) On 2017/02/15 10:35:12, jochen wrote: > I don't think that's possible? You're probably right - I was just playing it safe.
lgtm with comment addressed https://codereview.chromium.org/2694743004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/DOMTimer.cpp (right): https://codereview.chromium.org/2694743004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/DOMTimer.cpp:162: ExecutionContext* executionContext = getExecutionContext(); sorry for not catching this earlier, but |context| is already capturing executionContext, no need to create a duplicate variable here
https://codereview.chromium.org/2694743004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/DOMTimer.cpp (right): https://codereview.chromium.org/2694743004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/DOMTimer.cpp:162: ExecutionContext* executionContext = getExecutionContext(); On 2017/02/15 15:26:48, jochen wrote: > sorry for not catching this earlier, but |context| is already capturing > executionContext, no need to create a duplicate variable here Done.
The CQ bit was checked by delphick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexclarke@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2694743004/#ps60001 (title: "remove redundant copy of ExecutionContext")
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": 60001, "attempt_start_ts": 1487175913796630,
"parent_rev": "97f5f107d3c95b2a48c4dbb21befc5f7c810e190", "commit_rev":
"81f013144ccfc759101193455919b4c1d04c1715"}
Message was sent while issue was closed.
Description was changed from ========== Reset the nesting level for setInterval after completion. Previously after calling setInterval the nesting level would not be reset until after calling setTimeout, resulting in the 4ms clamping to apply even to subsequent setIntervals. BUG=331912 ========== to ========== Reset the nesting level for setInterval after completion. Previously after calling setInterval the nesting level would not be reset until after calling setTimeout, resulting in the 4ms clamping to apply even to subsequent setIntervals. BUG=331912 Review-Url: https://codereview.chromium.org/2694743004 Cr-Commit-Position: refs/heads/master@{#450756} Committed: https://chromium.googlesource.com/chromium/src/+/81f013144ccfc759101193455919... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/81f013144ccfc759101193455919... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
