1197682 - [Input Method API] Remove mozContentEvent usage of "supportsSwitching()".

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

Let's replace the mozContentEvent with a system-app only method.

Comment 1

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

Attachment: bug1197682.patch

This patches implements |navigator.mozInputMethod.setSupportsSwitchingTypes()| and hide it behind input-manage permission for System app. Doing so we remove the usage of mozContentEvent, and also make the interface WebIDL-compliant, and most importantly, testable.

I am simplifying the permission checking part of receiveMessage() in Keyboard.jsm because assumption in bug 971651 is no longer correct -- we should still handle the messages even if there isn't a formMM, and we should assert permissions for register and unregister messages.

There is another patch that would actually remove the mozContentEvent listener. I will attach that to this bug and merge it under another bug #.

Here is the try for the first patch, obviously second patch cannot be tested w/o Gaia patch.

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

Jan, could you help and review this? Feel free to pass if you are not available or you don't feel you should review this.

:smaug, with bug 1197700 the WebIDL change is much more clear. Could you super review this? Thanks.

Comment 2

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

Attachment: <to be moved> bug1197682-removeContentEvent.patch

... and this is the patch remove the mozContentEvent glue from shell.js.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8654753 [details] [diff] [review]
bug1197682.patch

(I'm still not quite sure why 'input-manage' can't see all the stuff app with 'input' can, though, I guess this is a way for force certain API usage. So fine.)

Comment 4

[Jan Jongboom [:janjongboom] (inactive)]

What commit did you branched this off? Doesn't apply clean on mc.

Comment 5

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

Attachment: bug1197682.patch

It should be on top of bug 1197700. Patch rebased to apply on the new patch there.

Comment 6

[Jan Jongboom [:janjongboom] (inactive)]

FYI 

bug1197682.patch:562: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

Comment 7

[Jan Jongboom [:janjongboom] (inactive)]

2. We should also fix in InputMethod.webidl, it says 'recommented' not 'recommended' above supportsSwitching.
3. mochitest.ini used to be sorted alphabetically, but not anymore

Comment 8

[Jan Jongboom [:janjongboom] (inactive)]

tim, do you know if/how I can send a chrome event from a test?

  var event = document.createEvent('CustomEvent');
  event.initCustomEvent('mozContentEvent', true, true, {
    type: 'inputmethod-update-layouts',
    layouts: {
      'text': 2,
      'password': 2,
      'number': 1
    }
  });
  window.dispatchEvent(event);

does not work... I think I need to set a 'Im a system app' call somewhere, but where.

Comment 9

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

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #7)
> 2. We should also fix in InputMethod.webidl, it says 'recommented' not
> 'recommended' above supportsSwitching.
> 3. mochitest.ini used to be sorted alphabetically, but not anymore

I can address these. Thanks!

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #8)
> tim, do you know if/how I can send a chrome event from a test?
> 
>   var event = document.createEvent('CustomEvent');
>   event.initCustomEvent('mozContentEvent', true, true, {
>     type: 'inputmethod-update-layouts',
>     layouts: {
>       'text': 2,
>       'password': 2,
>       'number': 1
>     }
>   });
>   window.dispatchEvent(event);
> 
> does not work... I think I need to set a 'Im a system app' call somewhere,
> but where.

You can't, that's why we want to replace mozChromeEvent/mozContentEvent with API methods/events defined by WebIDL...

Comment 10

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

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #9)
> You can't, that's why we want to replace mozChromeEvent/mozContentEvent with
> API methods/events defined by WebIDL...

There was a lengthy discussion on dev-b2g titled "Proposal to get rid of some of the shell.js <-> System app custom events, a.k.a. mozChromeEvent" FYI.

Comment 11

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

Attachment: bug1197682.patch

comment addressed.

Comment 12

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

Comment on attachment 8654754 [details] [diff] [review]
<to be moved> bug1197682-removeContentEvent.patch

Let's wait for Gaia to switch over so you could actually test on it, if you insist. :)

Comment 13

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8659024 [details] [diff] [review]
bug1197682.patch

r=me for Keyboard.jsm except for changes in receiveMessage, MozKeyboard.js and test.

Comment 14

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #10)
> (In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to
> queue) from comment #9)
> > You can't, that's why we want to replace mozChromeEvent/mozContentEvent with
> > API methods/events defined by WebIDL...
> 
> There was a lengthy discussion on dev-b2g titled "Proposal to get rid of
> some of the shell.js <-> System app custom events, a.k.a. mozChromeEvent"
> FYI.

Yeah I agree, but I wanted to get the test working against old codebase to verify we don't break stuff. I mocked that out now while testing, so I'm fairly confident we're not breaking anything.

Comment 15

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

Attachment: Patch to commit

Comment 16

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

Attachment: bug1197700.patch

I think below is what's needed since the patch now hides the constructors behind a permission. Strangely, MozInputMethod (constructor of navigator.mozInputMethod) is not listed in the test.

:smaug, could you bless the additional change with review? Thanks.

diff --git a/dom/tests/mochitest/general/test_interfaces.html b/dom/tests/mochitest/general/test_interfaces.html
--- a/dom/tests/mochitest/general/test_interfaces.html
+++ b/dom/tests/mochitest/general/test_interfaces.html
@@ -781,19 +781,19 @@ var interfaceNamesInGlobalScope =
     "MozContactChangeEvent",
 // IMPORTANT: Do not change this list without review from a DOM peer!
     "MozCSSKeyframeRule",
 // IMPORTANT: Do not change this list without review from a DOM peer!
     "MozCSSKeyframesRule",
 // IMPORTANT: Do not change this list without review from a DOM peer!
     {name: "MozEmergencyCbModeEvent", b2g: true},
 // IMPORTANT: Do not change this list without review from a DOM peer!
-    {name: "MozInputContext", b2g: true},
-// IMPORTANT: Do not change this list without review from a DOM peer!
-    {name: "MozInputMethodManager", b2g: true},
+    {name: "MozInputContext", b2g: true, permission: ["input"]},
+// IMPORTANT: Do not change this list without review from a DOM peer!
+    {name: "MozInputMethodManager", b2g: true, permission: ["input"]},
 // IMPORTANT: Do not change this list without review from a DOM peer!
     "MozMmsMessage",
 // IMPORTANT: Do not change this list without review from a DOM peer!
     {name: "MozMobileCellInfo", b2g: true},
 // IMPORTANT: Do not change this list without review from a DOM peer!
     {name: "MozMobileConnection", b2g: true},
 // IMPORTANT: Do not change this list without review from a DOM peer!
     {name: "MozMobileConnectionArray", b2g: true},

Comment 17

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

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

Comment 18

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

Comment on attachment 8659264 [details] [diff] [review]
bug1197700.patch

Wrong bug.

Comment 19

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

I am removing checkin-needed because this should not reach inbound before bug 1197700 and it should not be merged into m-c in the same merge.

Comment 20

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

Attachment: Patch to commit

Actually get the order right.

Comment 21

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

Attachment: Patch to comment

Rebase against bug 1197700.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58a0ed06b69c

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/58a0ed06b69c