906096 - Ensure keyboard continues to work in Firefox Nightly + fix tests because of move to WebIDL

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Kevin Grandon :kgrandon]

Third party keyboard support is landing soon: https://github.com/mozilla-b2g/gaia/pull/11527

We will probably need to make some updates to our implementation to ensure that it continues to work.

Comment 1

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Jan helped to integrate part of the Gecko changes for 3rd-party IME in Bug 902562.
However, that bug needs to be updated since Gecko has more changes that need to happen in Firefox Nightly (Bug 899073).

Comment 2

[Jan Jongboom [:janjongboom] (inactive)]

Sidenote: need to fix the IME tests as well because of API changes.

Comment 3

[Jan Jongboom [:janjongboom] (inactive)]

Hi Boris, a.t.m. we build .xpt from the .idl and then load that in FF nightly, but of course I can't build xpt from webidl. Is there something similar for webidl? Because at the moment building InputMethod support requires a build flag to be set that isn't enabled for FF Nightly, so we should support to an about:config flag if that's not possible.

Comment 4

[Boris Zbarsky [:bzbarsky]]

WebIDL bindings are compiled into libxul, so they can't be dynamically added at runtime...  You want to compile the binding unconditionally (but perhaps only expose it based on a pref or whatnot).

Comment 5

[Yuan Xulei [:yxl] (Leave from May 18)]

I think we need wait for bug 905567 - Expose navigator.MozInputMethod to desktop build.

Comment 6

[Jan Jongboom [:janjongboom] (inactive)]

Ah figures, I was already doing that for this bug now... Hope kanru and I didn't do the same thing as I'm almost done haha.

Comment 7

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Gecko patch

Comment 8

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Gaia patch

Comment 9

[Jan Jongboom [:janjongboom] (inactive)]

I have attached patches for Gaia and Gecko. The keyboard part in desktop helper can go if we're done porting keyboard app to use mozInputMethod.

Comment 11

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Jan Jongboom [:janjongboom] from comment #6)
> Ah figures, I was already doing that for this bug now... Hope kanru and I
> didn't do the same thing as I'm almost done haha.

The plan is a bit different. We want to move the whole mozInputMethod impl out of b2g/ and make it pref-enabled so it could be built into Nightly. I just started something. Do you want to do it or me to finish it?

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 793048 [details] [diff] [review]
Gecko patch

Review of attachment 793048 [details] [diff] [review]:
-----------------------------------------------------------------

Do not modify the return value of the WebIDL. Instead you could add [Pref="dom.mozInputMethod.enabled"] to each interface.

Nit: Don't include irrelevant whitespace changes.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Note that returning null from a JS-implemented init method is not allowed right now: it'll fatally assert in a debug build and quietly do random stuff in an opt one...

Comment 14

[Jan Jongboom [:janjongboom] (inactive)]

> Do not modify the return value of the WebIDL. Instead you could add
> [Pref="dom.mozInputMethod.enabled"] to each interface.
You mean in the init function, right?
> Nit: Don't include irrelevant whitespace changes.
Ok

(In reply to Boris Zbarsky [:bz] from comment #13)
> Note that returning null from a JS-implemented init method is not allowed
> right now: it'll fatally assert in a debug build and quietly do random stuff
> in an opt one...
Hmm, I got this from https://github.com/mozilla/mozilla-central/blob/master/dom/alarm/AlarmsManager.js#L159. Now I see that it doesn't have a webidl file, so I guess that's why it's wrong. I'll change it. Thanks.

Comment 15

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Gecko patch v2

Kan-Ru, Boris, thanks for the swift feedback. I've updated the patch. Pref moved to webidl file and removed whitespace changes.

Comment 16

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 793381 [details] [diff] [review]
Gecko patch v2

Review of attachment 793381 [details] [diff] [review]:
-----------------------------------------------------------------

Move all MozInputMethod related file under b2g/components to somewhere in dom/, maybe dom/inputmethod. Also we need to load Keyboard.jsm on Desktop if dom.mozInputMethod.enabled is true. Also you need to change BrowserElement to load the form.js file.

And you need a DOM peer to review this.

::: dom/webidl/InputMethod.webidl
@@ +5,5 @@
>   */
>  
>  [JSImplementation="@mozilla.org/b2g-inputmethod;1",
> + NavigatorProperty="mozInputMethod",
> + Pref="dom.mozInputMethod.enabled"]

Also MozInputContext and MozInputMethodManager

@@ +58,5 @@
>  // e.g., the text field does no longer exist, the app is being switched to background, and etc.
>  [JSImplementation="@mozilla.org/b2g-inputcontext;1"]
>  interface MozInputContext: EventTarget {
> +    // The tag name of input field, which is enum of "input", "textarea", or "contenteditable"
> +    readonly attribute DOMString? type;

No need to change the return type here and below.

Comment 17

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Kan-Ru Chen [:kanru] from comment #16)
> No need to change the return type here and below.
Yes. Because when setting this to null when we invalidate the context the value will be "null" instead of null without the ?

Comment 18

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Jan Jongboom [:janjongboom] from comment #17)
> (In reply to Kan-Ru Chen [:kanru] from comment #16)
> > No need to change the return type here and below.
> Yes. Because when setting this to null when we invalidate the context the
> value will be "null" instead of null without the ?

Hmm. Then create another bug for this :)

Comment 19

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 793049 [details] [review]
Gaia patch

Sorry that I cannot evaluate this patch since I cannot run Gaia with Firefox Nightly now.
I also tried Aurora, but with no luck.

Jan,

May I know which version of FF you used to run ime test?
Thank you.

Comment 20

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Now, I can run Gaia with Firefox Nightly after updating to the new version.
However, when I tried to run the IME test within this patch, it would report error, like navigator.mozInputMethod is undefined.

It is ok to run IME test in Gaia master branch.

Comment 21

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Rudy Lu [:rudyl] from comment #20)
> Now, I can run Gaia with Firefox Nightly after updating to the new version.
> However, when I tried to run the IME test within this patch, it would report
> error, like navigator.mozInputMethod is undefined.
> 
> It is ok to run IME test in Gaia master branch.

You have to build FF Nightly with the Gecko patch to be able to run this.

Comment 22

[Jan Jongboom [:janjongboom] (inactive)]

(if you want a prebuilt version for OS/X I can give it to you)

Comment 23

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Kan-Ru Chen [:kanru] from comment #16)
> Also we need to load Keyboard.jsm on Desktop if
> dom.mozInputMethod.enabled is true. Also you need to change BrowserElement
> to load the form.js file.
This already happens. Or at least it runs in desktop, so I don't know why anything should be changed?

Comment 24

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Gecko patch v3

Comment 25

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 794056 [details] [diff] [review]
Gecko patch v3

Hi Kan-Ru, tested this in self built FF nightly but can't get a working gecko atm on my device atm.

Comment 26

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Jan Jongboom [:janjongboom] from comment #23)
> (In reply to Kan-Ru Chen [:kanru] from comment #16)
> > Also we need to load Keyboard.jsm on Desktop if
> > dom.mozInputMethod.enabled is true. Also you need to change BrowserElement
> > to load the form.js file.
> This already happens. Or at least it runs in desktop, so I don't know why
> anything should be changed?

I mean on Firefox Nightly without any extension. This would enable us to write mochitests for MozInputMethod.

Comment 27

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 794056 [details] [diff] [review]
Gecko patch v3

Review of attachment 794056 [details] [diff] [review]:
-----------------------------------------------------------------

This patch won't work without the extension in gaia.

::: dom/webidl/InputMethod.webidl
@@ +91,5 @@
>     readonly attribute long selectionEnd;
>  
> +   // The start and stop position of the selection.
> +   readonly attribute DOMString? textBeforeCursor;
> +   readonly attribute DOMString? textAfterCursor;

Added by accident?

Comment 28

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Before the patch here can land, I tried to run our "ready-to-merge-to-master" IME framework with the current Nightly,
https://github.com/RudyLu/gaia/tree/3rdParty_keyboard_support_commit

But it seems the keyboard manager cannot get the chromeEvent for input focus.

I guess we are not allowed to land our Gaia patch first while it cannot work with nightly? 
We have a bunch of work that depends on that patch. :(

Comment 29

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: link to the pull request

Hi,

The last commit in the pull request is a Gaia workaround so that our new IME framework can work in the current version of Firefox Nightly.

This patch is derived from an attempt to land this framework to Gaia master sooner and make other dependent work go on.

Tim and Jan,

Could one of you review this patch?
I will file a follow-up bug if that get landed.

Comment 30

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 795019 [details] [review]
link to the pull request

This is fine as long as this gets addressed and we do remember to remove the stuff.

https://github.com/RudyLu/gaia/commit/9535647ab0bddee9227f637058e198f839093de6#commitcomment-3936291

Comment 31

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Kan-Ru Chen [:kanru] from comment #27)
> Comment on attachment 794056 [details] [diff] [review]
> Gecko patch v3
> 
> Review of attachment 794056 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch won't work without the extension in gaia.
> 
> ::: dom/webidl/InputMethod.webidl
> @@ +91,5 @@
> >     readonly attribute long selectionEnd;
> >  
> > +   // The start and stop position of the selection.
> > +   readonly attribute DOMString? textBeforeCursor;
> > +   readonly attribute DOMString? textAfterCursor;
> 
> Added by accident?
We just updated the API to add these two attributes(https://wiki.mozilla.org/WebAPI/KeboardIME#Proposed_API), but Jan, let's file another bug to add these attributes.

Comment 32

[Jan Jongboom [:janjongboom] (inactive)]

No, this stuff has been there since landing the first patch for the new IME, the tests depend on it, it just has been deleted when moving over to WebIDL. So let's keep it here.

Comment 33

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 793049 [details] [review]
Gaia patch

Rudy, (and Yuan, Kan-Ru and Tim :-)). The issue with the chrome event has been resolved in bug 909686, I have attached the commit to this PR as well so we can test it.

Comment 34

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 794056 [details] [diff] [review]
Gecko patch v3

Kan-Ru, can we make a follow up patch to remove dependency on desktop-helper completely instead of putting this into this issue as well? Think the major point was to get it up and running first. Additional work would not be required before the 1.2 freeze and the tests are currently not in mochi anyway.

Comment 35

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Jan Jongboom [:janjongboom] from comment #34)
> Comment on attachment 794056 [details] [diff] [review]
> Gecko patch v3
> 
> Kan-Ru, can we make a follow up patch to remove dependency on desktop-helper
> completely instead of putting this into this issue as well? Think the major
> point was to get it up and running first. Additional work would not be
> required before the 1.2 freeze and the tests are currently not in mochi
> anyway.

Well, enable mozInputMethod on Nightly was what bug 905567 was trying to achieve. Since you closed that issue I was expecting it to be fixed here ;)

Without removing forms.js and keyboard.jsm et al. from desktop-helper, tests will still break when we change something from the gecko side, so I really hope we could fix this as soon as possible.

Comment 36

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 794056 [details] [diff] [review]
Gecko patch v3

Review of attachment 794056 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the moz.build fixed.

You need a dom peer to review things under dom/

::: dom/inputmethod/moz.build
@@ +3,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +PARALLEL_DIRS += ['interfaces']

I think there is no such directory.

Comment 37

[Yuan Xulei [:yxl] (Leave from May 18)]

Reopen bug 905567 and let's finish regarding parts there:-)

Comment 38

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: 906096_v4.diff

Comment 39

[Yuan Xulei [:yxl] (Leave from May 18)]

The commit message should be like this ;-)

Bug 906096 - Move InputMethod API behind a pref instead of build option. r=kanru

Comment 40

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 796005 [details] [diff] [review]
906096_v4.diff

Jonas, if you're not the appropriate person to review this, please reassign.

Comment 41

[Jan Jongboom [:janjongboom] (inactive)]

Checkin needed for 906096_v4.diff.

Comment 42

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 793049 [details] [review]
Gaia patch

Hi Jan,

Sorry for the delay, finally I've got Firefox nightly built and run your tests. 

The tests could work, except as I mentioned in the pull request.
I might encounter an error if I clicked on an input field first (say the open search) and then tried to run your test.

If we can address that, that would be good.
Thanks.

Comment 43

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 793049 [details] [review]
Gaia patch

Looks good, so r=me if the question above will be addressed by the patch you mentioned.

Thanks again for this. 
Great work!

Comment 44

[Ryan VanderMeulen [:RyanVM]]

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

Comment 45

[Jan Jongboom [:janjongboom] (inactive)]

Gaia stuff landed in https://github.com/mozilla-b2g/gaia/commit/9a6e241416724a08dc1524569df96180c4ba0358

Please note that we need to land bug 909686 before the chrome events will work...

Comment 46

[Ryan VanderMeulen [:RyanVM]]

Please don't resolve bugs until the patch lands on mozilla-central. As it is, this had to be backed out due to completely breaking the B2G test framework.
https://hg.mozilla.org/integration/b2g-inbound/rev/fbc4a0581cda

https://tbpl.mozilla.org/php/getParsedLog.php?id=27127148&tree=B2g-Inbound

Comment 47

[Jan Jongboom [:janjongboom] (inactive)]

Yuan, any clue what this might be?

Comment 48

[Yuan Xulei [:yxl] (Leave from May 18)]

From the tbpl log, it seems the patch doen't export Keyboard.jsm properly. b2g/chrome/content/shell.js fails to import Keyboard.jsm and make the test framework broken.

Comment 49

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: 906096_v5.diff

Comment 50

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: 906096_v5.diff

Comment 51

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 797213 [details] [diff] [review]
906096_v5.diff

Jonas, Kan-Ru, Yuan, no clue what went wrong here but there was stuff missing from the patch which caused B2G desktop not to run anymore. I've added an up to date patch, from which I've built B2G desktop and all should be well.

Comment 52

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 797213 [details] [diff] [review]
906096_v5.diff

Review of attachment 797213 [details] [diff] [review]:
-----------------------------------------------------------------

Kan-Ru's review is enough here.

Comment 53

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 797213 [details] [diff] [review]
906096_v5.diff

Review of attachment 797213 [details] [diff] [review]:
-----------------------------------------------------------------

You forgot to move the files Keyboard.jsm and MozKeyboard.js to dom/inputmethod/

Actually I think we should extract the new MozInputMethod parts from Keyboard.jsm and MozKeyboard.js and leave MozKeyboard under b2g/. But if you want to move them together then I'm ok with that.

Please push to try server before checkin.

::: dom/inputmethod/Makefile.in
@@ +1,1 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public

You don't need this file.

Comment 54

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

(In reply to Jan Jongboom [:janjongboom] from comment #45)
> Gaia stuff landed in
> https://github.com/mozilla-b2g/gaia/commit/
> 9a6e241416724a08dc1524569df96180c4ba0358
> 
> Please note that we need to land bug 909686 before the chrome events will
> work...


In order to land 3rd-party IME framework, I just backed out this Gaia change with 74f170bba5bbe1649e3d38a02a1803d842c61698.
This is because this would break using ff nightly to run our new 3rd-party IME framework.

Comment 55

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Kan-Ru Chen [:kanru] from comment #53)
> Comment on attachment 797213 [details] [diff] [review]
> 906096_v5.diff
> 
> Review of attachment 797213 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You forgot to move the files Keyboard.jsm and MozKeyboard.js to
> dom/inputmethod/

No, it's there but the diff page doesn't show it. If you check the raw diff you'll see:

```
diff --git a/b2g/components/MozKeyboard.js b/dom/inputmethod/MozKeyboard.js
rename from b2g/components/MozKeyboard.js
rename to dom/inputmethod/MozKeyboard.js
```

So should be OK. I'll push to try server.

Comment 56

[Ed Morley [:emorley]]

Something in this gaia.json auto-commit:
https://hg.mozilla.org/integration/b2g-inbound/rev/31ca106e662c

...broke the gaia unit tests, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=27215367&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=27214598&tree=B2g-Inbound

The auto-commit corresponds to:
https://hg.mozilla.org/integration/gaia-central/pushloghtml?fromchange=c258483698afbf2c2b7f27f8fa16922d0ee48880&tochange=75e0fbd934e2c835db9ecd10ec6b2900b357d53c

So this and the others in that push have been backed out:
https://github.com/mozilla-b2g/gaia/commit/c2f23f132accb38af4a46f9171bc6be25fdb99a1
https://github.com/mozilla-b2g/gaia/commit/9a8098e8a6b86a32da0eef08e507ab689435dfee
https://github.com/mozilla-b2g/gaia/commit/884d0af45bc605c8e943e3080b2a909461795953
https://github.com/mozilla-b2g/gaia/commit/6a653be7c8b34f263342ead2037b4f8f3c2aff0e
https://github.com/mozilla-b2g/gaia/commit/cfcb28b5069054c1ad631c5bf4ed9c2a94e06d8b

Please run the gaia unit tests before pushing if possible! :-)

Comment 57

[Jan Jongboom [:janjongboom] (inactive)]

Waiting for access to try server. If someone else can push it, I'd be happy.

Comment 58

[Yuan Xulei [:yxl] (Leave from May 18)]

Could you help to test the Jan's gecko patch on try server?

Comment 59

[Kan-Ru Chen [:kanru] (UTC+9)]

https://tbpl.mozilla.org/?tree=Try&rev=d884e52fd86d

Comment 60

[Kan-Ru Chen [:kanru] (UTC+9)]

https://tbpl.mozilla.org/?tree=Try&rev=20e785fa30b4 try again

Comment 61

[Jan Jongboom [:janjongboom] (inactive)]

Thanks Kan-Ru. I don't understand why building B2G locally is fine and on try it complains about my makefile. Argh. /builds/slave/try-lx-d-000000000000000000000/build/config/rules.mk:54: *** Variable LIBRARY_NAME is defined in /builds/slave/try-lx-d-000000000000000000000/build/obj-firefox/dom/inputmethod/Makefile. It should only be defined in moz.build files.  Stop.


Anyway I'll wait for my Try access (if ever) because I want to avoid this async feedback loop. :-)

Comment 62

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Jan Jongboom [:janjongboom] from comment #61)
> Thanks Kan-Ru. I don't understand why building B2G locally is fine and on
> try it complains about my makefile. Argh.
> /builds/slave/try-lx-d-000000000000000000000/build/config/rules.mk:54: ***
> Variable LIBRARY_NAME is defined in
> /builds/slave/try-lx-d-000000000000000000000/build/obj-firefox/dom/
> inputmethod/Makefile. It should only be defined in moz.build files.  Stop.
> 
> 
> Anyway I'll wait for my Try access (if ever) because I want to avoid this
> async feedback loop. :-)

I forgot to remove the Makefile.in. See the second try which builds fine. (comment 60)

Comment 63

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Gecko patch v6

This is the gecko patch as sent to the try server. Thanks a ton Kan-Ru. I'll buy you a beer :-)

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/008139ec2abb

Comment 65

[Ryan VanderMeulen [:RyanVM]]

Backed out for Gaia UI test and B2G mochitest-4 failures.
https://hg.mozilla.org/integration/b2g-inbound/rev/bf1919549854

https://tbpl.mozilla.org/php/getParsedLog.php?id=27375924&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=27374428&tree=B2g-Inbound

Comment 66

[Ryan VanderMeulen [:RyanVM]]

This also caused B2G mochitest-3 failures.
https://tbpl.mozilla.org/php/getParsedLog.php?id=27376825&tree=B2g-Inbound

Comment 67

[Jan Jongboom [:janjongboom] (inactive)]

Argh #&*#

Comment 68

[Jan Jongboom [:janjongboom] (inactive)]

Alright, I have a green TRY @ https://tbpl.mozilla.org/?tree=Try&rev=946609699177&jobname=b2g_try_emulator_dep ...

Comment 69

[Jan Jongboom [:janjongboom] (inactive)]

Nominating koi? for blocking bug 912951

Comment 70

[Ryan VanderMeulen [:RyanVM]]

I don't see any tests run on that push. Not landing until I see passing tests. This should give you what you want:
try: -b o -p emulator,linux64_gecko -u mochitests,gaia-ui-test -t none

Comment 71

[Jan Jongboom [:janjongboom] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=88e6ed3e12c5

Comment 72

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Gaia patch (needs relanding)

Comment 73

[Jan Jongboom [:janjongboom] (inactive)]

So I ran the mochitests locally (finally!) and here they pass. Pf.

Comment 74

[Kan-Ru Chen [:kanru] (UTC+9)]

\o/

Comment 75

[Jan Jongboom [:janjongboom] (inactive)]

No, not \o/ because they still fail on try :p

Comment 76

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Hi all,
   I found keyboard manager will call |inputFocusChange(evt)| twice both in Desktop and Devices, since as following code.

   https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/keyboard_manager.js#L122
   https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/keyboard_manager.js#L130

   Can we remove line#130?

Comment 77

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to GaryChen [:GaryChen] from comment #76)
>   
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> keyboard_manager.js#L130
> 
>    Can we remove line#130?
Yes, we don't need it after bug 909686 was fixed.

Comment 78

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Gecko patch v6 rebased

I rebased your patch and it seems everything is fine...

Jan, can you push it to try?

Comment 79

[Jan Jongboom [:janjongboom] (inactive)]

Try running @ https://tbpl.mozilla.org/?tree=Try&rev=fd2a7cb435bc

Comment 80

[Jan Jongboom [:janjongboom] (inactive)]

Try is still orange

Comment 81

[Yuan Xulei [:yxl] (Leave from May 18)]

From the mochitest log, I find the following errors:
 [JavaScript Error: "Keyboard is undefined" {file: "chrome://browser/content/shell.js" line: 1071}]

shell.js failed to import |Keyboard| on emulator from 
 Cu.import('resource://gre/modules/Keyboard.jsm'); 

To resolve it we need change the following lines in Keyboard.jsm

  let Keyboard = {
  ...
  }

to
  this.Keyboard = {
  ...
  }

Comment 82

[Jan Jongboom [:janjongboom] (inactive)]

Didn't help... https://tbpl.mozilla.org/?tree=Try&rev=b35bf65d80ec

Comment 83

[Yuan Xulei [:yxl] (Leave from May 18)]

Hi Jan,  the following error has been fixed:
 [JavaScript Error: "Keyboard is undefined" {file: "chrome://browser/content/shell.js" line: 1071}]

But it is not the cause that makes mochitest failed.

It seems the cause of the mochitest 3's failure is that mentioned in comment 13. Without keyboard permission, we can still access navigator.mozInputMethod...
But I still don't know what causes mochitest 4 failed.

Comment 84

[Jan Jongboom [:janjongboom] (inactive)]

Comment 13 is already addressed, so now we're back in the dark again...

Comment 85

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Gecko patch v7

I didn't notice gaia-ui test was busted.

I made gaia-ui test green by adding `MozKeyboard.js` and `InputMethod.manifest` to b2g/installer/package-manifest.in and browser/installer/package-manifest.in.

https://tbpl.mozilla.org/?tree=Try&rev=b956a76b5951

We only need to fix the mochitest now.
The dawn is coming.

Comment 86

[Yuan Xulei [:yxl] (Leave from May 18)]

It seems we have resolved the mochitest issue with patch of comment 85:

https://bugzilla.mozilla.org/attachment.cgi?id=810342

Let's wait for the full tests from try server:

https://tbpl.mozilla.org/?tree=Try&rev=1253760499a7

Comment 87

[Jan Jongboom [:janjongboom] (inactive)]

*Making drum roll*

Comment 88

[Yuan Xulei [:yxl] (Leave from May 18)]

The patch hadn't been pushed to the try server in Comment 86 :-(

Let's have another try:
https://tbpl.mozilla.org/?tree=Try&rev=a4d7c7b21434

Anyway b2g emulator is green: 
https://tbpl.mozilla.org/?tree=Try&rev=79b7eed61328

Comment 89

[Ryan VanderMeulen [:RyanVM]]

I don't see those xpcshell failures on any other trees. Running a quick spot-check on Try before pushing.
https://tbpl.mozilla.org/?tree=Try&rev=0a056196d770

Comment 90

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #89)
> I don't see those xpcshell failures on any other trees. Running a quick
> spot-check on Try before pushing.
> https://tbpl.mozilla.org/?tree=Try&rev=0a056196d770

Yep, they're real. Sorry :(

Comment 91

[Yuan Xulei [:yxl] (Leave from May 18)]

Thank you, Ryan.

security/manager/ssl/tests/unit/test_signed_apps-marketplace.js assumes the keyboard component only available on b2g platform with the following code:

 const isB2G = ("@mozilla.org/b2g-keyboard;1" in Components.classes);

The patch of this bug makes the assumption false and then breaks this xpcshell test. We need another way to detect the b2g platform.

Comment 92

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: xpcshell-fix

Brian, could you help to review? A small fix with test_signed_apps-marketplace.js.

Comment 93

[[:fabrice] Fabrice Desré]

Comment on attachment 810622 [details] [diff] [review]
xpcshell-fix

Review of attachment 810622 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/tests/unit/test_signed_apps-marketplace.js
@@ +1,3 @@
>  "use strict";
>  
> +const isB2G = ("@mozilla.org/network/protocol;1?name=sms" in Components.classes);

hu, that's not really better. Use something like instead:

// see b2g/confvars.sh for the app id
Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo).ID == "{3c2e2abc-06d4-11e1-ac3b-374f68613e61}"

Comment 94

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Fabrice Desré [:fabrice] from comment #93)
> Comment on attachment 810622 [details] [diff] [review]
> xpcshell-fix
> 
> Review of attachment 810622 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/manager/ssl/tests/unit/test_signed_apps-marketplace.js
> @@ +1,3 @@
> >  "use strict";
> >  
> > +const isB2G = ("@mozilla.org/network/protocol;1?name=sms" in Components.classes);
> 
> hu, that's not really better. Use something like instead:
> 
> // see b2g/confvars.sh for the app id
> Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo).ID ==
> "{3c2e2abc-06d4-11e1-ac3b-374f68613e61}"

The following code runs correctly on firefox:

  const isB2G = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo).ID === "{3c2e2abc-06d4-11e1-ac3b-374f68613e61}";

But throws an error with xpcshell test:

  Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"

It seems we can't get app info when running xpcshell test.

Comment 95

[[:fabrice] Fabrice Desré]

:( Then use one component that we are sure to only have in b2g, like @mozilla.org/b2g-process-global;1

Comment 96

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: xpcshell-fix

Use '@mozilla.org/b2g-process-global;1' to detect b2g platform.

Comment 97

[[:fabrice] Fabrice Desré]

Comment on attachment 810687 [details] [diff] [review]
xpcshell-fix

Review of attachment 810687 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment addressed

::: security/manager/ssl/tests/unit/test_signed_apps-marketplace.js
@@ +1,4 @@
>  "use strict";
>  
> +const isB2G = ("@mozilla.org/b2g-process-global;1" in Cc); 
> +              

Please remove all this trailing whitespace.

Comment 98

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: part1 gecko patch v7 rebased

Rebased for checking in.

Comment 99

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: part2 gecko patch

Remove trailing white spaces.

Comment 100

[Yuan Xulei [:yxl] (Leave from May 18)]

https://tbpl.mozilla.org/?tree=Try&rev=f96039510489

Comment 101

[Ryan VanderMeulen [:RyanVM]]

Gecko: https://hg.mozilla.org/integration/b2g-inbound/rev/20fd078c8c25
Master: https://github.com/mozilla-b2g/gaia/commit/680af02b60247df4afb823680d244b7e503ad25a

Comment 102

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Needs to go to aurora and gaia/v1.2 too.

Comment 103

[Ryan VanderMeulen [:RyanVM]]

See my earlier comment. We have bug queries to handle uplifts.

Comment 104

[Ryan VanderMeulen [:RyanVM]]

Gaia patch reverted due to Gaia UI test & Travis failures.
https://github.com/mozilla-b2g/gaia/commit/73455116277ace93108d4f0992535338562fefc6

https://tbpl.mozilla.org/php/getParsedLog.php?id=28465565&tree=B2g-Inbound

Setting this to [leave open] so it isn't resolved when the Gecko bits hit m-c.

Comment 105

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

https://hg.mozilla.org/mozilla-central/rev/20fd078c8c25

Comment 106

[Yuan Xulei [:yxl] (Leave from May 18)]

Hi Jan, gecko part has been landed and can you fix gaia part?

Comment 107

[Jan Jongboom [:janjongboom] (inactive)]

Yeah spoke to Ryan about it on Friday already. Will do on Monday.

Comment 108

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5347bbc42e0c

The Gaia patch still needs landing on master and uplifting to the v1.2 branch. When that is done, please resolve the bug and set status-b2g-v1.2 to fixed. In the future, it's easier for tracking purposes to have separate bugs for Gecko and Gaia changes, FWIW :)

Comment 109

[Jan Jongboom [:janjongboom] (inactive)]

Relanded gaia in https://github.com/mozilla-b2g/gaia/commit/900348f2118b7d17e0c90f1942976d7da547dcc9 ... Let's see...

Comment 112

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 900348f2118b7d17e0c90f1942976d7da547dcc9 to:
v1.2: 1d147155c8a56b44ae4a8d89afc6eff5e19c6cc9