Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2476393003:
V8ObjectBuilder::addString support Nullable. (Closed)
DescriptionV8ObjectBuilder::addString support Nullable.
There are some logics which compare its value is null.
if its value is null then call addNull().
These kinds of codes are not so cool.
ex) RTCSessionDescription.cpp:77 and PaymentResponse.cpp:33
To simplify this logic, we could support nullable flag.
Committed: https://crrev.com/fa7233194702581620e76348fb555cbc0c3db582
Cr-Commit-Position: refs/heads/master@{#430926}
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : Fix typo. #Patch Set 4 : Add V8ObjectBuilderTest #
Total comments: 2
Patch Set 5 : Change to addStringOrNull and replace to StringView. #
Total comments: 2
Patch Set 6 : Fix nit. #
Messages
Total messages: 30 (14 generated)
Description was changed from ========== V8ObjectBuilder::addString support Nullable. ========== to ========== V8ObjectBuilder::addString support Nullable. There are some logics which compare its value is null. if its value is null then call addNull(). These kinds of codes are not so cool. ex) RTCSessionDescription.cpp:77 and PaymentResponse.cpp:33 To simplify this logic we should could nullable flag. ==========
corona10@gmail.com changed reviewers: + esprehn@chromium.org
@esprehn PTAL
Description was changed from ========== V8ObjectBuilder::addString support Nullable. There are some logics which compare its value is null. if its value is null then call addNull(). These kinds of codes are not so cool. ex) RTCSessionDescription.cpp:77 and PaymentResponse.cpp:33 To simplify this logic we should could nullable flag. ========== to ========== V8ObjectBuilder::addString support Nullable. There are some logics which compare its value is null. if its value is null then call addNull(). These kinds of codes are not so cool. ex) RTCSessionDescription.cpp:77 and PaymentResponse.cpp:33 To simplify this logic we could nullable flag. ==========
Description was changed from ========== V8ObjectBuilder::addString support Nullable. There are some logics which compare its value is null. if its value is null then call addNull(). These kinds of codes are not so cool. ex) RTCSessionDescription.cpp:77 and PaymentResponse.cpp:33 To simplify this logic we could nullable flag. ========== to ========== V8ObjectBuilder::addString support Nullable. There are some logics which compare its value is null. if its value is null then call addNull(). These kinds of codes are not so cool. ex) RTCSessionDescription.cpp:77 and PaymentResponse.cpp:33 To simplify this logic, we could nullable flag. ==========
Description was changed from ========== V8ObjectBuilder::addString support Nullable. There are some logics which compare its value is null. if its value is null then call addNull(). These kinds of codes are not so cool. ex) RTCSessionDescription.cpp:77 and PaymentResponse.cpp:33 To simplify this logic, we could nullable flag. ========== to ========== V8ObjectBuilder::addString support Nullable. There are some logics which compare its value is null. if its value is null then call addNull(). These kinds of codes are not so cool. ex) RTCSessionDescription.cpp:77 and PaymentResponse.cpp:33 To simplify this logic, we could support nullable flag. ==========
corona10@gmail.com changed reviewers: + tkent@chromium.org
On 2016/11/08 15:08:56, dhna wrote: > @esprehn > PTAL @tkent Can you take it look also?
tkent@chromium.org changed reviewers: + bashi@chromium.org, yukishiino@chromium.org - esprehn@chromium.org, tkent@chromium.org
On 2016/11/09 at 03:48:27, corona10 wrote: > On 2016/11/08 15:08:56, dhna wrote: > > @esprehn > > PTAL > > @tkent > Can you take it look also? +bashi, yukishiino
Looks good, but would you mind adding unittests for V8ObjectBuilder? i.e., add V8ObjectBuilderTest.cpp.
On 2016/11/09 04:09:01, haraken wrote: > Looks good, but would you mind adding unittests for V8ObjectBuilder? i.e., add > V8ObjectBuilderTest.cpp. @haraken Thank you for review it. I will update it tonight. (I'm live in GMT+9.)
On 2016/11/09 04:18:23, dhna wrote: > On 2016/11/09 04:09:01, haraken wrote: > > Looks good, but would you mind adding unittests for V8ObjectBuilder? i.e., add > > V8ObjectBuilderTest.cpp. > > @haraken > > Thank you for review it. > I will update it tonight. (I'm live in GMT+9.) For reviewers, I added unittests for V8ObjectBuilder. PTAL
https://codereview.chromium.org/2476393003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8ObjectBuilder.h (right): https://codereview.chromium.org/2476393003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8ObjectBuilder.h:32: V8ObjectBuilder& addString(const String& name, It's okay to follow the old style, but const StringView& name, const StringView& value, is now recommended. https://codereview.chromium.org/2476393003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8ObjectBuilder.h:34: bool isNullable = false); In general, we should avoid a boolean flag, I think. Given the following code: x.addString("key", value, true); It's not clear what the last argument means. Blink's coding style recommended enum values instead. x.addString("key", value, V8ObjectBuilder::Nullable); In this specific case, my preference is: x.addStringOrNull("key", value); then, it's short and crystal clear.
On 2016/11/09 07:25:50, Yuki wrote: > https://codereview.chromium.org/2476393003/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/V8ObjectBuilder.h (right): > > https://codereview.chromium.org/2476393003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8ObjectBuilder.h:32: > V8ObjectBuilder& addString(const String& name, > It's okay to follow the old style, but > const StringView& name, > const StringView& value, > is now recommended. > > https://codereview.chromium.org/2476393003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8ObjectBuilder.h:34: bool isNullable > = false); > In general, we should avoid a boolean flag, I think. Given the following code: > x.addString("key", value, true); > It's not clear what the last argument means. > > Blink's coding style recommended enum values instead. > x.addString("key", value, V8ObjectBuilder::Nullable); > > In this specific case, my preference is: > x.addStringOrNull("key", value); > then, it's short and crystal clear. @Yuki Thank you for review my codes. I fixed it as your comment :-) PTAL.
LGTM with nits. Thanks for working on this. https://codereview.chromium.org/2476393003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8ObjectBuilderTest.cpp (right): https://codereview.chromium.org/2476393003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8ObjectBuilderTest.cpp:15: TEST(V8ObjectBuilderTest, addString) { nit: I'd suggest to sort the test cases in the same order as *.h i.e. addNull, addBoolean, addNumber, ... https://codereview.chromium.org/2476393003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8ObjectBuilderTest.cpp:17: { nit: You don't need to create a new block. Without this block, V8TestingScope works fine.
On 2016/11/09 12:17:18, Yuki wrote: > LGTM with nits. > > Thanks for working on this. > > https://codereview.chromium.org/2476393003/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/V8ObjectBuilderTest.cpp (right): > > https://codereview.chromium.org/2476393003/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8ObjectBuilderTest.cpp:15: > TEST(V8ObjectBuilderTest, addString) { > nit: I'd suggest to sort the test cases in the same order as *.h > i.e. addNull, addBoolean, addNumber, ... > > https://codereview.chromium.org/2476393003/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8ObjectBuilderTest.cpp:17: { > nit: You don't need to create a new block. > Without this block, V8TestingScope works fine. @Yuki Done.
On 2016/11/09 12:28:09, dhna wrote: > On 2016/11/09 12:17:18, Yuki wrote: > > LGTM with nits. > > > > Thanks for working on this. > > > > > https://codereview.chromium.org/2476393003/diff/80001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/bindings/core/v8/V8ObjectBuilderTest.cpp > (right): > > > > > https://codereview.chromium.org/2476393003/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/V8ObjectBuilderTest.cpp:15: > > TEST(V8ObjectBuilderTest, addString) { > > nit: I'd suggest to sort the test cases in the same order as *.h > > i.e. addNull, addBoolean, addNumber, ... > > > > > https://codereview.chromium.org/2476393003/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/V8ObjectBuilderTest.cpp:17: { > > nit: You don't need to create a new block. > > Without this block, V8TestingScope works fine. > > @Yuki > Done. And thanks to review my CL.
The CQ bit was checked by corona10@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by corona10@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== V8ObjectBuilder::addString support Nullable. There are some logics which compare its value is null. if its value is null then call addNull(). These kinds of codes are not so cool. ex) RTCSessionDescription.cpp:77 and PaymentResponse.cpp:33 To simplify this logic, we could support nullable flag. ========== to ========== V8ObjectBuilder::addString support Nullable. There are some logics which compare its value is null. if its value is null then call addNull(). These kinds of codes are not so cool. ex) RTCSessionDescription.cpp:77 and PaymentResponse.cpp:33 To simplify this logic, we could support nullable flag. ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== V8ObjectBuilder::addString support Nullable. There are some logics which compare its value is null. if its value is null then call addNull(). These kinds of codes are not so cool. ex) RTCSessionDescription.cpp:77 and PaymentResponse.cpp:33 To simplify this logic, we could support nullable flag. ========== to ========== V8ObjectBuilder::addString support Nullable. There are some logics which compare its value is null. if its value is null then call addNull(). These kinds of codes are not so cool. ex) RTCSessionDescription.cpp:77 and PaymentResponse.cpp:33 To simplify this logic, we could support nullable flag. Committed: https://crrev.com/fa7233194702581620e76348fb555cbc0c3db582 Cr-Commit-Position: refs/heads/master@{#430926} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/fa7233194702581620e76348fb555cbc0c3db582 Cr-Commit-Position: refs/heads/master@{#430926} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
