Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2161993004:
Make geometry struct_trait shared by Blink and Chromium (Closed)
|
Created:
4 years, 5 months ago by xlai (Olivia) Modified:
4 years, 3 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, blink-reviews, blink-reviews-api_chromium.org, cc-bugs_chromium.org, chromium-reviews, darin (slow to review), dglazkov+blink, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake geometry struct_trait shared by Blink and Chromium
This CL is part of the effort to import compositor_frame.mojom to
offscreen_canvas_surface.mojom in Blink. The compositor_frame.mojom depends on
ui/gfx/geometry/mojo/geometry.mojom, cc/ipc/surface_id.mojom, etc. This CL
makes geometry struct_traits to be a partial template specialization so that
it can be specialized in blink mojom types and in chromium mojom types without
code duplication.
BUG=629566
Patch Set 1 #
Total comments: 4
Patch Set 2 : Remove geometry.typemap from Blink build #Patch Set 3 : Only geometry struct traits #
Messages
Total messages: 30 (13 generated)
The CQ bit was checked by xlai@chromium.org to run a CQ dry run
Description was changed from ========== Organize typemaps in Blink and make geometry struct_trait shared by Blink and Chromium This CL is part of the effort to import compositor_frame.mojom to offscreen_canvas_surface.mojom in Blink. The compositor_frame.mojom depends on ui/gfx/geometry/mojo/geometry.mojom, cc/ipc/surface_id.mojom, etc. This CL makes geometry struct_traits to be a partial template specialization so that it can be specialized in blink mojom types and in chromium mojom types without code duplication. This CL also organizes the typemaps in Blinki GN build. Previously, we dump typemaps in Blink to third_party/WebKit/Source/platform/mojo/blink_typemaps.gni, even though some of them are used by third_party/WebKit/public/. This is wrong and inconsistent with GYP build. So I added a new typemaps.gni in WebKit/public to collect them separately. BUG=629566 ========== to ========== Organize typemaps in Blink and make geometry struct_trait shared by Blink and Chromium This CL is part of the effort to import compositor_frame.mojom to offscreen_canvas_surface.mojom in Blink. The compositor_frame.mojom depends on ui/gfx/geometry/mojo/geometry.mojom, cc/ipc/surface_id.mojom, etc. This CL makes geometry struct_traits to be a partial template specialization so that it can be specialized in blink mojom types and in chromium mojom types without code duplication. This CL also organizes the typemaps in Blinki GN build. Previously, we dump typemaps in Blink to third_party/WebKit/Source/platform/mojo/blink_typemaps.gni, even though some of them are used by third_party/WebKit/public/. This is wrong and inconsistent with GYP build. So I added a new typemaps.gni in WebKit/public to collect them separately. BUG=629566 ==========
xlai@chromium.org changed reviewers: + fsamuel@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xlai@chromium.org changed reviewers: + pdr@chromium.org
pdr@chromium.org: Please review changes in third_party/WebKit/public/ and third_party/WebKit/Source/platform/ to see if my re-organization in GN build for typemaps in Blink is fine. fsamuel: I started my templatization from geometry.mojom. PTAL.
geometry_struct_traits.h lgtm
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_...)
xlai@chromium.org changed reviewers: + dcheng@chromium.org
xlai@chromium.org changed reviewers: + yzshen@chromium.org
pdr@chromium.org changed reviewers: + jbroman@chromium.org
I'm not very familiar with this area. Just a couple nits about indentation. @jbroman, can you take a look at this too? https://codereview.chromium.org/2161993004/diff/1/third_party/WebKit/public/b... File third_party/WebKit/public/blink.gyp (right): https://codereview.chromium.org/2161993004/diff/1/third_party/WebKit/public/b... third_party/WebKit/public/blink.gyp:132: 'includes': [ nit: indentation https://codereview.chromium.org/2161993004/diff/1/third_party/WebKit/public/b... third_party/WebKit/public/blink.gyp:153: 'includes': [ Nit: indentation
Description was changed from ========== Organize typemaps in Blink and make geometry struct_trait shared by Blink and Chromium This CL is part of the effort to import compositor_frame.mojom to offscreen_canvas_surface.mojom in Blink. The compositor_frame.mojom depends on ui/gfx/geometry/mojo/geometry.mojom, cc/ipc/surface_id.mojom, etc. This CL makes geometry struct_traits to be a partial template specialization so that it can be specialized in blink mojom types and in chromium mojom types without code duplication. This CL also organizes the typemaps in Blinki GN build. Previously, we dump typemaps in Blink to third_party/WebKit/Source/platform/mojo/blink_typemaps.gni, even though some of them are used by third_party/WebKit/public/. This is wrong and inconsistent with GYP build. So I added a new typemaps.gni in WebKit/public to collect them separately. BUG=629566 ========== to ========== Organize typemaps in Blink and make geometry struct_trait shared by Blink and Chromium This CL is part of the effort to import compositor_frame.mojom to offscreen_canvas_surface.mojom in Blink. The compositor_frame.mojom depends on ui/gfx/geometry/mojo/geometry.mojom, cc/ipc/surface_id.mojom, etc. This CL makes geometry struct_traits to be a partial template specialization so that it can be specialized in blink mojom types and in chromium mojom types without code duplication. This CL also organizes the typemaps in Blink GN build. Previously, we dump typemaps in Blink to third_party/WebKit/Source/platform/mojo/blink_typemaps.gni, even though some of them are used by third_party/WebKit/public/. This is wrong and inconsistent with GYP build. So I added a new typemaps.gni in WebKit/public to collect them separately. BUG=629566 ==========
https://codereview.chromium.org/2161993004/diff/1/ui/gfx/geometry/mojo/geomet... File ui/gfx/geometry/mojo/geometry_struct_traits.h (right): https://codereview.chromium.org/2161993004/diff/1/ui/gfx/geometry/mojo/geomet... ui/gfx/geometry/mojo/geometry_struct_traits.h:21: template <typename T> Why do we need to templatize this? Some of these methods probably should have been defined out-of-line as well (which I think templatizing will make harder)
If I read this correctly, this maps gfx.mojom.* to gfx::* in Blink as well as Chromium. Do we want to do that (at the moment Blink doesn't use ui/gfx/ in core or modules, which have every right to use the Blink version of mojo bindings)? yzshen, it seems to me that we should be able to include the gfx struct traits without putting it in the typemap (the generated DataViews etc. seem to be flexible enough to allow for struct traits for a variety of types to exist). Is there a limitation that makes this necessary? cc esprehn, who has given some thought to our plan for geometry types going forward and might have thoughts.
On 2016/07/20 18:25:31, jbroman wrote: > If I read this correctly, this maps gfx.mojom.* to gfx::* in Blink as well as > Chromium. I guess this is not what the current mapping in mojo does. The same custom type gfx::* is mapped to two different mojom types in Blink and Chromium (in fact, same type, but just in two different namespaces). This is the reason why I have to use templates in struct_traits. Take surface_id as an example. You could take a look a surface_id.mojom.h and surface_id.mojom-blink.h. They are mojom types in two different namespaces. https://codereview.chromium.org/2161993004/diff/1/ui/gfx/geometry/mojo/geomet... File ui/gfx/geometry/mojo/geometry_struct_traits.h (right): https://codereview.chromium.org/2161993004/diff/1/ui/gfx/geometry/mojo/geomet... ui/gfx/geometry/mojo/geometry_struct_traits.h:21: template <typename T> On 2016/07/20 18:16:10, dcheng wrote: > Why do we need to templatize this? Some of these methods probably should have > been defined out-of-line as well (which I think templatizing will make harder) My ultimate intention is to use compositor_frame.mojom in offscreen_canvas_surface.mojom in Blink. compositor_frame.mojom depends on render_pass.mojom, which in turn depends on geometry.mojom. I want to do it step by step, so templatizing geometry.mojom first. There will be more CLs coming along to templatize more mojom files until eventually compositor_frame.mojom could be used in Blink. (See crbug.com/629566) The template is fully specialized as gfx.mojom.Insets in Chromium and as gfx.mojom.blink.Insets in Blink. If I do not use a template here, then I have to create one more typemap and one more struct_trait in Blink, which look quite similar to this file except that every gfx::mojom here is replaced to gfx::mojom::blink. That will be too much code duplication. Another similar example is surface_id_struct_trait.h.
On 2016/07/20 18:25:31, jbroman wrote: > Do we want to do that (at the moment Blink doesn't use ui/gfx/ in core > or modules, which have every right to use the Blink version of mojo bindings)? > > cc esprehn, who has given some thought to our plan for geometry types going > forward and might have thoughts. Okay, I get what you mean: whether ui/gfx/geometry/mojo/geometry.mojom should be mapped to a Blink-version geometry type instead of the various types in ui/gfx/geometry/. esprehn@: Is there such a blink-version geometry types ready? How about ui/gfx/transform.h (It's going to be the next one I'll work with)? fsamuel@: If there are no available Blink-version custom types in ui/gfx and potentially the other 20 mojom files that I'm going to do typemapping in Blink, then I would think adding and improving every single custom type that constitute compositor_frame.mojom seems too stretchy for the canvas team. I had discussion with junov@ and we think if that's the case, we may drop the original plan of sending compositor_frame in mojo calls from Blink.
On 2016/07/20 18:25:31, jbroman wrote: > If I read this correctly, this maps gfx.mojom.* to gfx::* in Blink as well as > Chromium. Do we want to do that (at the moment Blink doesn't use ui/gfx/ in core > or modules, which have every right to use the Blink version of mojo bindings)? > > yzshen, it seems to me that we should be able to include the gfx struct traits > without putting it in the typemap (the generated DataViews etc. seem to be > flexible enough to allow for struct traits for a variety of types to exist). Is > there a limitation that makes this necessary? Correct. Including a StructTraits doesn't mean that we have to put it in the typemap: - StructTraits<Foo, CustomFoo> provides conversion functions between the wire format of mojo type Foo and a CustomFoo. - putting it in the typemap means in generated mojo interfaces, we want to use CustomFoo in place of Foo (which will need to use StructTraits<Foo, CustomFoo> to do necessary serialization/deserialization). > > cc esprehn, who has given some thought to our plan for geometry types going > forward and might have thoughts.
On 2016/07/20 23:46:06, xlai (Olivia) wrote: > On 2016/07/20 18:25:31, jbroman wrote: > > Do we want to do that (at the moment Blink doesn't use ui/gfx/ in core > > or modules, which have every right to use the Blink version of mojo bindings)? > > > > cc esprehn, who has given some thought to our plan for geometry types going > > forward and might have thoughts. > > Okay, I get what you mean: whether ui/gfx/geometry/mojo/geometry.mojom should > be mapped to a Blink-version geometry type instead of the various types in > ui/gfx/geometry/. > > esprehn@: Is there such a blink-version geometry types ready? How about > ui/gfx/transform.h (It's going to be the next one I'll work with)? > > fsamuel@: If there are no available Blink-version custom types in ui/gfx and > potentially the other 20 mojom files that I'm going to do typemapping in Blink, > then I would think adding and improving every single custom type that > constitute compositor_frame.mojom seems too stretchy for the canvas team. I had > discussion with junov@ and we think if that's the case, we may drop the original > plan of sending compositor_frame in mojo calls from Blink. Not trying to suggest which direction we should take. But I would like to point out: the idea of defining mojom data types and then do custom typemapping is exactly for scenarios where two sides want to use different types which share the same wire format.
I don't think we want to use the gfx types in blink yet, unless you're confining all of this usage to platform/. We need to unify the blink types and the gfx types. For example blink's matrix has inline assembly fast paths that need to be merged into gfx. I don't think we have type mappings to convert from gfx -> platform/geometry setup yet. You'd need to add those.
Based on suggestion from yzshen1@ and jbroman@, I did a local experiment and learn that I do not need to include geometry.typemap into Blink, even in the future in Blink I'm going to use compositor_frame.mojom that depend on geometry.mojom. As a result, no change in Blink build (except some re-arrangement). jbroman@: PTAL. Hope this time it satisfies the Blink DEPS requirement. dcheng@: The change in geometry_struct_traits.h is to make it ready to do the type mapping between Blink/Chromium mojom types and ui/gfx/geometry user types, which is necessary to support the type mapping in many cc mojom files. I'm trying to do things step by step.
Could you please split the geometry_struct_traits.h change into a separate CL? It doesn't seem part of your rearranging typemaps.
Description was changed from ========== Organize typemaps in Blink and make geometry struct_trait shared by Blink and Chromium This CL is part of the effort to import compositor_frame.mojom to offscreen_canvas_surface.mojom in Blink. The compositor_frame.mojom depends on ui/gfx/geometry/mojo/geometry.mojom, cc/ipc/surface_id.mojom, etc. This CL makes geometry struct_traits to be a partial template specialization so that it can be specialized in blink mojom types and in chromium mojom types without code duplication. This CL also organizes the typemaps in Blink GN build. Previously, we dump typemaps in Blink to third_party/WebKit/Source/platform/mojo/blink_typemaps.gni, even though some of them are used by third_party/WebKit/public/. This is wrong and inconsistent with GYP build. So I added a new typemaps.gni in WebKit/public to collect them separately. BUG=629566 ========== to ========== Make geometry struct_trait shared by Blink and Chromium This CL is part of the effort to import compositor_frame.mojom to offscreen_canvas_surface.mojom in Blink. The compositor_frame.mojom depends on ui/gfx/geometry/mojo/geometry.mojom, cc/ipc/surface_id.mojom, etc. This CL makes geometry struct_traits to be a partial template specialization so that it can be specialized in blink mojom types and in chromium mojom types without code duplication. BUG=629566 ==========
xlai@chromium.org changed reviewers: - jbroman@chromium.org, pdr@chromium.org, yzshen@chromium.org
How many more struct traits do we need to do this for? It is an oversight that the original Read() methods weren't out-of-lined, and templating makes that impossible. Are we going to need to template a bunch of other struct traits?
On 2016/07/22 17:27:03, jbroman wrote: > Could you please split the geometry_struct_traits.h change into a separate CL? > It doesn't seem part of your rearranging typemaps. I have extracted those reorgnization parts to https://codereview.chromium.org/2172333002/. This CL is now only related to change in geometry_struct_traits.h
On 2016/07/22 17:49:27, dcheng wrote: > How many more struct traits do we need to do this for? > > It is an oversight that the original Read() methods weren't out-of-lined, and > templating makes that impossible. Are we going to need to template a bunch of > other struct traits? Yes, exactly. The exhaustive list of struct_traits that I'm going to templatize are: cc/ipc/compositor_frame_struct_traits cc/ipc/compositor_frame_metadata_struct_traits cc/ipc/render_pass_struct_traits cc/ipc/render_pass_id_struct_traits cc/ipc/transferable_resource_struct_traits cc/ipc/selection_struct_traits cc/ipc/shared_quad_state_struct_traits cc/ipc/quads_struct_traits cc/ipc/filter_operations_struct_traits cc/ipc/filter_operation_struct_traits ui/gfx/mojo/transform_struct_traits ui/gfx/mojo/selection_bound_struct_traits gpu/ipc/common/mailbox_struct_traits gpu/ipc/common/mailbox_holder_struct_traits gpu/ipc/common/sync_token_struct_traits skia/public/interfaces/image_filter_struct_traits mojo/common/common_custom_types_struct_traits ui/events/mojo/latency_info_struct_traits In total it is 18 struct_traits. Note that these struct_traits themselves are already templates by themselves. But I'm just changing the full templates to partial templates. Two major reasons that I must do this are (+cc yzshen to verify): 1. I need to use compositor_frame.mojom in Blink, and it depends on the other 17 mojom files. 2. Given the current differentiation btw Blink and Chromium in mojo, there is no other way to get around this. If I do not make them partial templates, then the only other way is to add 18 more struct_traits in Blink to specifically do type conversion between Blink mojom types and custom types.
On 2016/07/22 18:09:48, xlai (Olivia) wrote: > On 2016/07/22 17:49:27, dcheng wrote: > > How many more struct traits do we need to do this for? > > > > It is an oversight that the original Read() methods weren't out-of-lined, and > > templating makes that impossible. Are we going to need to template a bunch of > > other struct traits? > > Yes, exactly. > The exhaustive list of struct_traits that I'm going to templatize are: > > cc/ipc/compositor_frame_struct_traits > cc/ipc/compositor_frame_metadata_struct_traits > cc/ipc/render_pass_struct_traits > cc/ipc/render_pass_id_struct_traits > cc/ipc/transferable_resource_struct_traits > cc/ipc/selection_struct_traits > cc/ipc/shared_quad_state_struct_traits > cc/ipc/quads_struct_traits > cc/ipc/filter_operations_struct_traits > cc/ipc/filter_operation_struct_traits > ui/gfx/mojo/transform_struct_traits > ui/gfx/mojo/selection_bound_struct_traits > gpu/ipc/common/mailbox_struct_traits > gpu/ipc/common/mailbox_holder_struct_traits > gpu/ipc/common/sync_token_struct_traits > skia/public/interfaces/image_filter_struct_traits > mojo/common/common_custom_types_struct_traits > ui/events/mojo/latency_info_struct_traits > > In total it is 18 struct_traits. > > Note that these struct_traits themselves are already > templates by themselves. But I'm just changing the full > templates to partial templates. > > Two major reasons that I must do this are (+cc yzshen to verify): > 1. I need to use compositor_frame.mojom in Blink, and it > depends on the other 17 mojom files. > 2. Given the current differentiation btw Blink and Chromium > in mojo, there is no other way to get around this. If > I do not make them partial templates, then the only other > way is to add 18 more struct_traits in Blink to specifically > do type conversion between Blink mojom types and custom > types. Some of these Read methods() are very large and having them inline is going to result in a fair amount of bloat. If it's possible to define these in a .cc even with the partial specialization, then I have no objection. But I don't think it is, in which case, we need to find another solution.
Message was sent while issue was closed.
To tie up the loose threads, this CL is abandoned and closed. Instead, a completely different approach to import compositor_frame typemap to Blink that no longer raises any of the concerns here is done in another CL (https://codereview.chromium.org/2285433002/), after yzshen@ has completed implementation change in mojo to allow Blink and Chromium and share the same DataView (crbug.com/632061). On 2016/07/23 01:43:27, dcheng wrote: > On 2016/07/22 18:09:48, xlai (Olivia) wrote: > > On 2016/07/22 17:49:27, dcheng wrote: > > > How many more struct traits do we need to do this for? > > > > > > It is an oversight that the original Read() methods weren't out-of-lined, > and > > > templating makes that impossible. Are we going to need to template a bunch > of > > > other struct traits? > > > > Yes, exactly. > > The exhaustive list of struct_traits that I'm going to templatize are: > > > > cc/ipc/compositor_frame_struct_traits > > cc/ipc/compositor_frame_metadata_struct_traits > > cc/ipc/render_pass_struct_traits > > cc/ipc/render_pass_id_struct_traits > > cc/ipc/transferable_resource_struct_traits > > cc/ipc/selection_struct_traits > > cc/ipc/shared_quad_state_struct_traits > > cc/ipc/quads_struct_traits > > cc/ipc/filter_operations_struct_traits > > cc/ipc/filter_operation_struct_traits > > ui/gfx/mojo/transform_struct_traits > > ui/gfx/mojo/selection_bound_struct_traits > > gpu/ipc/common/mailbox_struct_traits > > gpu/ipc/common/mailbox_holder_struct_traits > > gpu/ipc/common/sync_token_struct_traits > > skia/public/interfaces/image_filter_struct_traits > > mojo/common/common_custom_types_struct_traits > > ui/events/mojo/latency_info_struct_traits > > > > In total it is 18 struct_traits. > > > > Note that these struct_traits themselves are already > > templates by themselves. But I'm just changing the full > > templates to partial templates. > > > > Two major reasons that I must do this are (+cc yzshen to verify): > > 1. I need to use compositor_frame.mojom in Blink, and it > > depends on the other 17 mojom files. > > 2. Given the current differentiation btw Blink and Chromium > > in mojo, there is no other way to get around this. If > > I do not make them partial templates, then the only other > > way is to add 18 more struct_traits in Blink to specifically > > do type conversion between Blink mojom types and custom > > types. > > Some of these Read methods() are very large and having them inline is going to > result in a fair amount of bloat. If it's possible to define these in a .cc even > with the partial specialization, then I have no objection. But I don't think it > is, in which case, we need to find another solution. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
