1201407 - Replace mozChromeEvent/mozContentEvent with real events in InputMethod API

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

Let's work on the remaining events in one off patch. I have a WIP ready.

Comment 1

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

Attachment: bug1201407.patch

This is the current WIP and I want to keep a copy here. I tried to create new DOM Events by relying on codegen, but it turned out codegen can't emit an C impl of an Event that has arrays or objects as properties. Nor does the Event can contain any methods.

I will update the patch and have all events fall back to the CustomEvent instead. I think I can still document the object properties exposed from evt.detail with WebIDL, so I will try that.

Comment 2

[[:fabrice] Fabrice Desré]

If your webidl is valid, you should ask someone like bz if the codegen can be fixed.

Comment 3

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

(In reply to [:fabrice] Fabrice Desré from comment #2)
> If your webidl is valid, you should ask someone like bz if the codegen can
> be fixed.

It doesn't make sense to make this bug blocked by a codegen feature. I will certainly file separate bugs for these features.

Comment 4

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

Attachment: bug1201407.patch

This is my second failed attempt -- which try to expose a WebIDL-defined interface from evt.detail of the CustomEvent.

It mostly works except two:

1) All the "dictionary attributes" should be converted to interfaces, which makes the code even more complex -- I don't know if the generated code accepts JS "interface" implemented by static JS objects or if I need to create constructors for all of these interfaces.

2) The method |waitUntil()| exposed from the interface is not callable -- calling from the content results an error "Property waitUntil() is not callable" when typeof detail.waitUntil() does return "function".

I am just converting this patch into simple Cu.cloneInto() and finish it...

Comment 5

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

Comment on attachment 8657670 [details] [diff] [review]
bug1201407.patch

The silver lining is all the written IDLs are posted on Bugzilla so we could recycled it once I realized how to properly implement them.

Comment 6

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

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #4)
> Created attachment 8657670 [details] [diff] [review]
> bug1201407.patch
> 
> This is my second failed attempt -- which try to expose a WebIDL-defined
> interface from evt.detail of the CustomEvent.
> 
> It mostly works except two:
> 
> 1) All the "dictionary attributes" should be converted to interfaces, which
> makes the code even more complex -- I don't know if the generated code
> accepts JS "interface" implemented by static JS objects or if I need to
> create constructors for all of these interfaces.
> 
> 2) The method |waitUntil()| exposed from the interface is not callable --
> calling from the content results an error "Property waitUntil() is not
> callable" when typeof detail.waitUntil() does return "function".
> 
> I am just converting this patch into simple Cu.cloneInto() and finish it...

I think I found the problem ...

Comment 7

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

Attachment: bug1201407.patch

This is the patch that exposes the event properties detail object of CustomEvent, and with detail objects defined in WebIDL.

It turned out I don't need to implement a JS constructor to implement an WebIDL interface; I can expose an JS object as a defined WebIDL interface.

It also turned out evt.detail object can contain methods. It was a silly mistake of mine.

With this patch I've moved all instance of mozChomeEvent usage into the IM API itself; once Gaia have moved away from mozChromeEvent we can safely remove all of them. I will plan the landing for us to have some overlap between Gaia & Gecko version to ensure there is least disruptions.

Noted that the waitUntil() design is copied from Service Worker ExtendableEvent; platform awaits for the passed promise to resolve/reject.

:smaug, could you super review one more WebIDL change?

:jan, this is the last non-trivial Gecko patch to review. Thanks.

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

Comment 8

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

Attachment: bug1201407-removeContentEvent.patch

... and this is the patch to remove mozChromeEvents from InputMethod API.

Comment 9

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

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

shell.js stuff needs to be removed too.

Comment 10

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

Comment on attachment 8657711 [details] [diff] [review]
bug1201407.patch

>+  /**
>+   * CustomEvent dispatches to System when there is an input to handle.
>+   * If the API consumer failed to handle and call preventDefault(),
>+   * there will be a message printed on the console.
>+   *
>+   * evt.detail is defined by MozInputContextFocusEventDetail.
>+   */
>+  [CheckAnyPermissions="input-manage"]
>+  attribute EventHandler oninputcontextfocus;
>+
>+  /**
>+   * Event dispatches to System when there is no longer an input to handle.
>+   * If the API consumer failed to handle and call preventDefault(),
>+   * there will be a message printed on the console.
>+   */
>+  [CheckAnyPermissions="input-manage"]
>+  attribute EventHandler oninputcontextblur;
>+
>+  /**
>+   * Event dispatches to System when there is a showAll() call.
>+   * If the API consumer failed to handle and call preventDefault(),
>+   * there will be a message printed on the console.
>+   */
>+  [CheckAnyPermissions="input-manage"]
>+  attribute EventHandler onshowallrequest;
>+
>+  /**
>+   * Event dispatches to System when there is a next() call.
>+   * If the API consumer failed to handle and call preventDefault(),
>+   * there will be a message printed on the console.
>+   */
>+  [CheckAnyPermissions="input-manage"]
>+  attribute EventHandler onnextrequest;
>+
>+  /**
>+   * Event dispatches to System when there is a addInput() call.
>+   * The API consumer must call preventDefault() to indicate the event is
>+   * consumed, otherwise the request is not considered handled even if
>+   * waitUntil() was called.
>+   *
>+   * evt.detail is defined by MozInputRegistryEventDetail.
>+   */
>+  [CheckAnyPermissions="input-manage"]
>+  attribute EventHandler onaddinputrequest;
>+
>+  /**
>+   * Event dispatches to System when there is a removeInput() call.
>+   * The API consumer must call preventDefault() to indicate the event is
>+   * consumed, otherwise the request is not considered handled even if
>+   * waitUntil() was called.
>+   *
>+   * evt.detail is defined by MozInputRegistryEventDetail.
>+   */
>+  [CheckAnyPermissions="input-manage"]
>+  attribute EventHandler onremoveinputrequest;
>+};

So is the same instance of MozInputMethodManager used in different contexts? I mean in context where there is only 'input' and in context where there is
'input-manage' permission? Since adding permission check just to event handler property doesn't of course prevent the event to be dispatched and one can always
use addEventListener.
I think the event dispatching code in MozKeyboard.js should somehow assert that the events aren't dispatched when MozInputMethodManager's window object 
doesn't have 'input-manage' permission


>+/**
>+ * Detail of the inputcontextfocus event.
>+ */
>+[JSImplementation="@mozilla.org/b2g-imm-focus;1",
>+ Pref="dom.mozInputMethod.enabled",
>+ CheckAnyPermissions="input-manage"]
>+interface MozInputContextFocusEventDetail {
>+  readonly attribute MozInputMethodInputContextTypes type;
>+  /**
>+   * The input type of the focused input.
>+   */
>+  readonly attribute MozInputMethodInputContextInputTypes inputType;
>+
>+  /**
>+   * The following is only needed for rendering and handling "option" input types,
>+   * in System app.
>+   */
>+
>+  /**
>+   * Current value of the input/select element.
>+   */
>+  readonly attribute DOMString? value;
>+  /**
>+   * An object representing all the <optgroup> and <option> elements
>+   * in the <select> element.
>+   */
>+  readonly attribute MozInputContextChoicesInfo? choices;
>+  /**
>+   * Max/min value of <input>
>+   */
>+  readonly attribute DOMString? min;
>+  readonly attribute DOMString? max;
>+};
Hmm, wouldn't it be easier to just have a separate event interface which has these properties. 
That event interface could be implemented using event codegenerator.
(the interface should still have Pref and CheckAnyPermissions checks)


>+
>+/**
>+ * Information about the options within the <select> element.
>+ */
>+[JSImplementation="@mozilla.org/b2g-inputcontext-choices-into;1",
>+ Pref="dom.mozInputMethod.enabled",
>+ CheckAnyPermissions="input-manage"]
>+interface MozInputContextChoicesInfo {
>+  readonly attribute boolean multiple;
>+  [Constant,Cached]
>+  readonly attribute sequence<MozInputMethodChoiceDict>? choices;
>+};
Does this need to be interface?


>+interface MozInputRegistryEventDetail {
>+  /**
>+   * Manifest URL of the requesting app.
>+   */
>+  readonly attribute DOMString manifestURL;
>+  /**
>+   * ID of the input
>+   */
>+  readonly attribute DOMString inputId;
>+  /**
>+   * Input manifest of the input to add.
>+   * Null for removeinputrequest event.
>+   */
>+  readonly attribute MozInputMethodInputManifest? inputManifest;
>+  /**
>+   * Resolve or Reject the addInput() or removeInput() call when the passed
>+   * promises are resolved.
>+   */
>+  [Throws]
>+  void waitUntil(Promise<any> p);
>+};
Hmm, I don't have yet opinion on this interface. waitUntil looks like some ServiceWorker thing, and ServiceWorker events are messy.
But I guess waitUntil works in this case.

One thing in the implementation .... looks like Promise.jsm is used, but I'd prefer if normal Promise API was used.



>+[JSImplementation="@mozilla.org/b2g-imm-input-registry;1",
>+ Pref="dom.mozInputMethod.enabled",
>+ CheckAnyPermissions="input-manage"]
>+interface MozInputMethodInputManifest {
>+  readonly attribute DOMString launch_path;
>+  readonly attribute DOMString name;
>+  readonly attribute DOMString? description;
>+  [Constant,Cached]
>+  readonly attribute sequence<MozInputMethodInputContextInputTypes> types;
> };
> 
Could this be a dictionary?

Comment 11

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

Thank you for the quick response. Let me ask for some clarification first before delivering the next patch.

(In reply to Olli Pettay [:smaug] from comment #10)
> Comment on attachment 8657711 [details] [diff] [review]
> bug1201407.patch
> 
> So is the same instance of MozInputMethodManager used in different contexts?
> I mean in context where there is only 'input' and in context where there is
> 'input-manage' permission? Since adding permission check just to event
> handler property doesn't of course prevent the event to be dispatched and
> one can always
> use addEventListener.
> I think the event dispatching code in MozKeyboard.js should somehow assert
> that the events aren't dispatched when MozInputMethodManager's window object 
> doesn't have 'input-manage' permission
> 

Thanks for bring this up; there is indeed some checks in place in both MozKeyboard.js initialization and Keyboard.jsm handling; Keyboard.jsm will never forward/dispatch input-manage handling messages to processes/MozKeyboard instances w/o input-manage permission, so I think we are fine here. Unless you think that's not enough and we should assert the permission at the moment of message dispatching (why?) or we should not be using DOM events at all (why?).

> 
> >+/**
> >+ * Detail of the inputcontextfocus event.
> >+ */
> >+[JSImplementation="@mozilla.org/b2g-imm-focus;1",
> >+ Pref="dom.mozInputMethod.enabled",
> >+ CheckAnyPermissions="input-manage"]
> >+interface MozInputContextFocusEventDetail {
> >+  readonly attribute MozInputMethodInputContextTypes type;
> >+  /**
> >+   * The input type of the focused input.
> >+   */
> >+  readonly attribute MozInputMethodInputContextInputTypes inputType;
> >+
> >+  /**
> >+   * The following is only needed for rendering and handling "option" input types,
> >+   * in System app.
> >+   */
> >+
> >+  /**
> >+   * Current value of the input/select element.
> >+   */
> >+  readonly attribute DOMString? value;
> >+  /**
> >+   * An object representing all the <optgroup> and <option> elements
> >+   * in the <select> element.
> >+   */
> >+  readonly attribute MozInputContextChoicesInfo? choices;
> >+  /**
> >+   * Max/min value of <input>
> >+   */
> >+  readonly attribute DOMString? min;
> >+  readonly attribute DOMString? max;
> >+};
> Hmm, wouldn't it be easier to just have a separate event interface which has
> these properties. 
> That event interface could be implemented using event codegenerator.
> (the interface should still have Pref and CheckAnyPermissions checks)
> 

Codegen does not accept arrays as attributes. It also does not support methods on the event. The previous patches was my failed attempts on these.

Dictionaries can't be used as attributes either (in fact, it can't be used as attribute on any interface).

Are you asking for a different "optionfocus" event that only handles option input types to separate my use case of "inputcontextfocus" here? That definitely works for me (since they are indeed handled differently in System app).

> >+
> >+/**
> >+ * Information about the options within the <select> element.
> >+ */
> >+[JSImplementation="@mozilla.org/b2g-inputcontext-choices-into;1",
> >+ Pref="dom.mozInputMethod.enabled",
> >+ CheckAnyPermissions="input-manage"]
> >+interface MozInputContextChoicesInfo {
> >+  readonly attribute boolean multiple;
> >+  [Constant,Cached]
> >+  readonly attribute sequence<MozInputMethodChoiceDict>? choices;
> >+};
> Does this need to be interface?
> 

As previously stated, dictionaries can't be attributes -- I tried and the build failed, so I presume that's by design and I should create an interface here. Are you saying this is a bug in the parser rather than a WebIDL rule?

> 
> >+interface MozInputRegistryEventDetail {
> >+  /**
> >+   * Manifest URL of the requesting app.
> >+   */
> >+  readonly attribute DOMString manifestURL;
> >+  /**
> >+   * ID of the input
> >+   */
> >+  readonly attribute DOMString inputId;
> >+  /**
> >+   * Input manifest of the input to add.
> >+   * Null for removeinputrequest event.
> >+   */
> >+  readonly attribute MozInputMethodInputManifest? inputManifest;
> >+  /**
> >+   * Resolve or Reject the addInput() or removeInput() call when the passed
> >+   * promises are resolved.
> >+   */
> >+  [Throws]
> >+  void waitUntil(Promise<any> p);
> >+};
> Hmm, I don't have yet opinion on this interface. waitUntil looks like some
> ServiceWorker thing, and ServiceWorker events are messy.
> But I guess waitUntil works in this case.

Interesting opinion on Service Worker. Love to hear more from you on that.

As of the waitUntil() interface, I have previously bought that up at dev-webapi titled "Service Worker APIs as conventions to DOMEvent interactions with Promises?" and there wasn't any strong disagreement. So I figured the interface can be copied to be used beyond Service Worker of "platform waiting for async-ly thing from content". There are interfaces designed predates waitUntil() and I mentioned that in the post too.

> 
> One thing in the implementation .... looks like Promise.jsm is used, but I'd
> prefer if normal Promise API was used.
> 

How do I access normal Promise API in MozKeyboard.js? I know there are many impl of Promise in Gecko but I didn't follow the evolution. Sorry about that.

> 
> 
> >+[JSImplementation="@mozilla.org/b2g-imm-input-registry;1",
> >+ Pref="dom.mozInputMethod.enabled",
> >+ CheckAnyPermissions="input-manage"]
> >+interface MozInputMethodInputManifest {
> >+  readonly attribute DOMString launch_path;
> >+  readonly attribute DOMString name;
> >+  readonly attribute DOMString? description;
> >+  [Constant,Cached]
> >+  readonly attribute sequence<MozInputMethodInputContextInputTypes> types;
> > };
> > 
> Could this be a dictionary?

See above comment on interface & dictionary. I would love this to be a dictionary so I don't have to declare it twice!

Comment 12

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

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #11)
> Thanks for bring this up; there is indeed some checks in place in both
> MozKeyboard.js initialization and Keyboard.jsm handling; Keyboard.jsm will
> never forward/dispatch input-manage handling messages to
> processes/MozKeyboard instances w/o input-manage permission, so I think we
> are fine here.
Ok, sounds good. All we should guarantee that the events which event handlers are behind a certain permission check aren't dispatched
in a context which doesn't have that permission the event handlers need.


> 
> Codegen does not accept arrays as attributes. It also does not support
> methods on the event.
No methods sure, because such event interface is very unusual. So far pretty much only service worker events have had that.
The codegen does accept sequences, but ok, perhaps it can't figure out sequence<MozInputMethodChoiceDict> case.


> Dictionaries can't be used as attributes either (in fact, it can't be used
> as attribute on any interface).
dictionaries can be used as attributes (but I wasn't even asking that here), 
see for example TestInterfaceJS or ActivityRequestHandler which has .source

> 
> Are you asking for a different "optionfocus" event that only handles option
> input types to separate my use case of "inputcontextfocus" here?
No I wasn't asking that


> > Does this need to be interface?
> > 
> 
> As previously stated, dictionaries can't be attributes -- I tried and the
> build failed, so I presume that's by design and I should create an interface
> here. Are you saying this is a bug in the parser rather than a WebIDL rule?
Well, based on other interfaces we have, dictionaries-as-attributes is supported.



> Interesting opinion on Service Worker. Love to hear more from you on that.
(Service workers is a bit over-engineered API, but won't say much more, since I don't know the reasoning for various
pieces in the API.)


> 
> As of the waitUntil() interface, I have previously bought that up at
> dev-webapi titled "Service Worker APIs as conventions to DOMEvent
> interactions with Promises?" and there wasn't any strong disagreement. So I
> figured the interface can be copied to be used beyond Service Worker of
> "platform waiting for async-ly thing from content". There are interfaces
> designed predates waitUntil() and I mentioned that in the post too.
Right.  I had actually event.respondWith in my mind. Service workers' event.respondWith is totally hackish and against the normal
DOM Event handling. waitUntil is actually better since it seems to deal with multiple promises.
(DOM Events are one-to-multiple notification, but Service Workers' respondWith try to restrict it to one-to-one, but only in case one uses respondWith. )



> How do I access normal Promise API in MozKeyboard.js? I know there are many
> impl of Promise in Gecko but I didn't follow the evolution. Sorry about that.
Isn't 'new Promise(function(){})' working in JS components?
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Promise.webidl#20 sure hints it should.

Comment 13

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

Attachment: bug1201407.patch

Patch updated according to the comments.

-- The attributes of *Detail are denoted as dictionaries instead of interfaces.
-- The import of Promise.jsm is removed
-- The tests has been re-constructed to ensure app (with input perm and no input-mgmt perm) does not receive the events.

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

The patch is done on top of bug 1197700 and bug 1197682. The plan is to land (a) these new "features" in one week, and (b) switch Gaia to use them in another, and (c) remove the mozChromeEvents in the next week.

So that's 3 Gecko patches in (a), 2 Gaia patches in (b), and 1 Gecko patch in (c).

Comment 14

[Jan Jongboom [:janjongboom] (inactive)]

bug1201407-removeContentEvent.patch does not apply cleanly on a branch with  Bug 1197700, Bug 1197682 part 1 & 2, and  bug1201407.patch.

Also damn, so much patches. It's hard to verify nothing breaks given that we don't have gaia patches in place yet.

Comment 15

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

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

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

Comment 16

[Jan Jongboom [:janjongboom] (inactive)]

This seems to be wrong: https://bugzilla.mozilla.org/attachment.cgi?id=8658579&action=diff#a/dom/inputmethod/Keyboard.jsm_sec4

As you have 'System:Blur' as a different event type later on. Or am I missing something?

Comment 17

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

Attachment: bug1201407.patch

You were right, I missed that ...

I've also changed the message name of Keyboard:Register to Keyboard:RegisterSync just to get the naming consistent. Also add a few dump() to prevent some future footguns.

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

Comment 18

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

Attachment: bug1201407.patch

What you have found should be addressed in bug 1197700 so I did that and rebase the patches accordingly.

There is no need for another try push here since end result is the same.

Comment 19

[Jan Jongboom [:janjongboom] (inactive)]

1. MozInputMethodManager seems to grow pretty hairy with the 2 permissions there. Should we split this up at some point?
2. The new methods are tagged 'CheckAnyPermissions', but old methods 'CheckAllPermissions'. I don't think it matters but it's a nit.

Comment 20

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8659736 [details] [diff] [review]
bug1201407.patch

I'm gonna r+ here, but I don't feel 100% confident about this patch. Test coverage seems good though and as far as I know it also works as planned, but only way to really find out is to change gaia around tbh.

nit: I'd change System:Next and System:ShowAll to something like System:InputMethod:Next. I could imagine another process reusing these names at some point (f.e. System:InputRegistry:Done:OK is prefixed so not very consequent).

Comment 21

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

Attachment: Patch to comment

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/171eb742e015

Comment 23

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a68945a2ac

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/310477a8720f

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/310477a8720f