Closed Bug 758129 Opened 8 years ago Closed 8 years ago

Add screen size option to the b2g desktop client

Categories

(Firefox OS Graveyard :: General, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: djf, Assigned: djf)

Details

Attachments

(1 file, 4 obsolete files)

We need a way to be able to launch the b2g desktop client at different screen sizes and pixel densities. This is especially true as we start testing hardware that is not 480x800.

The attached patch adds a --screen=WIDTHxHEIGHT@DPI command-line option.
Attachment #626714 - Flags: review?(21)
Comment on attachment 626714 [details] [diff] [review]
implement the --screen option for b2g

>+++ b/b2g/chrome/content/shell.js

I wonder if this code does not worth a new command_line.js file.

>+// Set up the screen size and pixel density of the screen
>+// based on the --screen command-line option, if there is one
>+// TODO: support multiple device pixels per CSS pixel
>+// TODO: can I actually apply a transform to make it appear
>+// the correct physical size on the host screen?
>+(function() {
>+  // These are the named screens we support
>+  var screens = {
>+    iphone: {
>+      name: 'Apple iPhone', width:320, height:480,  dpi:163
>+    },
>+    ipad: {
>+      name: 'Apple iPad', width:1024, height:768,  dpi:132
>+    },
>+    nexus_s: {
>+      name: 'Samsung Nexus S', width:480, height:800, dpi:235
>+    },
>+    galaxy_s2: {
>+      name: 'Samsung Galaxy SII (I9100)', width:480, height:800, dpi:219
>+    },
>+    galaxy_nexus: {
>+      name: 'Samsung Galaxy Nexus', width:720, height:1280, dpi:316
>+    },
>+    galaxy_tab: {
>+      name: 'Samsung Galaxy Tab 10.1', width:800, height:1280, dpi:149
>+    },
>+    wildfire: {
>+      name: 'HTC Wildfire', width:240, height:320, dpi:125
>+    },
>+    tattoo: {
>+      name: 'HTC Tattoo', width:240, height:320, dpi:143
>+    },
>+    salsa: {
>+      name: 'HTC Salsa', width:320, height:480, dpi:170
>+    },
>+    chacha: {
>+      name: 'HTC ChaCha', width:320, height:480, dpi:222
>+    },
>+  };

Add '' around properties names and add a space after :

>+
>+  // Print usage info for --screen and exit
>+  function usage() {
>+    dump('The --screen argument specifies the desired resolution and\n');
>+    dump('pixel density of the simulated device screen. Use it like this:\n');
>+    dump('\t--screen=WIDTHxHEIGHT\n');
>+    dump('\t--screen=WIDTHxHEIGHT@DOTS_PER_INCH\n');
>+    dump('For example:\n');
>+    dump('\t--screen=480x800@240\n');
>+    dump('You can also specify certain device names:\n');
>+    for(var p in screens)
>+      dump('\t--screen=' + p + '\t// ' + screens[p].name + '\n');

dump does not print on the console by default. It does with the generated profile because I have added this option in DEBUG=1 mode but you likely want to use something else.

>+
>+    // Exit the b2g client
>+    Cc['@mozilla.org/toolkit/app-startup;1']
>+      .getService(Ci.nsIAppStartup)
>+      .quit(Ci.nsIAppStartup.eAttemptQuit);
>+  }

Services.startup.quit(Ci.nsIStartup.eAttemptQuit);

>+
>+  // Get the command line
>+  var args = window.arguments[0].QueryInterface(Ci.nsICommandLine);
>+
>+  // Get the screen argument
>+  try {
>+    var screenarg = args.handleFlagWithParam('screen', false);
>+    if (screenarg === null) // If none, nothing to process here
>+      return;

Add a line break

>+    if (screenarg == "")    // If no value, its an error
>+      usage();
>+  }
>+  catch(e) {
>+    // If getting the argument value fails, its an error
>+    usage();
>+  }
>+  
>+  var width, height, dpi;
>+  if (screenarg in screens) {
>+    var screen = screens[screenarg];
>+    width = screen.width;
>+    height = screen.height;
>+    dpi = screen.dpi;

Just to bikeshed and add a useless command, i wonder if you can use destructuring here ;)

>+  }
>+  else {

nit: } else {

>+    // screen argument is WIDTHxHEIGHT[@DPI]
>+    var match = screenarg.match(/^(\d+)x(\d+)(@(\d+))?$/);
>+
>+    if (match == null)
>+      usage();
>+    
>+    width = parseInt(match[1], 10);
>+    height = parseInt(match[2], 10);
>+    if (match[4])
>+      dpi = parseInt(match[4], 10);
>+  }
>+  
>+  if (!width || !height)
>+    usage();
>+
>+  // Set the screen width and height
>+  var shell = document.getElementById('shell');
>+  shell.width = width;
>+  shell.height = height;

window.resize?

>+  
>+  // If a density was specified, set it, too
>+  if (dpi) {
>+    Cc["@mozilla.org/preferences-service;1"]
>+      .getService(Ci.nsIPrefBranch)
>+      .setIntPref('layout.css.dpi', dpi);
>+  }


Services.prefs.setIntPref('layout.css.dpi', dpi);
Attachment #626714 - Flags: review?(21)
(In reply to Vivien Nicolas (:vingtetun) from comment #1)
> Comment on attachment 626714 [details] [diff] [review]
> implement the --screen option for b2g
> 
> >+++ b/b2g/chrome/content/shell.js
> 
> I wonder if this code does not worth a new command_line.js file.

That would be nice. shell.js is starting to be obese.

> >+// Set up the screen size and pixel density of the screen
> >+// based on the --screen command-line option, if there is one
> >+// TODO: support multiple device pixels per CSS pixel
> >+// TODO: can I actually apply a transform to make it appear
> >+// the correct physical size on the host screen?

I'd like --screen=fullscreen to be also supported. That would be handy when using b2g on non-android devices while still wanted to run fullscreen (eg. running b2g on a TV). You just have to set sizemode=fullscreen on the main xul window.

> >+    chacha: {
> >+      name: 'HTC ChaCha', width:320, height:480, dpi:222
> >+    },
> >+  };
> 
> Add '' around properties names and add a space after :

Vivien, we don't all have broken editors ;)
This version supports --screen=full and can rescale screens so they appear to be the correct physical size.
Attachment #626714 - Attachment is obsolete: true
Attachment #627309 - Flags: review?(fabrice)
Comment on attachment 627309 [details] [diff] [review]
cleaner, more powerful implementation

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

Almost there!

::: b2g/chrome/content/screen.js
@@ +3,5 @@
> +// based on the --screen command-line option, if there is one.
> +// 
> +// TODO: support multiple device pixels per CSS pixel
> +// 
> +window.addEventListener('DOMContentLoaded', function setupScreen() {

Use the ContentStart event instead, it fires once.

@@ +14,5 @@
> +  const Cc = Components.classes;
> +  const Ci = Components.interfaces;
> +  const Cu = Components.utils;
> +  Cu.import('resource://gre/modules/Services.jsm');
> +  

you can move all this boilerplate outside of the event listener

@@ +22,5 @@
> +  // The <browser> element inside it
> +  let browser = document.getElementById('homescreen');
> +
> +  // Bring the b2g window to the front
> +  window.focus();

why do we need that?

@@ +97,5 @@
> +
> +  // If the value of --screen ends with !, we'll be scaling the output
> +  if (screenarg[screenarg.length-1] === '!') {
> +    rescale = true;
> +    screenarg = screenarg.substring(0, screenarg.length-1);

nit: spaces around the - operator

@@ +134,5 @@
> +  // In order to do rescaling, we set the <browser> tag to the specified
> +  // width and height, and then use a CSS transform to scale it so that
> +  // it appears at the correct size on the host display.  We also set
> +  // the size of the <window> element to that scaled target size.
> +  let scale = rescale ? hostDPI/dpi : 1;

nit: spaces around the / operator
Attachment #627309 - Flags: review?(fabrice) → review-
Attachment #627309 - Attachment is obsolete: true
Attachment #627484 - Flags: review?(fabrice)
Fabrice,

Thanks for the review. The new attachment addresses your comments. If I move the Cc, Ci, Cu stuff out of the event handler I get redef error when shell.js runs, so I've just removed them and will rely on them being defined elsewhere before this event handler runs.

Nice to know about ContentStart.  The window.focus() was a failed experiment in fixing a different bug (that the b2g window doesn't appear on top when it starts up). But that code didn't fix the problem anyway, and I forgot to remove it.

I fixed the spacing nits, though I have to say that I think that in the context of a ternary operator, the division is easier to read without the spaces around the /...
Comment on attachment 627484 [details] [diff] [review]
address Fabrice's review comments

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

Thanks for the update David, the code looks fine now.

After talking with Vivien, we agreed that we want this kind of functionality since it's a very useful tool for developers, but we're not really comfortable shipping it in the product. Can you take a look at converting that to an extension? (we support packaging extensions as part of the gaia build system in $gaia/tools/extensions)

It means that you'll have to implement your changes in a JS xpcom component that implements nsICommandLineHandler (see http://mxr.mozilla.org/mozilla-central/source/mobile/xul/components/BrowserCLH.js for instance)
Attachment #627484 - Flags: review?(fabrice)
Can't I just use an #ifdef or #ifndef in shell.xul (it already has one) so that screen.js is only included on the desktop and not on the phone?  Vivien: Fabrice wanted to know what you think of this idea.

In an ideal world, I'd love to take the time to learn about writing Firefox extensions, but I simply don't have time for that now.  #ifdef I can do. Otherwise, I'll just have to assign the bug back to nobody and hope that someone else picks it up.
The emulator already allows us to set different screen sizes. I'd rather we move our testing and development to the emulator where we can because it runs the complete stack. The x86 emulator is reasonably fast and I hear rumours it builds on OSX now.
Does the emulator have an option for emulating pixel density? Is it documented? 

Our apps may well rely on CSS media queries that target specific device resolutions.
(In reply to David Flanagan from comment #10)
> Does the emulator have an option for emulating pixel density?

You can use

 -skin ­­<width>x<height>

to set the overall resolution and

 -scale N

to scale the emulated pixels compared to your real (host) pixels. These are command line args for the emulator, btw.

> Is it documented?

I couldn't find it in the obvious places (e.g. http://developer.android.com/guide/developing/devices/emulator.html). :(

> Our apps may well rely on CSS media queries that target specific device
> resolutions.

Certainly. Would they be able to query for something that we might not be able to emulate? Or let me ask this way: is there something we the B2G desktop build can do that we can't wit the above solution?
> > Our apps may well rely on CSS media queries that target specific device
> > resolutions.
> 
> Certainly. Would they be able to query for something that we might not be
> able to emulate? Or let me ask this way: is there something we the B2G
> desktop build can do that we can't wit the above solution?

I've never used the emulator. I've never seen anyone use the emulator. Maybe I'm the only Gaia dev who doesn't use it, but I have no idea how it works. If I'm trying to emulate a Nexus S, for example, do CSS media queries for min-resolution and max-resolution and the CSS mozmm unit work exactly the way they would on the actual device? That's what this patch is trying to achieve for the b2g desktop client.

Even if the emulator does exactly what we need, it can't hurt to add an #ifdef and support this option for b2g desktop, too.
(In reply to David Flanagan from comment #12)
> I've never used the emulator. I've never seen anyone use the emulator. Maybe
> I'm the only Gaia dev who doesn't use it, but I have no idea how it works.

We're hoping to be able to make nightly builds of emulator images available soon, and then it should be possible to easily test Gaia apps in a device-like environment.

> If I'm trying to emulate a Nexus S, for example, do CSS media queries for
> min-resolution and max-resolution and the CSS mozmm unit work exactly the
> way they would on the actual device?

I do not know. Is there a test case you can point me to? I'll be happy to find out.
The screen.js script will be bunded as part of b2g but will only run on the desktop, not on the device.
Attachment #627484 - Attachment is obsolete: true
Attachment #628466 - Flags: review?(fabrice)
Attachment #628466 - Attachment is patch: true
Comment on attachment 628466 [details] [diff] [review]
wrap the screen.js script tag in #ifndef ANDROID

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

r=me with comment addressed

::: b2g/chrome/jar.mn
@@ +11,4 @@
>    content/dbg-browser-actors.js         (content/dbg-browser-actors.js)
>  * content/shell.xul                     (content/shell.xul)
>  * content/shell.js                      (content/shell.js)
> +  content/screen.js                     (content/screen.js)

we need to #ifdef there also
Attachment #628466 - Flags: review?(fabrice) → review+
This attachment fixes the one remaining problem that Fabrice identified in his r+ of the previous patch.
Attachment #628466 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1861a6f27d8a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.