Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1999623002:
[Autofill] Add typemap for autofill:FormFieldData and autofill::FormData. (Closed)
|
Created:
4 years, 7 months ago by leonhsl(Using Gerrit) Modified:
4 years, 6 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, bondd+autofillwatch_chromium.org, browser-components-watch_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, estade+watch_chromium.org, jam, jdonnelly+autofillwatch_chromium.org, qsr+mojo_chromium.org, rouslan+autofill_chromium.org, vabr+watchlistautofill_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[autofill] Add typemap for autofill:FormFieldData and autofill::FormData.
This will facilitate the work to mojofy those IPC messages carrying these
two types of struct, which I will do with follow up CLs.
TEST=components_unittests --gtest_filter=AutofillTypeTraitsTestImpl*
BUG=582391
Committed: https://crrev.com/77adee7146a554a4bcf4046988f9183d7707b54f
Cr-Commit-Position: refs/heads/master@{#399880}
Patch Set 1 #Patch Set 2 : Fix gyp #Patch Set 3 : Fix gn check #
Total comments: 3
Patch Set 4 : Fix gyp #
Total comments: 11
Patch Set 5 : Address comments from Vaclav, Yuzhu and Daniel #Patch Set 6 : Rebase only #
Total comments: 4
Patch Set 7 : Lambda expressions(default capture --> explicit capture) #Patch Set 8 : Rebase and add TODO comments #Patch Set 9 : Rebase only #Patch Set 10 : Leverage EnumTraits #
Total comments: 10
Patch Set 11 : Address comments from dcheng #Messages
Total messages: 84 (28 generated)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999623002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1999623002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999623002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1999623002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
leon.han@intel.com changed reviewers: + mathp@chromium.org, vabr@chromium.org, yzshen@chromium.org
Hi, Would you PTAL at ps#3? yzshen@: For review as mojo expert. And, I leave one inline question there, would you please help to give some hints? Thanks~ vabr@, mathp@: For review as autofill OWNERs. I'd like to land this CL firstly, this would be a basis to migrate those IPCs carrying FormData and FormFieldData. I want to move forward step by step, to ensure every step is correct. Thanks~ https://codereview.chromium.org/1999623002/diff/40001/components/autofill/con... File components/autofill/content/public/interfaces/autofill_types.mojom (right): https://codereview.chromium.org/1999623002/diff/40001/components/autofill/con... components/autofill/content/public/interfaces/autofill_types.mojom:16: enum TextDirection { Where should we put this enum? I think it should be put at a common place instead of here but have no idea where should be.. WDYT?
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999623002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1999623002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, thank you! Vaclav https://codereview.chromium.org/1999623002/diff/60001/components/autofill/con... File components/autofill/content/public/cpp/autofill_types_struct_traits_unittest.cc (right): https://codereview.chromium.org/1999623002/diff/60001/components/autofill/con... components/autofill/content/public/cpp/autofill_types_struct_traits_unittest.cc:45: std::vector<const char*> kOptions = {"Option1", "Option2", "Option3", const std::vector...
yzshen@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng for StructTraits security review. https://codereview.chromium.org/1999623002/diff/40001/components/autofill/con... File components/autofill/content/public/interfaces/autofill_types.mojom (right): https://codereview.chromium.org/1999623002/diff/40001/components/autofill/con... components/autofill/content/public/interfaces/autofill_types.mojom:16: enum TextDirection { On 2016/05/20 07:05:46, leonhsl wrote: > Where should we put this enum? I think it should be put at a common place > instead of here but have no idea where should be.. WDYT? Before there are more interfaces referring to this enum, I am fine that it stays here. One possible place is mojo/common. There were some concerns about having mojo definitions in the base/ folder directly. https://codereview.chromium.org/1999623002/diff/60001/components/autofill.gypi File components/autofill.gypi (right): https://codereview.chromium.org/1999623002/diff/60001/components/autofill.gyp... components/autofill.gypi:316: 'target_name': 'autofill_content_types_mojo_bindings_mojom', Out of curiosity: Why are the types placed in a target separate from autofill_content_mojo_bindings_mojom? https://codereview.chromium.org/1999623002/diff/60001/components/autofill/con... File components/autofill/content/public/interfaces/autofill_types.mojom (right): https://codereview.chromium.org/1999623002/diff/60001/components/autofill/con... components/autofill/content/public/interfaces/autofill_types.mojom:24: string label = ""; Now that you have mapped this to a custom type, do you still care about setting the default value to ""? Because that only affects the constructor of auto-generated struct type, which you probably won't use.
https://codereview.chromium.org/1999623002/diff/60001/components/autofill/con... File components/autofill/content/public/cpp/autofill_types_struct_traits.cc (right): https://codereview.chromium.org/1999623002/diff/60001/components/autofill/con... components/autofill/content/public/cpp/autofill_types_struct_traits.cc:38: out->role = static_cast<autofill::FormFieldData::RoleAttribute>(data.role()); Is this being used already? It looks like it's just introduced. Can we make this just do enum validation from the start? =) https://codereview.chromium.org/1999623002/diff/60001/components/autofill/con... File components/autofill/content/public/interfaces/autofill_types.mojom (right): https://codereview.chromium.org/1999623002/diff/60001/components/autofill/con... components/autofill/content/public/interfaces/autofill_types.mojom:34: bool is_checkable; Is it possible for is_checked to be true but is_checkable to be false?
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999623002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1999623002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999623002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1999623002/100001
Thanks all for kindly review~ Uploaded ps#5 to address comments and ps#6 is only for rebase. Would you PTAL again? Thanks~ hi, Vaclav, would you please also help to confirm the inline question about is_checked & is_checkable? Thanks~ https://codereview.chromium.org/1999623002/diff/40001/components/autofill/con... File components/autofill/content/public/interfaces/autofill_types.mojom (right): https://codereview.chromium.org/1999623002/diff/40001/components/autofill/con... components/autofill/content/public/interfaces/autofill_types.mojom:16: enum TextDirection { On 2016/05/23 16:59:13, yzshen1 wrote: > On 2016/05/20 07:05:46, leonhsl wrote: > > Where should we put this enum? I think it should be put at a common place > > instead of here but have no idea where should be.. WDYT? > > Before there are more interfaces referring to this enum, I am fine that it stays > here. > > One possible place is mojo/common. There were some concerns about having mojo > definitions in the base/ folder directly. Understood. Then I'd like to keep it here now. Thanks~ https://codereview.chromium.org/1999623002/diff/60001/components/autofill.gypi File components/autofill.gypi (right): https://codereview.chromium.org/1999623002/diff/60001/components/autofill.gyp... components/autofill.gypi:316: 'target_name': 'autofill_content_types_mojo_bindings_mojom', On 2016/05/23 16:59:13, yzshen1 wrote: > Out of curiosity: Why are the types placed in a target separate from > autofill_content_mojo_bindings_mojom? Because in future these autofill types would also be referenced by mojom files from //components/password_manager/content/public/interfaces which I'm planning to create in follow up CLs, so I think we'd better put autofill types in a separate target to avoid password_manager interfaces depending on autofill interfaces ;-) https://codereview.chromium.org/1999623002/diff/60001/components/autofill/con... File components/autofill/content/public/cpp/autofill_types_struct_traits.cc (right): https://codereview.chromium.org/1999623002/diff/60001/components/autofill/con... components/autofill/content/public/cpp/autofill_types_struct_traits.cc:38: out->role = static_cast<autofill::FormFieldData::RoleAttribute>(data.role()); On 2016/05/23 20:43:44, dcheng wrote: > Is this being used already? It looks like it's just introduced. Can we make this > just do enum validation from the start? =) Yeah it's just introduced. Addressed all TODOs of this CL. https://codereview.chromium.org/1999623002/diff/60001/components/autofill/con... File components/autofill/content/public/cpp/autofill_types_struct_traits_unittest.cc (right): https://codereview.chromium.org/1999623002/diff/60001/components/autofill/con... components/autofill/content/public/cpp/autofill_types_struct_traits_unittest.cc:45: std::vector<const char*> kOptions = {"Option1", "Option2", "Option3", On 2016/05/23 12:47:37, vabr (Chromium) wrote: > const std::vector... Done. Thanks~ https://codereview.chromium.org/1999623002/diff/60001/components/autofill/con... File components/autofill/content/public/interfaces/autofill_types.mojom (right): https://codereview.chromium.org/1999623002/diff/60001/components/autofill/con... components/autofill/content/public/interfaces/autofill_types.mojom:24: string label = ""; On 2016/05/23 16:59:13, yzshen1 wrote: > Now that you have mapped this to a custom type, do you still care about setting > the default value to ""? > Because that only affects the constructor of auto-generated struct type, which > you probably won't use. Yeah it's right. Done. Thanks~ https://codereview.chromium.org/1999623002/diff/60001/components/autofill/con... components/autofill/content/public/interfaces/autofill_types.mojom:34: bool is_checkable; On 2016/05/23 20:43:44, dcheng wrote: > Is it possible for is_checked to be true but is_checkable to be false? Checked around is_checked & is_checkable and I suppose if is_checkable was false then is_checked should always be false based on current code logic, although there's no any test code checking this now. But as I have no very deep understanding on the codes, maybe Vaclav can help to confirm this as autofill OWNER. Thanks ;-)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Still LGTM. https://codereview.chromium.org/1999623002/diff/60001/components/autofill/con... File components/autofill/content/public/interfaces/autofill_types.mojom (right): https://codereview.chromium.org/1999623002/diff/60001/components/autofill/con... components/autofill/content/public/interfaces/autofill_types.mojom:34: bool is_checkable; On 2016/05/24 06:38:20, leonhsl wrote: > On 2016/05/23 20:43:44, dcheng wrote: > > Is it possible for is_checked to be true but is_checkable to be false? > > Checked around is_checked & is_checkable and I suppose if is_checkable was false > then is_checked should always be false based on current code logic, although > there's no any test code checking this now. But as I have no very deep > understanding on the codes, maybe Vaclav can help to confirm this as autofill > OWNER. Thanks ;-) Agreed, if is_checkable is false, then is_checked will be false. I filed http://crbug.com/614280 to make this explicit in the code. (And we even have an intern starting next week who could do that as their first CL.)
On 2016/05/24 at 07:40:30, vabr wrote: > Still LGTM. > > https://codereview.chromium.org/1999623002/diff/60001/components/autofill/con... > File components/autofill/content/public/interfaces/autofill_types.mojom (right): > > https://codereview.chromium.org/1999623002/diff/60001/components/autofill/con... > components/autofill/content/public/interfaces/autofill_types.mojom:34: bool is_checkable; > On 2016/05/24 06:38:20, leonhsl wrote: > > On 2016/05/23 20:43:44, dcheng wrote: > > > Is it possible for is_checked to be true but is_checkable to be false? > > > > Checked around is_checked & is_checkable and I suppose if is_checkable was false > > then is_checked should always be false based on current code logic, although > > there's no any test code checking this now. But as I have no very deep > > understanding on the codes, maybe Vaclav can help to confirm this as autofill > > OWNER. Thanks ;-) > > Agreed, if is_checkable is false, then is_checked will be false. I filed http://crbug.com/614280 to make this explicit in the code. (And we even have an intern starting next week who could do that as their first CL.) Thanks for this work! lgtm
lgtm
https://codereview.chromium.org/1999623002/diff/100001/components/autofill/co... File components/autofill/content/public/cpp/autofill_types_struct_traits.cc (right): https://codereview.chromium.org/1999623002/diff/100001/components/autofill/co... components/autofill/content/public/cpp/autofill_types_struct_traits.cc:18: case FormFieldData::RoleAttribute::ROLE_ATTRIBUTE_PRESENTATION: Hm, there's no way to have a separate StructTraits for this that FormFieldData's StructTraits can delegate to? This means that everyone that wants to embed this enum will need to copy these snippets around, which seems unfortunate. https://codereview.chromium.org/1999623002/diff/100001/components/autofill/co... File components/autofill/content/public/cpp/autofill_types_struct_traits_unittest.cc (right): https://codereview.chromium.org/1999623002/diff/100001/components/autofill/co... components/autofill/content/public/cpp/autofill_types_struct_traits_unittest.cc:52: proxy->PassFormFieldData(input, [&](const FormFieldData& passed) { Note that while the external Google C++ style guide now allows default captures (https://google.github.io/styleguide/cppguide.html#Lambda_expressions), the Chromium C++11 style page still indicates that they're disallowed (http://chromium-cpp.appspot.com/). If you want to use this, I'd send an email to cxx@chromium.org asking for this to be updated with the appropriate rationale.
https://codereview.chromium.org/1999623002/diff/100001/components/autofill/co... File components/autofill/content/public/cpp/autofill_types_struct_traits.cc (right): https://codereview.chromium.org/1999623002/diff/100001/components/autofill/co... components/autofill/content/public/cpp/autofill_types_struct_traits.cc:18: case FormFieldData::RoleAttribute::ROLE_ATTRIBUTE_PRESENTATION: On 2016/05/24 22:03:48, dcheng wrote: > Hm, there's no way to have a separate StructTraits for this that FormFieldData's > StructTraits can delegate to? This means that everyone that wants to embed this > enum will need to copy these snippets around, which seems unfortunate. No. There isn't support for "EnumTraits" currently. It shouldn't be hard to add it if we decide to. But I wonder whether it makes sense to suggest people to define their enum type in mojom instead.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999623002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1999623002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: None
leon.han@intel.com changed reviewers: + blundell@chromium.org
Uploaded ps#7 to address comments, would you PTAL? Thanks~ +blundell@ for review as //components OWNER. Thanks~ https://codereview.chromium.org/1999623002/diff/100001/components/autofill/co... File components/autofill/content/public/cpp/autofill_types_struct_traits_unittest.cc (right): https://codereview.chromium.org/1999623002/diff/100001/components/autofill/co... components/autofill/content/public/cpp/autofill_types_struct_traits_unittest.cc:52: proxy->PassFormFieldData(input, [&](const FormFieldData& passed) { On 2016/05/24 22:03:48, dcheng wrote: > Note that while the external Google C++ style guide now allows default captures > (https://google.github.io/styleguide/cppguide.html#Lambda_expressions), the > Chromium C++11 style page still indicates that they're disallowed > (http://chromium-cpp.appspot.com/). If you want to use this, I'd send an email > to mailto:cxx@chromium.org asking for this to be updated with the appropriate > rationale. Ah, Thanks a lot for pointing out this and knowledge sharing about the style guide~ Here I'd like to use explicit capture instead. Done.
On 2016/05/25 07:31:23, leonhsl wrote: > +blundell@ for review as //components OWNER. Thanks~ > You should specify what you want blundell@ to look at. Given the approvals so far I assume all but autofill?
On 2016/05/25 07:38:02, vabr (Chromium) wrote: > On 2016/05/25 07:31:23, leonhsl wrote: > > +blundell@ for review as //components OWNER. Thanks~ > > > > You should specify what you want blundell@ to look at. Given the approvals so > far I assume ? Yeah I want blundell@ to review all but autofill part. Thanks for pointing out this~
lgtm
Soft ping.. dcheng@, Thanks~
On 2016/05/27 at 02:12:33, leon.han wrote: > Soft ping.. dcheng@, Thanks~ Are you able to just use the mojom enum values directly rather than having to convert them? I'm really not a fan of tying the enum conversion directly into the StructTraits of other classes, since this will cause a bunch of duplication.
On 2016/05/27 15:32:35, dcheng wrote: > On 2016/05/27 at 02:12:33, leon.han wrote: > > Soft ping.. dcheng@, Thanks~ > > Are you able to just use the mojom enum values directly rather than having to > convert them? I'm really not a fan of tying the enum conversion directly into > the StructTraits of other classes, since this will cause a bunch of duplication. Oh, sorry for my misunderstanding, I thought the consensus had been made that we provide no 'EnumStraits' so I did not continue about it. To solve this, maybe we need to add typemap support for enum to let the generated codes always use existing enum definition directly(as struct member, or as function parameter), just as Yuzhu said, this way there will be no need to define enum type in mojom files. I think this will be an additional feature of Mojo bindings so I want to know Yuzhu's comment about this. If the decision made, I'm happy to try to prepare a CL for review, but for this CL could we just land now then the autofill mojofication work can proceed? Because later we can revisit for enum change easily. Thanks~
On 2016/05/29 12:30:19, leonhsl wrote: > On 2016/05/27 15:32:35, dcheng wrote: > > On 2016/05/27 at 02:12:33, leon.han wrote: > > > Soft ping.. dcheng@, Thanks~ > > > > Are you able to just use the mojom enum values directly rather than having to > > convert them? I'm really not a fan of tying the enum conversion directly into > > the StructTraits of other classes, since this will cause a bunch of > duplication. > > Oh, sorry for my misunderstanding, I thought the consensus had been made that we > provide no 'EnumStraits' so I did not continue about it. Hi, Leon. IIUC, Daniel's question is whether it makes sense to use the mojom enums to replace the existing enums everywhere, so that there is no need to do conversion. > To solve this, maybe we need to add typemap support for enum to let the > generated codes always use existing enum definition directly(as struct member, > or as function parameter), just as Yuzhu said, this way there will be no need to > define enum type in mojom files. If you don't want to define enum in mojom, then this is something similar to the concept of "native-only" struct. "native-only" struct was designed to reuse the existing IPC::ParamsTraits and reduce the pain of switching from IPC to Mojo. I personally wish that we don't have to go down that direction if possible. :) > I think this will be an additional feature of > Mojo bindings so I want to know Yuzhu's comment about this. If the decision > made, I'm happy to try to prepare a CL for review, but for this CL could we just > land now then the autofill mojofication work can proceed? Because later we can > revisit for enum change easily. Thanks~
On 2016/05/29 12:30:19, leonhsl wrote: > On 2016/05/27 15:32:35, dcheng wrote: > > On 2016/05/27 at 02:12:33, leon.han wrote: > > > Soft ping.. dcheng@, Thanks~ > > > > Are you able to just use the mojom enum values directly rather than having to > > convert them? I'm really not a fan of tying the enum conversion directly into > > the StructTraits of other classes, since this will cause a bunch of > duplication. > > Oh, sorry for my misunderstanding, I thought the consensus had been made that we > provide no 'EnumStraits' so I did not continue about it. Hi, Leon. IIUC, Daniel's question is whether it makes sense to use the mojom enums to replace the existing enums everywhere, so that there is no need to do conversion. > To solve this, maybe we need to add typemap support for enum to let the > generated codes always use existing enum definition directly(as struct member, > or as function parameter), just as Yuzhu said, this way there will be no need to > define enum type in mojom files. If you don't want to define enum in mojom, then this is something similar to the concept of "native-only" struct. "native-only" struct was designed to reuse the existing IPC::ParamsTraits and reduce the pain of switching from IPC to Mojo. I personally wish that we don't have to go down that direction if possible. :) > I think this will be an additional feature of > Mojo bindings so I want to know Yuzhu's comment about this. If the decision > made, I'm happy to try to prepare a CL for review, but for this CL could we just > land now then the autofill mojofication work can proceed? Because later we can > revisit for enum change easily. Thanks~
On 2016/05/31 20:47:29, yzshen1 wrote: > On 2016/05/29 12:30:19, leonhsl wrote: > > On 2016/05/27 15:32:35, dcheng wrote: > > > On 2016/05/27 at 02:12:33, leon.han wrote: > > > > Soft ping.. dcheng@, Thanks~ > > > > > > Are you able to just use the mojom enum values directly rather than having > to > > > convert them? I'm really not a fan of tying the enum conversion directly > into > > > the StructTraits of other classes, since this will cause a bunch of > > duplication. > > > > Oh, sorry for my misunderstanding, I thought the consensus had been made that > we > > provide no 'EnumStraits' so I did not continue about it. > > Hi, Leon. > > IIUC, Daniel's question is whether it makes sense to use the mojom enums to > replace the existing enums everywhere, so that there is no need to do > conversion. > > > > To solve this, maybe we need to add typemap support for enum to let the > > generated codes always use existing enum definition directly(as struct member, > > or as function parameter), just as Yuzhu said, this way there will be no need > to > > define enum type in mojom files. > If you don't want to define enum in mojom, then this is something similar to the > concept of "native-only" struct. > "native-only" struct was designed to reuse the existing IPC::ParamsTraits and > reduce the pain of switching from IPC to Mojo. > > I personally wish that we don't have to go down that direction if possible. :) > > > > I think this will be an additional feature of > > Mojo bindings so I want to know Yuzhu's comment about this. If the decision > > made, I'm happy to try to prepare a CL for review, but for this CL could we > just > > land now then the autofill mojofication work can proceed? Because later we can > > revisit for enum change easily. Thanks~ Hi, Yuzhu, Thanks a lot for explanation. Yeah I read about reusing IPC::ParamsTraits before from your document ;-) and understand if possible we should avoid this. Hi, Daniel, I'll start to figure out how could we replace existing enum usage everywhere with the mojom enum instead.
On 2016/06/01 at 02:53:38, leon.han wrote: > On 2016/05/31 20:47:29, yzshen1 wrote: > > On 2016/05/29 12:30:19, leonhsl wrote: > > > On 2016/05/27 15:32:35, dcheng wrote: > > > > On 2016/05/27 at 02:12:33, leon.han wrote: > > > > > Soft ping.. dcheng@, Thanks~ > > > > > > > > Are you able to just use the mojom enum values directly rather than having > > to > > > > convert them? I'm really not a fan of tying the enum conversion directly > > into > > > > the StructTraits of other classes, since this will cause a bunch of > > > duplication. > > > > > > Oh, sorry for my misunderstanding, I thought the consensus had been made that > > we > > > provide no 'EnumStraits' so I did not continue about it. > > > > Hi, Leon. > > > > IIUC, Daniel's question is whether it makes sense to use the mojom enums to > > replace the existing enums everywhere, so that there is no need to do > > conversion. > > > > > > > To solve this, maybe we need to add typemap support for enum to let the > > > generated codes always use existing enum definition directly(as struct member, > > > or as function parameter), just as Yuzhu said, this way there will be no need > > to > > > define enum type in mojom files. > > If you don't want to define enum in mojom, then this is something similar to the > > concept of "native-only" struct. > > "native-only" struct was designed to reuse the existing IPC::ParamsTraits and > > reduce the pain of switching from IPC to Mojo. > > > > I personally wish that we don't have to go down that direction if possible. :) > > > > > > > I think this will be an additional feature of > > > Mojo bindings so I want to know Yuzhu's comment about this. If the decision > > > made, I'm happy to try to prepare a CL for review, but for this CL could we > > just > > > land now then the autofill mojofication work can proceed? Because later we can > > > revisit for enum change easily. Thanks~ > > Hi, Yuzhu, Thanks a lot for explanation. Yeah I read about reusing IPC::ParamsTraits > before from your document ;-) and understand if possible we should avoid this. > > Hi, Daniel, I'll start to figure out how could we replace existing enum usage everywhere > with the mojom enum instead. OK thanks... FWIW, this seems to be coming up a lot. So we might need to think of a better solution in Mojo anyway... Maybe we could allow TypeConverters only for enums, since (non-extensible) mojo enums are validated on the wire anyway.
On 2016/06/02 00:19:48, dcheng wrote: > On 2016/06/01 at 02:53:38, leon.han wrote: > > On 2016/05/31 20:47:29, yzshen1 wrote: > > > On 2016/05/29 12:30:19, leonhsl wrote: > > > > On 2016/05/27 15:32:35, dcheng wrote: > > > > > On 2016/05/27 at 02:12:33, leon.han wrote: > > > > > > Soft ping.. dcheng@, Thanks~ > > > > > > > > > > Are you able to just use the mojom enum values directly rather than > having > > > to > > > > > convert them? I'm really not a fan of tying the enum conversion directly > > > into > > > > > the StructTraits of other classes, since this will cause a bunch of > > > > duplication. > > > > > > > > Oh, sorry for my misunderstanding, I thought the consensus had been made > that > > > we > > > > provide no 'EnumStraits' so I did not continue about it. > > > > > > Hi, Leon. > > > > > > IIUC, Daniel's question is whether it makes sense to use the mojom enums to > > > replace the existing enums everywhere, so that there is no need to do > > > conversion. > > > > > > > > > > To solve this, maybe we need to add typemap support for enum to let the > > > > generated codes always use existing enum definition directly(as struct > member, > > > > or as function parameter), just as Yuzhu said, this way there will be no > need > > > to > > > > define enum type in mojom files. > > > If you don't want to define enum in mojom, then this is something similar to > the > > > concept of "native-only" struct. > > > "native-only" struct was designed to reuse the existing IPC::ParamsTraits > and > > > reduce the pain of switching from IPC to Mojo. > > > > > > I personally wish that we don't have to go down that direction if possible. > :) > > > > > > > > > > I think this will be an additional feature of > > > > Mojo bindings so I want to know Yuzhu's comment about this. If the > decision > > > > made, I'm happy to try to prepare a CL for review, but for this CL could > we > > > just > > > > land now then the autofill mojofication work can proceed? Because later we > can > > > > revisit for enum change easily. Thanks~ > > > > Hi, Yuzhu, Thanks a lot for explanation. Yeah I read about reusing > IPC::ParamsTraits > > before from your document ;-) and understand if possible we should avoid this. > > > > Hi, Daniel, I'll start to figure out how could we replace existing enum usage > everywhere > > with the mojom enum instead. > > OK thanks... FWIW, this seems to be coming up a lot. So we might need to think > of a better solution in Mojo anyway... > > Maybe we could allow TypeConverters only for enums, since (non-extensible) mojo > enums are validated on the wire anyway. Yeah it really would be many places to replace the enum usage and also need to touch ios implementation.. Thanks a lot for kindly consideration~ And sorry I'm not clear about 'allow TypeConverters only for enums', does it mean that the static_cast way in ps#4 is enough? Thanks~
On 2016/06/02 at 05:34:20, leon.han wrote: > On 2016/06/02 00:19:48, dcheng wrote: > > On 2016/06/01 at 02:53:38, leon.han wrote: > > > On 2016/05/31 20:47:29, yzshen1 wrote: > > > > On 2016/05/29 12:30:19, leonhsl wrote: > > > > > On 2016/05/27 15:32:35, dcheng wrote: > > > > > > On 2016/05/27 at 02:12:33, leon.han wrote: > > > > > > > Soft ping.. dcheng@, Thanks~ > > > > > > > > > > > > Are you able to just use the mojom enum values directly rather than > > having > > > > to > > > > > > convert them? I'm really not a fan of tying the enum conversion directly > > > > into > > > > > > the StructTraits of other classes, since this will cause a bunch of > > > > > duplication. > > > > > > > > > > Oh, sorry for my misunderstanding, I thought the consensus had been made > > that > > > > we > > > > > provide no 'EnumStraits' so I did not continue about it. > > > > > > > > Hi, Leon. > > > > > > > > IIUC, Daniel's question is whether it makes sense to use the mojom enums to > > > > replace the existing enums everywhere, so that there is no need to do > > > > conversion. > > > > > > > > > > > > > To solve this, maybe we need to add typemap support for enum to let the > > > > > generated codes always use existing enum definition directly(as struct > > member, > > > > > or as function parameter), just as Yuzhu said, this way there will be no > > need > > > > to > > > > > define enum type in mojom files. > > > > If you don't want to define enum in mojom, then this is something similar to > > the > > > > concept of "native-only" struct. > > > > "native-only" struct was designed to reuse the existing IPC::ParamsTraits > > and > > > > reduce the pain of switching from IPC to Mojo. > > > > > > > > I personally wish that we don't have to go down that direction if possible. > > :) > > > > > > > > > > > > > I think this will be an additional feature of > > > > > Mojo bindings so I want to know Yuzhu's comment about this. If the > > decision > > > > > made, I'm happy to try to prepare a CL for review, but for this CL could > > we > > > > just > > > > > land now then the autofill mojofication work can proceed? Because later we > > can > > > > > revisit for enum change easily. Thanks~ > > > > > > Hi, Yuzhu, Thanks a lot for explanation. Yeah I read about reusing > > IPC::ParamsTraits > > > before from your document ;-) and understand if possible we should avoid this. > > > > > > Hi, Daniel, I'll start to figure out how could we replace existing enum usage > > everywhere > > > with the mojom enum instead. > > > > OK thanks... FWIW, this seems to be coming up a lot. So we might need to think > > of a better solution in Mojo anyway... > > > > Maybe we could allow TypeConverters only for enums, since (non-extensible) mojo > > enums are validated on the wire anyway. > > Yeah it really would be many places to replace the enum usage and also need to touch ios implementation.. Thanks a lot for kindly consideration~ And sorry I'm not clear about 'allow TypeConverters only for enums', does it mean that the static_cast way in ps#4 is enough? Thanks~ I've started a thread on chromium-mojo@ about this. Sorry for the delay, but I think we should try to figure out a good way forward. The current solutions aren't very satisfactory.
On 2016/06/03 04:17:57, dcheng wrote: > On 2016/06/02 at 05:34:20, leon.han wrote: > > On 2016/06/02 00:19:48, dcheng wrote: > > > On 2016/06/01 at 02:53:38, leon.han wrote: > > > > On 2016/05/31 20:47:29, yzshen1 wrote: > > > > > On 2016/05/29 12:30:19, leonhsl wrote: > > > > > > On 2016/05/27 15:32:35, dcheng wrote: > > > > > > > On 2016/05/27 at 02:12:33, leon.han wrote: > > > > > > > > Soft ping.. dcheng@, Thanks~ > > > > > > > > > > > > > > Are you able to just use the mojom enum values directly rather than > > > having > > > > > to > > > > > > > convert them? I'm really not a fan of tying the enum conversion > directly > > > > > into > > > > > > > the StructTraits of other classes, since this will cause a bunch of > > > > > > duplication. > > > > > > > > > > > > Oh, sorry for my misunderstanding, I thought the consensus had been > made > > > that > > > > > we > > > > > > provide no 'EnumStraits' so I did not continue about it. > > > > > > > > > > Hi, Leon. > > > > > > > > > > IIUC, Daniel's question is whether it makes sense to use the mojom enums > to > > > > > replace the existing enums everywhere, so that there is no need to do > > > > > conversion. > > > > > > > > > > > > > > > > To solve this, maybe we need to add typemap support for enum to let > the > > > > > > generated codes always use existing enum definition directly(as struct > > > member, > > > > > > or as function parameter), just as Yuzhu said, this way there will be > no > > > need > > > > > to > > > > > > define enum type in mojom files. > > > > > If you don't want to define enum in mojom, then this is something > similar to > > > the > > > > > concept of "native-only" struct. > > > > > "native-only" struct was designed to reuse the existing > IPC::ParamsTraits > > > and > > > > > reduce the pain of switching from IPC to Mojo. > > > > > > > > > > I personally wish that we don't have to go down that direction if > possible. > > > :) > > > > > > > > > > > > > > > > I think this will be an additional feature of > > > > > > Mojo bindings so I want to know Yuzhu's comment about this. If the > > > decision > > > > > > made, I'm happy to try to prepare a CL for review, but for this CL > could > > > we > > > > > just > > > > > > land now then the autofill mojofication work can proceed? Because > later we > > > can > > > > > > revisit for enum change easily. Thanks~ > > > > > > > > Hi, Yuzhu, Thanks a lot for explanation. Yeah I read about reusing > > > IPC::ParamsTraits > > > > before from your document ;-) and understand if possible we should avoid > this. > > > > > > > > Hi, Daniel, I'll start to figure out how could we replace existing enum > usage > > > everywhere > > > > with the mojom enum instead. > > > > > > OK thanks... FWIW, this seems to be coming up a lot. So we might need to > think > > > of a better solution in Mojo anyway... > > > > > > Maybe we could allow TypeConverters only for enums, since (non-extensible) > mojo > > > enums are validated on the wire anyway. > > > > Yeah it really would be many places to replace the enum usage and also need to > touch ios implementation.. Thanks a lot for kindly consideration~ And sorry I'm > not clear about 'allow TypeConverters only for enums', does it mean that the > static_cast way in ps#4 is enough? Thanks~ > > I've started a thread on chromium-mojo@ about this. Sorry for the delay, but I > think we should try to figure out a good way forward. The current solutions > aren't very satisfactory. Hi, Thanks for information~ Yeah I saw the thread in chromium-mojo and understood we need to figure out a better way transmitting enum. Hmm,, before we have a conclusion and get the conclusion implemented, could we just land this CL now with a TODO comment then later we can rework here to apply the new enum solution? I want to use this CL as basis of my follow-up IPCs conversion CLs, so landing this now would relieve me from some rebase work and enable me use the trybots to verify the follow-up CLs easily.. Thanks~
Patchset #8 (id:140001) has been deleted
On 2016/06/03 07:48:41, leonhsl wrote: > On 2016/06/03 04:17:57, dcheng wrote: > > On 2016/06/02 at 05:34:20, leon.han wrote: > > > On 2016/06/02 00:19:48, dcheng wrote: > > > > On 2016/06/01 at 02:53:38, leon.han wrote: > > > > > On 2016/05/31 20:47:29, yzshen1 wrote: > > > > > > On 2016/05/29 12:30:19, leonhsl wrote: > > > > > > > On 2016/05/27 15:32:35, dcheng wrote: > > > > > > > > On 2016/05/27 at 02:12:33, leon.han wrote: > > > > > > > > > Soft ping.. dcheng@, Thanks~ > > > > > > > > > > > > > > > > Are you able to just use the mojom enum values directly rather > than > > > > having > > > > > > to > > > > > > > > convert them? I'm really not a fan of tying the enum conversion > > directly > > > > > > into > > > > > > > > the StructTraits of other classes, since this will cause a bunch > of > > > > > > > duplication. > > > > > > > > > > > > > > Oh, sorry for my misunderstanding, I thought the consensus had been > > made > > > > that > > > > > > we > > > > > > > provide no 'EnumStraits' so I did not continue about it. > > > > > > > > > > > > Hi, Leon. > > > > > > > > > > > > IIUC, Daniel's question is whether it makes sense to use the mojom > enums > > to > > > > > > replace the existing enums everywhere, so that there is no need to do > > > > > > conversion. > > > > > > > > > > > > > > > > > > > To solve this, maybe we need to add typemap support for enum to let > > the > > > > > > > generated codes always use existing enum definition directly(as > struct > > > > member, > > > > > > > or as function parameter), just as Yuzhu said, this way there will > be > > no > > > > need > > > > > > to > > > > > > > define enum type in mojom files. > > > > > > If you don't want to define enum in mojom, then this is something > > similar to > > > > the > > > > > > concept of "native-only" struct. > > > > > > "native-only" struct was designed to reuse the existing > > IPC::ParamsTraits > > > > and > > > > > > reduce the pain of switching from IPC to Mojo. > > > > > > > > > > > > I personally wish that we don't have to go down that direction if > > possible. > > > > :) > > > > > > > > > > > > > > > > > > > I think this will be an additional feature of > > > > > > > Mojo bindings so I want to know Yuzhu's comment about this. If the > > > > decision > > > > > > > made, I'm happy to try to prepare a CL for review, but for this CL > > could > > > > we > > > > > > just > > > > > > > land now then the autofill mojofication work can proceed? Because > > later we > > > > can > > > > > > > revisit for enum change easily. Thanks~ > > > > > > > > > > Hi, Yuzhu, Thanks a lot for explanation. Yeah I read about reusing > > > > IPC::ParamsTraits > > > > > before from your document ;-) and understand if possible we should avoid > > this. > > > > > > > > > > Hi, Daniel, I'll start to figure out how could we replace existing enum > > usage > > > > everywhere > > > > > with the mojom enum instead. > > > > > > > > OK thanks... FWIW, this seems to be coming up a lot. So we might need to > > think > > > > of a better solution in Mojo anyway... > > > > > > > > Maybe we could allow TypeConverters only for enums, since (non-extensible) > > mojo > > > > enums are validated on the wire anyway. > > > > > > Yeah it really would be many places to replace the enum usage and also need > to > > touch ios implementation.. Thanks a lot for kindly consideration~ And sorry > I'm > > not clear about 'allow TypeConverters only for enums', does it mean that the > > static_cast way in ps#4 is enough? Thanks~ > > > > I've started a thread on chromium-mojo@ about this. Sorry for the delay, but I > > think we should try to figure out a good way forward. The current solutions > > aren't very satisfactory. > > Hi, Thanks for information~ Yeah I saw the thread in chromium-mojo and > understood we need to figure out a better way transmitting enum. Hmm,, before we > have a conclusion and get the conclusion implemented, could we just land this CL > now with a TODO comment then later we can rework here to apply the new enum > solution? I want to use this CL as basis of my follow-up IPCs conversion CLs, so > landing this now would relieve me from some rebase work and enable me use the > trybots to verify the follow-up CLs easily.. Thanks~ Ping dcheng@
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999623002/160001
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 leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999623002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
Patchset #10 (id:200001) has been deleted
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999623002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dcheng@, would you PTAL at ps#10? thanks
Ping dcheng@
https://codereview.chromium.org/1999623002/diff/220001/components/autofill/co... File components/autofill/content/public/cpp/autofill_types_struct_traits.cc (right): https://codereview.chromium.org/1999623002/diff/220001/components/autofill/co... components/autofill/content/public/cpp/autofill_types_struct_traits.cc:44: I think this should be NOTREACHED() as well? Mojo still validates the enum values over the wire, so this should never happen. https://codereview.chromium.org/1999623002/diff/220001/components/autofill/co... components/autofill/content/public/cpp/autofill_types_struct_traits.cc:73: Ditto. https://codereview.chromium.org/1999623002/diff/220001/components/autofill/co... components/autofill/content/public/cpp/autofill_types_struct_traits.cc:112: etc. https://codereview.chromium.org/1999623002/diff/220001/components/autofill/co... File components/autofill/content/public/interfaces/autofill_types.mojom (right): https://codereview.chromium.org/1999623002/diff/220001/components/autofill/co... components/autofill/content/public/interfaces/autofill_types.mojom:38: uint64 max_length; Can we/should we clamp this at a reasonable value? It seems quite unlikely for max length to ever be that big... and our renderer process would probably die before that.
dcheng@, thanks for your time to review. Uploaded ps#11 to address comments, please take another look. Thanks. https://codereview.chromium.org/1999623002/diff/220001/components/autofill/co... File components/autofill/content/public/cpp/autofill_types_struct_traits.cc (right): https://codereview.chromium.org/1999623002/diff/220001/components/autofill/co... components/autofill/content/public/cpp/autofill_types_struct_traits.cc:44: On 2016/06/13 20:59:36, dcheng wrote: > I think this should be NOTREACHED() as well? Mojo still validates the enum > values over the wire, so this should never happen. Done. https://codereview.chromium.org/1999623002/diff/220001/components/autofill/co... components/autofill/content/public/cpp/autofill_types_struct_traits.cc:73: On 2016/06/13 20:59:36, dcheng wrote: > Ditto. Done. https://codereview.chromium.org/1999623002/diff/220001/components/autofill/co... components/autofill/content/public/cpp/autofill_types_struct_traits.cc:112: On 2016/06/13 20:59:36, dcheng wrote: > etc. Done. https://codereview.chromium.org/1999623002/diff/220001/components/autofill/co... File components/autofill/content/public/interfaces/autofill_types.mojom (right): https://codereview.chromium.org/1999623002/diff/220001/components/autofill/co... components/autofill/content/public/interfaces/autofill_types.mojom:38: uint64 max_length; On 2016/06/13 20:59:36, dcheng wrote: > Can we/should we clamp this at a reasonable value? It seems quite unlikely for > max length to ever be that big... and our renderer process would probably die > before that. This means we should change autofill::FormFieldData::max_length to uint32_t? Please see https://cs.chromium.org/chromium/src/components/autofill/core/common/form_fie... for reference. And, would you please clarify what's the meaning of "our renderer process would probably die before that"? Does it mean that just transmitting an uint64 value will cause renderer process die?
https://codereview.chromium.org/1999623002/diff/220001/components/autofill/co... File components/autofill/content/public/interfaces/autofill_types.mojom (right): https://codereview.chromium.org/1999623002/diff/220001/components/autofill/co... components/autofill/content/public/interfaces/autofill_types.mojom:38: uint64 max_length; On 2016/06/14 01:46:04, leonhsl wrote: > On 2016/06/13 20:59:36, dcheng wrote: > > Can we/should we clamp this at a reasonable value? It seems quite unlikely for > > max length to ever be that big... and our renderer process would probably die > > before that. > > This means we should change autofill::FormFieldData::max_length to uint32_t? > Please see > https://cs.chromium.org/chromium/src/components/autofill/core/common/form_fie... > for reference. > And, would you please clarify what's the meaning of "our renderer process would > probably die > before that"? Does it mean that just transmitting an uint64 value will cause > renderer process die? The serialization of the uint64 would be fine. But I'm not sure that the renderer would be able to actually allocate that much storage for a string [1]... so letting max length be this large seems kind of pointless. [1] partition alloc limits itself to 2GB per process: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/allocator/...
https://codereview.chromium.org/1999623002/diff/220001/components/autofill/co... File components/autofill/content/public/interfaces/autofill_types.mojom (right): https://codereview.chromium.org/1999623002/diff/220001/components/autofill/co... components/autofill/content/public/interfaces/autofill_types.mojom:38: uint64 max_length; On 2016/06/14 04:20:56, dcheng wrote: > On 2016/06/14 01:46:04, leonhsl wrote: > > On 2016/06/13 20:59:36, dcheng wrote: > > > Can we/should we clamp this at a reasonable value? It seems quite unlikely > for > > > max length to ever be that big... and our renderer process would probably > die > > > before that. > > > > This means we should change autofill::FormFieldData::max_length to uint32_t? > > Please see > > > https://cs.chromium.org/chromium/src/components/autofill/core/common/form_fie... > > for reference. > > And, would you please clarify what's the meaning of "our renderer process > would > > probably die > > before that"? Does it mean that just transmitting an uint64 value will cause > > renderer process die? > > The serialization of the uint64 would be fine. But I'm not sure that the > renderer would be able to actually allocate that much storage for a string > [1]... so letting max length be this large seems kind of pointless. > > [1] partition alloc limits itself to 2GB per process: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/allocator/... max_length is for length limit check, nowhere will try to allocate a string with max_length storage. Just like https://cs.chromium.org/chromium/src/components/autofill/core/common/form_fie... said, uint64 is chosen deliberately. So, if we want to address this 'too large' issue, we'd better to file a new issue to discuss? Actually I don't think this is still within this CL's coverage. thanks.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999623002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by leon.han@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, vabr@chromium.org, yzshen@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/1999623002/#ps240001 (title: "Address comments from dcheng")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999623002/240001
Message was sent while issue was closed.
Committed patchset #11 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [autofill] Add typemap for autofill:FormFieldData and autofill::FormData. This will facilitate the work to mojofy those IPC messages carrying these two types of struct, which I will do with follow up CLs. TEST=components_unittests --gtest_filter=AutofillTypeTraitsTestImpl* BUG=582391 ========== to ========== [autofill] Add typemap for autofill:FormFieldData and autofill::FormData. This will facilitate the work to mojofy those IPC messages carrying these two types of struct, which I will do with follow up CLs. TEST=components_unittests --gtest_filter=AutofillTypeTraitsTestImpl* BUG=582391 Committed: https://crrev.com/77adee7146a554a4bcf4046988f9183d7707b54f Cr-Commit-Position: refs/heads/master@{#399880} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/77adee7146a554a4bcf4046988f9183d7707b54f Cr-Commit-Position: refs/heads/master@{#399880} |
