Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2709703004:
Clean up V8ObjectConstructor (Closed)
DescriptionClean up V8ObjectConstructor
- Remove unused methods (and references to Document)
- Move V8ScriptRunner::instantiateObject into V8ObjectConstructor
BUG=682322
Review-Url: https://codereview.chromium.org/2709703004
Cr-Commit-Position: refs/heads/master@{#452522}
Committed: https://chromium.googlesource.com/chromium/src/+/37298765ba284eadee3de618fff59582969eacba
Patch Set 1 #
Total comments: 5
Patch Set 2 : Inline instantiateObject #
Total comments: 3
Patch Set 3 : Rebase #
Messages
Total messages: 25 (13 generated)
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Description was changed from ========== Clean up V8ObjectConstructor - Remove unused methods - Move V8ScriptRunner::instantiateObject into V8ObjectConstructor BUG=682322 ========== to ========== Clean up V8ObjectConstructor - Remove unused methods (and references to Document) - Move V8ScriptRunner::instantiateObject into V8ObjectConstructor BUG=682322 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
adithyas@chromium.org changed reviewers: + jbroman@chromium.org
https://codereview.chromium.org/2709703004/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8ObjectConstructor.cpp (right): https://codereview.chromium.org/2709703004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8ObjectConstructor.cpp:60: v8::MaybeLocal<v8::Object> V8ObjectConstructor::instantiateObject( Why not go further and just inline this into newInstance (since it apparently has no other call sites)? Heck, you can even default the last two arguments to that (as you are to this function). https://codereview.chromium.org/2709703004/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8ObjectConstructor.h (right): https://codereview.chromium.org/2709703004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8ObjectConstructor.h:82: v8::Local<v8::Value> argv[] = 0); nit: nullptr, since this is a pointer you're defaulting https://codereview.chromium.org/2709703004/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.h (right): https://codereview.chromium.org/2709703004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.h:119: static v8::MaybeLocal<v8::Object> instantiateObject( You have deleted the definition of this function, so can you also delete the declaration?
https://codereview.chromium.org/2709703004/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8ObjectConstructor.cpp (right): https://codereview.chromium.org/2709703004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8ObjectConstructor.cpp:60: v8::MaybeLocal<v8::Object> V8ObjectConstructor::instantiateObject( On 2017/02/21 at 22:10:28, jbroman wrote: > Why not go further and just inline this into newInstance (since it apparently has no other call sites)? Heck, you can even default the last two arguments to that (as you are to this function). Okay, done. https://codereview.chromium.org/2709703004/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.h (right): https://codereview.chromium.org/2709703004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.h:119: static v8::MaybeLocal<v8::Object> instantiateObject( On 2017/02/21 at 22:10:28, jbroman wrote: > You have deleted the definition of this function, so can you also delete the declaration? Oops, forgot to do that.
haraken@chromium.org changed reviewers: + haraken@chromium.org
LGTM https://codereview.chromium.org/2709703004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8ObjectConstructor.cpp (right): https://codereview.chromium.org/2709703004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8ObjectConstructor.cpp:43: function->NewInstance(isolate->GetCurrentContext(), argc, argv); Would you keep this function in V8ScriptRunner? I'd prefer centralizing all functions that may trigger a script execution (and thus require the microtask scope) in V8ScriptRunner. Nit: I'm not sure how NewInstance can trigger a script execution though. Then why do we need the microtask scope?
https://codereview.chromium.org/2709703004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8ObjectConstructor.cpp (right): https://codereview.chromium.org/2709703004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8ObjectConstructor.cpp:43: function->NewInstance(isolate->GetCurrentContext(), argc, argv); On 2017/02/21 at 23:16:15, haraken wrote: > Would you keep this function in V8ScriptRunner? I'd prefer centralizing all functions that may trigger a script execution (and thus require the microtask scope) in V8ScriptRunner. > > Nit: I'm not sure how NewInstance can trigger a script execution though. Then why do we need the microtask scope? v8::Function::NewInstance invokes the function (which is why it takes argc/argv), which at some call sites can be author-provided.
https://codereview.chromium.org/2709703004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8ObjectConstructor.cpp (right): https://codereview.chromium.org/2709703004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8ObjectConstructor.cpp:43: function->NewInstance(isolate->GetCurrentContext(), argc, argv); On 2017/02/21 at 23:16:15, haraken wrote: > Would you keep this function in V8ScriptRunner? I'd prefer centralizing all functions that may trigger a script execution (and thus require the microtask scope) in V8ScriptRunner. > V8ScriptRunner has a lot of functions that I cannot move to platform/. Should I just create a separate file for script execution functions that will be used in platform/? I'm also not sure how many other functions in V8ScriptRunner will be used in platform (this may be the only one, and it is only used once).
lgtm I don't have strong feelings about whether this should ultimately live in V8ScriptRunner. At the moment there are clearly no call sites outside Source/bindings/ to V8ScriptRunner::instantiateObject (else this CL would not compile), so I think that question is separable from the stated purpose of this CL.
On 2017/02/22 23:17:01, jbroman wrote: > lgtm > > I don't have strong feelings about whether this should ultimately live in > V8ScriptRunner. At the moment there are clearly no call sites outside > Source/bindings/ to V8ScriptRunner::instantiateObject (else this CL would not > compile), so I think that question is separable from the stated purpose of this > CL. Makes sense. LGTM.
The CQ bit was checked by adithyas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by adithyas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, jbroman@chromium.org Link to the patchset: https://codereview.chromium.org/2709703004/#ps40001 (title: "Rebase")
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": 1487863218122040,
"parent_rev": "da0829e4642d9eb31f9454be3e6168cffdb4d257", "commit_rev":
"37298765ba284eadee3de618fff59582969eacba"}
Message was sent while issue was closed.
Description was changed from ========== Clean up V8ObjectConstructor - Remove unused methods (and references to Document) - Move V8ScriptRunner::instantiateObject into V8ObjectConstructor BUG=682322 ========== to ========== Clean up V8ObjectConstructor - Remove unused methods (and references to Document) - Move V8ScriptRunner::instantiateObject into V8ObjectConstructor BUG=682322 Review-Url: https://codereview.chromium.org/2709703004 Cr-Commit-Position: refs/heads/master@{#452522} Committed: https://chromium.googlesource.com/chromium/src/+/37298765ba284eadee3de618fff5... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/37298765ba284eadee3de618fff5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
