bluetooth: Introduce Web Bluetooth mojom interface

bluetooth: Introduce Web Bluetooth mojom interface

This is the first step to moving Web Bluetooth over to mojo. This patch
does the following:

- Creates a new WebBluetoothError mojo enum in a new web_bluetooth.mojom.
  This enum should be accessible from both content and blink.
- Modifies WebKit/public/BUILD.gn and WebKit/public/blink.gyp to
  generate the headers for the new mojom interface.
- Modifies WebKit/public/BUILD.gn, WebKit/Source/BUILD.gn,
  WebKit/public/blink_headers.gyp, and WebKit/Source/config.gyp to
  include files in gen/third_party/WebKit
- Modifies content/browser/DEPS so that the new headers can be accessed
  from content/browser.

Follow up patch with Web Bluetooth Service: http://crrev.com/1775953004

BUG=508771
Committed: https://crrev.com/b160cb7e2eab15906113b1119adb8841498f75b5
Cr-Commit-Position: refs/heads/master@{#381525}

[ortuno, 2016-03-09 22:13:31]

Patchset #1 (id:1) has been deleted

[ortuno, 2016-03-09 22:14:04]

ortuno@chromium.org changed reviewers:
+ rockot@chromium.org

[ortuno, 2016-03-09 22:14:05]

rockot: PTAL

[ortuno, 2016-03-09 23:10:28]

Description was changed from

==========
bluetooth: Introduce Web Bluetooth mojom interface

This is the first step to moving Web Bluetooth over to mojo. This patch
does the following:

- Creates a new WebBluetoothError mojo enum in a new web_bluetooth.mojom.
  This enum should be accessible from both content and blink.
- Modifies WebKit/public/BUILD.gn and WebKit/public/blink.gyp to
  generate the headers for the new mojom interface.
- Modifies WebKit/public/BUILD.gn, WebKit/Source/BUILD.gn,
  WebKit/public/blink_headers.gyp, and WebKit/Source/config.gyp to
  include files in gen/third_party/WebKit
- Modifies content/browser/DEPS so that the new headers can be accessed
  from content/browser.

BUG=508771
==========

to

==========
bluetooth: Introduce Web Bluetooth mojom interface

This is the first step to moving Web Bluetooth over to mojo. This patch
does the following:

- Creates a new WebBluetoothError mojo enum in a new web_bluetooth.mojom.
  This enum should be accessible from both content and blink.
- Modifies WebKit/public/BUILD.gn and WebKit/public/blink.gyp to
  generate the headers for the new mojom interface.
- Modifies WebKit/public/BUILD.gn, WebKit/Source/BUILD.gn,
  WebKit/public/blink_headers.gyp, and WebKit/Source/config.gyp to
  include files in gen/third_party/WebKit
- Modifies content/browser/DEPS so that the new headers can be accessed
  from content/browser.

Follow up patch with Web Bluetooth Service: http://crrev.com/1775953004

BUG=508771
==========

[Ken Rockot(use gerrit already), 2016-03-10 02:41:16]

https://codereview.chromium.org/1784613002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/blink.gyp (right):

https://codereview.chromium.org/1784613002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/blink.gyp:85: 'target_name': 'mojo_bindings',
i don't think we want just a single target for all mojo bindings. they should be
separated per module or something. a blink owner should weigh in on this though

[ortuno, 2016-03-10 04:02:04]

ortuno@chromium.org changed reviewers:
+ darin@chromium.org, esprehn@chromium.org, jam@chromium.org

[ortuno, 2016-03-10 04:02:05]

Hi!

Web Bluetooth is starting to move from IPC::Message to Mojo. As a first step we
are just adding a new mojom file with a single enum. Eventually this mojom file
will hold the interface for our Web Bluetooth Service (You can see how that will
look like in the follow up patch: http://crrev.com/1775953004). The generated
headers should be accessible from both content and blink, so that we can use the
enum and eventually implement the Mojo Service in content/browser. I talked to
rockot and he mentioned that third_party/WebKit/public/platform should be the
place to add the .mojom file, so this patch adds the .mojom file there and
modifies any BUILD files and gyp files so that the headers are accessible from
blink and content. What do you all think about this? Is public/platform the
place to put .mojom files?

[esprehn, 2016-03-11 07:00:38]

This seems fine to me, this does mean the browser process would include the
interface defs from third_party/WebKit/public/platform/modules/bluetooth, I
guess that's okay?

https://codereview.chromium.org/1784613002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/blink.gyp (right):

https://codereview.chromium.org/1784613002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/blink.gyp:85: 'target_name': 'mojo_bindings',
On 2016/03/10 at 02:41:16, Ken Rockot wrote:
> i don't think we want just a single target for all mojo bindings. they should
be separated per module or something. a blink owner should weigh in on this
though

Yeah we can have a mega target, but it should be made up multiple smaller
targets probably.

[Ken Rockot(use gerrit already), 2016-03-14 14:13:51]

On 2016/03/11 at 07:00:38, esprehn wrote:
> This seems fine to me, this does mean the browser process would include the
interface defs from third_party/WebKit/public/platform/modules/bluetooth, I
guess that's okay?

Yeah I think that's going to be a necessary change. In general there shouldn't
need to be any restrictions on what code is allowed to depend on mojom in
public/platform.

Ideally we could add a DEPS rule to //content like
"+third_party/WebKit/public/platform/**.mojom.h" but that's not supported. Maybe
we should have a common root for all the mojom in Blink then? Something like
third_party/WebKit/public/mojom, and you'd have
public/mojom/bluetooth/foo.mojom, etc.

> 
>
https://codereview.chromium.org/1784613002/diff/40001/third_party/WebKit/publ...
> File third_party/WebKit/public/blink.gyp (right):
> 
>
https://codereview.chromium.org/1784613002/diff/40001/third_party/WebKit/publ...
> third_party/WebKit/public/blink.gyp:85: 'target_name': 'mojo_bindings',
> On 2016/03/10 at 02:41:16, Ken Rockot wrote:
> > i don't think we want just a single target for all mojo bindings. they
should be separated per module or something. a blink owner should weigh in on
this though
> 
> Yeah we can have a mega target, but it should be made up multiple smaller
targets probably.

[jam, 2016-03-14 16:35:24]

lgtm

On 2016/03/14 14:13:51, Ken Rockot wrote:
> On 2016/03/11 at 07:00:38, esprehn wrote:
> > This seems fine to me, this does mean the browser process would include the
> interface defs from third_party/WebKit/public/platform/modules/bluetooth, I
> guess that's okay?
> 
> Yeah I think that's going to be a necessary change. In general there shouldn't
> need to be any restrictions on what code is allowed to depend on mojom in
> public/platform.

+1

> 
> Ideally we could add a DEPS rule to //content like
> "+third_party/WebKit/public/platform/**.mojom.h" but that's not supported.
Maybe
> we should have a common root for all the mojom in Blink then? Something like
> third_party/WebKit/public/mojom, and you'd have
> public/mojom/bluetooth/foo.mojom, etc.

it seems better, code location wise, to keep the mojom in the same directory as
the other public bluetooth files (and same for other features). imo we shouldn't
make finding files harder because of deficiencies in our tooling (checkdeps), so
we should just improve that. in the meantime, adding individual generated files
in DEPS like this cl does is the best solution.

> 
> > 
> >
>
https://codereview.chromium.org/1784613002/diff/40001/third_party/WebKit/publ...
> > File third_party/WebKit/public/blink.gyp (right):
> > 
> >
>
https://codereview.chromium.org/1784613002/diff/40001/third_party/WebKit/publ...
> > third_party/WebKit/public/blink.gyp:85: 'target_name': 'mojo_bindings',
> > On 2016/03/10 at 02:41:16, Ken Rockot wrote:
> > > i don't think we want just a single target for all mojo bindings. they
> should be separated per module or something. a blink owner should weigh in on
> this though
> > 
> > Yeah we can have a mega target, but it should be made up multiple smaller
> targets probably.

https://codereview.chromium.org/1784613002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/blink.gyp (right):

https://codereview.chromium.org/1784613002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/blink.gyp:85: 'target_name': 'mojo_bindings',
On 2016/03/11 07:00:38, esprehn wrote:
> On 2016/03/10 at 02:41:16, Ken Rockot wrote:
> > i don't think we want just a single target for all mojo bindings.

in practice, why is one target not good enough?
> they should
> be separated per module or something. a blink owner should weigh in on this
> though
> 
> Yeah we can have a mega target, but it should be made up multiple smaller
> targets probably.

[Ken Rockot(use gerrit already), 2016-03-14 17:14:24]

On Mon, Mar 14, 2016 at 9:35 AM, <jam@chromium.org> wrote:

> lgtm
>
> On 2016/03/14 14:13:51, Ken Rockot wrote:
> > On 2016/03/11 at 07:00:38, esprehn wrote:
> > > This seems fine to me, this does mean the browser process would
> include the
> > interface defs from
> third_party/WebKit/public/platform/modules/bluetooth, I
> > guess that's okay?
> >
> > Yeah I think that's going to be a necessary change. In general there
> shouldn't
> > need to be any restrictions on what code is allowed to depend on mojom in
> > public/platform.
>
> +1
>
> >
> > Ideally we could add a DEPS rule to //content like
> > "+third_party/WebKit/public/platform/**.mojom.h" but that's not
> supported.
> Maybe
> > we should have a common root for all the mojom in Blink then? Something
> like
> > third_party/WebKit/public/mojom, and you'd have
> > public/mojom/bluetooth/foo.mojom, etc.
>
> it seems better, code location wise, to keep the mojom in the same
> directory as
> the other public bluetooth files (and same for other features). imo we
> shouldn't
> make finding files harder because of deficiencies in our tooling
> (checkdeps), so
> we should just improve that. in the meantime, adding individual generated
> files
> in DEPS like this cl does is the best solution.
>

SGTM


>
>
> >
> > >
> > >
> >
>
>
https://codereview.chromium.org/1784613002/diff/40001/third_party/WebKit/publ...
> > > File third_party/WebKit/public/blink.gyp (right):
> > >
> > >
> >
>
>
https://codereview.chromium.org/1784613002/diff/40001/third_party/WebKit/publ...
> > > third_party/WebKit/public/blink.gyp:85: 'target_name': 'mojo_bindings',
> > > On 2016/03/10 at 02:41:16, Ken Rockot wrote:
> > > > i don't think we want just a single target for all mojo bindings.
> they
> > should be separated per module or something. a blink owner should weigh
> in on
> > this though
> > >
> > > Yeah we can have a mega target, but it should be made up multiple
> smaller
> > targets probably.
>
>
>
>
>
https://codereview.chromium.org/1784613002/diff/40001/third_party/WebKit/publ...
> File third_party/WebKit/public/blink.gyp (right):
>
>
>
https://codereview.chromium.org/1784613002/diff/40001/third_party/WebKit/publ...
> third_party/WebKit/public/blink.gyp:85: 'target_name': 'mojo_bindings',
> On 2016/03/11 07:00:38, esprehn wrote:
> > On 2016/03/10 at 02:41:16, Ken Rockot wrote:
> > > i don't think we want just a single target for all mojo bindings.
>
> in practice, why is one target not good enough?
>

Because it means that any component which implements any Blink mojom
interface will need to generate bindings for every Blink mojom interface.
In practice this is fine for content because content is a monolithic client
implementation, but I don't see much disadvantage to maintaining more
granular mojom targets. It would be sad (IMHO) if for example the
device/bluetooth unittest suite had a dependency on all Blink mojom.


>
> > they should
> > be separated per module or something. a blink owner should weigh in on
> this
> > though
> >
> > Yeah we can have a mega target, but it should be made up multiple
> smaller
> > targets probably.
>
> https://codereview.chromium.org/1784613002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-03-14 17:14:24]

On Mon, Mar 14, 2016 at 9:35 AM, <jam@chromium.org> wrote:

> lgtm
>
> On 2016/03/14 14:13:51, Ken Rockot wrote:
> > On 2016/03/11 at 07:00:38, esprehn wrote:
> > > This seems fine to me, this does mean the browser process would
> include the
> > interface defs from
> third_party/WebKit/public/platform/modules/bluetooth, I
> > guess that's okay?
> >
> > Yeah I think that's going to be a necessary change. In general there
> shouldn't
> > need to be any restrictions on what code is allowed to depend on mojom in
> > public/platform.
>
> +1
>
> >
> > Ideally we could add a DEPS rule to //content like
> > "+third_party/WebKit/public/platform/**.mojom.h" but that's not
> supported.
> Maybe
> > we should have a common root for all the mojom in Blink then? Something
> like
> > third_party/WebKit/public/mojom, and you'd have
> > public/mojom/bluetooth/foo.mojom, etc.
>
> it seems better, code location wise, to keep the mojom in the same
> directory as
> the other public bluetooth files (and same for other features). imo we
> shouldn't
> make finding files harder because of deficiencies in our tooling
> (checkdeps), so
> we should just improve that. in the meantime, adding individual generated
> files
> in DEPS like this cl does is the best solution.
>

SGTM


>
>
> >
> > >
> > >
> >
>
>
https://codereview.chromium.org/1784613002/diff/40001/third_party/WebKit/publ...
> > > File third_party/WebKit/public/blink.gyp (right):
> > >
> > >
> >
>
>
https://codereview.chromium.org/1784613002/diff/40001/third_party/WebKit/publ...
> > > third_party/WebKit/public/blink.gyp:85: 'target_name': 'mojo_bindings',
> > > On 2016/03/10 at 02:41:16, Ken Rockot wrote:
> > > > i don't think we want just a single target for all mojo bindings.
> they
> > should be separated per module or something. a blink owner should weigh
> in on
> > this though
> > >
> > > Yeah we can have a mega target, but it should be made up multiple
> smaller
> > targets probably.
>
>
>
>
>
https://codereview.chromium.org/1784613002/diff/40001/third_party/WebKit/publ...
> File third_party/WebKit/public/blink.gyp (right):
>
>
>
https://codereview.chromium.org/1784613002/diff/40001/third_party/WebKit/publ...
> third_party/WebKit/public/blink.gyp:85: 'target_name': 'mojo_bindings',
> On 2016/03/11 07:00:38, esprehn wrote:
> > On 2016/03/10 at 02:41:16, Ken Rockot wrote:
> > > i don't think we want just a single target for all mojo bindings.
>
> in practice, why is one target not good enough?
>

Because it means that any component which implements any Blink mojom
interface will need to generate bindings for every Blink mojom interface.
In practice this is fine for content because content is a monolithic client
implementation, but I don't see much disadvantage to maintaining more
granular mojom targets. It would be sad (IMHO) if for example the
device/bluetooth unittest suite had a dependency on all Blink mojom.


>
> > they should
> > be separated per module or something. a blink owner should weigh in on
> this
> > though
> >
> > Yeah we can have a mega target, but it should be made up multiple
> smaller
> > targets probably.
>
> https://codereview.chromium.org/1784613002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2016-03-15 23:39:14]

lgtm

[Ken Rockot(use gerrit already), 2016-03-16 15:08:05]

lgtm

[Ken Rockot(use gerrit already), 2016-03-16 15:14:07]

btw we have been discussing the right home for mojom. around and around we go,
but i am still advocating for not putting the mojom in blink. it's OK for now
though because there is still not a consensus here :)

[ortuno, 2016-03-16 17:33:51]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-03-16 17:33:51]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
esprehn@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1784613002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-16 17:34:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1784613002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1784613002/100001

[commit-bot: I haz the power, 2016-03-16 20:44:44]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-03-16 20:46:28]

Description was changed from

==========
bluetooth: Introduce Web Bluetooth mojom interface

This is the first step to moving Web Bluetooth over to mojo. This patch
does the following:

- Creates a new WebBluetoothError mojo enum in a new web_bluetooth.mojom.
  This enum should be accessible from both content and blink.
- Modifies WebKit/public/BUILD.gn and WebKit/public/blink.gyp to
  generate the headers for the new mojom interface.
- Modifies WebKit/public/BUILD.gn, WebKit/Source/BUILD.gn,
  WebKit/public/blink_headers.gyp, and WebKit/Source/config.gyp to
  include files in gen/third_party/WebKit
- Modifies content/browser/DEPS so that the new headers can be accessed
  from content/browser.

Follow up patch with Web Bluetooth Service: http://crrev.com/1775953004

BUG=508771
==========

to

==========
bluetooth: Introduce Web Bluetooth mojom interface

This is the first step to moving Web Bluetooth over to mojo. This patch
does the following:

- Creates a new WebBluetoothError mojo enum in a new web_bluetooth.mojom.
  This enum should be accessible from both content and blink.
- Modifies WebKit/public/BUILD.gn and WebKit/public/blink.gyp to
  generate the headers for the new mojom interface.
- Modifies WebKit/public/BUILD.gn, WebKit/Source/BUILD.gn,
  WebKit/public/blink_headers.gyp, and WebKit/Source/config.gyp to
  include files in gen/third_party/WebKit
- Modifies content/browser/DEPS so that the new headers can be accessed
  from content/browser.

Follow up patch with Web Bluetooth Service: http://crrev.com/1775953004

BUG=508771

Committed: https://crrev.com/b160cb7e2eab15906113b1119adb8841498f75b5
Cr-Commit-Position: refs/heads/master@{#381525}
==========

[commit-bot: I haz the power, 2016-03-16 20:46:29]

Patchset 5 (id:??) landed as
https://crrev.com/b160cb7e2eab15906113b1119adb8841498f75b5
Cr-Commit-Position: refs/heads/master@{#381525}