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

Issue 1035623002: bindings: Use Maybe APIs in ScriptPromise (Closed)

Created:
5 years, 9 months ago by bashi
Modified:
5 years, 9 months ago
Reviewers:
haraken, yhirano
CC:
blink-reviews, blink-reviews-bindings_chromium.org, vivekg_samsung, arv+blink, vivekg
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

bindings: Use Maybe APIs in ScriptPromise Replacing following APIs. - Promise::Resolver::New - Promise::Resolver::Resolve - Promise::Resolver::Reject - Promise::Catch - Promise::Then BUG=462402 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192509

Patch Set 1 #

Total comments: 12

Patch Set 2 : #

Total comments: 7

Patch Set 3 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+42 lines, -25 lines) Patch
M Source/bindings/core/v8/ScriptPromise.h View 1 1 chunk +1 line, -1 line 0 comments Download
M Source/bindings/core/v8/ScriptPromise.cpp View 1 5 chunks +13 lines, -13 lines 0 comments Download
M Source/bindings/core/v8/ScriptPromisePropertyBase.cpp View 1 2 2 chunks +10 lines, -7 lines 0 comments Download
M Source/bindings/core/v8/ScriptValue.h View 1 2 chunks +14 lines, -0 lines 0 comments Download
M Source/bindings/templates/methods.cpp View 1 2 chunks +2 lines, -2 lines 0 comments Download
M Source/bindings/tests/results/core/V8TestInterface.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M Source/bindings/tests/results/modules/V8TestInterfacePartial.cpp View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 15 (3 generated)
bashi
Hirano-san, PTAL? Here is an announcement of V8 API change. https://groups.google.com/d/msg/v8-users/gQVpp1HmbqM/W_ZIMIZP4SgJ We are going to ...
5 years, 9 months ago (2015-03-25 03:52:29 UTC) #2
haraken
https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/ScriptPromise.cpp File Source/bindings/core/v8/ScriptPromise.cpp (right): https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/ScriptPromise.cpp#newcode170 Source/bindings/core/v8/ScriptPromise.cpp:170: if (!v8::Promise::Resolver::New(isolate->GetCurrentContext()).ToLocal(&resolver)) It seems better to pass ScriptState* to ...
5 years, 9 months ago (2015-03-25 04:29:59 UTC) #3
bashi
https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/ScriptPromise.cpp File Source/bindings/core/v8/ScriptPromise.cpp (right): https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/ScriptPromise.cpp#newcode170 Source/bindings/core/v8/ScriptPromise.cpp:170: if (!v8::Promise::Resolver::New(isolate->GetCurrentContext()).ToLocal(&resolver)) On 2015/03/25 04:29:59, haraken wrote: > > ...
5 years, 9 months ago (2015-03-25 04:56:00 UTC) #4
haraken
LGTM https://codereview.chromium.org/1035623002/diff/20001/Source/bindings/core/v8/ScriptPromisePropertyBase.h File Source/bindings/core/v8/ScriptPromisePropertyBase.h (right): https://codereview.chromium.org/1035623002/diff/20001/Source/bindings/core/v8/ScriptPromisePropertyBase.h#newcode74 Source/bindings/core/v8/ScriptPromisePropertyBase.h:74: // v8::Isolate*. Yeah, let's fix this in a ...
5 years, 9 months ago (2015-03-25 05:08:29 UTC) #5
bashi
https://codereview.chromium.org/1035623002/diff/20001/Source/bindings/core/v8/ScriptPromisePropertyBase.h File Source/bindings/core/v8/ScriptPromisePropertyBase.h (right): https://codereview.chromium.org/1035623002/diff/20001/Source/bindings/core/v8/ScriptPromisePropertyBase.h#newcode74 Source/bindings/core/v8/ScriptPromisePropertyBase.h:74: // v8::Isolate*. On 2015/03/25 05:08:29, haraken wrote: > > ...
5 years, 9 months ago (2015-03-25 07:25:32 UTC) #6
bashi
https://codereview.chromium.org/1035623002/diff/20001/Source/bindings/core/v8/ScriptValue.h File Source/bindings/core/v8/ScriptValue.h (right): https://codereview.chromium.org/1035623002/diff/20001/Source/bindings/core/v8/ScriptValue.h#newcode67 Source/bindings/core/v8/ScriptValue.h:67: ASSERT(isEmpty() || m_scriptState); On 2015/03/25 07:25:32, bashi1 wrote: > ...
5 years, 9 months ago (2015-03-25 07:35:01 UTC) #7
yhirano
https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp File Source/bindings/core/v8/ScriptPromisePropertyBase.cpp (right): https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp#newcode112 Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:112: v8::Local<v8::Context> context = m_isolate->GetCurrentContext(); On 2015/03/25 04:56:00, bashi1 wrote: ...
5 years, 9 months ago (2015-03-25 07:39:08 UTC) #8
bashi
https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp File Source/bindings/core/v8/ScriptPromisePropertyBase.cpp (right): https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp#newcode112 Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:112: v8::Local<v8::Context> context = m_isolate->GetCurrentContext(); On 2015/03/25 07:39:08, yhirano wrote: ...
5 years, 9 months ago (2015-03-25 08:01:46 UTC) #9
yhirano
lgtm
5 years, 9 months ago (2015-03-25 08:09:13 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1035623002/40001
5 years, 9 months ago (2015-03-25 08:09:52 UTC) #13
haraken
https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp File Source/bindings/core/v8/ScriptPromisePropertyBase.cpp (right): https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp#newcode112 Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:112: v8::Local<v8::Context> context = m_isolate->GetCurrentContext(); On 2015/03/25 07:39:08, yhirano wrote: ...
5 years, 9 months ago (2015-03-25 09:04:23 UTC) #14
commit-bot: I haz the power
5 years, 9 months ago (2015-03-25 09:19:42 UTC) #15
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192509

Powered by Google App Engine
This is Rietveld 408576698