Closed Bug 1133741 Opened 5 years ago Closed 5 years ago

[Shinano][Aries] Expose CameraFocusAdjust button

Categories

(Firefox OS Graveyard :: GonkIntegration, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox38 fixed)

RESOLVED FIXED
2.2 S6 (20feb)
Tracking Status
firefox38 --- fixed

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

Attachments

(2 files, 3 obsolete files)

Android supports for a camera focus button (KEY_CAMERA_FOCUS in linux_input.h), we should handle it.
Summary: [Shinano][Aries] Support CameraFocus button → [Shinano][Aries] Expose CameraFocus button
Blocks: 1133965
Attachment #8565937 - Flags: review?(masayuki)
(In reply to Alexandre LISSY :gerard-majax from comment #3)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e01101830c9

There are Android build failures, because it's already defined there.
Attachment #8565937 - Attachment is obsolete: true
Attachment #8565937 - Flags: review?(masayuki)
Comment on attachment 8566025 [details] [diff] [review]
Add camera focus hardware button r=masayuki

"CameraFocus" isn't defined in D3E. You should define it with "Moz" prefix. And suggest it in the spec bug if you think that it's better name:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=21121

And why don't you define it on Android too? I guess that the key events are not fired on Firefox for Android, but it's okay to be defined.
Attachment #8566025 - Flags: review-
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #7)
> Comment on attachment 8566025 [details] [diff] [review]
> Add camera focus hardware button r=masayuki
> 
> "CameraFocus" isn't defined in D3E. You should define it with "Moz" prefix.
> And suggest it in the spec bug if you think that it's better name:
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=21121

Ok, I'll go with MozCameraFocus.

> 
> And why don't you define it on Android too? I guess that the key events are
> not fired on Firefox for Android, but it's okay to be defined.

It's already defined, see the first try. Build on Android failed because AKEYCODE_FOCUS is already being used there.
Attachment #8566025 - Attachment is obsolete: true
Comment on attachment 8566439 [details] [diff] [review]
Add camera focus hardware button r=masayuki

Now exposed as MozCameraFocus :)
Attachment #8566439 - Flags: review?(masayuki)
(In reply to Alexandre LISSY :gerard-majax from comment #11)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=982d48d9b926

Failed because of the Android definition:
> /builds/slave/try-and-api-9-0000000000000000/build/widget/android/nsWindow.cpp: In function 'mozilla::KeyNameIndex ConvertAndroidKeyCodeToKeyNameIndex(mozilla::AndroidGeckoEvent&)':
> /builds/slave/try-and-api-9-0000000000000000/build/widget/android/nsWindow.cpp:1378:9: error: duplicate case value
> In file included from /builds/slave/try-and-api-9-0000000000000000/build/widget/android/nsWindow.cpp:1323:0:
> /builds/slave/try-and-api-9-0000000000000000/build/widget/NativeKeyToDOMKeyName.h:1305:1: error: previously used here

Are you okay if we #ifdef MOZ_WIDGET_GONK his ?
Flags: needinfo?(masayuki)
Comment on attachment 8566439 [details] [diff] [review]
Add camera focus hardware button r=masayuki

> /******************************************************************************
>  * Our Internal Key Values (must have "Moz" prefix)
>  *****************************************************************************/
> DEFINE_KEYNAME_INTERNAL(PrintableKey, "MozPrintableKey")
> DEFINE_KEYNAME_INTERNAL(HomeScreen, "MozHomeScreen")
>+DEFINE_KEYNAME_WITH_SAME_NAME(MozCameraFocus)

No, please use DEFINE_KEYNAME_INTERNAL(CameraFocus, "MozCameraFocus") and please suggest this name in the spec bug mentioned above.

> /******************************************************************************
>  * Device Keys
>  *****************************************************************************/
> DEFINE_KEYNAME_WITH_SAME_NAME(BrightnessDown)
> DEFINE_KEYNAME_WITH_SAME_NAME(BrightnessUp)
> DEFINE_KEYNAME_WITH_SAME_NAME(Camera)
>+DEFINE_KEYNAME_WITH_SAME_NAME(CameraFocus)

Remove this.
>+// CameraFocus
>+KEY_MAP_ANDROID (MozCameraFocus, AKEYCODE_FOCUS)

So, this should be just CameraFocus.

>-    0, // FOCUS
>+    NS_VK_F2, // FOCUS

I don't think you need the hacky keyCode mapping because you can refer the CameraFocus key value, why do you do this?

(In reply to Alexandre LISSY :gerard-majax from comment #12)
> (In reply to Alexandre LISSY :gerard-majax from comment #11)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=982d48d9b926
> 
> Failed because of the Android definition:
> > /builds/slave/try-and-api-9-0000000000000000/build/widget/android/nsWindow.cpp: In function 'mozilla::KeyNameIndex ConvertAndroidKeyCodeToKeyNameIndex(mozilla::AndroidGeckoEvent&)':
> > /builds/slave/try-and-api-9-0000000000000000/build/widget/android/nsWindow.cpp:1378:9: error: duplicate case value
> > In file included from /builds/slave/try-and-api-9-0000000000000000/build/widget/android/nsWindow.cpp:1323:0:
> > /builds/slave/try-and-api-9-0000000000000000/build/widget/NativeKeyToDOMKeyName.h:1305:1: error: previously used here
> 
> Are you okay if we #ifdef MOZ_WIDGET_GONK his ?

No, you just forget to remove this line:
http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#1378

All Android key code values are checked for detecting unexpected change causes breaking the mapping on Android.
Flags: needinfo?(masayuki)
Attachment #8566439 - Flags: review?(masayuki) → review-
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #13)
> Comment on attachment 8566439 [details] [diff] [review]
> Add camera focus hardware button r=masayuki
> 
> > /******************************************************************************
> >  * Our Internal Key Values (must have "Moz" prefix)
> >  *****************************************************************************/
> > DEFINE_KEYNAME_INTERNAL(PrintableKey, "MozPrintableKey")
> > DEFINE_KEYNAME_INTERNAL(HomeScreen, "MozHomeScreen")
> >+DEFINE_KEYNAME_WITH_SAME_NAME(MozCameraFocus)
> 
> No, please use DEFINE_KEYNAME_INTERNAL(CameraFocus, "MozCameraFocus") and
> please suggest this name in the spec bug mentioned above.

I don't care about the name. I had not saw that you already suggested something on the W3C bug.

Then should we use CameraFocusAdjust or MozCameraFocusAdjust ?

DEFINE_KEYNAME_INTERNAL(CameraFocusAdjust, "MozCameraFocusAdjust") ?

> 
> > /******************************************************************************
> >  * Device Keys
> >  *****************************************************************************/
> > DEFINE_KEYNAME_WITH_SAME_NAME(BrightnessDown)
> > DEFINE_KEYNAME_WITH_SAME_NAME(BrightnessUp)
> > DEFINE_KEYNAME_WITH_SAME_NAME(Camera)
> >+DEFINE_KEYNAME_WITH_SAME_NAME(CameraFocus)
> 
> Remove this.
> >+// CameraFocus
> >+KEY_MAP_ANDROID (MozCameraFocus, AKEYCODE_FOCUS)
> 
> So, this should be just CameraFocus.
> 
> >-    0, // FOCUS
> >+    NS_VK_F2, // FOCUS
> 
> I don't think you need the hacky keyCode mapping because you can refer the
> CameraFocus key value, why do you do this?
> 
> (In reply to Alexandre LISSY :gerard-majax from comment #12)
> > (In reply to Alexandre LISSY :gerard-majax from comment #11)
> > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=982d48d9b926
> > 
> > Failed because of the Android definition:
> > > /builds/slave/try-and-api-9-0000000000000000/build/widget/android/nsWindow.cpp: In function 'mozilla::KeyNameIndex ConvertAndroidKeyCodeToKeyNameIndex(mozilla::AndroidGeckoEvent&)':
> > > /builds/slave/try-and-api-9-0000000000000000/build/widget/android/nsWindow.cpp:1378:9: error: duplicate case value
> > > In file included from /builds/slave/try-and-api-9-0000000000000000/build/widget/android/nsWindow.cpp:1323:0:
> > > /builds/slave/try-and-api-9-0000000000000000/build/widget/NativeKeyToDOMKeyName.h:1305:1: error: previously used here
> > 
> > Are you okay if we #ifdef MOZ_WIDGET_GONK his ?
> 
> No, you just forget to remove this line:
> http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.
> cpp#1378
> 
> All Android key code values are checked for detecting unexpected change
> causes breaking the mapping on Android.

Right, I did not knew I should remove it from there.
(In reply to Alexandre LISSY :gerard-majax from comment #14)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #13)
> > Comment on attachment 8566439 [details] [diff] [review]
> > Add camera focus hardware button r=masayuki
> > 
> > > /******************************************************************************
> > >  * Our Internal Key Values (must have "Moz" prefix)
> > >  *****************************************************************************/
> > > DEFINE_KEYNAME_INTERNAL(PrintableKey, "MozPrintableKey")
> > > DEFINE_KEYNAME_INTERNAL(HomeScreen, "MozHomeScreen")
> > >+DEFINE_KEYNAME_WITH_SAME_NAME(MozCameraFocus)
> > 
> > No, please use DEFINE_KEYNAME_INTERNAL(CameraFocus, "MozCameraFocus") and
> > please suggest this name in the spec bug mentioned above.
> 
> I don't care about the name. I had not saw that you already suggested
> something on the W3C bug.
> 
> Then should we use CameraFocusAdjust or MozCameraFocusAdjust ?
> 
> DEFINE_KEYNAME_INTERNAL(CameraFocusAdjust, "MozCameraFocusAdjust") ?

Yes, this is what I suggested to W3C.
Thanks. New try with new patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1ac15e5d2e2

I've applied it locally on my Aries device, and it does the job, exposing as MozCameraFocusAdjust.
Attachment #8566439 - Attachment is obsolete: true
Attachment #8566983 - Flags: review?(masayuki)
Attachment #8566983 - Flags: review?(masayuki) → review+
Keywords: checkin-needed
Summary: [Shinano][Aries] Expose CameraFocus button → [Shinano][Aries] Expose CameraFocusAdjust button
https://hg.mozilla.org/mozilla-central/rev/9c1093e7a128
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S6 (20feb)
You need to log in before you can comment on or make changes to this bug.