1023837 - Consider removing LightType/LightMode/FlashMode from HalType.h

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Kilik Kuo [:kikuo] (inactive)]

Aroused from (Bug 1017463) (Comment 20), LightType/FlashMode/LightMode are only used in GonkHal.cpp, so consider to remove these enum from HalType.h,and make them accessible in GonkHal.cpp only.

Comment 1

[Joe Young]

Does anyone take this bug ?
I want to take this bug as my good-first-bug.

Comment 2

[Sam Lin[:neopleo]]

Hi, Marco.

Joe is my team member and he would like to take this bug.

If you think it's okay for him to take this bug, please set him as the assignee.

Thanks.

Comment 3

[Marco Chen [:mchen]]

(In reply to Sam Lin from comment #2)
Yes, of course.
Thanks for your contribution.

Comment 4

[Joe Young]

I found that these enum is not only used by GonkHal.cpp.

In PHal.ipdl:
using mozilla::hal::FlashMode from "mozilla/HalTypes.h";
using mozilla::hal::LightType from "mozilla/HalTypes.h";
using mozilla::hal::LightMode from "mozilla/HalTypes.h";

struct LightConfiguration {
  LightType light;
  LightMode mode;
  FlashMode flash;
  uint32_t flashOnMS;
  uint32_t flashOffMS;
  uint32_t color;
};

If remove these enum ("LightType"、"LightMode"、"FlashMode") from HalTypes.h
then build failed.

build failed message:
In file included from ../../ipc/ipdl/_ipdlheaders/mozilla/dom/PContentParent.h:42:0,
                 from ../../../gecko/chrome/src/nsChromeRegistryChrome.cpp:6:
../../dist/include/mozilla/HalTypes.h:216:20: error: 'LightType' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:216:20: error: 'LightType' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:216:43: error: template argument 1 is invalid
../../dist/include/mozilla/HalTypes.h:218:14: error: 'LightType' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:218:14: error: 'LightType' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:219:14: error: 'eHalLightID_Backlight' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:219:14: error: 'eHalLightID_Backlight' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:220:14: error: 'eHalLightID_Count' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:220:14: error: 'eHalLightID_Count' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:220:45: error: template argument 1 is invalid
../../dist/include/mozilla/HalTypes.h:220:45: error: template argument 2 is invalid
../../dist/include/mozilla/HalTypes.h:220:45: error: template argument 3 is invalid
../../dist/include/mozilla/HalTypes.h:227:20: error: 'LightMode' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:227:20: error: 'LightMode' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:227:43: error: template argument 1 is invalid
../../dist/include/mozilla/HalTypes.h:229:14: error: 'LightMode' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:229:14: error: 'LightMode' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:230:14: error: 'eHalLightMode_User' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:230:14: error: 'eHalLightMode_User' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:231:14: error: 'eHalLightMode_Count' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:231:14: error: 'eHalLightMode_Count' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:231:47: error: template argument 1 is invalid
../../dist/include/mozilla/HalTypes.h:231:47: error: template argument 2 is invalid
../../dist/include/mozilla/HalTypes.h:231:47: error: template argument 3 is invalid
../../dist/include/mozilla/HalTypes.h:238:20: error: 'FlashMode' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:238:20: error: 'FlashMode' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:238:43: error: template argument 1 is invalid
../../dist/include/mozilla/HalTypes.h:240:14: error: 'FlashMode' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:240:14: error: 'FlashMode' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:241:14: error: 'eHalLightFlash_None' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:241:14: error: 'eHalLightFlash_None' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:242:14: error: 'eHalLightFlash_Count' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:242:14: error: 'eHalLightFlash_Count' is not a member of 'mozilla::hal'

So.....
How should I fix that problem !?

Comment 5

[Marco Chen [:mchen]]

Thanks for your contribution first.

And please refer to bug description that Bug 1017463 already removed these dependency.
May I know you really use the newest code base?

Comment 6

[Joe Young]

I update my code base to the newest one.

And i found that these enum still use in HalTypes.h.

In HalTypes.h: code 237 ~ 268

/**
 * Light type serializer.
 */
template <>
struct ParamTraits<mozilla::hal::LightType>
  : public ContiguousEnumSerializer<
             mozilla::hal::LightType,
             mozilla::hal::eHalLightID_Backlight,
             mozilla::hal::eHalLightID_Count>
{};

/**
 * Light mode serializer.
 */
template <>
struct ParamTraits<mozilla::hal::LightMode>
  : public ContiguousEnumSerializer<
             mozilla::hal::LightMode,
             mozilla::hal::eHalLightMode_User,
             mozilla::hal::eHalLightMode_Count>
{};

/**
 * Flash mode serializer.
 */
template <>
struct ParamTraits<mozilla::hal::FlashMode>
  : public ContiguousEnumSerializer<
             mozilla::hal::FlashMode,
             mozilla::hal::eHalLightFlash_None,
             mozilla::hal::eHalLightFlash_Count>
{};

So I build failed with some error.

Error Message:
../../dist/include/mozilla/HalTypes.h:241:20: error: 'LightType' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:241:20: error: 'LightType' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:241:43: error: template argument 1 is invalid
../../dist/include/mozilla/HalTypes.h:243:14: error: 'LightType' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:243:14: error: 'LightType' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:244:14: error: 'eHalLightID_Backlight' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:244:14: error: 'eHalLightID_Backlight' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:245:14: error: 'eHalLightID_Count' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:245:14: error: 'eHalLightID_Count' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:245:45: error: template argument 1 is invalid
../../dist/include/mozilla/HalTypes.h:245:45: error: template argument 2 is invalid
../../dist/include/mozilla/HalTypes.h:245:45: error: template argument 3 is invalid
../../dist/include/mozilla/HalTypes.h:252:20: error: 'LightMode' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:252:20: error: 'LightMode' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:252:43: error: template argument 1 is invalid
../../dist/include/mozilla/HalTypes.h:254:14: error: 'LightMode' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:254:14: error: 'LightMode' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:255:14: error: 'eHalLightMode_User' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:255:14: error: 'eHalLightMode_User' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:256:14: error: 'eHalLightMode_Count' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:256:14: error: 'eHalLightMode_Count' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:256:47: error: template argument 1 is invalid
../../dist/include/mozilla/HalTypes.h:256:47: error: template argument 2 is invalid
../../dist/include/mozilla/HalTypes.h:256:47: error: template argument 3 is invalid
../../dist/include/mozilla/HalTypes.h:263:20: error: 'FlashMode' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:263:20: error: 'FlashMode' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:263:43: error: template argument 1 is invalid
../../dist/include/mozilla/HalTypes.h:265:14: error: 'FlashMode' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:265:14: error: 'FlashMode' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:266:14: error: 'eHalLightFlash_None' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:266:14: error: 'eHalLightFlash_None' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:267:14: error: 'eHalLightFlash_Count' is not a member of 'mozilla::hal'
../../dist/include/mozilla/HalTypes.h:267:14: error: 'eHalLightFlash_Count' is not a member of 'mozilla::hal'


Any comment !?

Comment 7

[Kilik Kuo [:kikuo] (inactive)]

Hi Joe,
Since these types {LightType,FlashMode,LightMode} are no longer used through SandboxHal, which means they're not used in PHal.ipdl. I think these serializer[1] could be removed too.

[1] : https://developer.mozilla.org/en-US/docs/IPDL/Type_Serialization

Comment 8

[Joe Young]

Hi Kilik,

Thanks for your suggestion.
I removed these enum from HalTypes.h.

But, I have two ways to implement your request:
1. create a header file for these enum types, include by GonkHal.cpp
2. create these enum types in GonHal.cpp

Which way that you perfer !?

Comment 9

[Kilik Kuo [:kikuo] (inactive)]

Cool, I'd like to put these enum in GonkHal.cpp. 

But, just my own opinion.
If we're going to config new type of light in the future, 
we could add a new API in Hal.h (e.g. SetXXXBrightness), and through sandbox, 
finally call into GonkHal.cpp(in content process).
So basically, these enum will be only used in GonkHal.cpp, 
no need to create a new file for them. (might be included by other components mistakenly) 

What do you think ?

Comment 10

[Joe Young]

Attachment: 0001-Removing-LightType-LightMode-FlashMode-from-HalTypes.patch

Hi Kilik,

I think you are right.
This is my patch.

Comment 11

[Kilik Kuo [:kikuo] (inactive)]

Thanks Joe.
I think it's good.

And to be better (just my pov),
Since hal::{LightType,LightMode,FlashMode} are only used in GonkHal.cpp,
There should be no need to declare these enum via namespace hal.
e.g. |hal::LightType light;| can be |LightType light;|

Once you're ok, you could ask Dave Hylands [:dhylands](Module owner of HAL) for review.  :)

Comment 12

[Kilik Kuo [:kikuo] (inactive)]

Hello, Joe

Good work ~ You're almost there !  Is there any progress ?
Once you're done, you could click the "Detail" link just on the right side of the patch
and set review to "?". There'll be suggested reviewers for you to select. 

And you could also find out related reviewer from this page [1].
For more detail, you could follow steps in this site [2].

After getting "review+", push your patch to try server to get a testing result [3].
Then paste the result link (make sure the build & Test result are all OK) on bug comment. 
And add the checkin-needed keyword to this bug.
Done ! 

[1] http://hg.mozilla.org/mozilla-central/filelog/e6614d8d85f9/hal/gonk/GonkHal.cpp
[2] https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch
[3] https://wiki.mozilla.org/ReleaseEngineering/TryServer

(In reply to Joe Young from comment #10)
> Created attachment 8455105 [details] [diff] [review]
> 0001-Removing-LightType-LightMode-FlashMode-from-HalTypes.patch
> 
> Hi Kilik,
> 
> I think you are right.
> This is my patch.

Comment 13

[Joe Young]

Hi Kilik,

Sorry, Because I am fixing some multimedia bug on flatfish....
I will finish this bug ASAP.

Thank you very much.

Comment 14

[Kilik Kuo [:kikuo] (inactive)]

Ha, no need to be sorry ~
I just thought you might be busy or not familiar with the following patch submit procedure :) 
So I made a more detail explanation for you.
Hope it helps. :)
Thank you for your contribution ~

Comment 15

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Joe Young from comment #13)
> Hi Kilik,
> 
> Sorry, Because I am fixing some multimedia bug on flatfish....
> I will finish this bug ASAP.
> 
> Thank you very much.

Hello Joe,

Are you still working on this bug ? 
If not, I'm wondering if it's ok to assign this bug to our new team member as first bug for practice.
Looking forward to your reply. Thanks

Comment 16

[Joe Young]

Hi Kilik,

It's fine, you can reassign it.

Thank you !!

Comment 17

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Joe Young from comment #16)
> Hi Kilik,
> 
> It's fine, you can reassign it.
> 
> Thank you !!

Thank you for the previous effort :)

Comment 18

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: move the enum from HalType.h to GonkHal.cpp

Hi Kilik, please see this patch thank you.

Comment 19

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8541595 [details] [diff] [review]
move the enum from HalType.h to GonkHal.cpp

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

Hi Kevin, 
please fix the following nitpick, and I think it would be ready to be reviewed : )

::: hal/gonk/GonkHal.cpp
@@ +123,5 @@
> +  eHalLightID_Notifications = 4,
> +  eHalLightID_Attention     = 5,
> +  eHalLightID_Bluetooth     = 6,
> +  eHalLightID_Wifi          = 7,
> +  eHalLightID_Count         = 8  // This should stay at the end

It would be unnecessary to set the value for eHalLightID_Count, and it is more consistent to follow the style as below.

Comment 20

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Move LightType, LightMode and FlashMode from HalType.h to GonkHal.cpp

Hello Dave, this is my first bug. Could you help me to review this patch? Thank you.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97748051bf98

Comment 21

[Kevin Chen[:kechen] (UTC + 8)]

Hi Dave, if you have the time, could you help me to review this patch? Or could you recommand me someone to review it? Thank you for the help.

Comment 22

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8545088 [details] [diff] [review]
Move LightType, LightMode and FlashMode from HalType.h to GonkHal.cpp

Hi Vivien, this is my first bug. Could you help me to check this patch? Thank you.

Comment 23

[Dave Hylands [:dhylands]]

Comment on attachment 8455105 [details] [diff] [review]
0001-Removing-LightType-LightMode-FlashMode-from-HalTypes.patch

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

The change looks fine.

A couple of comments. It looks like you used git to create the patch. Please ensure that you use -U8 when you do this.
Also, the subject line should of the form:

Bug 1023837 - Remove LightType/LightMode/FlashMode from HalTypes.h

After the subject line, you can leave a blank line and then put a more detailed description of what the patch does (i.e. the "Removing LightType/LightMode/FlashMode from HalTypes.h" portion)

Comment 24

[Dave Hylands [:dhylands]]

Comment on attachment 8545088 [details] [diff] [review]
Move LightType, LightMode and FlashMode from HalType.h to GonkHal.cpp

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

It looks like this patch duplicates the contents of the other patch. Perhaps you intended to obsolete the earlier patch?

When you attach a new patch, you have an oppurtunity to obsolete old patches. To do it after that, then click on "Details" for the patch you want to obsolete, and then click on "edit details", and you'll see an "obsolete" checkbox.

I see that you used -U8 for this patch, and the subject line looks like it has the correct format.

You should also make sure that you fill in the attachment description in bugzilla and don't just use the default that it provides (which was 0001-Bug-1023837-Move-LightType-LightMode-FlashMode-from-.patch)

Good work.

Comment 25

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8545088 [details] [diff] [review]
Move LightType, LightMode and FlashMode from HalType.h to GonkHal.cpp

Thank you for the help, Dave. I already modified the content of the file description.

The following link is the result of try server.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97748051bf98

Comment 26

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Move LightType, LightMode and FlashMode from HalType.h to GonkHal.cpp

Modified the commit log with adding reviewer's name.
Carry the r+ from Dave in Comment 24

Comment 27

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/integration/b2g-inbound/rev/781a2d77d243

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/781a2d77d243