Bluetooth should use blink style bindings for mojo |
||||||||||
Issue descriptionhttps://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothError.h&q=file:webkit%20%22.mojom.h%22%20-file:gen%20lang:cpp&sq=package:chromium&dr=C&l=8 this should use the -blink.h bindings, not the std::string/std::array bindings.
,
May 9 2016
,
May 9 2016
,
May 11 2016
,
May 13 2016
We can't change to the blink variant. We need to be able to use the same enums in Sources/modules, content/renderer and content/browser. The blink variant generates enums in a different namespace.
,
May 13 2016
Lots of people are likely to run into the enum issue, so we should get the blink variant fixed rather than avoiding it. Let's file a bug against Mojo and block this on that?
,
May 13 2016
I don't think we should go out of our way to support this. The only viable option I can think of would be to generate separate (non-variant) headers for enums, so you'd have foo.mojom-enums.h which could be included anywhere. This would be unfortunate IMHO. Why do you need to use the same enum types in Source/modules and content? If it's only because you have this transitional phase where some but not all code has moved from content/renderer to Source/modules, wouldn't a reasonable solution be to let content/renderer include the both the mojom.h and the mojom-blink.h, and just static_cast where needed?
,
May 13 2016
Agreed with rockot's comment.
,
May 13 2016
I think it's acceptable to static_cast during an interim period, although it's inelegant. Gio might have other concrete problems in mind.
,
May 13 2016
I'm OK with static_cast. There are going to be a lot of them though :)
,
May 13 2016
Well, looks like including a blink variant header in chromium is a non-option: these of course pull in wtf dependencies, and require additional include paths which we should definitely not leak into content. One compromise would be for us to generate an additional foo.mojom-blink-enums.h header that chromium could include in cases such as this. But it's such a rare thing, and because this state should only be transient, I still would like to avoid adding too much builtin cruft to accommodate it. Talked with ortuno@ offline and one palatable option may be to just use an int32 alias at the blink/content API boundary and static_cast to a mojom enum on either side.
,
May 13 2016
Would there be other benefits from having a foo.mojom-common.h header? e.g. are there functions whose code can be shared between the two interfaces? I'm more reluctant to pass things through an int32 because it's harder to see locally that the static_cast is correct.
,
May 31 2016
What's the progress here? :)
,
Jun 1 2016
Is the static_cast an acceptable solution?
,
Jun 1 2016
*temporary solution. We'll remove it once we onion-soup web bluetooth.
,
Jun 1 2016
Seems fine. :)
,
Jun 1 2016
esprehn: If we include mojom-blink.h in WebBluetoothError like you suggest in Comment #1 then content/renderer, which includes WebBluetoothError, will indirectly include WTF types, which I believe is a layer violation. Is that OK?
,
Jun 1 2016
Ah yeah, I don't think you can do that. What do you suggest rockot@ ? :)
,
Jun 1 2016
Pretty sure WTF headers require additional include path(s) anyway which we don't want to introduce in content target configs, so it's not really possible even if we could rationalize it.
,
Jun 1 2016
I think rockot's solution still applies. We just have to get rid of WebBluetoothError and use an int32 when talking to the platform layer. FWIW I don't think the change is worth it. We are exchanging one bad include for no type safety and a bunch of static casts on basically all our functions. I think the best thing to do is to wait until we onion-soup Web Bluetooth; there is a presubmit check already so this shouldn't affect anyone.
,
Jun 1 2016
+1 #20 "FWIW I don't think the change is worth it. ..."
,
Jun 23 2016
Renaming Blink>Architecture to Blink>Internals
,
Jul 13 2016
,
Jul 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4928ab440dfc16789b8ec0d9c52717b78a41abb8 commit 4928ab440dfc16789b8ec0d9c52717b78a41abb8 Author: ortuno <ortuno@chromium.org> Date: Thu Jul 14 19:11:18 2016 bluetooth: Avoid including non-blink mojo bindings in blink code Blink should only use the blink variant of mojo bindings. Unfortunately, this means we can no longer use the same enum across content/renderer and blink. To get around this, the embedder interface (WebBluetooth) uses int32 instead of the enums and we cast to enum where appropriate. Also removes unnecessary dependencies on mojo bindings. BUG= 610415 Review-Url: https://codereview.chromium.org/2142813003 Cr-Commit-Position: refs/heads/master@{#405531} [modify] https://crrev.com/4928ab440dfc16789b8ec0d9c52717b78a41abb8/content/common/DEPS [modify] https://crrev.com/4928ab440dfc16789b8ec0d9c52717b78a41abb8/content/renderer/bluetooth/web_bluetooth_impl.cc [modify] https://crrev.com/4928ab440dfc16789b8ec0d9c52717b78a41abb8/content/renderer/bluetooth/web_bluetooth_impl.h [modify] https://crrev.com/4928ab440dfc16789b8ec0d9c52717b78a41abb8/device/vr/BUILD.gn [modify] https://crrev.com/4928ab440dfc16789b8ec0d9c52717b78a41abb8/media/blink/BUILD.gn [modify] https://crrev.com/4928ab440dfc16789b8ec0d9c52717b78a41abb8/third_party/WebKit/Source/modules/BUILD.gn [modify] https://crrev.com/4928ab440dfc16789b8ec0d9c52717b78a41abb8/third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp [modify] https://crrev.com/4928ab440dfc16789b8ec0d9c52717b78a41abb8/third_party/WebKit/Source/modules/bluetooth/BluetoothError.h [modify] https://crrev.com/4928ab440dfc16789b8ec0d9c52717b78a41abb8/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp [modify] https://crrev.com/4928ab440dfc16789b8ec0d9c52717b78a41abb8/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp [modify] https://crrev.com/4928ab440dfc16789b8ec0d9c52717b78a41abb8/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.h [modify] https://crrev.com/4928ab440dfc16789b8ec0d9c52717b78a41abb8/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp [modify] https://crrev.com/4928ab440dfc16789b8ec0d9c52717b78a41abb8/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.h [modify] https://crrev.com/4928ab440dfc16789b8ec0d9c52717b78a41abb8/third_party/WebKit/public/BUILD.gn [modify] https://crrev.com/4928ab440dfc16789b8ec0d9c52717b78a41abb8/third_party/WebKit/public/blink_headers.gypi [modify] https://crrev.com/4928ab440dfc16789b8ec0d9c52717b78a41abb8/third_party/WebKit/public/platform/modules/bluetooth/WebBluetooth.h [delete] https://crrev.com/615df311feca979548acd4f73ce3d7ac34449e3a/third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothError.h
,
Jul 14 2016
|
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by esprehn@chromium.org
, May 9 2016