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

RESOLVED FIXED

Status

Firefox OS
Gaia
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kgrandon, Assigned: janjongboom)

Tracking

unspecified
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+, firefox25 wontfix, firefox26 fixed, firefox27 fixed, b2g-v1.2 fixed)

Details

Attachments

(4 attachments, 12 obsolete attachments)

46 bytes, text/x-github-pull-request
timdream
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
janjongboom
: review+
Details | Review | Splinter Review
21.51 KB, patch
yxl
: review+
Details | Diff | Splinter Review
794 bytes, patch
yxl
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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.
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).
Sidenote: need to fix the IME tests as well because of API changes.
(Assignee)

Updated

4 years ago
Assignee: nobody → janjongboom
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.
Flags: needinfo?(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).
Flags: needinfo?(bzbarsky)
I think we need wait for bug 905567 - Expose navigator.MozInputMethod to desktop build.
Depends on: 905567
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.
Created attachment 793048 [details] [diff] [review]
Gecko patch
Attachment #793048 - Flags: review?(kchen)
Created attachment 793049 [details] [review]
Gaia patch
Attachment #793049 - Flags: review?(rlu)
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.
(Assignee)

Updated

4 years ago
Duplicate of this bug: 905567
(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 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.
Attachment #793048 - Flags: review?(kchen) → feedback-
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...
> 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.
Created attachment 793381 [details] [diff] [review]
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.
Attachment #793048 - Attachment is obsolete: true
Attachment #793381 - Flags: review?(kchen)
(Assignee)

Updated

4 years ago
Summary: Ensure keyboard continues to work in Firefox Nightly → Ensure keyboard continues to work in Firefox Nightly + fix tests because of move to WebIDL
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.
Attachment #793381 - Flags: review?(kchen) → review-
(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 ?
(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 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.
Attachment #793049 - Flags: review?(rlu)
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.
(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.
(if you want a prebuilt version for OS/X I can give it to you)
(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?
(Assignee)

Updated

4 years ago
Depends on: 908175
Created attachment 794056 [details] [diff] [review]
Gecko patch v3
Attachment #793381 - Attachment is obsolete: true
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.
Attachment #794056 - Flags: review?(kchen)
(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 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?
Attachment #794056 - Flags: review?(kchen) → review-
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. :(
Created attachment 795019 [details] [review]
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.
Attachment #795019 - Flags: review?(timdream)
Attachment #795019 - Flags: review?(janjongboom)
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
Attachment #795019 - Flags: review?(timdream)
Attachment #795019 - Flags: review?(janjongboom)
Attachment #795019 - Flags: review+
(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.
Flags: needinfo?(janjongboom)
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.
Flags: needinfo?(janjongboom)
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.
Attachment #793049 - Flags: review?(rlu)
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.
Attachment #794056 - Flags: review- → review?(kchen)
(Assignee)

Updated

4 years ago
Depends on: 909686
(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 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.
Attachment #794056 - Flags: review?(kchen) → review+
Reopen bug 905567 and let's finish regarding parts there:-)
Created attachment 796005 [details] [diff] [review]
906096_v4.diff
Attachment #794056 - Attachment is obsolete: true
Attachment #796005 - Flags: review+
The commit message should be like this ;-)

Bug 906096 - Move InputMethod API behind a pref instead of build option. r=kanru
Comment on attachment 796005 [details] [diff] [review]
906096_v4.diff

Jonas, if you're not the appropriate person to review this, please reassign.
Attachment #796005 - Flags: review?(jonas)
Attachment #796005 - Flags: review?(jonas) → review+
Checkin needed for 906096_v4.diff.
Keywords: checkin-needed
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.
Attachment #793049 - Flags: review?(rlu)
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!
Attachment #793049 - Flags: review+
https://hg.mozilla.org/integration/b2g-inbound/rev/cd6c3d55bbc9
Keywords: checkin-needed
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...
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Yuan, any clue what this might be?
Flags: needinfo?(xyuan)
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.
Flags: needinfo?(xyuan)
Created attachment 797212 [details] [diff] [review]
906096_v5.diff
Created attachment 797213 [details] [diff] [review]
906096_v5.diff
Attachment #797212 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #797212 - Flags: review?(kchen)
(Assignee)

Updated

4 years ago
Attachment #797212 - Flags: review?(kchen)
(Assignee)

Updated

4 years ago
Attachment #796005 - Attachment is obsolete: true
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.
Attachment #797213 - Flags: review?(kchen)
(Assignee)

Updated

4 years ago
Attachment #797213 - Flags: review?(jonas)
Comment on attachment 797213 [details] [diff] [review]
906096_v5.diff

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

Kan-Ru's review is enough here.
Attachment #797213 - Flags: review?(jonas)
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.
Attachment #797213 - Flags: review?(kchen) → review+
(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.
(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.
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! :-)
Waiting for access to try server. If someone else can push it, I'd be happy.
Could you help to test the Jan's gecko patch on try server?
Flags: needinfo?(kchen)
https://tbpl.mozilla.org/?tree=Try&rev=d884e52fd86d
Flags: needinfo?(kchen)
https://tbpl.mozilla.org/?tree=Try&rev=20e785fa30b4 try again
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. :-)
(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)
Created attachment 799360 [details] [diff] [review]
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 :-)
Attachment #797213 - Attachment is obsolete: true
Attachment #799360 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/008139ec2abb
Keywords: checkin-needed
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
This also caused B2G mochitest-3 failures.
https://tbpl.mozilla.org/php/getParsedLog.php?id=27376825&tree=B2g-Inbound
Argh #&*#
Alright, I have a green TRY @ https://tbpl.mozilla.org/?tree=Try&rev=946609699177&jobname=b2g_try_emulator_dep ...
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Blocks: 912951
Nominating koi? for blocking bug 912951
blocking-b2g: --- → koi?
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
Keywords: checkin-needed
https://tbpl.mozilla.org/?tree=Try&rev=88e6ed3e12c5
Created attachment 802175 [details] [review]
Gaia patch (needs relanding)
Attachment #793049 - Attachment is obsolete: true
Attachment #802175 - Flags: review+
So I ran the mochitests locally (finally!) and here they pass. Pf.
\o/
No, not \o/ because they still fail on try :p
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?
(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.
Created attachment 803913 [details] [diff] [review]
Gecko patch v6 rebased

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

Jan, can you push it to try?
Attachment #799360 - Attachment is obsolete: true
Try running @ https://tbpl.mozilla.org/?tree=Try&rev=fd2a7cb435bc
Try is still orange
Flags: needinfo?(xyuan)
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 = {
  ...
  }
Flags: needinfo?(xyuan)
Didn't help... https://tbpl.mozilla.org/?tree=Try&rev=b35bf65d80ec
Flags: needinfo?(xyuan)
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.
Flags: needinfo?(xyuan)
(Assignee)

Updated

4 years ago
No longer blocks: 912951
Comment 13 is already addressed, so now we're back in the dark again...
Flags: needinfo?(xyuan)
Created attachment 810342 [details] [diff] [review]
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.
Attachment #803913 - Attachment is obsolete: true
Flags: needinfo?(xyuan)
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
*Making drum roll*
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
Attachment #810342 - Flags: review+
Keywords: checkin-needed
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
(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 :(
Keywords: checkin-needed
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.
Created attachment 810622 [details] [diff] [review]
xpcshell-fix

Brian, could you help to review? A small fix with test_signed_apps-marketplace.js.
Attachment #810622 - Flags: review?(brian)
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}"
Attachment #810622 - Flags: review?(brian) → review-
(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.
Flags: needinfo?(fabrice)
:( Then use one component that we are sure to only have in b2g, like @mozilla.org/b2g-process-global;1
Flags: needinfo?(fabrice)
Created attachment 810687 [details] [diff] [review]
xpcshell-fix

Use '@mozilla.org/b2g-process-global;1' to detect b2g platform.
Attachment #810622 - Attachment is obsolete: true
Attachment #810687 - Flags: review?(fabrice)
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.
Attachment #810687 - Flags: review?(fabrice) → review+
Created attachment 810923 [details] [diff] [review]
part1 gecko patch v7 rebased

Rebased for checking in.
Attachment #810342 - Attachment is obsolete: true
Attachment #810923 - Flags: review+
Created attachment 810925 [details] [diff] [review]
part2 gecko patch

Remove trailing white spaces.
Attachment #810687 - Attachment is obsolete: true
Attachment #810925 - Flags: review+
https://tbpl.mozilla.org/?tree=Try&rev=f96039510489
Keywords: checkin-needed
Gecko: https://hg.mozilla.org/integration/b2g-inbound/rev/20fd078c8c25
Master: https://github.com/mozilla-b2g/gaia/commit/680af02b60247df4afb823680d244b7e503ad25a
Keywords: checkin-needed
Needs to go to aurora and gaia/v1.2 too.
blocking-b2g: koi? → koi+
status-b2g-v1.2: --- → affected
Keywords: checkin-needed
See my earlier comment. We have bug queries to handle uplifts.
Keywords: checkin-needed
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.
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/20fd078c8c25
status-firefox25: --- → wontfix
status-firefox26: --- → affected
status-firefox27: --- → fixed
Whiteboard: [leave open] → [leave open][checkin-needed-aurora]
Hi Jan, gecko part has been landed and can you fix gaia part?
Flags: needinfo?(janjongboom)
Yeah spoke to Ryan about it on Friday already. Will do on Monday.
Flags: needinfo?(janjongboom)
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 :)
status-firefox26: affected → fixed
Whiteboard: [leave open][checkin-needed-aurora] → [Gecko patch landed and uplifted - needs Gaia landing & uplift]
Blocks: 920977
(Assignee)

Updated

4 years ago
Depends on: 924893
(Assignee)

Updated

4 years ago
No longer depends on: 924893
Relanded gaia in https://github.com/mozilla-b2g/gaia/commit/900348f2118b7d17e0c90f1942976d7da547dcc9 ... Let's see...
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Duplicate of this bug: 924733
Whiteboard: [Gecko patch landed and uplifted - needs Gaia landing & uplift]
Duplicate of this bug: 905567
Uplifted 900348f2118b7d17e0c90f1942976d7da547dcc9 to:
v1.2: 1d147155c8a56b44ae4a8d89afc6eff5e19c6cc9
status-b2g-v1.2: affected → fixed
Attachment mime type: text/plain text/plain → text/x-github-pull-request text/x-github-pull-request
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.