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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 660863002: [DevTools] Added public method for async execution of scripts (Closed)

Created:
6 years, 2 months ago by kozyatinskiy1
Modified:
6 years, 2 months ago
Reviewers:
vsevik, aandrey, dcheng, pfeldman
CC:
arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, blink-reviews-dom_chromium.org, dglazkov+blink, eae+blinkwatch, falken, horo+watch_chromium.org, kinuko+worker_chromium.org, mkwst+moarreviews_chromium.org, rwlbuis, sof
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

[DevTools] Added public method for async execution of scripts This methods accept callback which will be called when scripts finished execution. R=vsevik@chromium.org BUG=410289 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184279

Patch Set 1 : #

Patch Set 2 : platform/WebExecuteScriptCallback -> web/WebScriptCallback #

Total comments: 5

Patch Set 3 : #

Total comments: 10

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Total comments: 12

Patch Set 7 : #

Total comments: 9

Patch Set 8 : #

Patch Set 9 : Added deprecated label for not suspendable execute methods #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+228 lines, -12 lines) Patch
A Source/web/SuspendableScriptExecutor.h View 1 2 3 4 5 6 7 1 chunk +43 lines, -0 lines 0 comments Download
A Source/web/SuspendableScriptExecutor.cpp View 1 2 3 4 5 6 7 1 chunk +76 lines, -0 lines 0 comments Download
M Source/web/WebLocalFrameImpl.h View 1 2 3 4 5 6 3 chunks +7 lines, -0 lines 0 comments Download
M Source/web/WebLocalFrameImpl.cpp View 1 2 3 4 5 6 7 5 chunks +30 lines, -12 lines 1 comment Download
A Source/web/WebScriptSource.cpp View 1 2 3 4 5 6 7 1 chunk +19 lines, -0 lines 0 comments Download
M Source/web/web.gypi View 1 2 3 4 5 6 7 2 chunks +3 lines, -0 lines 0 comments Download
M public/web/WebFrame.h View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M public/web/WebLocalFrame.h View 1 2 3 4 5 6 7 2 chunks +14 lines, -0 lines 0 comments Download
A public/web/WebScriptExecutionCallback.h View 1 2 3 4 1 chunk +28 lines, -0 lines 0 comments Download
M public/web/WebScriptSource.h View 1 2 3 4 5 6 7 2 chunks +6 lines, -0 lines 0 comments Download

Messages

Total messages: 27 (6 generated)
kozyatinskiy1
Vsevolod, please take a look.
6 years, 2 months ago (2014-10-16 16:57:36 UTC) #1
dcheng
6 years, 2 months ago (2014-10-16 16:58:43 UTC) #4
kozyatinskiy1
https://codereview.chromium.org/660863002/diff/40001/Source/bindings/core/v8/ScheduledActionWithCallback.cpp File Source/bindings/core/v8/ScheduledActionWithCallback.cpp (right): https://codereview.chromium.org/660863002/diff/40001/Source/bindings/core/v8/ScheduledActionWithCallback.cpp#newcode15 Source/bindings/core/v8/ScheduledActionWithCallback.cpp:15: #include "public/web/WebScriptCallback.h" For this line presubmit said: You added ...
6 years, 2 months ago (2014-10-17 08:44:21 UTC) #5
vsevik
https://codereview.chromium.org/660863002/diff/40001/Source/bindings/core/v8/ScheduledAction.cpp File Source/bindings/core/v8/ScheduledAction.cpp (right): https://codereview.chromium.org/660863002/diff/40001/Source/bindings/core/v8/ScheduledAction.cpp#newcode1 Source/bindings/core/v8/ScheduledAction.cpp:1: // Copyright 2014 The Chromium Authors. All rights reserved. ...
6 years, 2 months ago (2014-10-17 13:19:44 UTC) #6
dcheng
https://codereview.chromium.org/660863002/diff/40001/public/web/WebFrame.h File public/web/WebFrame.h (right): https://codereview.chromium.org/660863002/diff/40001/public/web/WebFrame.h#newcode306 public/web/WebFrame.h:306: const WebScriptSource&, WebScriptCallback*) = 0; Please address my comments ...
6 years, 2 months ago (2014-10-20 07:13:54 UTC) #7
vsevik
On 2014/10/20 07:13:54, dcheng wrote: > https://codereview.chromium.org/660863002/diff/40001/public/web/WebFrame.h > File public/web/WebFrame.h (right): > > https://codereview.chromium.org/660863002/diff/40001/public/web/WebFrame.h#newcode306 > ...
6 years, 2 months ago (2014-10-20 07:48:46 UTC) #8
dcheng
On 2014/10/20 07:48:46, vsevik wrote: > On 2014/10/20 07:13:54, dcheng wrote: > > https://codereview.chromium.org/660863002/diff/40001/public/web/WebFrame.h > ...
6 years, 2 months ago (2014-10-20 08:12:07 UTC) #9
vsevik
https://codereview.chromium.org/660863002/diff/80001/Source/core/dom/SuspendableScriptRunner.cpp File Source/core/dom/SuspendableScriptRunner.cpp (right): https://codereview.chromium.org/660863002/diff/80001/Source/core/dom/SuspendableScriptRunner.cpp#newcode36 Source/core/dom/SuspendableScriptRunner.cpp:36: execute(); // delete this executeAndDestroySelf() ? https://codereview.chromium.org/660863002/diff/80001/Source/web/WebLocalFrameImpl.h File Source/web/WebLocalFrameImpl.h ...
6 years, 2 months ago (2014-10-20 16:50:37 UTC) #11
kozyatinskiy1
On 2014/10/20 07:13:54, dcheng wrote: > https://codereview.chromium.org/660863002/diff/40001/public/web/WebFrame.h > File public/web/WebFrame.h (right): > > https://codereview.chromium.org/660863002/diff/40001/public/web/WebFrame.h#newcode306 > ...
6 years, 2 months ago (2014-10-21 12:46:37 UTC) #12
kozyatinskiy1
https://codereview.chromium.org/660863002/diff/80001/Source/core/dom/SuspendableScriptRunner.cpp File Source/core/dom/SuspendableScriptRunner.cpp (right): https://codereview.chromium.org/660863002/diff/80001/Source/core/dom/SuspendableScriptRunner.cpp#newcode36 Source/core/dom/SuspendableScriptRunner.cpp:36: execute(); // delete this On 2014/10/20 16:50:37, vsevik wrote: ...
6 years, 2 months ago (2014-10-21 12:46:47 UTC) #13
kozyatinskiy1
Pavel, PTAL.
6 years, 2 months ago (2014-10-21 14:33:38 UTC) #15
vsevik
https://codereview.chromium.org/660863002/diff/140001/Source/core/dom/SuspendableScriptRunner.cpp File Source/core/dom/SuspendableScriptRunner.cpp (right): https://codereview.chromium.org/660863002/diff/140001/Source/core/dom/SuspendableScriptRunner.cpp#newcode17 Source/core/dom/SuspendableScriptRunner.cpp:17: SuspendableScriptRunner::SuspendableScriptRunner(LocalFrame* frame, int worldID, const Vector<ScriptSourceCode>& sources, int extensionGroup, ...
6 years, 2 months ago (2014-10-22 13:29:00 UTC) #16
kozyatinskiy1
https://codereview.chromium.org/660863002/diff/140001/Source/core/dom/SuspendableScriptRunner.cpp File Source/core/dom/SuspendableScriptRunner.cpp (right): https://codereview.chromium.org/660863002/diff/140001/Source/core/dom/SuspendableScriptRunner.cpp#newcode17 Source/core/dom/SuspendableScriptRunner.cpp:17: SuspendableScriptRunner::SuspendableScriptRunner(LocalFrame* frame, int worldID, const Vector<ScriptSourceCode>& sources, int extensionGroup, ...
6 years, 2 months ago (2014-10-22 13:53:03 UTC) #17
vsevik
lgtm
6 years, 2 months ago (2014-10-22 14:06:53 UTC) #18
pfeldman
https://codereview.chromium.org/660863002/diff/160001/Source/core/dom/SuspendableScriptExecutor.cpp File Source/core/dom/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/660863002/diff/160001/Source/core/dom/SuspendableScriptExecutor.cpp#newcode68 Source/core/dom/SuspendableScriptExecutor.cpp:68: delete this; You are at risk of deleting it ...
6 years, 2 months ago (2014-10-22 15:38:17 UTC) #19
pfeldman
https://codereview.chromium.org/660863002/diff/160001/Source/core/dom/SuspendableScriptExecutor.cpp File Source/core/dom/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/660863002/diff/160001/Source/core/dom/SuspendableScriptExecutor.cpp#newcode68 Source/core/dom/SuspendableScriptExecutor.cpp:68: delete this; Lets comment on this.
6 years, 2 months ago (2014-10-22 15:42:56 UTC) #20
kozyatinskiy1
https://codereview.chromium.org/660863002/diff/160001/Source/core/dom/SuspendableScriptExecutor.cpp File Source/core/dom/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/660863002/diff/160001/Source/core/dom/SuspendableScriptExecutor.cpp#newcode68 Source/core/dom/SuspendableScriptExecutor.cpp:68: delete this; On 2014/10/22 15:42:55, pfeldman wrote: > Lets ...
6 years, 2 months ago (2014-10-22 16:46:58 UTC) #21
pfeldman
lgtm.
6 years, 2 months ago (2014-10-23 14:53:25 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/660863002/200001
6 years, 2 months ago (2014-10-23 14:59:45 UTC) #24
commit-bot: I haz the power
Committed patchset #9 (id:200001) as 184279
6 years, 2 months ago (2014-10-23 16:00:02 UTC) #25
aandrey
6 years, 2 months ago (2014-10-23 18:02:58 UTC) #27
Message was sent while issue was closed. 
https://codereview.chromium.org/660863002/diff/200001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/660863002/diff/200001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:794: SuspendableScriptExecutor* executor = new
SuspendableScriptExecutor(frame(), 0, sources, 0, userGesture, callback);
Should anyone create a OwnPtr<SuspendableScriptExecutor> it'll be
use-after-free, if I got it right Thus, I'd make the constructor private in
favor of a static SuspendableScriptExecutor::createAndRun()

Powered by Google App Engine
This is Rietveld 408576698