1197751 - [TV][2.6] Write a event shuttle in Gecko

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Eric Tsai (no more review request accepted)]

Event shutter handles commands from http server and send key events to Gecko event loop for remote control.

Comment 1

[Eric Tsai (no more review request accepted)]

Functions to do:
1) Dispatch keyboard event to app
2) Dispatch touch/mouse event to app
3) Send string from remote to focused and input element

Init KeyboardEvent and dispatchEvent to shell.html (Service.wm.getMostRecentWindow) or system app (by get iframe in body), KeyboardEvent doesn't route to app. Will try nsIAppShell, nsITextProcessor, uinput for 1 and 2

Comment 2

[Eric Tsai (no more review request accepted)]

Current status:
1) Can dispatch KeyEvent to b2g-desktop TV, client side sends 4 arrow keys, back and enter. For special key like home or power key, doesn't work now.
2) Can dispatch mouse event to b2g-desktop TV, but there is no mouse cursor in b2g. @lchang will help to provide a WIP for system app which draws cursor in system app
3) Handle string is more complex, we need to grab keyboard control to prevent origin virtual keyboard show when send string, and return keyboard control in a async func call. But send string is done in http server sandbox when handle http request. Sandbox is destroyed after handleRequest returns. We need another way to handle string dispatch, maybe create another permanent service for this?

Comment 3

[Eric Tsai (no more review request accepted)]

For sprint 6, @lchang and I finish:

1) gecko based http server on b2g-desktop
2) smart phone control page
3) event shutter for touch and single click, 4 arrow keys, back and return key

In sprint 7, plan for
1) build http server on flame
2) move remote control to a independent service
3) touch event performance tuning
4) survey eventsource for server triggered message to client

Comment 4

[Eric Tsai (no more review request accepted)]

latest event shutter code in public: https://github.com/luke-chang/ajax-websocket-test

Comment 5

[Eric Tsai (no more review request accepted)]

Attachment: 0001-add-event-shutter-service.patch

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@fabrice, in attachment 8663559 [details] [diff] [review] we use httpd.js to serve a web page on TV for providing remote controller UI over HTTP. User can use any browser to load this web page on local network to remote control TV. I'd like to know your opinion about the architecture and folder structure.

Comment 7

[Eric Tsai (no more review request accepted)]

Update requirement:
- touch move and click events
- touch panel gesture (to send ARROW keys)
- send page down/up or wheel event according to current mode
- send back/home/option/pin key event

Comment 8

[Eric Tsai (no more review request accepted)]

Requirement added:
- send string to input field

Comment 9

[[:fabrice] Fabrice Desré]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #6)
> @fabrice, in attachment 8663559 [details] [diff] [review] we use httpd.js to
> serve a web page on TV for providing remote controller UI over HTTP. User
> can use any browser to load this web page on local network to remote control
> TV. I'd like to know your opinion about the architecture and folder
> structure.

The folder structure looks ok. About the architecture, it seems like this could have share some code with what the flyweb team is targetting. The main difference is that the server side here is chrome code while they want to expose an api to web content, but that should be workable. Their spec is at https://github.com/flyweb/spec

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to [:fabrice] Fabrice Desré from comment #9)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #6)
> > @fabrice, in attachment 8663559 [details] [diff] [review] we use httpd.js to
> > serve a web page on TV for providing remote controller UI over HTTP. User
> > can use any browser to load this web page on local network to remote control
> > TV. I'd like to know your opinion about the architecture and folder
> > structure.
> 
> The folder structure looks ok. About the architecture, it seems like this
> could have share some code with what the flyweb team is targetting. The main
> difference is that the server side here is chrome code while they want to
> expose an api to web content, but that should be workable. Their spec is at
> https://github.com/flyweb/spec

Yes, we are trying to create the building block for the flyweb, however, flyweb cannot catch up with the TV v2.5 schedule. That's why the event shuttle talks to httpd server directly in current patch. We can migrate to flyweb interface after flyweb code is ready and it'll be a follow-up bug.

Comment 11

[Eric Tsai (no more review request accepted)]

Attachment: Add server script client.sjs to handle ajax request of remote control events

Hi Fabrice, please help to review the event shutter, client.sjs. This patch depends on https://bugzilla.mozilla.org/attachment.cgi?id=8673024 and gaia works on https://github.com/luke-chang/gaia/tree/tv_remote_control_page/

Comment 12

[Eric Tsai (no more review request accepted)]

Comment on attachment 8673027 [details] [diff] [review]
Add server script client.sjs to handle ajax request of remote control events

Hi Masayuki san, we are working on Remote Control service for firefox OS TV. :schien suggest you can help review these DOM related control in sandbox. Could you help on this?

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8673027 [details] [diff] [review]
Add server script client.sjs to handle ajax request of remote control events

Although, I must not be the best person to review this, but I should do it only around the event handling.

>+function handleClickEvent (event)

nit: don't insert a white space after function name.

>+{
>+  let type = 'navigator:browser';
>+  let shell = Services.wm.getMostRecentWindow(type);
>+  var utils = shell.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                     .getInterface(Components.interfaces.nsIDOMWindowUtils);
>+  var isCursorMode = false;
>+
>+  isCursorMode = (getSharedState("isCursorMode") == "true");
>+  if(isCursorMode) {
>+    var x = isNaN(getState("x")) ? 0 : parseInt(getState("x"));
>+    var y = isNaN(getState("y")) ? 0 : parseInt(getState("y"));
>+
>+    ["mousedown",  "mouseup"].forEach(function(mouseType) {
>+      utils.sendMouseEvent (mouseType, x, y, 0, 1, 0);

I don't like you to use nsIDOMWindowUtils.sendMouseEvent() because it's marked as "synthesized for tests". However, we don't have other ways to dispatch mouse events, though...

>+     });
>+
>+     if (event.type == "dblclick") {
>+       ["mousedown",  "mouseup"].forEach(function(mouseType) {
>+         utils.sendMouseEvent (mouseType, x, y, 0, 2, 0);
>+       });
>+     }
>+  } else {
>+    handleKeyboardEvent ('DOM_VK_RETURN');

Hmm, please don't use legacy keyCode names as constants anymore. You should design it with KeyboardEvent.key. (I'm not sure what KeyboardEvent.code values are good for remote controller, though)

>+function handleTouchEvent (event)

I'm not familiar with touch events.

>+function handleScrollEvent (event)

"scroll event" sounds odd. An event fired after scrolled is called so. Looks like handleWheelEvent?

>+{
>+  let type = 'navigator:browser';
>+  let shell = Services.wm.getMostRecentWindow(type);
>+  var utils = shell.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                     .getInterface(Components.interfaces.nsIDOMWindowUtils);
>+
>+  var x = isNaN(getState("x")) ? 0 : parseInt(getState("x"));
>+  var y = isNaN(getState("y")) ? 0 : parseInt(getState("y"));
>+  var sy = isNaN(getState("sy")) ? 0 : parseInt(getState("sy"));
>+  var isCursorMode = false;
>+
>+  let detail = event.detail;
>+
>+  isCursorMode = (getSharedState("isCursorMode") == "true");
>+  if(isCursorMode) {
>+    utils.sendWheelEvent (x, y,
>+      0, detail.dy - sy , 0, shell.WheelEvent.DOM_DELTA_LINE,
>+      0, 0, 0, 0);

I feel odd. The contents are never overflown horizontally? This only supports vertical scroll.

If aDeltaX and aDeltaY are always int values, please set same values to aLineOrPageDeltaX and aLineOrPageDeltaY.


>+
>+    setState ("sy", detail.dy.toString());
>+  } else {
>+    if (event.type == "touchend") {
>+      switch (detail.swipe) {
>+        case "up":
>+        case "down":
>+          handleKeyboardEvent ('DOM_VK_PAGE_' + detail.swipe.toUpperCase());

I don't know what the cursor mode, though. If you don't want to dispatch keyboard event, you can use wheel event with DOM_DELTA_PAGE.

>+          break;
>+      }
>+    }
>+  }
>+}
>+
>+function handleKeyboardEvent (keyCodeName)
>+{
>+  debug('key: ' + keyCodeName);
>+
>+  const nsIDOMKeyEvent = Ci.nsIDOMKeyEvent;
>+  let type = "navigator:browser";
>+  let shell = Services.wm.getMostRecentWindow(type);
>+  var utils = shell.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                     .getInterface(Components.interfaces.nsIDOMWindowUtils);
>+
>+  var x = isNaN(getState("x")) ? 0 : parseInt(getState("x"));
>+  var y = isNaN(getState("y")) ? 0 : parseInt(getState("y"));
>+
>+  if (keyCodeName == "DOM_VK_CONTEXT_MENU") {
>+    utils.sendMouseEvent ("contextmenu", x, y, 0, 0, 0);

Why don't you dispatch "ContextMenu" key event? Won't work something?

>+  } else {
>+    ["keydown",  "keypress", "keyup"].forEach(function(keyType) {
>+      var keyCode = nsIDOMKeyEvent[keyCodeName];
>+      var modifiers = 0;
>+      var happened = utils.sendKeyEvent(keyType, keyCode, 0, modifiers);

Don't use nsIDOMWindowUtils.sendKeyEvent(). It's already been obsoleted. Please use nsITextInputProcessor. Then, you can set all attributes including .key and .code. See bug 1137557. The patches must help you to understand.

>+function handleInputEvent (detail)

This sounds odd to me. InputEvent is an existing event which is fired after editing <input type="text"> or something.

I don't understand what this function does. So, I'm not sure the better name for this.

>+function handleRequest(request, response)

I don't know what is this function... I cannot understand who can call this and when this is called.

>+    case "keypress":
>+      handleKeyboardEvent(event.detail);
>+      break;

If this is caused by DOM keypress event, it means that if preceding keydown event is consumed by web contents or web apps, keypress event is never fired. Is this intentional behavior?

>+    case "touchstart":
>+    case "touchmove":
>+    case "touchend":
>+      debug(JSON.stringify(event));
>+      handleTouchEvent (event);
>+      break;
>+    case "scrollstart":
>+    case "scrollmove":
>+    case "scrollend":

I don't know what these events are...


Anyway, you need to redesign handleKeyboardEvent() at least.

# And I have two urgent bugs which should be landed 44 and I'm going to go to TPAC next week. So, I don't have much time. If it's okay me to review next patch at Nov, let me know.

Comment 14

[Eric Tsai (no more review request accepted)]

Thanks Masayuki san, I will describe why we have this patch and comment inline later.

Remote Control service is a web server in FxOS TV, provides a control page for smart phone. User can utilize the page to control TV via sending event. The server script receives http request and transfer to relative actions.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #13)
> Comment on attachment 8673027 [details] [diff] [review]
> Add server script client.sjs to handle ajax request of remote control events
> 
> Although, I must not be the best person to review this, but I should do it
> only around the event handling.
> 
> >+function handleClickEvent (event)
> 
> nit: don't insert a white space after function name.
> 

will fix this

> >+{
> >+  let type = 'navigator:browser';
> >+  let shell = Services.wm.getMostRecentWindow(type);
> >+  var utils = shell.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> >+                     .getInterface(Components.interfaces.nsIDOMWindowUtils);
> >+  var isCursorMode = false;
> >+
> >+  isCursorMode = (getSharedState("isCursorMode") == "true");
> >+  if(isCursorMode) {
> >+    var x = isNaN(getState("x")) ? 0 : parseInt(getState("x"));
> >+    var y = isNaN(getState("y")) ? 0 : parseInt(getState("y"));
> >+
> >+    ["mousedown",  "mouseup"].forEach(function(mouseType) {
> >+      utils.sendMouseEvent (mouseType, x, y, 0, 1, 0);
> 
> I don't like you to use nsIDOMWindowUtils.sendMouseEvent() because it's
> marked as "synthesized for tests". However, we don't have other ways to
> dispatch mouse events, though...
> 
> >+     });
> >+
> >+     if (event.type == "dblclick") {
> >+       ["mousedown",  "mouseup"].forEach(function(mouseType) {
> >+         utils.sendMouseEvent (mouseType, x, y, 0, 2, 0);
> >+       });
> >+     }
> >+  } else {
> >+    handleKeyboardEvent ('DOM_VK_RETURN');
> 
> Hmm, please don't use legacy keyCode names as constants anymore. You should
> design it with KeyboardEvent.key. (I'm not sure what KeyboardEvent.code
> values are good for remote controller, though)
> 

Will rewrite keyboard event using TIP, and see if it will conflict with active input method.
Besides conflict, my concern is we will need to handle 4 arrow keys, page up/down, back, home, context menu.
Home key's target element is system app, directly, not normal target element. I'm afraid TIP can't do so.

> >+function handleScrollEvent (event)
> 
> "scroll event" sounds odd. An event fired after scrolled is called so. Looks
> like handleWheelEvent?
> 

Yes, rename to handleWheelEvent

> >+{
> >+  let type = 'navigator:browser';
> >+  let shell = Services.wm.getMostRecentWindow(type);
> >+  var utils = shell.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> >+                     .getInterface(Components.interfaces.nsIDOMWindowUtils);
> >+
> >+  var x = isNaN(getState("x")) ? 0 : parseInt(getState("x"));
> >+  var y = isNaN(getState("y")) ? 0 : parseInt(getState("y"));
> >+  var sy = isNaN(getState("sy")) ? 0 : parseInt(getState("sy"));
> >+  var isCursorMode = false;
> >+
> >+  let detail = event.detail;
> >+
> >+  isCursorMode = (getSharedState("isCursorMode") == "true");
> >+  if(isCursorMode) {
> >+    utils.sendWheelEvent (x, y,
> >+      0, detail.dy - sy , 0, shell.WheelEvent.DOM_DELTA_LINE,
> >+      0, 0, 0, 0);
> 
> I feel odd. The contents are never overflown horizontally? This only
> supports vertical scroll.
> 
> If aDeltaX and aDeltaY are always int values, please set same values to
> aLineOrPageDeltaX and aLineOrPageDeltaY.
> 

Yes, original design on control page is vertical scroll only. But after discuss with gaia engineer :lchang,
will provide both vertical/horizontal.

> 
> >+
> >+    setState ("sy", detail.dy.toString());
> >+  } else {
> >+    if (event.type == "touchend") {
> >+      switch (detail.swipe) {
> >+        case "up":
> >+        case "down":
> >+          handleKeyboardEvent ('DOM_VK_PAGE_' + detail.swipe.toUpperCase());
> 
> I don't know what the cursor mode, though. If you don't want to dispatch
> keyboard event, you can use wheel event with DOM_DELTA_PAGE.
> 

The only use case of mouse cursor on TV is browser, for now. For other use case, we consider spatial navigation.
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/TVs_connected_devices/TV_remote_control_navigation

Send arrow keys and page up/down when in spatial navigation mode (not cursor mode, like browser).

> >+
> >+function handleKeyboardEvent (keyCodeName)
> >+{
> >+  debug('key: ' + keyCodeName);
> >+
> >+  const nsIDOMKeyEvent = Ci.nsIDOMKeyEvent;
> >+  let type = "navigator:browser";
> >+  let shell = Services.wm.getMostRecentWindow(type);
> >+  var utils = shell.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> >+                     .getInterface(Components.interfaces.nsIDOMWindowUtils);
> >+
> >+  var x = isNaN(getState("x")) ? 0 : parseInt(getState("x"));
> >+  var y = isNaN(getState("y")) ? 0 : parseInt(getState("y"));
> >+
> >+  if (keyCodeName == "DOM_VK_CONTEXT_MENU") {
> >+    utils.sendMouseEvent ("contextmenu", x, y, 0, 0, 0);
> 
> Why don't you dispatch "ContextMenu" key event? Won't work something?

Yes, I've tried but doesn't work. Will see if it works on TIP.

> 
> >+function handleInputEvent (detail)
> 
> This sounds odd to me. InputEvent is an existing event which is fired after
> editing <input type="text"> or something.
> 
> I don't understand what this function does. So, I'm not sure the better name
> for this.
> 

It receives string from control page and use InputMethod to insert the string.
It may cause an InputEvent, I will think a better naming.

> >+function handleRequest(request, response)
> 
> I don't know what is this function... I cannot understand who can call this
> and when this is called.

It's the entry point from web server, so , just leave it here.

> 
> >+    case "keypress":
> >+      handleKeyboardEvent(event.detail);
> >+      break;
> 
> If this is caused by DOM keypress event, it means that if preceding keydown
> event is consumed by web contents or web apps, keypress event is never
> fired. Is this intentional behavior?
> 

No, it's from control page, we just want to synthesis key press on TV from remote.

> >+    case "touchstart":
> >+    case "touchmove":
> >+    case "touchend":
> >+      debug(JSON.stringify(event));
> >+      handleTouchEvent (event);
> >+      break;
> >+    case "scrollstart":
> >+    case "scrollmove":
> >+    case "scrollend":
> 
> I don't know what these events are...
> 

These are the same as "keypress", sent from control page.

> 
> Anyway, you need to redesign handleKeyboardEvent() at least.
> 
> # And I have two urgent bugs which should be landed 44 and I'm going to go
> to TPAC next week. So, I don't have much time. If it's okay me to review
> next patch at Nov, let me know.

Thanks for your comment. I will try to make another patch before TPAC. During TPAC, do you recommend who can also help on this?

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> Home key's target element is system app, directly, not normal target element. I'm afraid TIP
> can't do so.

If nsIDOMWindowUtils works fine, nsITextInputProcessor should work fine too. nsITextInputProcessor hides and handle some complicated rules of DOM KeyboardEvent and DOM CompositionEvent. Additionally, it supports all attributes to set to dispatching KeyboardEvent. This is just alternative way to dispatch KeyboardEvent.

>> Anyway, you need to redesign handleKeyboardEvent() at least.
>> 
>> # And I have two urgent bugs which should be landed 44 and I'm going to go
>> to TPAC next week. So, I don't have much time. If it's okay me to review
>> next patch at Nov, let me know.
> 
> Thanks for your comment. I will try to make another patch before TPAC. During TPAC, do you
> recommend who can also help on this?

At least around keyboard and wheel, I should review them. However, smaug could review that. But anyway, this patch should be reviewed by somebody who know input handling of Gaia?

Comment 16

[Eric Tsai (no more review request accepted)]

Attachment: Add server script client.sjs to handle ajax request of remote control events

Update sendWheelEvent and using TIP send key press. For key press, the only key doesn't work on my b2g-desktop is ContextMenu, so I set to mouse event instead.
Also fix some coding style like space after function name.

Comment 17

[Eric Tsai (no more review request accepted)]

Comment on attachment 8678012 [details] [diff] [review]
Add server script client.sjs to handle ajax request of remote control events

Hi :smaug, I may need your help to review this patch. I've followed by Masayuki's comment. I will be good if you can have more comment.

Comment 18

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

Comment on attachment 8678012 [details] [diff] [review]
Add server script client.sjs to handle ajax request of remote control events

Trying to understand the setup here.
What executes client.sjs and when?
I assume httpd.js is running somewhere, in the system process? What guarantees it handles only
"safe" connections from local network?

>+function handleTouchEvent(event)
>+{
>+  let type = 'navigator:browser';
>+  let shell = Services.wm.getMostRecentWindow(type);
>+  var utils = shell.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                     .getInterface(Components.interfaces.nsIDOMWindowUtils);
>+
>+  var x = isNaN(getState("x")) ? 0 : parseInt(getState("x"));
>+  var y = isNaN(getState("y")) ? 0 : parseInt(getState("y"));
ok, so we start from coordinates 0,0


>+  let detail = event.detail;
>+  x = startX + detail.dx * 2;
>+  y = startY + detail.dy * 2;
then detail.d* is added.
Why * 2 ? Shouldn't that be something which depends on the remote client screen size and local screen size or something.
2 looks just rather magical.


>+    sendChromeEvent('move-cursor', {
>+      state: event.type.substring(5),
.substring(5) could use a comment (removing "touch")

>+  else {
>+    // Send spatial navigation key when not in cursor mode
>+    if (event.type == "touchend") {
>+      switch (detail.swipe) {
So is it document somewhere what kind of data event object may contain?

>+function handleWheelEvent(event)
>+{
>+  let type = 'navigator:browser';
>+  let shell = Services.wm.getMostRecentWindow(type);
>+  var utils = shell.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                     .getInterface(Components.interfaces.nsIDOMWindowUtils);
>+
>+  var x = isNaN(getState("x")) ? 0 : parseInt(getState("x"));
>+  var y = isNaN(getState("y")) ? 0 : parseInt(getState("y"));
>+  var sx = isNaN(getState("sx")) ? 0 : parseInt(getState("sx"));
>+  var sy = isNaN(getState("sy")) ? 0 : parseInt(getState("sy"));
>+  var isCursorMode = false;
>+  let detail = event.detail;
>+
>+  isCursorMode = (getSharedState("isCursorMode") == "true");
>+  if(isCursorMode) {
>+    utils.sendWheelEvent (x, y,
>+      detail.dx - sx, detail.dy - sy , 0, shell.WheelEvent.DOM_DELTA_LINE,
>+      0, detail.dx - sx, detail.dy - sy, 0);
>+
>+    setState ("sx", detail.dx.toString());
>+    setState ("sy", detail.dy.toString());

Why we need to store sx/sy? Why couldn't we always just use dx/dy values? 

And nit, no space between function name and (. Same also elsewhere.


>+    if (event.type == "scrollend") {
scrollend? That is surprising event name for something which then
does pageup/down
>+function handleKeyboardEvent(keyCodeName)
>+{
>+  debug('key: ' + keyCodeName);
>+
>+  let type = "navigator:browser";
>+  let shell = Services.wm.getMostRecentWindow(type);
>+  var utils = shell.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                     .getInterface(Components.interfaces.nsIDOMWindowUtils);
>+
>+  // Unable to send ContextMenu/DOM_VK_CONTEXT_MENU via TIP, it's the only key event send mouse event instead
because contentmenu event is actually a mouseevent.

>+  if (keyCodeName == "DOM_VK_CONTEXT_MENU") {
>+    var x = isNaN(getState("x")) ? 0 : parseInt(getState("x"));
>+    var y = isNaN(getState("y")) ? 0 : parseInt(getState("y"));
>+    utils.sendMouseEvent ("contextmenu", x, y, 0, 0, 0);
You could move getting WindowUtils to happen right before this call, since this is the only place it is used.


>+function handleRemoteInputEvent(detail)
What is this for? 'input' in DOM is a certain event, but this is about something else.
I can't find 'grant-input' anywhere in mozilla-central.
It is some gaia thing?

But this function certain needs some comment about what it is about, since the other functions
seem to be about normal DOM stuff, but this function is some gaia specific thing


>+function handleCustomEvent(event)
>+{
>+  sendChromeEvent(event.type, event.detail);
>+}
well, ok, this is gaia specific too, but the name hints that it is very much custom :)


So, I'm mostly requesting some more information about the setup and comments in the code.

Comment 19

[Eric Tsai (no more review request accepted)]

(In reply to Olli Pettay [:smaug] from comment #18)
> Trying to understand the setup here.
> What executes client.sjs and when?
> I assume httpd.js is running somewhere, in the system process? What
> guarantees it handles only
> "safe" connections from local network?

I've added more description in new patch. client.sjs is eval in sandbox by RemoteControlService.jsm (bug 1197749).
The connection between user and TV is HTTP now. Bug 1215457 will provide secure connection for the feature.


> then detail.d* is added.
> Why * 2 ? Shouldn't that be something which depends on the remote client
> screen size and local screen size or something.
> 2 looks just rather magical.
> 

After discuss with UX designer Tori, I switch back to startX + dx and startY + dy. Bug 1222372 will do more complex enhancement.

> .substring(5) could use a comment (removing "touch")

Agree, already did this.

> 
> >+  else {
> >+    // Send spatial navigation key when not in cursor mode
> >+    if (event.type == "touchend") {
> >+      switch (detail.swipe) {
> So is it document somewhere what kind of data event object may contain?

Add comment for this.

> >+  var sx = isNaN(getState("sx")) ? 0 : parseInt(getState("sx"));
> >+  var sy = isNaN(getState("sy")) ? 0 : parseInt(getState("sy"));
> >+  if(isCursorMode) {
> >+    utils.sendWheelEvent (x, y,
> >+      detail.dx - sx, detail.dy - sy , 0, shell.WheelEvent.DOM_DELTA_LINE,
> >+      0, detail.dx - sx, detail.dy - sy, 0);
> >+
> >+    setState ("sx", detail.dx.toString());
> >+    setState ("sy", detail.dy.toString());
> 
> Why we need to store sx/sy? Why couldn't we always just use dx/dy values? 
> 

Already in comment, reason is dx/dy is from starting point, but aDeltaX/aDeltaY is for each wheel event. So I need previous dx/dy and get diff for this time.

> 
> 
> >+    if (event.type == "scrollend") {
> scrollend? That is surprising event name for something which then
> does pageup/down

We get a gesture indicates it's up or down when scroll ends, then do page up/down at this time.


> >+  if (keyCodeName == "DOM_VK_CONTEXT_MENU") {
> >+    var x = isNaN(getState("x")) ? 0 : parseInt(getState("x"));
> >+    var y = isNaN(getState("y")) ? 0 : parseInt(getState("y"));
> >+    utils.sendMouseEvent ("contextmenu", x, y, 0, 0, 0);
> You could move getting WindowUtils to happen right before this call, since
> this is the only place it is used.

Fixed in new patch

> 
> 
> >+function handleRemoteInputEvent(detail)
> What is this for? 'input' in DOM is a certain event, but this is about
> something else.
> I can't find 'grant-input' anywhere in mozilla-central.
> It is some gaia thing?
> 
> But this function certain needs some comment about what it is about, since
> the other functions
> seem to be about normal DOM stuff, but this function is some gaia specific
> thing
> 
> 
> >+function handleCustomEvent(event)
> >+{
> >+  sendChromeEvent(event.type, event.detail);
> >+}
> well, ok, this is gaia specific too, but the name hints that it is very much
> custom :)
> 
> 
> So, I'm mostly requesting some more information about the setup and comments
> in the code.

Yes, I add more description in new patch.

Comment 20

[Eric Tsai (no more review request accepted)]

Attachment: 0001-Bug-1197751-Add-event-shutter-of-RemoteControlServic.patch

Update according to :smaug 's comment. Please help to see if the patch looks better now.

Comment 21

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

Comment on attachment 8684137 [details] [diff] [review]
0001-Bug-1197751-Add-event-shutter-of-RemoteControlServic.patch

>+++ b/b2g/remotecontrol/client.sjs
>@@ -0,0 +1,494 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+/*
>+ * client.sjs is following of remote control service (bug 1197749).
>+ * While RemoteControlService receives AJAX request, it creates a sandbox and eval clients.sjs in the sandbox.
Maybe something like

"When RemoteControlService receives an AJAX request, it creates a sandbox and executes clients.sjs in the sandbox"



>+ * While launch an app, TV system app will notify RemoteControlService current mode belongs to this app.
and perhaps "When launching and app, TV system app will notify RemoteControlService about the mode the app wants to use."

>+ * For example, browser is cursor mode, user can use virtual touchpad on client page to control app.
>+ * Not cursor mode, while user swipe on virtual touchpad, client.sjs treat event as arrow keys
"When not in cursor mode, if user swipes on virtual touchpad, client.sjs treats the event as an arrow key."


>+ * Client.sjs handles following events (not DOM event name), and synthesize corresponding DOM events:
>+ * - keypress: use TIP (Text Input Processor) send KeyboardEvent
>+ * - touchstart/move/end: in cursor mode, use nsIDOMWindowUtils send MouseEvent; in other mode, dispatch arrow keys
>+ * - scrollmove/end: in cursor mode, use nsIDOMWindowUtils send WheelEvent; in other mode, dispatch page up/down
>+ * - click, dblclick: in cursor mode, use nsIDOMWindowUtils send MouseEvent; in other mode, dispatch return key
>+ * - input: send remote input to TV system app, TV system app acts as an InputMethod
So this is still unclear. This is not about (DOM) input event but about something else, and it isn't clear to me what it is actually about.
https://wiki.mozilla.org/Firefox_OS/Remote_Control#Input_Events talks about input events, but could we call it something else?
TextInput perhaps?

>+ * - custom: dispatch to TV system app directly, like press PIN is not a keypress, need system app handle it
what is PIN here?


>+// Send ChromeEvent, as custom event communicate with Gaia TV system app
as a
to communicate

>+// Receved event format: { type: 'custom', action: <custom action name, string>, ... }
Received

>+// sendMouseEvent in cursor mode to move cursor position, or dispatch 4 arrow keys from swipe
I'm not seeing the function ever dispatch 4 arrow keys (or should it say array key events).
Should the comment be something like "or dispatch one of the 4 arrow keys"

>+// Receved event format: { type: 'touchstart', detail: { width: <client panel width>, height: <client panel height>} }
Received

>+// { type: 'touchmove', detail: { dx: <dx to start point>, dy: <dy to start point>} }
>+// { type: 'touchend', detail: { dx: <dx to start point>, dy: <dy to start point>}, swipe: <gesture of the touch, up/down/left/right> }
>+function handleTouchEvent(event)
>+{
>+  let type = 'navigator:browser';
>+  let shell = Services.wm.getMostRecentWindow(type);
>+  var utils = shell.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                   .getInterface(Components.interfaces.nsIDOMWindowUtils);
>+  var cursorPos = getCursorPosition();
>+  var x = cursorPos["x"];
>+  var y = cursorPos["y"];
>+  var startX = 0;
>+  var startY = 0;
>+  var isCursorMode = (getSharedState("isCursorMode") == "true");
I'm seeing var isCursorMode = (getSharedState("isCursorMode") == "true"); in many places.
Perhaps add a helper function:
function isInCursorMode() {
  return getSharedState("isCursorMode") == "true";
}


>+  // Calcualte new cursor position based on startX/dx, startY/dy when receiving each touchmove/touchend
Calculate

>+  else {
>+    // Send 4 direction key when not in cursor mode
This also talks about sending 4 keys, but isn't about 'one of the 4 arrow keys'

>+    if (event.type == "touchend") {
>+      switch (detail.swipe) {
>+        case "up":
>+        case "down":
>+        case "left":
>+        case "right":
>+          handleKeyboardEvent('DOM_VK_' + detail.swipe.toUpperCase());
>+          break;
>+      }
>+    }
>+  }
>+}


>+// sendWheelEvent in cursor mode to scroll pages, or dispatch page up/down KeyboardEvent
>+// Receved event format: { type: 'scrollstart', detail: { width: <client panel width>, height: <client panel height>} }
Received


>+// Clone from form.js to get KeyName
Any chance to put the function to some helper script which can be then used in both places?


>+// Use TIP (TextInputProcessor) to synthesize KeyboardEvent. But for ContextMenu, transfer to a mouseevent
>+// Receved event format: { type: 'keypress', detail: { <KeyEvent constant, string, sush as "DOM_VK_RETURN"> } }
Received


>+// Receive from client page and paste a string to focused input field, done by Gaia TV system app
>+// Gaia TV system app bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1203045
>+// Received event format { type: 'input', detail: {
>+//     clear: <whether to clear the entire string in the current focused input field, boolean>,
>+//     string: <new string to append, string>,
>+//     keycode: <a specified key to be pressed after the string inputted, integer>  } }
>+function handleRemoteInputEvent(detail)
So could we call this handleRemoteTextInputEvent or something. I think that would be a bit more clear, since we aren't
dealing with (DOM) input events here.

>+{
>+  DEBUG && debug('input: ' + JSON.stringify(detail));
>+
>+  if (getState("inputPending") == "true") {
>+    DEBUG && debug("ERROR: Has a pending input request!");
>+    return;
>+  }
Can this actually ever happen? If so, shouldn't we queue the events and not just drop them?

>+
>+  let sysApp = SystemAppProxy.getFrame().contentWindow;
>+  let mozIM = sysApp.navigator.mozInputMethod;
>+  let icChangeTimeout = null;
>+
>+  function icChangeHandler() {
>+    mozIM.removeEventListener('inputcontextchange', icChangeHandler);
>+    if (icChangeTimeout) {
>+      sysApp.clearTimeout(icChangeTimeout);
>+      icChangeTimeout = null;
>+    }
>+
>+    if (mozIM.inputcontext) {
>+      sendChromeEvent('input-string', detail);
>+    } else {
>+      DEBUG && debug('ERROR: No inputcontext!');
>+    }
>+
>+    mozIM.setActive(false);
>+    sendChromeEvent('grant-input', { value: false });
>+    setState("inputPending", "");
>+  }
>+
>+  setState("inputPending", "true");
>+  sendChromeEvent('grant-input', { value: true });
>+  mozIM.setActive(true);
>+  mozIM.addEventListener('inputcontextchange', icChangeHandler);
Should you add the events listener before setting mozIM active?

So, I'd like to see some clarifications here. Sorry about the delay in review.

Comment 22

[Eric Tsai (no more review request accepted)]

Luke, could you please help to provide more detail on how remote input method works?

Comment 23

[Luke Chang [:lchang] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #21)
> >+ * Client.sjs handles following events (not DOM event name), and synthesize corresponding DOM events:
> >+ * - keypress: use TIP (Text Input Processor) send KeyboardEvent
> >+ * - touchstart/move/end: in cursor mode, use nsIDOMWindowUtils send MouseEvent; in other mode, dispatch arrow keys
> >+ * - scrollmove/end: in cursor mode, use nsIDOMWindowUtils send WheelEvent; in other mode, dispatch page up/down
> >+ * - click, dblclick: in cursor mode, use nsIDOMWindowUtils send MouseEvent; in other mode, dispatch return key
> >+ * - input: send remote input to TV system app, TV system app acts as an InputMethod
> So this is still unclear. This is not about (DOM) input event but about
> something else, and it isn't clear to me what it is actually about.
> https://wiki.mozilla.org/Firefox_OS/Remote_Control#Input_Events talks about
> input events, but could we call it something else?
> TextInput perhaps?

This event represents a feature that users can type strings on their mobile device (a.k.a. the remote control client) and send the entire string to TV at a time. This feature makes users be able to easily input some complex string via the input method software and touch screen on their phone instead of a TV remote controller with limited keys that is definitely hard to use for typing.

I agree that the naming of "input" may cause confusion since it actually doesn't mean the native DOM input event, as you mentioned. So it should be better if we change the the naming of its handler to something like "handleRemoteTextInputEvent". However, regarding the event name itself, since it's just a protocol name used between client and server (similar to "scrollmove", "scrollend" above), I would suggest keeping it as-is. What do you think?

Comment 24

[Eric Tsai (no more review request accepted)]

>>+{
>>+  DEBUG && debug('input: ' + JSON.stringify(detail));
>>+
>>+  if (getState("inputPending") == "true") {
>>+    DEBUG && debug("ERROR: Has a pending input request!");
>>+    return;
>>+  }
> Can this actually ever happen? If so, shouldn't we queue the events and not just drop them?

In Luke's design in client page, it will sync remote text input every 200ms, replace old string. The only situation is the event is the last event. I will open a new bug to queue remote text input.

Luke, please also help to reply if we need add event listener before set mozIM active.

Comment 25

[Luke Chang [:lchang] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #21)
> >+  setState("inputPending", "true");
> >+  sendChromeEvent('grant-input', { value: true });
> >+  mozIM.setActive(true);
> >+  mozIM.addEventListener('inputcontextchange', icChangeHandler);
> Should you add the events listener before setting mozIM active?

Sorry, missed this question.

Our design is assuming that the event handler of "grant-input" in Gaia is a synchronized function so we can set "mozIM" active right after the previous line is finished without depending on other events. However, I'm not sure if there is another event that we should listen to before doing this?

Comment 26

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

I would really prefer to not use 'input event' in this case. 'input event' is such an overloaded concept, and also masayuki had some concerns in comment 13.
TextInput or RemoteTextInput or some such would be easier to understand, IMO.

Comment 27

[Luke Chang [:lchang] (inactive)]

Ok, we'll change the event name to "textinput". Thanks.

Comment 28

[Eric Tsai (no more review request accepted)]

> >+// Clone from form.js to get KeyName
> Any chance to put the function to some helper script which can be then used
> in both places?
> 

Open bug 1224432 for follow-up. Currently only forms.js, EventUtils.js and client.sjs use this function.

Comment 29

[Eric Tsai (no more review request accepted)]

Attachment: 0001-Bug-1197751-Add-event-shutter-of-RemoteControlServic.patch

Hi :smaug, I've updated according to your comment in attachment 8684137 [details] [diff] [review]. Please help to review again, thanks!

Comment 30

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

Comment on attachment 8686982 [details] [diff] [review]
0001-Bug-1197751-Add-event-shutter-of-RemoteControlServic.patch

>From 041016557a70d7ef959f44e7a2b9c634c3773db3 Mon Sep 17 00:00:00 2001
>From: MDTsai <[e-mail]>
>Date: Tue, 13 Oct 2015 18:28:55 +0800
>Subject: [PATCH] Bug 1197751 - Add event shutter of RemoteControlService
Btw, I don't quite understand what shutter means in this context. Isn't this all like "remote event receiver"?


Curious, was it ever thought if tv system app would actually connect to the client using EventSource?
Though, I guess that would be worse option since it would force client (remote controller) to implement a server  -
meaning that remote controller would be possibly limited to one type of devices only. ok, not so good idea :)




>+//>+function handleClickEvent(event)
>+{
>+  if (isInCursorMode()) {
>+    let type = 'navigator:browser';
>+    let shell = Services.wm.getMostRecentWindow(type);
>+    var utils = shell.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                     .getInterface(Components.interfaces.nsIDOMWindowUtils);
>+    var cursorPos = getCursorPosition();
>+
>+    ["mousedown",  "mouseup"].forEach(function(mouseType) {
>+      utils.sendMouseEvent(mouseType, cursorPos["x"], cursorPos["y"], 0, 1, 0);
wouldn't cursorPos.x, cursorPos.y work here (and be a bit easier to read than [] syntax)

>+     });
>+
>+     if (event.type == "dblclick") {
>+       ["mousedown",  "mouseup"].forEach(function(mouseType) {
>+         utils.sendMouseEvent(mouseType, cursorPos["x"], cursorPos["y"], 0, 2, 0);
ditto

>+function handleTouchEvent(event)
>+{
>+  let type = 'navigator:browser';
>+  let shell = Services.wm.getMostRecentWindow(type);
>+  var utils = shell.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                   .getInterface(Components.interfaces.nsIDOMWindowUtils);
>+  var cursorPos = getCursorPosition();
>+  var x = cursorPos["x"];
>+  var y = cursorPos["y"];
and here too

>+  var startX = 0;
>+  var startY = 0;
>+
>+  // Each event is independent, when touchstart, we set startX/startY for following touchmove/touchend
>+  switch (event.type) {
>+    case "touchstart":
>+      startX = cursorPos["x"];
>+      startY = cursorPos["y"];
why not just use x and y here?


>+  }
>+  else {
at least in our C++ code this should be } else {
>+function handleWheelEvent(event)
>+{
>+  let detail = event.detail;
>+
>+  if(isInCursorMode()) {
>+    let type = 'navigator:browser';
>+    let shell = Services.wm.getMostRecentWindow(type);
>+    var utils = shell.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                    .getInterface(Components.interfaces.nsIDOMWindowUtils);
>+    var cursorPos = getCursorPosition();
>+    var x = cursorPos["x"];
>+    var y = cursorPos["y"];
again, why not just cursorPos.x cursorPos.y 

>+
>+    // dx/dy is from starting point, but aDeltaX/aDeltaY to sendWheelEvent is current delta value
>+    // So we use sx/sy to store previous dx/dy, use dx-sx to get aDeltaX, dy-sy to get aDeltaY
>+    var sx = isNaN(parseInt(getState("sx"))) ? 0 : parseInt(getState("sy"));
>+    var sy = isNaN(parseInt(getState("sy"))) ? 0 : parseInt(getState("sy"));
>+
>+    utils.sendWheelEvent(x, y,
>+      detail.dx - sx, detail.dy - sy , 0, shell.WheelEvent.DOM_DELTA_LINE,
>+      0, detail.dx - sx, detail.dy - sy, 0);
We deal with pixels here in dx/y and sx/y, at least based on their names, but we send DOM_DELTA_LINE?
Why not DOM_DELTA_PIXEL?

>+  // Send ContextMenu/DOM_VK_CONTEXT_MENU via sendMouseEvent, the only one not via TIP
>+  if (keyCodeName == "DOM_VK_CONTEXT_MENU") {
>+    var utils = shell.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                    .getInterface(Components.interfaces.nsIDOMWindowUtils);
align .getInterface under .QueryInterface

>+    var cursorPos = getCursorPosition();
>+    utils.sendMouseEvent("contextmenu", cursorPos["x"], cursorPos["y"], 0, 0, 0);
again, why ["x"], why not just .x etc


r- mostly because of wheel event usage, since that needs some testing.

Comment 31

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Olli Pettay [:smaug] from comment #30)
> Comment on attachment 8686982 [details] [diff] [review]
> 0001-Bug-1197751-Add-event-shutter-of-RemoteControlServic.patch
> 
> >From 041016557a70d7ef959f44e7a2b9c634c3773db3 Mon Sep 17 00:00:00 2001
> >From: MDTsai <[e-mail]>
> >Date: Tue, 13 Oct 2015 18:28:55 +0800
> >Subject: [PATCH] Bug 1197751 - Add event shutter of RemoteControlService
> Btw, I don't quite understand what shutter means in this context. Isn't this
> all like "remote event receiver"?
Should be event "shuttle", which routes the remote event to the corresponding window / app.

Comment 32

[Eric Tsai (no more review request accepted)]

> >Subject: [PATCH] Bug 1197751 - Add event shutter of RemoteControlService
> Btw, I don't quite understand what shutter means in this context. Isn't this
> all like "remote event receiver"?

Fix wording as comment 31

> >+    // dx/dy is from starting point, but aDeltaX/aDeltaY to sendWheelEvent is current delta value
> >+    // So we use sx/sy to store previous dx/dy, use dx-sx to get aDeltaX, dy-sy to get aDeltaY
> >+    var sx = isNaN(parseInt(getState("sx"))) ? 0 : parseInt(getState("sy"));
> >+    var sy = isNaN(parseInt(getState("sy"))) ? 0 : parseInt(getState("sy"));
> >+
> >+    utils.sendWheelEvent(x, y,
> >+      detail.dx - sx, detail.dy - sy , 0, shell.WheelEvent.DOM_DELTA_LINE,
> >+      0, detail.dx - sx, detail.dy - sy, 0);
> We deal with pixels here in dx/y and sx/y, at least based on their names,
> but we send DOM_DELTA_LINE?
> Why not DOM_DELTA_PIXEL?

Originally we use DOM_DELTA_LINE to make it scroll faster on 4K TV, but I switch back to DOM_DELTA_PIXEL now.
Bug 1224995 is a follow-up bug to implement enhancement on each devices.

Comment 33

[Eric Tsai (no more review request accepted)]

Attachment: 0001-Bug-1197751-Add-event-shuttle-of-RemoteControlServic.patch

Comment 34

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

Sure, delta_line sure makes scrolling a lot faster, since it possibly ends up causing scrolling 16x more pixels (or whatever the page happens to use for a line). But if we're dealing with pixels, we should do pixel scrolling IMO, and if some acceleration is needed, either send more pixels or 
tweak mousewheel.default.delta_multiplier_x/y/z preferences

Comment 35

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

Comment on attachment 8687769 [details] [diff] [review]
0001-Bug-1197751-Add-event-shuttle-of-RemoteControlServic.patch

>+function debug(message)
>+{
>+    dump("client.sjs:" + message + '\n');
>+}
nit, you use 2 spaces for indentation elsewhere, but 4 here.
(2 is correct)

>+      var utils = shell.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                      .getInterface(Components.interfaces.nsIDOMWindowUtils);
nit, align .getInterface under .QueryInterface

Comment 36

[Eric Tsai (no more review request accepted)]

Attachment: Patch 1. Event shuttle of RemoteControlService

Fix install patch in b2g/installer/package-manifest.in

Comment 37

[Eric Tsai (no more review request accepted)]

Attachment: Patch 1. Event shuttle of RemoteControlService

Fix define macro issues that will lead files be packaged without macro.

Comment 38

[Eric Tsai (no more review request accepted)]

Attachment: Patch 1. Event shuttle of RemoteControlService

:schien, could you help to review the patch since previous patch is based on http.js. command.js is not changed after smaug's review.

Comment 39

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8753238 [details] [review]
Patch 1. Event shuttle of RemoteControlService

please see my comment on pull request.

Comment 40

[Eric Tsai (no more review request accepted)]

Comment on attachment 8753238 [details] [review]
Patch 1. Event shuttle of RemoteControlService

Hi :schien, I've updated according to your comment on PR. Please see if there is anything needs to update.

Comment 41

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8753238 [details] [review]
Patch 1. Event shuttle of RemoteControlService

generally looks good but some comment need to be addressed.

Comment 42

[Eric Tsai (no more review request accepted)]

Comment on attachment 8753238 [details] [review]
Patch 1. Event shuttle of RemoteControlService

Hi schien, I've updated according to your comment for commit 1293799. Please help to review b2g part, thanks!

Comment 43

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8753238 [details] [review]
Patch 1. Event shuttle of RemoteControlService

r+ with my comment addressed. please add "r=smaug,schien" in your commit log.

Comment 44

[Eric Tsai (no more review request accepted)]

Comment on attachment 8753238 [details] [review]
Patch 1. Event shuttle of RemoteControlService

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 1197751
User impact if declined: No feature
Testing completed: self-tested
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: N/A

Comment 45

[Josh Cheng [:josh]]

Comment on attachment 8753238 [details] [review]
Patch 1. Event shuttle of RemoteControlService

Approve for TV 2.6

Comment 46

[Eric Tsai (no more review request accepted)]

:xeonchen, could you help to merge this patch to gecko-b2g? Thanks.

Comment 47

[Gary Chen [:xeonchen]]

https://github.com/mozilla-b2g/gecko-b2g/commit/9f233a9b2c912dc45021db56e215090e2b955975