Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2464793004:
Mojo C++ bindings: some improvements for String16 struct traits. (Closed)
|
Created:
4 years, 1 month ago by yzshen1 Modified:
4 years, 1 month ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, blink-reviews, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMojo C++ bindings: some improvements for String16 struct traits.
- map nullable String16 to WTF::String (instead of WTF::Optional<WTF::String>);
- avoid unnecessary copies and null checks;
- add one more test case.
BUG=653209
Committed: https://crrev.com/510a9fab8b58db770bc5ce698612dec9b7163df8
Cr-Commit-Position: refs/heads/master@{#429397}
Patch Set 1 #Patch Set 2 : . #
Total comments: 4
Patch Set 3 : . #Patch Set 4 : simplify context object and future reduce heap allocation #Patch Set 5 : . #
Total comments: 6
Patch Set 6 : . #Patch Set 7 : . #
Total comments: 8
Patch Set 8 : . #Patch Set 9 : . #
Messages
Total messages: 41 (25 generated)
The CQ bit was checked by yzshen@chromium.org 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...
The CQ bit was checked by yzshen@chromium.org 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...
yzshen@chromium.org changed reviewers: + esprehn@chromium.org, tsepez@chromium.org, zqzhang@chromium.org
Hi, Would you please take a look? Thanks! Elliot: everything Tom: security Zhiqiang: FYI
https://codereview.chromium.org/2464793004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp (right): https://codereview.chromium.org/2464793004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:16: // |str| must outlive this object. This worries me. It's not just a question of str outliving it, but the underlying StringImpl, no? This could get invalidated by operations on the string. Are we sure this can't happen?
Thanks Elliot! https://codereview.chromium.org/2464793004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp (right): https://codereview.chromium.org/2464793004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:16: // |str| must outlive this object. On 2016/10/31 22:43:42, Tom Sepez wrote: > This worries me. It's not just a question of str outliving it, but the > underlying StringImpl, no? This could get invalidated by operations on the > string. Are we sure this can't happen? It shouldn't be a problem. This ConstCArrayHolder is created, used and destructed during the serialization of the string, which is done before returning control to the user code.. For example, WTF::String someStr; someMojoInterfacePtr->DoSomthing(someStr); // DoSomething takes a const-ref of WTF::String. ConstCArrayHolder is created/used/destructed before DoSomething() returns. (Maybe this scary comment is not useful at all. Shall I remove the comment?)
On 2016/10/31 22:56:34, yzshen1 (OOO Sep25-Oct29) wrote: > Thanks Elliot! (Oops, I meant: Thanks Tom!) > > https://codereview.chromium.org/2464793004/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp > (right): > > https://codereview.chromium.org/2464793004/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:16: // > |str| must outlive this object. > On 2016/10/31 22:43:42, Tom Sepez wrote: > > This worries me. It's not just a question of str outliving it, but the > > underlying StringImpl, no? This could get invalidated by operations on the > > string. Are we sure this can't happen? > > It shouldn't be a problem. This ConstCArrayHolder is created, used and > destructed during the serialization of the string, which is done before > returning control to the user code.. For example, > > WTF::String someStr; > someMojoInterfacePtr->DoSomthing(someStr); // DoSomething takes a const-ref of > WTF::String. > > ConstCArrayHolder is created/used/destructed before DoSomething() returns. > > (Maybe this scary comment is not useful at all. Shall I remove the comment?)
https://codereview.chromium.org/2464793004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp (right): https://codereview.chromium.org/2464793004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:16: // |str| must outlive this object. > (Maybe this scary comment is not useful at all. Shall I remove the comment?) I'd vote for removal. Alternatively, you could put a WTF::String into this structure and assign it to it, which just does a pointer assignment plus a ref-count increment, and would keep the impl alive no matter what else happened.
The CQ bit was checked by yzshen@chromium.org 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...
https://codereview.chromium.org/2464793004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp (right): https://codereview.chromium.org/2464793004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:16: // |str| must outlive this object. On 2016/10/31 23:15:55, Tom Sepez wrote: > > (Maybe this scary comment is not useful at all. Shall I remove the comment?) > I'd vote for removal. Alternatively, you could put a WTF::String into this > structure and assign it to it, which just does a pointer assignment plus a > ref-count increment, and would keep the impl alive no matter what else happened. Removed comment. I personally prefer not to hold a ref to WTF::String. Although the cost is probably small, I would like to avoid it since performance is importance for such a basic type. Besides, I am pretty confident that |str| stays valid during the lifetime of ConstCArrayHolder. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sgtm. Thanks for improving the null-string case.
The CQ bit was checked by yzshen@chromium.org 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...
The CQ bit was checked by yzshen@chromium.org 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...
Description was changed from ========== Mojo C++ bindings: some improvements for String16 struct traits. - map nullable String16 to WTF::String (instead of WTF::Optional<WTF::String>); - avoid unnecessary copies and null checks; - add one more test case. BUG=653209 ========== to ========== Mojo C++ bindings: some improvements for String16 struct traits. - map nullable String16 to WTF::String (instead of WTF::Optional<WTF::String>); - avoid unnecessary copies and null checks; - add one more test case. BUG=653209 ==========
yzshen@chromium.org changed reviewers: + haraken@chromium.org
Also +Kentaro for OWNER review since Elliot is OOO currently. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Still got red bots, but I think this works. https://codereview.chromium.org/2464793004/diff/80001/mojo/common/common_cust... File mojo/common/common_custom_types_struct_traits.h (right): https://codereview.chromium.org/2464793004/diff/80001/mojo/common/common_cust... mojo/common/common_custom_types_struct_traits.h:28: return !version.IsValid(); nit: funny indent (and below). https://codereview.chromium.org/2464793004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp (right): https://codereview.chromium.org/2464793004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:28: base::string16* context = nullptr; nit: maybe early return is clearer, eg. if (!input.is8Bit()) return nullptr; return new base::... https://codereview.chromium.org/2464793004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.h (right): https://codereview.chromium.org/2464793004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.h:20: static void* SetUpContext(const WTF::String& input); nit: can we avoid the cast to void*? It's always a string16* ?
The CQ bit was checked by yzshen@chromium.org to run a CQ dry run
Thanks Tom! https://codereview.chromium.org/2464793004/diff/80001/mojo/common/common_cust... File mojo/common/common_custom_types_struct_traits.h (right): https://codereview.chromium.org/2464793004/diff/80001/mojo/common/common_cust... mojo/common/common_custom_types_struct_traits.h:28: return !version.IsValid(); On 2016/11/01 17:22:12, Tom Sepez wrote: > nit: funny indent (and below). Done. https://codereview.chromium.org/2464793004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp (right): https://codereview.chromium.org/2464793004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:28: base::string16* context = nullptr; On 2016/11/01 17:22:12, Tom Sepez wrote: > nit: maybe early return is clearer, eg. > if (!input.is8Bit()) > return nullptr; > > return new base::... Good idea. Thanks! https://codereview.chromium.org/2464793004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.h (right): https://codereview.chromium.org/2464793004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.h:20: static void* SetUpContext(const WTF::String& input); On 2016/11/01 17:22:12, Tom Sepez wrote: > nit: can we avoid the cast to void*? It's always a string16* ? It is the current requirement of the interface. SetUpContext returns a void* as context. Different StructTraits specializations have different context. However, there is a feature request to make this more strongly typed (or at least more explicitly typed). That is not done yet.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> However, there is a feature request to make this more strongly typed (or at > least more explicitly typed). That is not done yet. Ok. I'll give you a LGTM assuming the bots are happy.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with comments. https://codereview.chromium.org/2464793004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp (right): https://codereview.chromium.org/2464793004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:18: output->swap(result); Maybe we can introduce String::setToNull(). https://codereview.chromium.org/2464793004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:39: auto context_obj = static_cast<base::string16*>(context); contextObject https://codereview.chromium.org/2464793004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:41: return ConstCArray<uint16_t>( Can you add DCHECK(input.is8Bit())? https://codereview.chromium.org/2464793004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:46: return ConstCArray<uint16_t>( Can you add DCHECK(!input.is8Bit())?
Thanks, Kentaro! https://codereview.chromium.org/2464793004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp (right): https://codereview.chromium.org/2464793004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:18: output->swap(result); On 2016/11/02 01:58:46, haraken wrote: > > Maybe we can introduce String::setToNull(). I realized that what I did is basically the same as "*output = String();" So I changed to that. https://codereview.chromium.org/2464793004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:39: auto context_obj = static_cast<base::string16*>(context); On 2016/11/02 01:58:46, haraken wrote: > > contextObject Done. https://codereview.chromium.org/2464793004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:41: return ConstCArray<uint16_t>( On 2016/11/02 01:58:46, haraken wrote: > > Can you add DCHECK(input.is8Bit())? Done. https://codereview.chromium.org/2464793004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:46: return ConstCArray<uint16_t>( On 2016/11/02 01:58:46, haraken wrote: > > Can you add DCHECK(!input.is8Bit())? Done.
The CQ bit was checked by yzshen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2464793004/#ps160001 (title: ".")
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 ========== Mojo C++ bindings: some improvements for String16 struct traits. - map nullable String16 to WTF::String (instead of WTF::Optional<WTF::String>); - avoid unnecessary copies and null checks; - add one more test case. BUG=653209 ========== to ========== Mojo C++ bindings: some improvements for String16 struct traits. - map nullable String16 to WTF::String (instead of WTF::Optional<WTF::String>); - avoid unnecessary copies and null checks; - add one more test case. BUG=653209 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Mojo C++ bindings: some improvements for String16 struct traits. - map nullable String16 to WTF::String (instead of WTF::Optional<WTF::String>); - avoid unnecessary copies and null checks; - add one more test case. BUG=653209 ========== to ========== Mojo C++ bindings: some improvements for String16 struct traits. - map nullable String16 to WTF::String (instead of WTF::Optional<WTF::String>); - avoid unnecessary copies and null checks; - add one more test case. BUG=653209 Committed: https://crrev.com/510a9fab8b58db770bc5ce698612dec9b7163df8 Cr-Commit-Position: refs/heads/master@{#429397} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/510a9fab8b58db770bc5ce698612dec9b7163df8 Cr-Commit-Position: refs/heads/master@{#429397} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
