See https://bugzilla.mozilla.org/show_bug.cgi?id=962233#c15

Not sure who to request review from, so r=fabrice? for now. Feel free to punt to someone else. This was actually pretty easy once I wrapped my head around the IPC design. The messages went in place pretty conveniently and it didn't require any real complex design changes.

Comment on attachment 8368410 [details] [diff] [review] Send messages from widget directly to keyboard when there are no listeners Also asking for r=janjongboom? due to work on the surrounding code and offering to review keyboard patches :)

Comment on attachment 8368410 [details] [diff] [review] Send messages from widget directly to keyboard when there are no listeners Review of attachment 8368410 [details] [diff] [review]: ----------------------------------------------------------------- lgtm, but I need to think a bit more about it. meta-nit: can you create your patches with a 8 lines context? ::: dom/inputmethod/forms.js @@ +525,4 @@ > json.charCode, json.modifiers); > this._editing = false; > > + if (this._contentHasKeyListeners) { it looks like this could be (this._contentHasKeyListeners && json.requestId) @@ +530,5 @@ > + sendAsyncMessage("Forms:SendKey:Result:OK", { > + requestId: json.requestId > + }); > + } > + else if (json.requestId && !doKeypress) { nit: } else if (...) { @@ +672,5 @@ > + }, > + > + sendContentHasKeyListeners: function fa_sendContentHasKeyListeners(target, force) { > + let infos = els.getListenerInfoFor(target, {}); > + for (var i = 0; i < infos.length; i++) { nit: s/var/let @@ +685,5 @@ > + } > + > + // We should only send a message to the parent if we are being forced to or > + // if whether or not there are event listeners has changed. > + if (hasListeners != this._contentHasKeyListeners || force) { nit: s/!=/!==

Comment on attachment 8368410 [details] [diff] [review] Send messages from widget directly to keyboard when there are no listeners I can't really overview the impact of this patch, so redirect to yxl. Sorry Doug that it took so long.

Comment on attachment 8368410 [details] [diff] [review] Send messages from widget directly to keyboard when there are no listeners Review of attachment 8368410 [details] [diff] [review]: ----------------------------------------------------------------- It seems there is a race condition issue: If the event listener was added during input and a |sendkey| message is sent to Keyboard.jsm before Keyboard.jsm gets updated. MozKeyboard.js may get an incorrect "Keyboard:SendKey:Result:OK" message. ::: dom/inputmethod/forms.js @@ +678,5 @@ > + // types fired, but only "keypress" can actually be preventDefaulted. > + // Because of this, we only care about whether or not there are "keypress" > + // event listeners. > + if (infos[i].type == 'keypress' && !infos[i].inSystemEventGroup) { > + hasListeners = true; You need declare |hasListeners| before using it.

(In reply to Yuan Xulei [:yxl] from comment #5) > It seems there is a race condition issue: > > If the event listener was added during input and a |sendkey| message is sent > to Keyboard.jsm before Keyboard.jsm gets updated. MozKeyboard.js may get an > incorrect "Keyboard:SendKey:Result:OK" message. Fabrice and I talked about this. Most Gecko-side event listener IPC code is pretty bad at handling cases like that, where event listeners are being added and removed during use. For example, the panning and zooming code has the same issue. We made the decision to not try to deal with situations like that in the 100% correct way because it would require such a huge performance hit for something that will rarely happen and people shouldn't really be doing anyways. The consequences of this, I think, are that we accidentally let through a key press that we shouldn't have. > ::: dom/inputmethod/forms.js > @@ +678,5 @@ > > + // types fired, but only "keypress" can actually be preventDefaulted. > > + // Because of this, we only care about whether or not there are "keypress" > > + // event listeners. > > + if (infos[i].type == 'keypress' && !infos[i].inSystemEventGroup) { > > + hasListeners = true; > > You need declare |hasListeners| before using it. Yikes, good catch.

Addressed review/feedback comments. I tried this again today and it didn't provide a perceptible increase in performance anymore, so I don't think we should uplift it to 1.3 or 1.3T. Having said that, this will help us with hotpathing key presses and reducing IPC overhead in the future, so I still think we should land it.

Comment on attachment 8375117 [details] [diff] [review] Send messages from widget directly to keyboard when there are no listeners Review of attachment 8375117 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/inputmethod/Keyboard.jsm @@ +268,5 @@ > + data: { > + requestId: msg.json.requestId > + } > + }); > + } Why not move this logic to MozKeyboard.js and we could save more time to return directly from MozKeyboard.js. In addition, |_contentHasKeyListeners| is not reliable. It is possible that |_contentHasKeyListeners| in forms.js is just changed to false, but this |_contentHasKeyListeners| (in Keyboard.jsm) remains true. So we won't send |Result:OK| messsage here and when the forms.js gets |SendKey| message, it won't send |Result:OK| messsage as well. In this case, we will never get |Result:OK| messsage sent. I think we may set a flag to msg.data here if |Result:OK| message is sent in Keyboard.jsm. Then forms.js will check the flag to decide whether to send the |Result:OK| message.

tagged this so fxos perf team can track it.

(In reply to Doug Sherk (:drs) from comment #7) > > ... > > I tried this again today and it didn't provide a perceptible increase in > performance anymore, so I don't think we should uplift it to 1.3 or 1.3T. > Having said that, this will help us with hotpathing key presses and reducing > IPC overhead in the future, so I still think we should land it. Doug, do you still plan to land this and if so when?

(In reply to Mike Lee [:mlee] from comment #10) > Doug, do you still plan to land this and if so when? Yeah, I'll get back to this today. I'm better at profiling now compared to when I wrote this patch, so I think I'll be able to give better data on it.

We're dropping key presses with this patch under heavy load. I'm not sure why yet.

(In reply to Doug Sherk (:drs) from comment #12) > We're dropping key presses with this patch under heavy load. I'm not sure > why yet. Ok, I fixed this, but I'm having another issue now. Yuan, if I force sending the "Forms:SendKey:Result:OK" message (i.e. I set contentHasKeyListeners=true), then the keyboard's suggestions show up when typing. But with contentHasKeyListeners=false, the suggestions aren't appearing. I've looked all over the codebase trying to see what's listening for this event and I haven't found anything. I'm not creating a promise for the "Keyboard:SendKey" event, so I don't think it's that. Do you have any ideas?

The Latin keyboard creates promises for the "keyboard.sendKey": https://github.com/mozilla-b2g/gaia/blob/1da81b75ea2b08d4f9bc695b8c0b536f094d736b/apps/keyboard/js/imes/latin/latin.js#L402 If there is no event listener for the input field, the "keyboard.sendKey" will be resolved before the key events are actually generated. The patch makes the promise resolved earlier. I suspect it may cause race condition issue for the keyboard suggestions. Let's ask David to evaluate the impact of this patch.

I haven't really looked at that code since before it was Promise-based, and I barely understand what is going on with this patch. Maybe Rudy could tell you more.

If I understand correctly, does comment 13 implies that we won't return promise for "contentHasKeyListeners=false" case? We would update the suggestion after the sendKey promised is resolved, as Yuan mentioned in comment 14. If there is no promise or the promise is not resolved, we won't update the internal state of latin input method, so the suggestion won't show up. Make sense?

(In reply to Rudy Lu [:rudyl] from comment #16) > If I understand correctly, does comment 13 implies that we won't return > promise for "contentHasKeyListeners=false" case? We do return promise for "contentHasKeyListeners=false" case. But the promise will be resolved before the sendKey takes effect. > We would update the suggestion after the sendKey promised is resolved, as > Yuan mentioned in comment 14. > If there is no promise or the promise is not resolved, we won't update the > internal state of latin input method, so the suggestion won't show up. > > Make sense?

The suggestions don't wait for promises to resolve at this moment afaik, so that shouldn't have any side effect.

(In reply to Yuan Xulei [:yxl] from comment #14) > The Latin keyboard creates promises for the "keyboard.sendKey": > > https://github.com/mozilla-b2g/gaia/blob/ > 1da81b75ea2b08d4f9bc695b8c0b536f094d736b/apps/keyboard/js/imes/latin/latin. > js#L402 > > If there is no event listener for the input field, the "keyboard.sendKey" > will be resolved before the key events are actually generated. > > The patch makes the promise resolved earlier. I suspect it may cause race > condition issue for the keyboard suggestions. Thanks, this was really useful information. I got by this by returning null in the no-listener case, which is then handled by Gaia in the patch there.

Here's the Gaia side of this. I wanted to add tests, but it seems like latin_test.js is disabled. PR: https://github.com/mozilla-b2g/gaia/pull/20447

The performance benefits of these patches when the content process is lagging are pretty astounding. I made a couple of pages that intentionally lag the device by running a very expensive loop, but they also have a single input field. Watch the suggestion bar. With event listeners (or without event listeners, and without these patches): http://people.mozilla.org/~dsherk/input-listener.html http://youtu.be/pIjY3eRLPNY Without event listeners: http://people.mozilla.org/~dsherk/no-input-listener.html http://youtu.be/_Yyp28Gy-KI I'm sure it's also significantly faster even when content isn't lagging. I haven't profiled it though.

Comment on attachment 8439244 [details] [diff] [review] Resolve key presses immediately if MozKeyboard.sendKey() doesn't return a promise. Review of attachment 8439244 [details] [diff] [review]: ----------------------------------------------------------------- 1. With bug 1013570, we do need to return a promise in handleKey() function or the input chain would be broken. Please help look into click() method in latin IME that right now we would do the next sendKey() after all the previous ones are resolved. 2. We do have latin_test.js unit test, so please help add some unit tests around this new API behavior. Thank you. ::: apps/keyboard/js/imes/latin/latin.js @@ +469,5 @@ > + // We didn't get a promise from keyboard.sendKey, so the API must want us > + // to update our internal state immediately. This happens when content has > + // no event listeners, in which case there's no way for the event to be > + // intercepted and/or canceled. > + updateInternalState(keycode, repeat); I think we still need to return a promise here. See the overall comment for the reason.

Doug, quick question. We assume that if there are no listeners then the promise will always resolve right? And thus we can quickly resolve the promise and send back to keyboard process. Now what happens if the textbox in the content process has a |maxlength| attribute set. Then it can be rejected anway...

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #24) > Doug, quick question. We assume that if there are no listeners then the > promise will always resolve right? And thus we can quickly resolve the > promise and send back to keyboard process. Now what happens if the textbox > in the content process has a |maxlength| attribute set. Then it can be > rejected anway... Oh, that's a good point. We could just set contentHasKeyListeners if the focused input field has a maxlength attribute. So we'd unfortunately have to bail out of this fast path in that case. Does that seem reasonable to you? Since the behavior of the maxlength attribute is more predictable than arbitrary content's key event listeners, maybe we could duplicate the maxlength logic on the keyboard side. That seems like it would take too long and be too complicated, though.

Comment on attachment 8439243 [details] [diff] [review] Send messages from widget directly to keyboard when there are no listeners. Review of attachment 8439243 [details] [diff] [review]: ----------------------------------------------------------------- Could we add a new method with void return value instead of changing sendKey to boost the performace? It seems latin IME ignores the return value from sendKey. ::: dom/inputmethod/MozKeyboard.js @@ +578,3 @@ > }); > + } else { > + this._sendKeyMessage(keyCode, charCode, modifiers, repeat); It should return a promise ::: dom/webidl/InputMethod.webidl @@ +186,5 @@ > * Note that, if you want to send a key n times repeatedly, make sure set > * parameter repeat to true and invoke sendKey n-1 times, and then set > * repeat to false in the last invoke. > */ > + Promise? sendKey(long keyCode, long charCode, long modifiers, optional boolean repeat); I don't think it is a good way to change the behavior of this method.

How about adding |sendKeyNoReturn| method (or with other better name) for this? (In reply to Yuan Xulei [:yxl] from comment #26) > Comment on attachment 8439243 [details] [diff] [review] > Send messages from widget directly to keyboard when there are no listeners. > > Review of attachment 8439243 [details] [diff] [review]: > ----------------------------------------------------------------- > > Could we add a new method with void return value instead of changing sendKey > to boost the performace? > It seems latin IME ignores the return value from sendKey. > > ::: dom/inputmethod/MozKeyboard.js > @@ +578,3 @@ > > }); > > + } else { > > + this._sendKeyMessage(keyCode, charCode, modifiers, repeat); > > It should return a promise > > ::: dom/webidl/InputMethod.webidl > @@ +186,5 @@ > > * Note that, if you want to send a key n times repeatedly, make sure set > > * parameter repeat to true and invoke sendKey n-1 times, and then set > > * repeat to false in the last invoke. > > */ > > + Promise? sendKey(long keyCode, long charCode, long modifiers, optional boolean repeat); > > I don't think it is a good way to change the behavior of this method.

I am confused here, so sendKey would not return promise in some cases or?

(In reply to Yuan Xulei [:yxl] from comment #26) > Could we add a new method with void return value instead of changing sendKey > to boost the performace? > It seems latin IME ignores the return value from sendKey. We should not just optimize for our own flow but also for 3rd parties. Also I think it's a bug if we ignore sendKey result in latin IME but hey. :-)

I'd like (In reply to Rudy Lu [:rudyl] from comment #28) > I am confused here, so sendKey would not return promise in some cases or? No, we keep "sendKey" unchanged and introduce a new method "void sendKeyNoReturn" (which has no return value). (In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #29) > (In reply to Yuan Xulei [:yxl] from comment #26) > > Could we add a new method with void return value instead of changing sendKey > > to boost the performace? > > It seems latin IME ignores the return value from sendKey. > > We should not just optimize for our own flow but also for 3rd parties. Also > I think it's a bug if we ignore sendKey result in latin IME but hey. :-) Agree. That may break work suggestion and correction.

Doug, how much does the promise returned affect the performance? If promise doesn't downgrade performance, I'd like to give r+ if we keep returning promise. (In reply to Yuan Xulei [:yxl] from comment #26) > Comment on attachment 8439243 [details] [diff] [review] > Send messages from widget directly to keyboard when there are no listeners. > > Review of attachment 8439243 [details] [diff] [review]: > ----------------------------------------------------------------- > > Could we add a new method with void return value instead of changing sendKey > to boost the performace? > It seems latin IME ignores the return value from sendKey. > > ::: dom/inputmethod/MozKeyboard.js > @@ +578,3 @@ > > }); > > + } else { > > + this._sendKeyMessage(keyCode, charCode, modifiers, repeat); > > It should return a promise > > ::: dom/webidl/InputMethod.webidl > @@ +186,5 @@ > > * Note that, if you want to send a key n times repeatedly, make sure set > > * parameter repeat to true and invoke sendKey n-1 times, and then set > > * repeat to false in the last invoke. > > */ > > + Promise? sendKey(long keyCode, long charCode, long modifiers, optional boolean repeat); > > I don't think it is a good way to change the behavior of this method.

Note for me: Mason is working on bug 930939 which may give us additional benefits here. I need to investigate whether or not keyboard events begin from the main process and get synthesized into keyboard events from the keyboard process. If they do, then we benefit from those changes for free, but otherwise, we may need to do extra work to get those events off-main-thread.

Re-setting my needinfo flag.

Firefox OS is not being worked on