Closed Bug 1023837 Opened 6 years ago Closed 5 years ago

Consider removing LightType/LightMode/FlashMode from HalType.h

Categories

(Firefox OS Graveyard :: General, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S4 (23jan)

People

(Reporter: kikuo, Assigned: kechen)

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 3 obsolete files)

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.
Does anyone take this bug ?
I want to take this bug as my good-first-bug.
Flags: needinfo?(ckjboy2003)
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.
Flags: needinfo?(ckjboy2003) → needinfo?(mchen)
(In reply to Sam Lin from comment #2)
Yes, of course.
Thanks for your contribution.
Assignee: nobody → joeking11829
Flags: needinfo?(mchen)
Whiteboard: [good first bug]
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 !?
Flags: needinfo?(kikuo)
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?
Flags: needinfo?(kikuo)
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 !?
Flags: needinfo?(kikuo)
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
Flags: needinfo?(kikuo)
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 !?
Flags: needinfo?(kikuo)
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 ?
Flags: needinfo?(kikuo)
Hi Kilik,

I think you are right.
This is my patch.
Flags: needinfo?(kikuo)
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.  :)
Flags: needinfo?(kikuo)
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.
Hi Kilik,

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

Thank you very much.
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 ~
(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
Flags: needinfo?(joeking11829)
Hi Kilik,

It's fine, you can reassign it.

Thank you !!
Flags: needinfo?(joeking11829)
(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 :)
Assignee: joeking11829 → kechen
Hi Kilik, please see this patch thank you.
Attachment #8541595 - Flags: feedback?(kikuo)
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.
Attachment #8541595 - Flags: feedback?(kikuo) → feedback+
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
Attachment #8545088 - Flags: review?(dhylands)
Attachment #8541595 - Attachment is obsolete: true
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.
Flags: needinfo?(dhylands)
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.
Attachment #8545088 - Flags: review?(dhylands) → review?(21)
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 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.
Attachment #8545088 - Flags: review?(21) → review+
Flags: needinfo?(dhylands)
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
Attachment #8545088 - Attachment description: 0001-Bug-1023837-Move-LightType-LightMode-FlashMode-from-.patch → Move LightType, LightMode and FlashMode from HalType.h to GonkHal.cpp
Attachment #8455105 - Attachment is obsolete: true
Keywords: checkin-needed
Keywords: checkin-needed
Modified the commit log with adding reviewer's name.
Carry the r+ from Dave in Comment 24
Attachment #8545088 - Attachment is obsolete: true
Attachment #8552911 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/781a2d77d243
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S4 (23jan)
You need to log in before you can comment on or make changes to this bug.