Closed Bug 690670 Opened 13 years ago Closed 13 years ago

Create a place to put device interaction code

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: cjones, Assigned: cjones)

References

Details

(Whiteboard: [inbound])

Attachments

(4 files)

We're soon going to be inundated with platform-specific code that talks to hardware: usb, bluetooth, nfc, camera, joystick, vibrator, ...  Where would you put that code in m-c?  There's not really a good place.  Closest design-wise is widget/, but that's all wrong for device code.  Next up is xpcom/system+dom/system, but those interfaces are XPCOM-y, not particularly OOP friendly, and have a steep learning curve for non-gecko devs.

So I propose we make a new area.  I'm calling it hal/.  The API will be Hal.h  (Let the bikeshedding begin!)  XPCOM stops at the hal/ boundary.  APIs added to hal will require sandbox (content process) and fallback impls out of the box or they won't compile.  The goal is to give a clean interface to HW to DOM code, and a strong API boundary under which implementors can write platform-specific impls but don't have to know XPCOM/DOM.
roc, pinging you for this since you might be working here with WebRTC, and own widget/ which has the design problems nearest to hal.  Please feel free to kick this to someone else.

I'm not particularly happy with the macro goop but I don't have a better idea for maintaining a uniform API.  The rationale for having separate hal/hal_sandbox/hal_impl namespaces is that we can have bare functions defined with the same name and signature in all three, and callable by any process.  The alternative is a single C++ "interface class" with multiple singletons, called across the various backends as needed.  Since we'll need to mix-'n-match implementations across various platforms, that doesn't feel as clean to me, since we'd need an "interface" per separable API, with a bunch more singletons.

But if anyone has better ideas, let me know.
Attachment #563671 - Flags: superreview?(roc)
Comment on attachment 563671 [details] [diff] [review]
part 1: Add a hal/ directory in which we can add device-interaction code

Review of attachment 563671 [details] [diff] [review]:
-----------------------------------------------------------------

This mostly seems reasonable.

Do we really want to stick everything as functions in the Hal:: namespace? Does that scale? How are we going to allow backend selection? Will it be compile-time or run-time?

::: hal/Hal.cpp
@@ +61,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +}
> +
> +static bool
> +inSandbox()

InSandbox

::: hal/sandbox/SandboxHal.cpp
@@ +47,5 @@
> +using namespace mozilla::dom;
> +using namespace mozilla::hal;
> +
> +namespace mozilla {
> +namespace hal_sandbox {

We don't use _ spacing elsewhere. Would halsandbox or hal::sandbox be OK?

@@ +60,5 @@
> +    return sHal;
> +}
> +
> +void
> +vibrate(const nsTArray<uint32>& pattern)

Capital V?
Attachment #563669 - Flags: review?(bent.mozilla) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> Comment on attachment 563671 [details] [diff] [review] [diff] [details] [review]
> part 1: Add a hal/ directory in which we can add device-interaction code
> 
> Review of attachment 563671 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> This mostly seems reasonable.
> 
> Do we really want to stick everything as functions in the Hal:: namespace?

All the entry points would be in hal::.  Lots of code would be external classes accessed through hal, e.g. Joystick or UsbDevice, defined in "Joystick.h", say.  The entry point to get those individual objects would be an enumerate-kind of API.  The nuclear option is to expose Managers or Services.

> Does that scale? 

In what sense?

> How are we going to allow backend selection? Will it be
> compile-time or run-time?
> 

I expect it to be all compile time.  If we need to, we can add runtime selection for special cases.  The idea is to make the simple cases simple.  If complex cases need to use a service pattern with dynamic selection, they certainly can.

To mix 'n match implementations across platforms, we can have one file per separable API and select the file set at compile time, like libc does.

> @@ +47,5 @@
> > +using namespace mozilla::dom;
> > +using namespace mozilla::hal;
> > +
> > +namespace mozilla {
> > +namespace hal_sandbox {
> 
> We don't use _ spacing elsewhere. Would halsandbox or hal::sandbox be OK?
> 

"halsandbox" doesn't scan well for me.  "hal::sandbox" would be hard to implement because of the macro-ized API generation.  FWIW C++11 defines multi-word namespaces like_that, and these namespaces are interal to hal.  How much does it matter to you?

> @@ +60,5 @@
> > +    return sHal;
> > +}
> > +
> > +void
> > +vibrate(const nsTArray<uint32>& pattern)
> 
> Capital V?

Back the old js/src vs. gecko style debate ... I really hate there's not a clear choice when writing new code.  Since this is going to interact more closely with gecko than mfbt and you'll probably be sr'ing, I can go with gecko style.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> All the entry points would be in hal::.  Lots of code would be external
> classes accessed through hal, e.g. Joystick or UsbDevice, defined in
> "Joystick.h", say.  The entry point to get those individual objects would be
> an enumerate-kind of API.  The nuclear option is to expose Managers or
> Services.

So those objects will need to do their own IPC forwarding internally? OK.

> > Does that scale? 
> 
> In what sense?

Is "hal" going to become an absolutely enormous namespace full of stuff in one header file, and is that going to be a problem?

Probably not, but just asking.

> > @@ +47,5 @@
> > > +using namespace mozilla::dom;
> > > +using namespace mozilla::hal;
> > > +
> > > +namespace mozilla {
> > > +namespace hal_sandbox {
> > 
> > We don't use _ spacing elsewhere. Would halsandbox or hal::sandbox be OK?
> > 
> 
> "halsandbox" doesn't scan well for me.  "hal::sandbox" would be hard to
> implement because of the macro-ized API generation.  FWIW C++11 defines
> multi-word namespaces like_that, and these namespaces are interal to hal. 
> How much does it matter to you?

Not much. Don't worry about it.

> > @@ +60,5 @@
> > > +    return sHal;
> > > +}
> > > +
> > > +void
> > > +vibrate(const nsTArray<uint32>& pattern)
> > 
> > Capital V?
> 
> Back the old js/src vs. gecko style debate ... I really hate there's not a
> clear choice when writing new code.  Since this is going to interact more
> closely with gecko than mfbt and you'll probably be sr'ing, I can go with
> gecko style.

Good.
Comment on attachment 563671 [details] [diff] [review]
part 1: Add a hal/ directory in which we can add device-interaction code

Review of attachment 563671 [details] [diff] [review]:
-----------------------------------------------------------------

sr+ with the capitalization fix
Attachment #563671 - Flags: superreview?(roc) → superreview+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> > All the entry points would be in hal::.  Lots of code would be external
> > classes accessed through hal, e.g. Joystick or UsbDevice, defined in
> > "Joystick.h", say.  The entry point to get those individual objects would be
> > an enumerate-kind of API.  The nuclear option is to expose Managers or
> > Services.
> 
> So those objects will need to do their own IPC forwarding internally? OK.
> 

The protocol and "sandbox"-side implementation will be common across all platforms.  Each platform would need some "impl"-side code to IPC through the common interface, yes.

> > > Does that scale? 
> > 
> > In what sense?
> 
> Is "hal" going to become an absolutely enormous namespace full of stuff in
> one header file, and is that going to be a problem?
> 
> Probably not, but just asking.
> 

It won't be windows.h, but it might not be on the small side either.  I wouldn't expect it to be much larger or more complex than nsIWidget.h.
Comment on attachment 563671 [details] [diff] [review]
part 1: Add a hal/ directory in which we can add device-interaction code

Review of attachment 563671 [details] [diff] [review]:
-----------------------------------------------------------------

I've been updating my Battery API patch to apply on top of those, I have a few comments much closer to nits than anything else.

::: hal/sandbox/PHal.ipdl
@@ +1,3 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> + * vim: sw=2 ts=8 et :
> + */

You broke syntax coloration with vim. I think that should be:
+/* -*- Mode: C++; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 8 -*- */
+/* vim: set sw=4 ts=8 et tw=80 ft=cpp : */

::: hal/sandbox/SandboxHal.cpp
@@ +63,5 @@
> +void
> +vibrate(const nsTArray<uint32>& pattern)
> +{
> +    AutoInfallibleTArray<uint32, 8> p(pattern);
> +    Hal()->SendVibrate(p);

Hal() and vibrate() methods are using 4 spaces indentation while the rest of the file is using 2 spaces indentation.

@@ +72,5 @@
> +  NS_OVERRIDE virtual bool
> +  RecvVibrate(const InfallibleTArray<unsigned int>& pattern) {
> +    // Forward to hal::, not hal_impl::, because we might be a
> +    // subprocess of another sandboxed process.  The hal:: entry point
> +    // will do the right thing.

I don't think that comment should be here given that it would apply to all other methods added to HalParent, right?

@@ +73,5 @@
> +  RecvVibrate(const InfallibleTArray<unsigned int>& pattern) {
> +    // Forward to hal::, not hal_impl::, because we might be a
> +    // subprocess of another sandboxed process.  The hal:: entry point
> +    // will do the right thing.
> +    hal::vibrate(pattern);

Should be Vibrate. But I think roc already pointed that.
Blocks: 678694
> > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> > + * vim: sw=2 ts=8 et :
> > + */
> 
> You broke syntax coloration with vim. I think that should be:
> +/* -*- Mode: C++; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 8
> -*- */
> +/* vim: set sw=4 ts=8 et tw=80 ft=cpp : */

Hm ... I use those modelines all over the place, and in fact they're part of an emacs template I apply (http://blog.mozilla.com/cjones/2010/03/24/save-yourself-some-license-template-copypasta-in-emacs/).  No one has complained before AFAIK.  What's the syntax error?

BTW, your modelines specify 4-space indent.

> 
> ::: hal/sandbox/SandboxHal.cpp
> @@ +63,5 @@
> > +void
> > +vibrate(const nsTArray<uint32>& pattern)
> > +{
> > +    AutoInfallibleTArray<uint32, 8> p(pattern);
> > +    Hal()->SendVibrate(p);
> 
> Hal() and vibrate() methods are using 4 spaces indentation while the rest of
> the file is using 2 spaces indentation.
> 

Yeah, that's an emacs quirk with new files.  Will fix, thanks.

> @@ +72,5 @@
> > +  NS_OVERRIDE virtual bool
> > +  RecvVibrate(const InfallibleTArray<unsigned int>& pattern) {
> > +    // Forward to hal::, not hal_impl::, because we might be a
> > +    // subprocess of another sandboxed process.  The hal:: entry point
> > +    // will do the right thing.
> 
> I don't think that comment should be here given that it would apply to all
> other methods added to HalParent, right?
> 

Maybe, maybe not, depends on the API.  Where would you put it instead, in the current patch?
I guess missing "ft=cpp"?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> Created attachment 563669 [details] [diff] [review] [diff] [details] [review]
> part 0.1: Teach IPDL about uint* types, since those are what we have to use
> in "public" interfaces

Since when? uint* types are a js/src/-ism; Gecko uses PR* types.
"Public" meaning, "part of an external embedding interface".  We can't use stdint types in externally-used headers because lots of other code has stdint hacks for windows too.  js/src is setting the style here.
Sad. Was there any discussion?
Depends on: 692782
(In reply to Chris Jones [:cjones] [:warhammer] from comment #11)
> > > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> > > + * vim: sw=2 ts=8 et :
> > > + */
> > 
> > You broke syntax coloration with vim. I think that should be:
> > +/* -*- Mode: C++; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 8
> > -*- */
> > +/* vim: set sw=4 ts=8 et tw=80 ft=cpp : */

> BTW, your modelines specify 4-space indent.

IPDL files seem to use 4-space indent.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #12)
> I guess missing "ft=cpp"?

Maybe, yes.
You need to log in before you can comment on or make changes to this bug.