1191951 - [RedSquare] Modify developer tools and Mulet to emulate a flip phone and keypad

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Developer Tools, defect)

Description

[David Flanagan [:djf]]

For the red square project we need to be able to run gaia apps in an emulated flip phone.  This bug is for modifications to Mulet and devtools responsivemode to create that emulator.

Comment 1

[David Flanagan [:djf]]

I'm actively working on this and should have something ready for review soon.

Comment 2

[David Flanagan [:djf]]

This is also going to require my bug fix in bug 1191137.

Comment 3

[David Flanagan [:djf]]

Attachment: patch to emulate a flip phone with Mulet

This patch modifies responsivemode.jsm to add a new customizeUI(formfactor) function that calls buildPhoneUI or other methods depending on the formfactor value.  Supported formfactors are "phone" which just calls buildPhoneUI, "no-home-button", which calls buildPhoneUI, but leaves off the Home button and "flip" which calls a new function buildFlipPhoneUI() which creates a whole keypad of buttons. customizeUI() returns an object that specifies the width and height of the chrome that it has added to the basic size of the screen.

Ryan: would you review that part of the patch, please?

The other changes in this patch are in b2g/chrome/content/{desktop.js,screen.js}. In desktop.js there is a new function to figure out the desired form factor from the --form-factor command line argument or from the b2g.form.factor property. (Should I call it mulet.form.factor, perhaps?)
There are also some code simplifications in the way these two files call setSize() on the responsivemode UI, and code to use the chrome width and height returned by the customizeUI() function.

Alexandre, would you review this part of the patch? I hope you like the idea of a --form-factor command line argument and a way to simulate phones with no hardware home button.

Note that this patch requires the patch in bug 1191137 to work correctly.

Comment 4

[David Flanagan [:djf]]

Jan: since you cc'ed yourself on the bug, feel free to steal the review from Ryan if this is your area.  I don't know who does what in devtools.

Comment 5

[David Flanagan [:djf]]

Attachment: screenshot of a flip-phone stopwatch app running in Mulet with this patch

For context, here is what the flip phone emulator looks like when mulet is compiled with this patch and run with --form-factor=flip

The app running in the screenshot is a stopwatch that is controlled by the F1 OK and F2 buttons on the keypad

Comment 6

[:gerard-majax]

David, I fear this is code I am not a peer for reviewing :(.
Jonas, maybe you know who should review this?

Comment 7

[David Flanagan [:djf]]

Comment on attachment 8645227 [details] [diff] [review]
patch to emulate a flip phone with Mulet

Looks like I had the wrong Alexandre in the review request.

:ochameau would you review the desktop.js and screen.js changes please?

Comment 8

[David Flanagan [:djf]]

I've just learned that the screenshot attached to this bug may be shared during the all-hands meeting on Wednesday. So it would be nice if we could get this patch reviewed and landed Monday or Tuesday so that people can actually try it out with Mulet nightlies on Wednesday.

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8645227 [details] [diff] [review]
patch to emulate a flip phone with Mulet

Review of attachment 8645227 [details] [diff] [review]:
-----------------------------------------------------------------

It looks good codewise, but that is buggy on my linux.
Are you working on mac with retina?
I'll attach a screenshot with the flip mode, the button gets too big and overflow the responsive design phone skin.

::: b2g/chrome/content/desktop.js
@@ +174,5 @@
> +    // It would probably be better if we could just combine desktop.js
> +    // and screen.js into a single mulet.js file
> +    //
> +    responsive.setSize(desktop.width + desktop.chrome.paddingWidth,
> +                       desktop.height + desktop.chrome.paddingHeight);

It doesn't seem reasonable to merge desktop in screen.
I think the issue is in responsive, we are passing back values coming from it.
Responsive code returns desktop.chrome, which we are passing back again to its setSize method.
I think it is better to fix that in Responsive and calling setSize with actual sizes rather than working around the paddings from the callsites!

::: b2g/chrome/content/screen.js
@@ +27,2 @@
>    let DEFAULT_SCREEN = '320x480';
> +  if (desktop && desktop.formfactor === 'flip') {

You are somewhat lucky to have desktop.js's initResponsiveDesign being called from this code.
Both are called on ContentStart and all relies on the order of addEventListener(ContentStart calls.

It would be safer to define desktop.formfactor immediately:
 var desktop = {
   formfactor = getFormFactor()
 };

@@ +135,5 @@
>  
> +  if (isMulet) {
> +    desktop.width = width;
> +    desktop.height = height;
> +  }

I would rather define width/height on an object from screen.js rather than desktop as screen.js is taking care of that, not desktop.js.

::: browser/devtools/responsivedesign/responsivedesign.jsm
@@ +483,5 @@
> +  customizeUI: function(formfactor) {
> +    switch(formfactor) {
> +    case 'phone':
> +      return this.buildPhoneUI(true);
> +    case 'no-home-button':

I would have called that phone-no-home-btn or something alike.

@@ +615,5 @@
> +        { label: 'CLR', keycode: 'Backspace', id: 'clr'},
> +      ],
> +      [
> +        // On a flip phone it is the "End" key that acts like a home button.
> +        // We call the Send key "Home" becasue it goes with "End" and there

becasue -> because

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: buggy flip

Note that's I'm running that on a regular gaia.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8645227 [details] [diff] [review]
patch to emulate a flip phone with Mulet

Review of attachment 8645227 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/responsivedesign/responsivedesign.jsm
@@ +479,5 @@
>      this.stack.appendChild(this.resizeBarV);
>      this.stack.appendChild(this.resizeBarH);
>    },
>  
> +  customizeUI: function(formfactor) {

Nit: formfactor -> formFactor

Please add a comment about where this is called from, since it's not used inside this file.

@@ +481,5 @@
>    },
>  
> +  customizeUI: function(formfactor) {
> +    switch(formfactor) {
> +    case 'phone':

Nit: one more indent level in this block, as in this [example][1].

[1]: https://dxr.allizom.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/browser/devtools/debugger/debugger-controller.js#660

@@ +496,2 @@
>    // FxOS custom controls
> +  buildPhoneUI: function(includeHomeButton) {

Since we can use ES2015 in DevTools, I'd prefer an option arg for this, since it's clearer at the call site.  So:

    buildPhoneUI: function({ includeHomeButton }) {

and call with:

    return this.buildPhoneUI({ includeHomeButton: true });

@@ +556,5 @@
>      this.bottomToolbar = bottomToolbar;
>      this.container.appendChild(bottomToolbar);
> +
> +    return {
> +      paddingWidth: 32,

Storing all these magic numbers like this is not my favorite thing...  can you access them from the CSSOM as needed instead?

@@ +564,5 @@
> +
> +  // FxOS custom controls
> +  buildFlipPhoneUI: function () {
> +    this.stack.classList.add("fxos-mode");
> +    this.stack.classList.add("fxos-mode-flip");

I suppose `close` should also clear the extra flip class, to match the other FxOS class that is cleared there.

@@ +597,5 @@
> +    volumeButtons.appendChild(volumeUp);
> +    volumeButtons.appendChild(volumeDown);
> +    this.stack.appendChild(volumeButtons);
> +
> +    var keypadData = [

Nit: let

@@ +662,5 @@
> +
> +    let keypad = this.chromeDoc.createElement("vbox");
> +    keypad.className = "devtools-responsiveui-keypad";
> +
> +    for(let r = 0; r < keypadData.length; r++) {

Nit: for (

Could also use for of, up to you

@@ +668,5 @@
> +
> +      let row = this.chromeDoc.createElement("hbox");
> +      row.className = "devtools-responsiveui-keypad-row";
> +
> +      for(let k = 0; k < rowData.length; k++) {

Nit: for (

Could also use for of, up to you

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

One option here, if reviews are too painful, is that we just land these changes on the 2.2R branch.

Obviously it's better if we can do a real review, but I wanted to let you know that I think that's a valid option here since I'm aware that you guys are in a crunch.

Comment 13

[David Flanagan [:djf]]

Attachment: patch v2, with review comments addressed

ochameau and jryan: would you review this revised version of my flip-phone in Mulet patch, please?

I've addressed your review comments, and this is a significantly cleaner patch.  responsivemode.jsm now figures out the padding and border dimensions of the chrome automatically, so those numbers no longer need to be hardcoded, and that simplified things in the Mulet code as well.

This patch also contains a followup to bug 1191137. In that bug, I made it so the simulated hardware buttons in Mulet never took focus when clicked on. But this means that if the user types in the dev tools console, then the phone simulator window loses focus, and the hardware buttons do not work until the user clicks in somewhere in the window.  I've fixed that in SystemAppProxy (which responsivemode.jsm uses to send key events) so that we focus the window before sending a key event to it.

I'm going to need to land this code soon in the 2.2r branch. Landing it on master first would be great, but not strictly necessary. One good reason to get it landed on master is that it gives us --form-factor=phone-soft-home for simulating a phone with no hardware home button.

Note that the layout of the flip phone keypad, and the key events that it sends is preliminary, and is likely to change as we start working on the Red Square project on the 2.2r branch. So I'll probably be making changes on that branch that will get it out of sync with master. But having this initial patch landed on master will make it much easier to update master when the flip-phone emulator has stabilized on 2.2r.

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8656296 [details] [diff] [review]
patch v2, with review comments addressed

Review of attachment 8656296 [details] [diff] [review]:
-----------------------------------------------------------------

responsivedesign.jsm changes look good to me, thanks!

Alex is on PTO until Tuesday, hopefully he'll review soon after that.

::: browser/devtools/responsivedesign/responsivedesign.jsm
@@ +487,5 @@
> +   * design window. This method, and the build*UI() methods it calls are
> +   * not used here but are only for supporting external tools.
> +   *
> +   * See b2g/chrome/content/desktop.js.
> +   **/

Nit: I think just */ is the usual way to terminate these.

Comment 15

[David Flanagan [:djf]]

Landing this on 2.2r is going to be tricker than I expected. This patch relies on changes to SystemAppProxy to use TextInputProcessor to send mozbrowser(before|after)key(up|down) events before and after regular key events.  But TextInputProcessor was added in gecko 38 and 2.2r is based on gecko 37. This means that by uplifting bug 1191137 to 2.2r I busted the buttons in Mulet.  So I need to get that backed out and figure out another way to pass key events through to the running apps in the flip-phone case.  That's going to delay the uplift of this patch to 2.2r.

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 8656296 [details] [diff] [review]
patch v2, with review comments addressed

Review of attachment 8656296 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the tweaks, it looks much better!
Also, it does display fine on linux now.

::: b2g/chrome/content/screen.js
@@ +133,5 @@
>  
> +  if (isMulet) {
> +    mulet.width = width;
> +    mulet.height = height;
> +  }

The width/height isn't something mulet specific.
I would have called that variable differentely.
But the size is already exposed via GlobalSimulatorScreen.width and GlobalSimulatorScreen.height (set a few lines after).
So I think you can just use that instead.

Comment 17

[David Flanagan [:djf]]

We're no longer working on a flipphone product so this is not needed.