binding: Makes ExceptionState STACK_ALLOCATED(). (issue 2272313003 by yukishiino@chromium.org)

2 views
Skip to first unread message

yukis...@chromium.org

unread,
Aug 25, 2016, 9:41:41 AM8/25/16
to har...@chromium.org, ba...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org
Reviewers: haraken, bashi1
CL: https://codereview.chromium.org/2272313003/

Message:
Could you guys review this CL?

Description:
binding: Makes ExceptionState STACK_ALLOCATED().

Since we're going to make ExceptionStates scope-like objects at
http://crrev.com/2272613003 , ExceptionState should be
STACK_ALLOCATED().

BUG=

Affected files (+28, -156 lines):
M third_party/WebKit/Source/bindings/core/v8/ArrayValue.h
M third_party/WebKit/Source/bindings/core/v8/ArrayValue.cpp
M third_party/WebKit/Source/bindings/core/v8/Dictionary.h
M third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp
M third_party/WebKit/Source/bindings/core/v8/ExceptionState.h
D third_party/WebKit/Source/bindings/core/v8/OnStackObjectChecker.h
D third_party/WebKit/Source/bindings/core/v8/OnStackObjectChecker.cpp
M third_party/WebKit/Source/bindings/core/v8/v8.gypi


pe...@chromium.org

unread,
Aug 25, 2016, 11:31:35 AM8/25/16
to yukishii...@chromium.org, har...@chromium.org, ba...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org
My comments are nits, so you don't need my LG.


https://codereview.chromium.org/2272313003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ArrayValue.cpp
File third_party/WebKit/Source/bindings/core/v8/ArrayValue.cpp (right):

https://codereview.chromium.org/2272313003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ArrayValue.cpp#newcode42
third_party/WebKit/Source/bindings/core/v8/ArrayValue.cpp:42: return
blink::isUndefinedOrNull(m_array);
We don't need 'blink::' here.

https://codereview.chromium.org/2272313003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ArrayValue.h
File third_party/WebKit/Source/bindings/core/v8/ArrayValue.h (right):

https://codereview.chromium.org/2272313003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ArrayValue.h#newcode42
third_party/WebKit/Source/bindings/core/v8/ArrayValue.h:42: explicit
ArrayValue(const v8::Local<v8::Array>& array, v8::Isolate* isolate)
This is not related to your change, but we don't need 'explicit' here.

https://codereview.chromium.org/2272313003/

ba...@chromium.org

unread,
Aug 25, 2016, 7:13:54 PM8/25/16
to yukishii...@chromium.org, har...@chromium.org, pe...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org
lgtm once AnimationTest build failure is fixed.

https://codereview.chromium.org/2272313003/

har...@chromium.org

unread,
Aug 25, 2016, 9:08:52 PM8/25/16
to yukishii...@chromium.org, ba...@chromium.org, pe...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org

yukis...@chromium.org

unread,
Aug 30, 2016, 2:09:07 AM8/30/16
to har...@chromium.org, ba...@chromium.org, pe...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org

https://codereview.chromium.org/2272313003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ArrayValue.cpp
File third_party/WebKit/Source/bindings/core/v8/ArrayValue.cpp (right):

https://codereview.chromium.org/2272313003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ArrayValue.cpp#newcode42
third_party/WebKit/Source/bindings/core/v8/ArrayValue.cpp:42: return
blink::isUndefinedOrNull(m_array);
On 2016/08/25 15:31:18, peria wrote:
> We don't need 'blink::' here.

We do need it because ArrayValue::isUndefinedOrNull shadows
blink::isUndefinedOrNull.


https://codereview.chromium.org/2272313003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ArrayValue.h
File third_party/WebKit/Source/bindings/core/v8/ArrayValue.h (right):

https://codereview.chromium.org/2272313003/diff/40001/third_party/WebKit/Source/bindings/core/v8/ArrayValue.h#newcode42
third_party/WebKit/Source/bindings/core/v8/ArrayValue.h:42: explicit
ArrayValue(const v8::Local<v8::Array>& array, v8::Isolate* isolate)
On 2016/08/25 15:31:18, peria wrote:
> This is not related to your change, but we don't need 'explicit' here.

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 30, 2016, 2:09:54 AM8/30/16
to yukishii...@chromium.org, har...@chromium.org, ba...@chromium.org, pe...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 30, 2016, 4:14:31 AM8/30/16
to yukishii...@chromium.org, har...@chromium.org, ba...@chromium.org, pe...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org
Committed patchset #7 (id:120001)

https://codereview.chromium.org/2272313003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 30, 2016, 4:16:47 AM8/30/16
to yukishii...@chromium.org, har...@chromium.org, ba...@chromium.org, pe...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org
Patchset 7 (id:??) landed as
https://crrev.com/816254910aa039c21e728a7ff5d518174f9a2dfc
Cr-Commit-Position: refs/heads/master@{#415241}

https://codereview.chromium.org/2272313003/
Reply all
Reply to author
Forward
0 new messages