Last Comment Bug 690670 - Create a place to put device interaction code
: Create a place to put device interaction code
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
Mentors:
Depends on: 692782
Blocks: 678694 679966
  Show dependency treegraph
 
Reported: 2011-09-29 23:40 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2011-10-14 07:37 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 0: Add helpers for copy-constructing auto arrays from regular arrays (1.57 KB, patch)
2011-09-29 23:46 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
roc: review+
Details | Diff | Review
part 0.1: Teach IPDL about uint* types, since those are what we have to use in "public" interfaces (1016 bytes, patch)
2011-09-29 23:46 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Review
part 1: Add a hal/ directory in which we can add device-interaction code (28.91 KB, patch)
2011-09-29 23:52 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
roc: superreview+
Details | Diff | Review
part 2: Add build goop for hal/ (4.50 KB, patch)
2011-09-29 23:53 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
khuey: review+
Details | Diff | Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-29 23:40:57 PDT
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.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-29 23:46:33 PDT
Created attachment 563668 [details] [diff] [review]
part 0: Add helpers for copy-constructing auto arrays from regular arrays

Allows

  nsTArray y;
  nsAutoTArray x(y);
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-29 23:46:57 PDT
Created attachment 563669 [details] [diff] [review]
part 0.1: Teach IPDL about uint* types, since those are what we have to use in "public" interfaces
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-29 23:52:39 PDT
Created attachment 563671 [details] [diff] [review]
part 1: Add a hal/ directory in which we can add device-interaction code

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.
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-29 23:53:07 PDT
Created attachment 563672 [details] [diff] [review]
part 2: Add build goop for hal/
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-09-30 03:32:59 PDT
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?
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-30 17:52:27 PDT
(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.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-10-04 15:27:38 PDT
(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 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-10-04 15:28:30 PDT
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
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-04 15:34:28 PDT
(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 10 Mounir Lamouri (:mounir) 2011-10-05 04:54:33 PDT
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.
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-05 13:19:14 PDT
> > +/* -*- 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?
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-05 13:20:47 PDT
I guess missing "ft=cpp"?
Comment 14 :Ms2ger 2011-10-06 03:37:37 PDT
(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.
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-06 16:32:44 PDT
"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.
Comment 17 :Ms2ger 2011-10-07 02:13:23 PDT
Sad. Was there any discussion?
Comment 18 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-07 10:52:03 PDT
Of what?
Comment 19 Mounir Lamouri (:mounir) 2011-10-14 07:37:15 PDT
(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.

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