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

RESOLVED FIXED in 2.0 S5 (4july)

Status

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mchen, Assigned: kershaw)

Tracking

unspecified
2.0 S5 (4july)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

6 years ago
https://bugzilla.mozilla.org/show_bug.cgi?id=958963#c19
https://bugzilla.mozilla.org/show_bug.cgi?id=958963#c20

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

Updated

5 years ago
Assignee: nobody → kechang
Assignee

Comment 1

5 years ago
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.

Thanks.
Attachment #8443322 - Flags: review?(dhylands)
Comment on attachment 8443322 [details] [diff] [review]
Bug-964154-To-Detect-Whether-Headset-Event-is-Came-f.patch

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
http://dxr.mozilla.org/mozilla-central/source/hal/gonk/GonkSwitch.cpp#216
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-
Assignee

Comment 3

5 years ago
Hi Dave,

Thanks for the good comments.
I've uploaded the v2 patch. Please help to review it.
Attachment #8443322 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8445157 - Flags: review?(dhylands)
Comment on attachment 8445157 [details] [diff] [review]
DetectWhetherHeadsetEventIsFromInputDev-v2

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+
Assignee

Comment 6

5 years ago
Update the patch to carry review's name
Assignee

Comment 7

5 years ago
Attachment #8445157 - Attachment is obsolete: true
Attachment #8449127 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Keywords: checkin-needed
Target Milestone: --- → 2.0 S5 (4july)
https://hg.mozilla.org/mozilla-central/rev/bd0703d4cd88
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 1044963
You need to log in before you can comment on or make changes to this bug.