Closed Bug 1197682 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(2 files, 6 obsolete files)

Let's replace the mozContentEvent with a system-app only method.
Attached patch bug1197682.patch (obsolete) — Splinter Review
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.
Attachment #8654753 - Flags: superreview?(bugs)
Attachment #8654753 - Flags: review?(janjongboom)
... and this is the patch remove the mozContentEvent glue from shell.js.
Attachment #8654754 - Flags: review?(janjongboom)
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.)
Attachment #8654753 - Flags: superreview?(bugs) → superreview+
What commit did you branched this off? Doesn't apply clean on mc.
Flags: needinfo?(timdream)
Attached patch bug1197682.patch (obsolete) — Splinter Review
It should be on top of bug 1197700. Patch rebased to apply on the new patch there.
Attachment #8654753 - Attachment is obsolete: true
Attachment #8654753 - Flags: review?(janjongboom)
Flags: needinfo?(timdream)
Attachment #8658520 - Flags: superreview+
Attachment #8658520 - Flags: review?(janjongboom)
FYI 

bug1197682.patch:562: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
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
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.
Flags: needinfo?(timdream)
(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...
Flags: needinfo?(timdream)
(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.
Attached patch bug1197682.patch (obsolete) — Splinter Review
comment addressed.
Attachment #8658520 - Attachment is obsolete: true
Attachment #8658520 - Flags: review?(janjongboom)
Attachment #8659024 - Flags: superreview+
Attachment #8659024 - Flags: review?(janjongboom)
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. :)
Attachment #8654754 - Attachment filename: bug1197682-removeContentEvent.patch → <to be moved> bug1197682-removeContentEvent.patch
Attachment #8654754 - Flags: review?(janjongboom)
Attachment #8654754 - Attachment description: bug1197682-removeContentEvent.patch → <to be moved> bug1197682-removeContentEvent.patch
Attachment #8654754 - Attachment filename: <to be moved> bug1197682-removeContentEvent.patch → bug1197682-removeContentEvent.patch
Comment on attachment 8659024 [details] [diff] [review]
bug1197682.patch

r=me for Keyboard.jsm except for changes in receiveMessage, MozKeyboard.js and test.
Attachment #8659024 - Flags: review?(janjongboom) → review+
(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.
Attached patch Patch to commit (obsolete) — Splinter Review
Attachment #8659024 - Attachment is obsolete: true
Attachment #8659188 - Flags: superreview+
Attachment #8659188 - Flags: review+
Attachment #8659188 - Attachment is obsolete: false
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.
Attached patch Patch to commit (obsolete) — Splinter Review
Actually get the order right.
Attachment #8659188 - Attachment is obsolete: true
Attachment #8659698 - Flags: superreview+
Attachment #8659698 - Flags: review+
Attached patch Patch to commentSplinter Review
Rebase against bug 1197700.
Attachment #8659698 - Attachment is obsolete: true
Attachment #8659735 - Flags: superreview+
Attachment #8659735 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/58a0ed06b69c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.