New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 610415 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jul 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 508771
issue 610421



Sign in to add a comment

Bluetooth should use blink style bindings for mojo

Project Member Reported by esprehn@chromium.org, May 9 2016

Issue description

Labels: -Pri-3 Pri-2
Owner: ortuno@chromium.org
Status: Assigned (was: Untriaged)
Blocking: 610421

Comment 4 by ortuno@chromium.org, May 11 2016

Blocking: 508771

Comment 5 by ortuno@chromium.org, May 13 2016

Cc: esprehn@chromium.org
Status: WontFix (was: Assigned)
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.
Cc: roc...@chromium.org ortuno@chromium.org
Owner: ----
Status: Available (was: WontFix)
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?

Comment 7 by roc...@chromium.org, May 13 2016

Cc: yzshen@chromium.org
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?

Comment 8 by yzshen@chromium.org, May 13 2016

Agreed with rockot's comment.
I think it's acceptable to static_cast during an interim period, although it's inelegant. Gio might have other concrete problems in mind.
I'm OK with static_cast. There are going to be a lot of them though :)
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.
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.
What's the progress here? :)
Is the static_cast an acceptable solution?
*temporary solution. We'll remove it once we onion-soup web bluetooth.
Seems fine. :)
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?
Ah yeah, I don't think you can do that. What do you suggest rockot@ ? :)
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.
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.
+1 #20 "FWIW I don't think the change is worth it. ..."

Comment 22 by tkent@chromium.org, Jun 23 2016

Components: -Blink>Architecture Blink>Internals
Renaming Blink>Architecture to Blink>Internals

Cc: -ortuno@chromium.org
Owner: ortuno@chromium.org
Status: Started (was: Available)
Project Member

Comment 24 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)

Sign in to add a comment