712378 - Add libhardware.so interface to control lights

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

See https://github.com/andreasgal/B2G/issues/94#issuecomment-3223714 .

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hello Justin - I've been digging into Vibrate and Battery, since they seemed similar to adding in lights, though I've also been looking at radio and audio. The two paths seem to be: Add Lights into the HAL. This is done by adding functionality into the hal/gonk and then calling the hal_impl version from Hal.cpp. Do I need to stub out the same functions in hal/linux, windows, fallback, android, just so those will compile? Also, do I need to do the loop back in hal/sandbox? From there, create a manager in dom/lights (similar to the battery manager already there). Then add the new interfaces into dom/base/Navigator.cpp and idl. The alternative is to add in to dom/system/b2g (or maybe create a new dom/lights/b2g) and create and idl there and either directly handle the lights or create a proxy or daemon to control the lights (though I think that is over kill in this case). The latter seems more self contained and probably cleaner. However, it is a bit more opaque as to how it all hooks up and becomes available to the javascript. Probably need to read more of the XPCOM interface. Thoughts? -Jim Straus

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Let's do the following
 - add interface to Hal.h
 - add a Lights.cpp gonk impl to hal/gonk
 - add a Lights.cpp fallback stubs to hal/fallback.  Use this everywhere !gonk.
 - add a forwarding impl to hal/sandbox/SandboxHal.cpp

Trust me, that will be much simpler than implementing this in dom/system.

Comment 3

[Justin Lebar (not reading bugmail)]

Does comment 2 answer all your questions, Jim?

Comment 4

[Andreas Gal :gal]

I don't think we want DOM apis for most of this. The lights will be driven from inside Gecko. If wifi is on, we turn on the wifi light. If a notification is pending, we turn  on the notification light. For now just expose Gecko-internal privileged APIs.

Comment 5

[Justin Lebar (not reading bugmail)]

We do want DOM APIs for the backlight, keyboard light, and maybe softkey backlight, though, right?

(We could hardcode that the softkey backlight is on whenever the screen is on -- this is what my Nexus S does, and I think it's virtuous.  But it's a policy decision, so I think this is better left to JS code.)

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hi Jim, how is this work coming along?  We need these changes to better support the backlight of the "maguro" device.

Comment 7

[Jim Straus]

Attachment: Patch to Gecko

This adds GetLight and SetLight, modifies GetBrightness and SetBrightness to use the new interface and should expose the functions to js.

Comment 8

[Jim Straus]

Attachment: Patch to glue/gonk/device/samsung/c1-common

Extends liblights to support getting the state of a light.

Comment 9

[Jim Straus]

Attachment: Patch to glue/gonk/hardware/libhardware

Extends liblights to support getting the state of a light.

Comment 10

[Jim Straus]

Added patches.  Note that this extends the liblights interface to support reading the state of the lights.  On the Samsung, the button lights can't be read, but the backlight can.  Can someone please review?

Comment 11

[Philipp von Weitershausen [:philikon]]

Jim, patches against all the non-gecko stuff are best submitted as GitHub pull request. (Also, you don't get to set r+ yourself ;). You need to ask somebody to review your patch, by setting review to '?' and entering their email address in the corresponding field.)

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 587822 [details] [diff] [review]
Patch to Gecko

Hey Jim, I ran out of time to fully review this tonight.  Asap tomorrow.  But a few comments from scanning the patch
 - there are multiple patches concatenated here, but that doesn't fit within our mercurial/bugzilla workflow.  If the patches are logically distinct, please post them as separate attachments, "Part 1", "Part 2", etc.  If they're not logically distinct, please post a patch including all changes.  It looks to me like these patches should be concatenated.
 - what's the consumer of nsIHal?
 - we'll be able to test the lights implementation by using DOM APIs exposed to content, and then checking the hw state changes with virtual qemu devices.  I wouldn't bother with creating hal/tests.  This may sound a little odd, but we typically only test external interfaces; tests of internal interfaces like hal tend to not be worth their value.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 587825 [details] [diff] [review]
Patch to glue/gonk/hardware/libhardware

We can't extend the android hal API, because libhardware is typically provided as a proprietary blob.  Unfortunately we need to stay synced with upstream on this :(.

The "r-" here means, "this patch isn't going in the right direction, so let's take a different approach."

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 587824 [details] [diff] [review]
Patch to glue/gonk/device/samsung/c1-common

Is this in support of the added "get_light" hal API?  If so, the same comment applies here.

I'm clearing the request flag here to mean, "there are questions I need answered".

Comment 15

[Jim Straus]

Do we have to strictly support the Android libraries (I assume this is what you mean by staying in synch with upstream) or can we ask manufacturers to have extended versions of the libraries (that are fully backward compatible)?  Right now, I don't believe there is an abstract way to retrieve the state of the lights, which means that everyone has to build custom functions to control things like the backlight independent of libhardware.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We can ask, but for now we can't rely on it.  For example, the patch here would cause us to crash on the maguro, as things stand currently.  We should assume we can't change android-hal/libhardware.so at all until proven otherwise.

Yes, we'll need to track the light state in gecko.  C'est la guerre ;).

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 587822 [details] [diff] [review]
Patch to Gecko

Hi Jim,

It's pretty hard for me to review concatenated patches like this.  Can you either flatten or separate them into logically distinct pieces, posted separately?

Thanks!

Comment 18

[Jim Straus]

Attachment: Patch to Gecko to allow light controls

Notes for review:
  SetLight takes parameters of which light is being set
  the mode for setting,
  the flash mode for setting
  the flashOMS and flashOffMS for the user flash mode
  the color (32-bit values of ARGB, converted to a brightness if that's all that's supported)
  Hal.cpp -
    Added in calls to SetLight and GetLight
  Hal.h -
    Defines the calls to SetLight and Get Light, the various lights that can be set,
    the light modes and flash modes.
  FallbackHal.cpp
  	Default implementations of SetLight and GetLight.  SetLight does nothing and GetLight
  	returned full on.
  GonkHal.cpp
    Does the actual work, calling liblights.  I removed the references to get_light and am now
    caching the last value set in SetLight, returning that.  If/when we extend liblights.h, we
    can convert back.  There are conditional HAVEGETLIGHT in the code to allow for easy conversion.
    Note that SetScreenBrightness uses SetLight and GetScreenBrightness uses GetLight.  Also note
    that the old GetScreenBrightness actually read the screen brightness, but not in a generic way.
  PHal.ipdl
    Defines a structure and functions for GetLight and SetLight across process boundaries.
  nsIHal.idl
    Constants and functions for access from JS

Comment 19

[Jim Straus]

Chris.  Hopefully this will be easier to review.  No patches to gonk.  I'll create a new bug for that.

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 589365 [details] [diff] [review]
Patch to Gecko to allow light controls

Hi Jim,

Looks very good, thanks!  Some comments are below.

>diff --git a/dom/system/b2g/nsIHal.idl b/dom/system/b2g/nsIHal.idl

Since we don't have a consumer of this interface yet, let's pull the
nsIHal part of this patch out and save it for if we do.  Filing that
as a separate bug, like you did for bug 718897, would be great.  (We
may not ever need to use this interface directly from JS.)

>diff --git a/hal/Hal.h b/hal/Hal.h
>--- a/hal/Hal.h
>+++ b/hal/Hal.h
>@@ -41,16 +41,17 @@
> #define mozilla_Hal_h 1
> 
> #include "mozilla/hal_sandbox/PHal.h"
> #include "base/basictypes.h"
> #include "mozilla/Types.h"
> #include "nsTArray.h"
> #include "prlog.h"
> #include "mozilla/dom/battery/Types.h"
>+#include "nsString.h"

I don't believe that this #include is needed.

>+enum {
>+    HAL_HARDWARE_UNKNOWN = -1,
>+    HAL_HARDWARE_FAIL = 0,
>+    HAL_HARDWARE_SUCCESS = 1,
>+    HAL_LIGHT_ID_BACKLIGHT = 0,
>+    HAL_LIGHT_ID_KEYBOARD = 1,
>+    HAL_LIGHT_ID_BUTTONS = 2,
>+    HAL_LIGHT_ID_BATTERY = 3,
>+    HAL_LIGHT_ID_NOTIFICATIONS = 4,
>+    HAL_LIGHT_ID_ATTENTION = 5,
>+    HAL_LIGHT_ID_BLUETOOTH = 6,
>+    HAL_LIGHT_ID_WIFI = 7,
>+    HAL_LIGHT_ID_COUNT = 8,
>+    HAL_LIGHT_MODE_USER = 0,
>+    HAL_LIGHT_MODE_SENSOR = 1,
>+    HAL_LIGHT_FLASH_NONE = 0,
>+    HAL_LIGHT_FLASH_TIMED = 1,
>+    HAL_LIGHT_FLASH_HARDWARE = 2
>+};
>+

Since we're in C++ here, we can make these separate named |enum| types.  For example,

  enum LightType {
    LIGHT_BACKLIGHT,
    LIGHT_KEYBOARD,
    //...
  };

This will help the C++ compiler catch abuses of the API.

>+/**
>+ * Set the value of a light to a particular color, with a specific flash pattern.
>+ * light specifices which light.  See Hal.idl for the list of constants
>+ * mode specifies user set or based on ambient light sensor
>+ * flash specifies whether or how to flash the light
>+ * flashOnMS and flashOffMS specify the pattern for XXX flash mode
>+ * color specifies the color.  If the light doesn't support color, the given color is
>+ *    transformed into a brightness, or just an on/off if that is all the light is capable of.
>+ */
>+long SetLight(const long& light, const long& mode, const long& flash, const long& flashOnMS, const long& flashOffMS, const long& color);
>+long GetLight(const long& light, long *mode, long *flash, long *flashOnMS, long *flashOffMS,long *color);

Couple of things here

 - |long| is guaranteed to be the same size or smaller (Windows
    64-bit, sigh) than the ISA word size, so |const long&| doesn't
    save any stack space.  Using plain |long| arguments is fine, or
    |const long| if you want the C++ compiler to check immutability of
    the arguments within the function definition.

 - but, since you've already defined |struct LightConfiguration| for
   IPC, please use it here for the hal:: API.  That would allow
   writing the cleaner API

  bool SetLightConfig(LightType aWhich, const hal::LightConfiguration& aConfig);
  bool GetLightConfig(LightType aWhich, hal::LightConfiguration* aConfig);

(I wrote |bool| return values here because I'm not sure we need to
distinguish between HAL_HARDWARE_UNKNOWN and HAL_HARDWARE_FAIL.  We
can always change this later.)

>diff --git a/hal/gonk/GonkHal.cpp b/hal/gonk/GonkHal.cpp

>-const char *screenBrightnessFilename = "/sys/class/leds/lcd-backlight/brightness";

> double
> GetScreenBrightness()
> {

> void
> SetScreenBrightness(double brightness)
> {

\o/, these changes are righteous!

>+
>+struct Devices {
>+    light_device_t* lights[HAL_LIGHT_ID_COUNT];
>+};
>+
>+static Devices* devices = NULL;
>+

Another couple of small nits

 - the Gecko style for static variables is |static Foo sFoo|

 - what's the intended usage of the Devices struct?  We're not trying
   to free it on shutdown, and indeed that might be pretty hard.  I
   would recommend either

   * keep |struct Devices|, and have it use ClearOnShutdown to free
     the memory (xpcom/base/ClearOnShutdown.h)

   * or, changing |struct Devices| into

 static light_device_t sLights[LIGHT_COUNT];

and not bothering with freeing the memory for now.

>+light_device_t* get_device(hw_module_t* module, char const* name)

A few more nits

 - |static light_device_t*|

 - Gecko style for naming functions is LikeThis().  I don't like it
   personally, but that's the style.

>+/**
>+ * The state last set for the lights until liblights supports 
>+ * getting the light state. 
>+ * 
>+ * @author jstraus (1/13/2012)

We track author information using the " * Contributor(s):" section in
the file header, and we track blame using our version control tools.
Feel free to add yourself to the " * Contributor(s):" list in this
file! :)  But, this is annotation isn't necessary.

>+static light_state_t StoredLightState[HAL_LIGHT_ID_COUNT];
>+

Nit: naming style is |sStoredLightState|.

>+long
>+SetLight(const long& light, const long& mode, const long& flash, const long& flashOnMS, const long& flashOffMS, const long& color)
>+{
>+    light_state_t state;
>+
>+    if (!devices) {

Please refactor this initialization code into a helper function.

>+        int err;
>+        hw_module_t* module;
>+
>+        devices = (Devices*)malloc(sizeof(Devices));

If you keep |struct Devices|, please make this a call to |new
Devices()|, and memset all the pointers to 0 in the constructor.

>+        err = hw_get_module(LIGHTS_HARDWARE_MODULE_ID, (hw_module_t const**)&module);
>+        if (err == 0) {
>+            devices->lights[HAL_LIGHT_ID_BACKLIGHT]
>+                    = get_device(module, LIGHT_ID_BACKLIGHT);

This could be written more compactly with an auxiliary data structure like

 struct LightTypeName {
   LightType mType;
   const char* mName;
 } kLightIds[] = {
   LIGHT_BACKLIGHT, LIGHT_ID_KEYBOARD,
   //...
   LIGHT_COUNT, nsnull
 };

and then in this code, a |for| loop over the kLightIds.  (In Gecko
style, "k" means "constant".)

>+    memset(&state, 0, sizeof(light_state_t));
>+    state.color = color;
>+    state.flashMode = flash;
>+    state.flashOnMS = flashOnMS;
>+    state.flashOffMS = flashOffMS;
>+    state.brightnessMode = mode;
>+

Adding a helper to convert between |LightConfiguration| and
|light_state_t| might be useful.

>+long
>+GetLight(const long& light, long *mode, long *flash, long *flashOnMS, long *flashOffMS, long *color)
>+{

>+    *color = state.color;
>+    *flash = state.flashMode;
>+    *flashOnMS = state.flashOnMS;
>+    *flashOffMS = state.flashOffMS;
>+    *mode = state.brightnessMode;
>+

And similarly here, a helper for converting light_state_t ->
LightConfiguration.

>diff --git a/hal/sandbox/PHal.ipdl b/hal/sandbox/PHal.ipdl
>--- a/hal/sandbox/PHal.ipdl
>+++ b/hal/sandbox/PHal.ipdl
>@@ -43,16 +43,25 @@ include protocol PBrowser;
> namespace mozilla {
> 
> namespace hal {
>   struct BatteryInformation {
>     double level;
>     bool   charging;
>     double remainingTime;
>   };
>+  struct LightInformation {

This is just a naming nit, but I think |LightConfiguration| might be
clearer here.  This is used to get/set the requested parameters of a
particular light, not query any varying state.

>+    long light;

I don't feel particularly strongly about whether the light ID is part
of the configuration or not.  I could see arguments both ways.  I'll
leave that up to your judgment.  This should use the LightType enum we
add to Hal.h

>+    long mode;
>+    long flash;

Similarly, these should use more specific enum types.

>+    long flashOnMS;
>+    long flashOffMS;

Since |long| is an architecture-specific type, and our IPC system
works across processes that run with different architecture types (x86
vs. x86-64, currently) I generally discourage use of variable-sized
types in IPC decls.  I think uint32_t would work just as well here.

>+    long color;

This definitely needs to be a fixed-size type, uint32_t.

>+    long status;

Since this is the status of a particular request, not a general
status, I don't think it should live in LightConfiguration.  You can
"return" as many values in IPC response messages as you want, so if
this was added here to ensure only one return value, that's not
necessary.  For example,

  sync GetLight(LightType light) returns (bool status, LightInformation aLightInfo);

is perfectly legal.

>+    sync SetLight(long light, long mode, long flash, long flashOnMS, long flashOffMS, long color) returns (long status);
>+    sync GetLight(long light) returns (LightInformation aLightInfo);
> 

Let's use LightConfiguration for these, and split out status per above.

This was a fair number of comments, but it's really a bunch of small
style stuff.  This patch is close to ready to land.

(I cleared the review request to mean, "I would like to see an updated
patch with these comments addressed".)

Thanks!

Comment 21

[Jim Straus]

Attachment: Patch to Gecko to allow light controls

Notes for review:

nsIHal is pulled and in a separate bug for future inclusion.
Personally, I think we should expose as much as we can, so I don't want it to get lost.
You never know when someone will make creative use of a device.

Enums were created for the various constants and used throughout.
LightConfiguration (formally LightInformation) is the interface and used throughout.

I kept the hardware fail vs. hardware unknown.  If the light doesn't exist you get
hardware unknown.  If there is a problem actually controlling a light, you get
hardware fail.  May not make a difference, but conceivably one could iterate over the
lights with GetLight and see which ones exist.

Fixed the naming conventions and use of uint32_t.  I made the allocation a static.  It's
small and should never go away once initialized.

Thanks for the info on the ipdl being able to return more than one value.
Question:  Can enums be in the ipdl?  I didn't see an example, so they aren't in there now.

Comment 22

[Andreas Gal :gal]

Comment on attachment 590422 [details] [diff] [review]
Patch to Gecko to allow light controls

Jim, you have to set the requestee so chris gets notified of your review request. Add his email in the requestee field next to the ?.

Comment 23

[Andreas Gal :gal]

Comment on attachment 590422 [details] [diff] [review]
Patch to Gecko to allow light controls

I posted a couple comments. Chris is the module owner, so I can't actually review this.

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 590422 [details] [diff] [review]
Patch to Gecko to allow light controls

>diff --git a/hal/Hal.h b/hal/Hal.h

>+enum HALStatus {

Call this LightStatus.

You didn't convince me that this enum return is useful, because you
didn't describe a concrete use within Gecko.  The current users of
SetLight() don't even check the return value.  But this bug is
dragging on too long so let's just get the code landed.

I'll leave it up to you whether you think the complication to this
interface is worth a potential use in the future.  You know my opinion
;).

>+enum LightMode {

The semantics of this isn't obvious, please document it.

>+/**
>+ * Set the value of a light to a particular color, with a specific flash pattern.
>+ * light specifices which light.  See Hal.idl for the list of constants
>+ * mode specifies user set or based on ambient light sensor
>+ * flash specifies whether or how to flash the light
>+ * flashOnMS and flashOffMS specify the pattern for XXX flash mode
>+ * color specifies the color.  If the light doesn't support color, the given color is
>+ *    transformed into a brightness, or just an on/off if that is all the light is capable of.
>+ */
>+uint32_t SetLight(const hal::LightType& light, const hal::LightConfiguration &aConfig);
>+uint32_t GetLight(const hal::LightType& light, hal::LightConfiguration &aConfig);

What's the returned value here mean?  Docs need to describe that.  Is
it LightStatus?  If so, use the C++ type.  Or, save yourself and
future users some trouble and use bool ;).

>diff --git a/hal/fallback/FallbackHal.cpp b/hal/fallback/FallbackHal.cpp

>+#include "nsIHal.h"

This doesn't exist anymore.

>+uint32_t
>+SetLight(const LightType& light, const hal::LightConfiguration& aConfig)
>+{
>+  return HAL_HARDWARE_SUCCESS;

According to your docs above, this should have returned UNKNOWN.

This doesn't help convince me that the return enum is useful ;).

>diff --git a/hal/gonk/GonkHal.cpp b/hal/gonk/GonkHal.cpp

>+  int status;
>+  hal::LightType light = hal::HAL_LIGHT_ID_BACKLIGHT;
>+
>+  status = hal::GetLight(light, aConfig);

|status| is going to generate a compiler warning because it's a dead
variable.  Remove it.  Still not arguing for an enum return type
... ;)

>+  int brightness = aConfig.color() & 0xFF;
>+  return brightness / 255.0;

Add a note that we assume that the backlight is monochromatic so it
doesn't matter which color component we return.  This assumption is
maintained by SetScreenBrightness().

> void
> SetScreenBrightness(double brightness)

>   // Convert the value in [0, 1] to an int between 0 and 255, then write to a
>   // string.

This comment isn't true anymore.

>   int val = static_cast<int>(round(brightness * 255));

uint32_t val = int32_t(round(brightness * 255));

>+  int color = (val<<16) + (val<<8) + val;
>+  

uint32_t.

According to lights.h, you have to set the high byte to 0xff.  We have
a nice helper to manage color components somewhere in Gecko, but I
don't remember where and it's probably not worth the trouble here.

>diff --git a/hal/sandbox/PHal.ipdl b/hal/sandbox/PHal.ipdl

>+  struct LightConfiguration {
>+    uint32_t light;
>+    uint32_t mode;
>+    uint32_t flash;

You didn't address my previous comment to use the C++ types here.

>+    sync SetLight(uint32_t light, LightConfiguration aConfig) returns (uint32_t status);
>+    sync GetLight(uint32_t light) returns (LightConfiguration aConfig, uint32_t status);

Same here.

>diff --git a/hal/sandbox/SandboxHal.cpp b/hal/sandbox/SandboxHal.cpp

>+long

Wrong return value.

>+SetLight(const hal::LightType& light, const hal::LightConfiguration &aConfig)
>+{
>+  uint32_t status = -1;

Don't hard-code this value.  Either switch to bool or stick to the
named enum values.

Per your documentation above, I think you should use ERROR as the
default return here.  This is also not helping your case for the enum
return ;).

>+long

Same as above.

>+GetLight(const hal::LightType& light, hal::LightConfiguration &aConfig)
>+{
>+  uint32_t status = -1;

Same as above.

I'm a bit concerned about numerous comments that weren't addressed
here.  I'm going to need to see another version of the patch.  Please
comment here, or e-mail or ping me on IRC if you have questions.

Comment 25

[Jim Straus]

Attachment: Patch to Gecko to allow light controls

Figured out how to add enums to ipdl, changed the names to fit more the style (starting with leading "e", camel cased).  Changed code to make use of it.
Fixed up comments for the enumeration.
Changed return type to bool, false = failed, true = succeed.
HalStatus doesn't exist any more.

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 591334 [details] [diff] [review]
Patch to Gecko to allow light controls

>diff --git a/hal/Hal.h b/hal/Hal.h

>+/**
>+ * GGET the value of a light returninn a particular color, with a specific flash pattern.

Couple of typos here.

>+ * mode specifies user set or based on ambient light sensor
>+ * flash specifies whether or how to flash the light
>+ * flashOnMS and flashOffMS specify the pattern for XXX flash mode
>+ * color specifies the color.  If the light doesn't support color, the given color is
>+ * transformed into a brightness, or just an on/off if that is all the light is capable of.

You don't need to duplicate this part of the comment from SetLight().

>diff --git a/hal/HalTypes.h b/hal/HalTypes.h

>@@ -0,0 +1,108 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

>+/* ***** BEGIN LICENSE BLOCK *****

Please use the new license block

/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this file,
 * You can obtain one at http://mozilla.org/MPL/2.0/. */

I just switched my emacs macro over to it today.  Sorry for not
pointing this out before.

Please keep the modelines.

Looks good!  Please just fix up the minor nits above and let's get
this landed! :D

Comment 27

[Jim Straus]

Attachment: Patch to Gecko to allow light controls

Fixed typos, reduced comment, updated licenses across files

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 591621 [details] [diff] [review]
Patch to Gecko to allow light controls

Oh sorry ... I meant only update the license header on the new file you added.  But, thanks for doing this anyway, needed to be done. :)

Also, for future reference, if I mark a patch "r+" that means I don't need to see the changes you make to address my review comments.  If you feel like the changes should get another look-over, then by all means request one.  But it's not required.

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hi Jim, this patch doesn't apply anymore over mozilla-central revision 5b0900b3e71c (hg)

user:        Kyle Huey <[e-mail]>
date:        Tue Jan 31 11:38:24 2012 -0500
summary:     Bug 563318: Switch to MSVC 2010 on trunk. r=ted


$ hg qpush
applying 712378
patching file hal/sandbox/PHal.ipdl
Hunk #2 FAILED at 66
1 out of 2 hunks FAILED -- saving rejects to file hal/sandbox/PHal.ipdl.rej
patching file hal/sandbox/SandboxHal.cpp
Hunk #2 FAILED at 119
Hunk #3 FAILED at 234
2 out of 3 hunks FAILED -- saving rejects to file hal/sandbox/SandboxHal.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 712378


Please update the patch and I'll push it to tryserver for you.

Comment 30

[Andreas Gal :gal]

Attachment: patch

Comment 31

[Andreas Gal :gal]

Attachment: patch

Comment 32

[Andreas Gal :gal]

Rebased. We desperately need this patch.

Comment 33

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: updated

Rebased, lots of build-bustage fixes (Jim, need to make sure patches you post build! :) ), and addressed some review comments that were missed.

Comment 34

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/20289eb83e51

Comment 35

[Ed Morley [:emorley]]

Sorry had to backout since it conflicted with bug 697641's backout, which was causing failures on all native Android tests. (Can land after rebase).

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9f70582a48f

Comment 36

[Jim Straus]

Attachment: Patch to Gecko to allow light controls

Merged with latest m-c trunk

Comment 37

[Jim Straus]

Chris, I always do a build before submitting patches.

Comment 38

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Jim, please rebase attachment 593743 [details] [diff] [review], which contains numerous build fixes and addresses some review comments that were overlooked.  Thanks!

Comment 39

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/024993b62d27

Comment 40

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/024993b62d27