Closed Bug 1201407 Opened 9 years ago Closed 9 years ago

Replace mozChromeEvent/mozContentEvent with real events in InputMethod API

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(2 files, 7 obsolete files)

Let's work on the remaining events in one off patch. I have a WIP ready.
Attached patch bug1201407.patch (obsolete) — Splinter Review
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.
Attachment #8656947 - Attachment is obsolete: true
If your webidl is valid, you should ask someone like bz if the codegen can be fixed.
(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 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.
Attachment #8657670 - Attachment is obsolete: true
(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 ...
Attached patch bug1201407.patch (obsolete) — Splinter Review
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
Attachment #8657711 - Flags: superreview?(bugs)
Attachment #8657711 - Flags: review?(janjongboom)
... and this is the patch to remove mozChromeEvents from InputMethod API.
Attachment #8657712 - Flags: review?(janjongboom)
shell.js stuff needs to be removed too.
Attachment #8657712 - Attachment is obsolete: true
Attachment #8657712 - Flags: review?(janjongboom)
Attachment #8657713 - Flags: review?(janjongboom)
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?
Attachment #8657711 - Flags: superreview?(bugs) → superreview-
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!
Flags: needinfo?(bugs)
(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.
Flags: needinfo?(bugs)
Attached patch bug1201407.patch (obsolete) — Splinter Review
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).
Attachment #8657711 - Attachment is obsolete: true
Attachment #8657711 - Flags: review?(janjongboom)
Attachment #8658579 - Flags: superreview?(bugs)
Attachment #8658579 - Flags: review?(janjongboom)
Attachment #8658579 - Flags: superreview?(bugs) → superreview+
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.
Flags: needinfo?(timdream)
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. :)
Attachment #8657713 - Attachment filename: bug1201407-removeContentEvent.patch → <to be moved> bug1201407-removeContentEvent.patch
Flags: needinfo?(timdream)
Attachment #8657713 - Flags: review?(janjongboom)
Attachment #8657713 - Attachment description: bug1201407-removeContentEvent.patch → <to be moved> bug1201407-removeContentEvent.patch
Attachment #8657713 - Attachment filename: <to be moved> bug1201407-removeContentEvent.patch → bug1201407-removeContentEvent.patch
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?
Flags: needinfo?(timdream)
Attached patch bug1201407.patch (obsolete) — Splinter Review
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
Attachment #8658579 - Attachment is obsolete: true
Attachment #8658579 - Flags: review?(janjongboom)
Flags: needinfo?(timdream)
Attachment #8659706 - Flags: superreview+
Attachment #8659706 - Flags: review?(janjongboom)
Attached patch bug1201407.patch (obsolete) — Splinter Review
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.
Attachment #8659706 - Attachment is obsolete: true
Attachment #8659706 - Flags: review?(janjongboom)
Attachment #8659736 - Flags: superreview+
Attachment #8659736 - Flags: review?(janjongboom)
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 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).
Attachment #8659736 - Flags: review?(janjongboom) → review+
Attached patch Patch to commentSplinter Review
Attachment #8659736 - Attachment is obsolete: true
Attachment #8662179 - Flags: superreview+
Attachment #8662179 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/310477a8720f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: