944443 - Uplift simulator UI to b2g desktop

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Runtime, defect)

Description

[Alexandre Poirot [:ochameau] [PTO back August 21]]

It implies various tasks:
- figure out how to have a desktop UI that doesn't introduce risk for the device product (introducing new top level document, just for desktop can help)
- uplift the few UI features we have:
  * home button
  * rotate button (That would deserve its own bug and will most likely be done differently (sanely?) by tweaking nsScreen.cpp)
  * menu bar (I'm not sure it is really important)
- uplift shell.js fixes instead of hotfixing:
  https://github.com/mozilla/r2d2b2g/blob/master/prosthesis/content/shell.xul#L33-L51

Comment 1

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Attachment: Uplift home button from simulator to b2g desktop

Introduces a toolbar, under the system app, with just a home button.

Comment 2

[[:fabrice] Fabrice Desré]

Please don't land that for now. This change is making the assumption that b2g desktop == simulator. That's not true, for instance I have people in this year GSoC working on using b2g desktop as a linux DE. So we must have support for different products using the same WIDGET backend.

I see two ways to make progress there:
- use a pref to turn on your simulator bits.
- add a build time config MOZ_B2G_SIMULATOR. This means that we'll need different builds which can be a bit annoying but that's probably the cleanest solution.

Comment 3

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Right, I can use FXOS_SIMULATOR which already exists.
But I'm planning to turn it on in our builds (bug 920198), so that b2g desktop builds we are distributing with a gaia profile are going to have the simulator bits.
Is that ok fo you?

Comment 4

[[:fabrice] Fabrice Desré]

(In reply to Alexandre Poirot (:ochameau) from comment #3)
> Right, I can use FXOS_SIMULATOR which already exists.
> But I'm planning to turn it on in our builds (bug 920198), so that b2g
> desktop builds we are distributing with a gaia profile are going to have the
> simulator bits.
> Is that ok fo you?

Yes, thanks!

Comment 5

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Attachment: Uplift home button from simulator to b2g desktop

Same patch, but with some FXOS_SIMULATOR ifdefs

Comment 6

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Attachment: Uplift rotation feature from simulator to b2g desktop

Another patch, the last one. This time to uplift the rotation feature.
That patch is quite hacky and limited, but baked a while in the simulator.
I would like to uplift it to m-c so that we can spawn 1.4 simulator builds
out of m-c and starts deprecating the github repo.

This code is meant to be replaced with some sane integration into screen
platform code. Paul already started crafting such patch that would allow
faking screen size and orientation without such hack and that
would propagate the correct faked size over all platform codes.

Comment 7

[Alexandre Poirot [:ochameau] [PTO back August 21]]

https://tbpl.mozilla.org/?tree=Try&rev=4df907526483

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 8390670 [details] [diff] [review]
Uplift home button from simulator to b2g desktop

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

r=me with comments addressed (feel free to not rename desktop.css)

::: b2g/chrome/content/shell.html
@@ +18,5 @@
>    <script type="application/javascript;version=1.8"
>            src="chrome://b2g/content/shell.js"> </script>
>  
>  #ifndef MOZ_WIDGET_GONK
> +  <link rel="stylesheet" href="desktop.css" type="text/css">

that should be #ifdef FXOS_SIMULATOR too, and maybe renamed simulator.css

::: b2g/chrome/content/shell.js
@@ +302,5 @@
>  #ifdef MOZ_WIDGET_COCOA
> +    // See shell.html
> +    let hotfix = document.getElementById('placeholder');
> +    if (hotfix)
> +      container.removeChild(hotfix);

nit: {} even for single line if's

::: b2g/chrome/jar.mn
@@ +13,5 @@
>  * content/shell.html                    (content/shell.html)
>  * content/shell.js                      (content/shell.js)
>    content/shell.css                     (content/shell.css)
>    content/devtools.js                   (content/devtools.js)
>  #ifndef ANDROID

not your fault, but that should be #ifndef MOZ_WIDGET_GONK

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 8390672 [details] [diff] [review]
Uplift rotation feature from simulator to b2g desktop

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

::: b2g/chrome/content/screen.js
@@ +119,5 @@
>      // If any of the values came out 0 or NaN or undefined, display usage
>      if (!width || !height || !dpi)
>        usage();
>    }
>    

not your fault, but let's remove this whitespace

@@ +201,5 @@
> +
> +    // If the current app doesn't supports the default orientation,
> +    // still resize the window, but rotate its frame so that
> +    // it is displayed rotated on the side
> +    let shouldFlip = mozOrientationLocked &&

shouldn't this be !mozOrientationLocked?

@@ +213,5 @@
>    function print() {
>      let dump_enabled =
>        Services.prefs.getBoolPref('browser.dom.window.dump.enabled');
>  
>      if (!dump_enabled) 

whitespace! vade retro!

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8390672 [details] [diff] [review]
Uplift rotation feature from simulator to b2g desktop

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

It sounds good but I would like to see another version before r+'ing.

::: b2g/chrome/content/screen.js
@@ +167,5 @@
> +    }
> +
> +    if (shouldFlip) {
> +      // Display the system app with a 90° clockwise rotation
> +      let shift = Math.floor(Math.abs(frameWidth-frameHeight)/2);

nit: add spaces around the /

@@ +170,5 @@
> +      // Display the system app with a 90° clockwise rotation
> +      let shift = Math.floor(Math.abs(frameWidth-frameHeight)/2);
> +      browser.style.transform +=
> +        ' rotate(0.25turn) translate(-' + shift + 'px, -' + shift + 'px)';
> +    }

What about a |style| variable. Seems like you keep accessing browser.style.

@@ +175,5 @@
> +
> +    // Set the pixel density that we want to simulate.
> +    // This doesn't change the on-screen size, but makes
> +    // CSS media queries and mozmm units work right.
> +    Services.prefs.setIntPref('layout.css.dpi', dpi);

Maybe add a comment explaining that the CSS query for width/height is not going to work. And that we need a better support ? Or does it works ?

@@ +185,5 @@
> +  let defaultOrientation = width < height ? 'portrait' : 'landscape';
> +
> +  // Then resize on each rotation button click,
> +  // or when the system app lock/unlock the orientation
> +  Services.obs.addObserver(function (subject) {

nit: extra space after function, but it would be even better to give it a name.

@@ +190,5 @@
> +    let screen = subject.wrappedJSObject;
> +    let { mozOrientationLocked, screenOrientation } = screen;
> +
> +    // If we have an orientation different than the startup one,
> +    // we switch the sizes

Let's move this comment closer to the block that does the switching.

@@ +192,5 @@
> +
> +    // If we have an orientation different than the startup one,
> +    // we switch the sizes
> +    let newWidth = width,
> +        newHeight = height;

Use 2 |let| declarations.

::: b2g/components/GlobalSimulatorScreen.jsm
@@ +1,4 @@
> +/* 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/. */
> +this.EXPORTED_SYMBOLS = [ 'GlobalSimulatorScreen' ];

Add a line break before this instruction.

@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +this.EXPORTED_SYMBOLS = [ 'GlobalSimulatorScreen' ];
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;

Cc and Ci are unused in this file. Please remove them.

@@ +22,5 @@
> +  width: 0, height: 0,
> +
> +  lock: function() {
> +    this.mozOrientationLocked = true;
> +    Services.obs.notifyObservers(null, 'simulator-orientation-lock-change', null);

this.broadcastOrientationLockChange(); ?

@@ +28,5 @@
> +  },
> +
> +  unlock: function() {
> +    this.mozOrientationLocked = false;
> +    Services.obs.notifyObservers(null, 'simulator-orientation-lock-change', null);

this.broadcastOrientationLockChange(); ?

@@ +41,5 @@
> +  flipScreen: function() {
> +    if (this.screenOrientation == 'portrait') {
> +      this.screenOrientation = 'landscape';
> +      this.adjustWindowSize();
> +      this.broadcastOrientationChange();

Any reason that this sequence:

this.adjustWindowSize();
this.broadcastOrientationChange();

is the inverse order of:

this.broadcastOrientationLockChange();
this.adjustWindowSize();

?

::: b2g/components/SimulatorScreen.js
@@ +2,5 @@
> + * 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/. */
> +
> +let Ci = Components.interfaces;
> +let Cc = Components.classes;

Cc is unused please remove it.

@@ +12,5 @@
> +
> +XPCOMUtils.defineLazyModuleGetter(this, 'GlobalSimulatorScreen',
> +                                  'resource://gre/modules/GlobalSimulatorScreen.jsm');
> +
> +let DEBUG = true;

Turn that to false.

@@ +16,5 @@
> +let DEBUG = true;
> +let DEBUG_PREFIX = 'SimulatorScreen.js - ';
> +let debug = DEBUG ?
> +  function debug() dump(DEBUG_PREFIX + Array.slice(arguments) + '\n') :
> +  function() {};

An other simple way to declare that is to:

function debug() {
  //dump(DEBUG_PREFIX + Array.slice(arguments) + '\n');
}

Then when someone wants to debug he just needs to remove the comment.

@@ +37,5 @@
> +    let changed = orientation !== GlobalSimulatorScreen.mozOrientation;
> +
> +    if (orientation.match(/^portrait/) || orientation == 'default') {
> +      GlobalSimulatorScreen.mozOrientation = orientation;
> +      GlobalSimulatorScreen.lock();

What about passing |orientation| to GlobalSimulatorScreen.lock(); and let it update it's internal state, and call adjustWindowSize itself ?

Then it can returns if the orientation has changed or not and can replace your |changed| variable ?

@@ +40,5 @@
> +      GlobalSimulatorScreen.mozOrientation = orientation;
> +      GlobalSimulatorScreen.lock();
> +
> +      if (changed) {
> +        GlobalSimulatorScreen.adjustWindowSize();

It seems like adjustWindowSize is already called by GlobalSimulatorScreen.

@@ +45,5 @@
> +        fireOrientationEvent(window);
> +      }
> +
> +      return true;
> +    }

Add a line break.

@@ +56,5 @@
> +        fireOrientationEvent(window);
> +      }
> +
> +      return true;
> +    }

Add a line break after this block.

Btw, I'm not sure to see the difference between this block and the previous one ?

@@ +64,5 @@
> +  };
> +
> +  window.wrappedJSObject.screen.mozUnlockOrientation = function() {
> +    debug('mozOrientationUnlock from', origin);
> +    GlobalSimulatorScreen.unlock();

Does |unlock| can trigger a window resize as well ?

@@ +76,5 @@
> +    get: function () GlobalSimulatorScreen.height
> +  });
> +  Object.defineProperty(window.wrappedJSObject.screen, 'mozOrientation', {
> +    get: function () GlobalSimulatorScreen.mozOrientation
> +  });

Declare a |let screen = window.wrappedJSObject.screen;| and use that.

@@ +96,5 @@
> +      }
> +      case 'document-element-inserted': {
> +        let window = subject.defaultView;
> +        if (!window)
> +          return;

Add a line break after the return;

@@ +97,5 @@
> +      case 'document-element-inserted': {
> +        let window = subject.defaultView;
> +        if (!window)
> +          return;
> +        hookScreen(window);

Add a line break after hookScreen.

@@ +103,5 @@
> +        window.addEventListener('unload', function unload() {
> +          window.removeEventListener('unload', unload);
> +          self._windows.delete(window);
> +        });
> +        self._windows.add(window);

Seems like that instead of passing |self| you can do a local |windows| object, and use that across this case.

@@ +110,5 @@
> +      case 'simulator-orientation-change': {
> +        this._windows.forEach(fireOrientationEvent);
> +        break;
> +      }
> +    }

Add line breaks between each |case|.

Also the curly braces surrounding the cases seems unneeded here.

Comment 11

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Attachment: interdiff home button patch

Addressed attachment 8390670 [details] [diff] [review] review comments.

Comment 12

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Attachment: Uplift home button from simulator to b2g desktop

Comment 13

[Alexandre Poirot [:ochameau] [PTO back August 21]]

(In reply to Fabrice Desré [:fabrice] from comment #9)
> Comment on attachment 8390672 [details] [diff] [review]
> @@ +201,5 @@
> > +
> > +    // If the current app doesn't supports the default orientation,
> > +    // still resize the window, but rotate its frame so that
> > +    // it is displayed rotated on the side
> > +    let shouldFlip = mozOrientationLocked &&
> 
> shouldn't this be !mozOrientationLocked?

Hum yes, that condition sounds really weird, I changed it to:
  let shouldFlip = mozOrientation != screenOrientation;
(with various other changes to make SimulatorScreen and GlobalSimulatorScreen simplier, easier to read and with less bugs...)

Comment 14

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Attachment: interdiff rotation patch

Address attachment 8390672 [details] [diff] [review] comments.
In the first patch I just uplifted simulator code without really looking at it.
In this patch I tried to understand it for real and ended up indentifying many issues.
Like event being sent multiple times or not at all and screen.mozOrientation being wrong...

This new patch should be easier to understand and with less such bugs.

Comment 15

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Attachment: Uplift rotation feature from simulator to b2g desktop

Comment 16

[Alexandre Poirot [:ochameau] [PTO back August 21]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #10)
> Comment on attachment 8390672 [details] [diff] [review]
> @@ +175,5 @@
> > +
> > +    // Set the pixel density that we want to simulate.
> > +    // This doesn't change the on-screen size, but makes
> > +    // CSS media queries and mozmm units work right.
> > +    Services.prefs.setIntPref('layout.css.dpi', dpi);
> 
> Maybe add a comment explaining that the CSS query for width/height is not
> going to work. And that we need a better support ? Or does it works ?

I have no idea, I just moved that code; that should be independant of screen/orientation modification.
The screen size we are faking are only going to be visible to apps JS code.

> @@ +37,5 @@
> > +    let changed = orientation !== GlobalSimulatorScreen.mozOrientation;
> > +
> > +    if (orientation.match(/^portrait/) || orientation == 'default') {
> > +      GlobalSimulatorScreen.mozOrientation = orientation;
> > +      GlobalSimulatorScreen.lock();
> 
> What about passing |orientation| to GlobalSimulatorScreen.lock(); and let it
> update it's internal state, and call adjustWindowSize itself ?
> 
> Then it can returns if the orientation has changed or not and can replace
> your |changed| variable ?

I changed more than just that, the whole interaction between SimulatorScreen and GlobalSimulatorScreen was broken. Now GlobalSimulatorScreen handle most of the critical code and better decide when to dispatch new events or update the window rotation.

> @@ +56,5 @@
> > +        fireOrientationEvent(window);
> > +      }
> > +
> > +      return true;
> > +    }
> 
> Add a line break after this block.
> 
> Btw, I'm not sure to see the difference between this block and the previous
> one ?

Nothing, just old code being refactored too many times.

> 
> @@ +64,5 @@
> > +  };
> > +
> > +  window.wrappedJSObject.screen.mozUnlockOrientation = function() {
> > +    debug('mozOrientationUnlock from', origin);
> > +    GlobalSimulatorScreen.unlock();
> 
> Does |unlock| can trigger a window resize as well ?

Yes... it was buggy...

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8392866 [details] [diff] [review]
Uplift rotation feature from simulator to b2g desktop

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

Not sure if that's a splinter review bug or not but I can't see the rotate.png file in this patch ?

r+ with nits addressed and the rotate.png file.

::: b2g/chrome/content/desktop.js
@@ +37,5 @@
> +
> +  Cu.import("resource://gre/modules/GlobalSimulatorScreen.jsm");
> +  let rotateButton = document.getElementById('rotate-button');
> +  rotateButton.addEventListener('touchstart', function () {
> +    GlobalSimulatorScreen.flipScreen();

Sorry I didn't notice that in the first version. Maybe we want to do that on touchend ?

::: b2g/components/GlobalSimulatorScreen.jsm
@@ +9,5 @@
> +Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> +Cu.import('resource://gre/modules/Services.jsm');
> +
> +this.GlobalSimulatorScreen = {
> +  mozOrientationLocked: false,

nit: add a line break

@@ +15,5 @@
> +  mozOrientation: 'portrait',
> +
> +  // The restricted list of actual orientation that can be used
> +  // if mozOrientationLocked is true
> +  lockedOrientation: ['portrait'],

Any reason to start with something in this array ?

@@ +27,5 @@
> +  // Updated by screen.js
> +  width: 0, height: 0,
> +
> +  lock: function(orientation) {
> +    this.mozOrientationLocked = true;

nit: add a line break.

@@ +67,5 @@
> +    } else {
> +      // Otherwise, when the orientation isn't locked,
> +      // we just use the screen orientation
> +      orientation = this.screenOrientation;
> +    }

Can't you |let orientation = this.screenOrientation;| and update it only when the orientation is locked and the orientation is not part of any known orientation ?

::: b2g/components/SimulatorScreen.js
@@ +13,5 @@
> +                                  'resource://gre/modules/GlobalSimulatorScreen.jsm');
> +
> +let DEBUG_PREFIX = 'SimulatorScreen.js - ';
> +function debug() {
> +  dump(DEBUG_PREFIX + Array.slice(arguments) + '\n');

Comment this line before landing.

@@ +37,5 @@
> +
> +    // Normalize and do some checks against orientation input
> +    if (typeof(orientation) == 'string') {
> +      orientation = [orientation];
> +    }

nit: add a line break

Comment 18

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Attachment: Uplift rotation feature from simulator to b2g desktop

Addressed last comments, carrying over r+.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/2246fe6ceae0
https://hg.mozilla.org/integration/b2g-inbound/rev/2aa1cbf3cd21

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2246fe6ceae0
https://hg.mozilla.org/mozilla-central/rev/2aa1cbf3cd21

Comment 21

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Comment on attachment 8392858 [details] [diff] [review]
Uplift home button from simulator to b2g desktop

See bug 976773 comment 30.
We would like to build 1.4 simulators and need to uplift mostly desktop-specific patches.

This patch has naive modification to shell.html/shell.js.

Comment 22

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Comment on attachment 8392935 [details] [diff] [review]
Uplift rotation feature from simulator to b2g desktop

See bug 976773 comment 30.
We would like to build 1.4 simulators and need to uplift mostly desktop-specific patches.

This patch has naive modification to shell.html/shell.js.

Comment 23

[bhavana bajaj [:bajaj]]

(In reply to Alexandre Poirot (:ochameau) from comment #22)
> Comment on attachment 8392935 [details] [diff] [review]
> Uplift rotation feature from simulator to b2g desktop
> 
> See bug 976773 comment 30.
> We would like to build 1.4 simulators and need to uplift mostly
> desktop-specific patches.
> 
> This patch has naive modification to shell.html/shell.js.

A generic request for all the Simulator patches in queue on aurora.

Can you please fill the details that come-in with the approval request comment in here ? I know some of these may not directly apply to your change but I very curious on the risk , how has this been tested and this may not have any fallouts.

Comment 24

[Alexandre Poirot [:ochameau] [PTO back August 21]]

I tried to explain the bulk uplift in bug 976773 comment 30,
but yes, it wasn't as complete as the form.
Here is the formal information, for all these patches:

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined:
  No automatic release of 1.4 simulators.
Testing completed (on m-c, etc.): 
  All these patches are on mozilla-central for a few days/weeks.
  This one had some fallout which have been addressed in bug 988563.
  We shipped 1.4 simulator with all these patches and tested them quite a bit before releasing them.
  I applied all of them on 1.4 branch. Only one of them bitrot, bug 990057, which already contains a 1.4 specific patch.
  I also flashed my phone with this patch queue on 1.4 branch and I haven't seen big breakage.

  Finally, here is a try build of these patches on 1.4:
    https://tbpl.mozilla.org/?tree=Try&rev=3864eadf1d2f
    (There is some failure, but it looks like try is running mozilla-central test suites instead of aurora ones...)

Risk to taking this patch (and alternatives if risky): 
  Limited, most of these patches are desktop only.
  This one is (with its follow up 988563) the only one patching device codepath,
  but it is only very naive modification to shell.html/shell.js.
String or IDL/UUID changes made by this patch:
  none

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0abb229431e0
https://hg.mozilla.org/releases/mozilla-aurora/rev/73252b6d1598