Closed Bug 964154 Opened 9 years ago Closed 8 years ago

To Detect Whether Headset Event is Came from Input Dev or /sys Node in Runtime Not by Property


(Firefox OS Graveyard :: General, defect)

Gonk (Firefox OS)
Not set


(Not tracked)

2.0 S5 (4july)


(Reporter: mchen, Assigned: kershaw)




(1 file, 3 obsolete files)

According to comment above, the follow up bug is fired to remove property and detect source of headset event in run time.
Assignee: nobody → kechang
Hi Dave,

Could you help to review my proposed patch?
I am simply using the device path to check if headset plug/unplug is from uevent. Not sure if this is the correct way.

Attachment #8443322 - Flags: review?(dhylands)
Comment on attachment 8443322 [details] [diff] [review]

Review of attachment 8443322 [details] [diff] [review]:

I think that this needs to be reworked so that the path knowledge stays in SwitchHandlerHeadphone.

::: hal/gonk/GonkSwitch.cpp
@@ +326,5 @@
>    SwitchHandlerArray mHandler;
>    void Init()
>    {
> +    bool headphonesFromInputDev = 

nit: trailing space

@@ +327,5 @@
>    void Init()
>    {
> +    bool headphonesFromInputDev = 
> +      !ReadSysFile("sys/devices/virtual/switch/h2w/state", nullptr, 0);

I don't like the fact that this code essentially duplicates the code in SwitchHandler::GetInitialState for determining the path.

I think that the code flow should go something like this:

- Instantiate: SwitchHandlerHeadphone(SWITCH_HEADSET_DEVPATH)
- Ask the instantiated device if its present or not
- if the device is present, append the device to mHandler

It looks like SwitchHandlerArray use RefPtr
so you should uuse RefPtr to hold the pointer to the SwitchHandlerHeadphone.

::: widget/gonk/nsAppShell.cpp
@@ +1018,5 @@
>  void
>  nsAppShell::InitInputDevices()
>  {
> +    sDevInputAudioJack =
> +      !ReadSysFile("sys/devices/virtual/switch/h2w/state", nullptr, 0);

This is just wrong. This code shouldn't know about paths like this.

This should call into GonkSwitch to make the determination.
Attachment #8443322 - Flags: review?(dhylands) → review-
Hi Dave,

Thanks for the good comments.
I've uploaded the v2 patch. Please help to review it.
Attachment #8443322 - Attachment is obsolete: true
Attachment #8445157 - Flags: review?(dhylands)
Comment on attachment 8445157 [details] [diff] [review]

Review of attachment 8445157 [details] [diff] [review]:

This looks much cleaner. Thanks.

I'm have some concerns. Not necessarily with this patch per-say - but GonkSwitch doesn't appear to be threadsafe. I filed bug 1032410 to get that fixed.
Attachment #8445157 - Flags: review?(dhylands) → review+
Update the patch to carry review's name
Attachment #8445157 - Attachment is obsolete: true
Attachment #8449127 - Attachment is obsolete: true
Keywords: checkin-needed
Target Milestone: --- → 2.0 S5 (4july)
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 1044963
You need to log in before you can comment on or make changes to this bug.