1133741 - [Shinano][Aries] Expose CameraFocusAdjust button

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect)

Description

[:gerard-majax]

Android supports for a camera focus button (KEY_CAMERA_FOCUS in linux_input.h), we should handle it.

Comment 1

[:gerard-majax]

Attachment: Exposing CameraFocus from Gecko

Comment 2

[:gerard-majax]

Attachment: Add camera focus hardware button r=masayuki

Comment 3

[:gerard-majax]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e01101830c9

Comment 4

[:gerard-majax]

(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.

Comment 5

[:gerard-majax]

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

Comment 6

[:gerard-majax]

Attachment: Add camera focus hardware button r=masayuki

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 8

[:gerard-majax]

(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.

Comment 9

[:gerard-majax]

Attachment: Add camera focus hardware button r=masayuki

Comment 10

[:gerard-majax]

Comment on attachment 8566439 [details] [diff] [review]
Add camera focus hardware button r=masayuki

Now exposed as MozCameraFocus :)

Comment 11

[:gerard-majax]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=982d48d9b926

Comment 12

[:gerard-majax]

(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 ?

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 14

[:gerard-majax]

(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.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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.

Comment 16

[:gerard-majax]

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.

Comment 17

[:gerard-majax]

Attachment: Add camera focus hardware button r=masayuki

Comment 18

[:gerard-majax]

green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1ac15e5d2e2

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9c1093e7a128

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/9c1093e7a128