Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(62)

Issue 721033004: Implement WindowTimers.set{Timeout,Interval} without [Custom] (Closed)

Created:
6 years, 1 month ago by Jens Widell
Modified:
6 years, 1 month ago
CC:
arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, Inactive
Base URL:
https://chromium.googlesource.com/chromium/blink.git@idl-overload-with-variadic
Project:
blink
Visibility:
Public.

Description

Implement WindowTimers.set{Timeout,Interval} without [Custom] This allows us to drop two fairly big and largely duplicated chunks of custom bindings code in V8WindowCustom.cpp/V8WorkerGlobalScopeCustom.cpp. As a consequence, some more logic is added to DOMWindowTimers.cpp, which is now responsible for doing the "is this allowed?" checks, and creating the ScheduledAction objects. Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185538

Patch Set 1 #

Total comments: 15

Patch Set 2 : address comments #

Patch Set 3 : rebased #

Total comments: 12

Patch Set 4 : style fixes #

Patch Set 5 : more style fixes #

Patch Set 6 : resurrect call to V8GCForContextDispose::notifyIdle() #

Patch Set 7 : drop some more includes #

Total comments: 1

Patch Set 8 : refine notifyIdle hack #

Patch Set 9 : rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+117 lines, -185 lines) Patch
M Source/bindings/core/v8/ScheduledAction.h View 1 2 3 4 1 chunk +6 lines, -2 lines 0 comments Download
M Source/bindings/core/v8/ScheduledAction.cpp View 1 2 chunks +24 lines, -12 lines 0 comments Download
M Source/bindings/core/v8/custom/V8WindowCustom.cpp View 1 2 3 4 5 6 7 8 3 chunks +0 lines, -98 lines 0 comments Download
M Source/bindings/core/v8/custom/V8WorkerGlobalScopeCustom.cpp View 1 chunk +0 lines, -59 lines 0 comments Download
M Source/core/frame/DOMWindowTimers.h View 1 2 3 1 chunk +7 lines, -3 lines 0 comments Download
M Source/core/frame/DOMWindowTimers.cpp View 1 2 3 4 5 6 7 1 chunk +74 lines, -4 lines 0 comments Download
M Source/core/frame/WindowTimers.idl View 1 chunk +6 lines, -7 lines 0 comments Download

Messages

Total messages: 33 (6 generated)
Jens Widell
PTAL The try-bot results for PS#1 are irrelevant; it's being tested without https://codereview.chromium.org/723013003/ which it ...
6 years, 1 month ago (2014-11-13 12:31:25 UTC) #2
Jens Widell
https://codereview.chromium.org/721033004/diff/1/Source/bindings/core/v8/custom/V8WindowCustom.cpp File Source/bindings/core/v8/custom/V8WindowCustom.cpp (left): https://codereview.chromium.org/721033004/diff/1/Source/bindings/core/v8/custom/V8WindowCustom.cpp#oldcode83 Source/bindings/core/v8/custom/V8WindowCustom.cpp:83: exceptionState.throwDOMException(InvalidAccessError, "No script context is available in which to ...
6 years, 1 month ago (2014-11-13 12:37:47 UTC) #3
haraken
https://codereview.chromium.org/721033004/diff/1/Source/bindings/core/v8/ScheduledAction.cpp File Source/bindings/core/v8/ScheduledAction.cpp (right): https://codereview.chromium.org/721033004/diff/1/Source/bindings/core/v8/ScheduledAction.cpp#newcode49 Source/bindings/core/v8/ScheduledAction.cpp:49: ScheduledAction::ScheduledAction(ScriptState* scriptState, const ScriptValue& function, const Vector<ScriptValue>& arguments, v8::Isolate* ...
6 years, 1 month ago (2014-11-13 12:59:32 UTC) #4
Jens Widell
https://codereview.chromium.org/721033004/diff/1/Source/core/frame/DOMWindowTimers.cpp File Source/core/frame/DOMWindowTimers.cpp (right): https://codereview.chromium.org/721033004/diff/1/Source/core/frame/DOMWindowTimers.cpp#newcode52 Source/core/frame/DOMWindowTimers.cpp:52: return false; On 2014/11/13 12:59:31, haraken wrote: > > ...
6 years, 1 month ago (2014-11-13 13:12:42 UTC) #5
Jens Widell
https://codereview.chromium.org/721033004/diff/1/Source/bindings/core/v8/ScheduledAction.cpp File Source/bindings/core/v8/ScheduledAction.cpp (right): https://codereview.chromium.org/721033004/diff/1/Source/bindings/core/v8/ScheduledAction.cpp#newcode49 Source/bindings/core/v8/ScheduledAction.cpp:49: ScheduledAction::ScheduledAction(ScriptState* scriptState, const ScriptValue& function, const Vector<ScriptValue>& arguments, v8::Isolate* ...
6 years, 1 month ago (2014-11-13 13:26:48 UTC) #6
haraken
LGTM
6 years, 1 month ago (2014-11-13 14:56:58 UTC) #7
Jens Widell
Status update: This patch breaks a lot of tests according to the try-bots. Unfortunately, I ...
6 years, 1 month ago (2014-11-14 07:13:02 UTC) #8
haraken
+yutak I think setTimeout has a very good test coverage.
6 years, 1 month ago (2014-11-14 07:14:24 UTC) #10
Jens Widell
On 2014/11/14 07:14:24, haraken wrote: > +yutak > > I think setTimeout has a very ...
6 years, 1 month ago (2014-11-14 07:15:58 UTC) #11
Jens Widell
https://codereview.chromium.org/721033004/diff/40001/Source/bindings/core/v8/custom/V8WindowCustom.cpp File Source/bindings/core/v8/custom/V8WindowCustom.cpp (left): https://codereview.chromium.org/721033004/diff/40001/Source/bindings/core/v8/custom/V8WindowCustom.cpp#oldcode145 Source/bindings/core/v8/custom/V8WindowCustom.cpp:145: V8GCForContextDispose::instance().notifyIdle(); I've lost this call, BTW. That was not ...
6 years, 1 month ago (2014-11-14 07:23:58 UTC) #12
Yuta Kitamura
LGTM, just a number of stylistic issues. You need to figure out what's happening in ...
6 years, 1 month ago (2014-11-14 08:24:06 UTC) #13
Jens Widell
On 2014/11/14 08:24:06, Yuta Kitamura wrote: > LGTM, just a number of stylistic issues. Thanks! ...
6 years, 1 month ago (2014-11-14 10:19:28 UTC) #14
Jens Widell
haraken@: PTAL? In PS#6 I've resurrected the call to V8GCForContextDispose::notifyIdle() that was in the old ...
6 years, 1 month ago (2014-11-17 10:11:21 UTC) #15
haraken
> Notable difference: we now call it for both the Window and WorkerGlobalScope cases, whereas ...
6 years, 1 month ago (2014-11-17 10:27:39 UTC) #16
haraken
+jochen
6 years, 1 month ago (2014-11-17 10:27:51 UTC) #18
jochen (gone - plz use gerrit)
this idle call is really only used for sunspider. we should trigger it as little ...
6 years, 1 month ago (2014-11-17 10:34:03 UTC) #19
Jens Widell
On 2014/11/17 10:34:03, jochen (slow) wrote: > this idle call is really only used for ...
6 years, 1 month ago (2014-11-17 10:35:39 UTC) #20
jochen (gone - plz use gerrit)
On 2014/11/17 at 10:35:39, jl wrote: > On 2014/11/17 10:34:03, jochen (slow) wrote: > > ...
6 years, 1 month ago (2014-11-17 10:39:15 UTC) #21
Jens Widell
On 2014/11/17 10:39:15, jochen (slow) wrote: > On 2014/11/17 at 10:35:39, jl wrote: > > ...
6 years, 1 month ago (2014-11-17 10:50:28 UTC) #22
jochen (gone - plz use gerrit)
setTimeout only would make sense, yes
6 years, 1 month ago (2014-11-17 10:51:25 UTC) #23
Jens Widell
On 2014/11/17 10:51:25, jochen (slow) wrote: > setTimeout only would make sense, yes Patch updated ...
6 years, 1 month ago (2014-11-18 09:34:28 UTC) #24
haraken
On 2014/11/18 09:34:28, Jens Widell wrote: > On 2014/11/17 10:51:25, jochen (slow) wrote: > > ...
6 years, 1 month ago (2014-11-18 09:36:36 UTC) #25
jochen (gone - plz use gerrit)
lgtm
6 years, 1 month ago (2014-11-18 13:42:00 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/721033004/140001
6 years, 1 month ago (2014-11-18 16:14:40 UTC) #28
commit-bot: I haz the power
Failed to apply patch for Source/bindings/core/v8/custom/V8WindowCustom.cpp: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
6 years, 1 month ago (2014-11-18 16:14:55 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/721033004/160001
6 years, 1 month ago (2014-11-18 16:46:23 UTC) #32
commit-bot: I haz the power
6 years, 1 month ago (2014-11-18 18:01:46 UTC) #33
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185538

Powered by Google App Engine
This is Rietveld 408576698