This repository has been archived by the owner. It is now read-only.

refactor simulator screen #377

Merged
merged 38 commits into from Mar 29, 2013

Conversation

Projects
None yet
3 participants
Member

rpl commented Mar 14, 2013

big refactoring on the prosthesis side focused on the screen simulation components,
window resizing and "gaia window management tweaks".

  • remove any DOM related code from the GlobalSimulatorScreen javascript module
  • propagate orientation change to visible windows using mozilla parent-child messaging
  • new screen orientatio change listener implementation (from DOMFMRadioChild)
  • relaunch app if already running (prevents iframe leaking due to rapid kill+run sequence)
  • fix app orientation on b2g-desktop not already running
  • isolated DOM related code into prosthesis/content/window_manager.js
  • use mutation_summary.js to detect changes in the gaia app iframes dom elements list

should fix #346

Member

rpl commented Mar 17, 2013

in the last commit (838fc33) I've removed unused windowManagerEvents and moved remote geolocation code in its own module, because debugging some strange behavior in this branch I've noticed that geolocation message exchange on the remote debugger protocol doesn't interact correctly with the other simulator commands over the same connection,

How did the geolocation messages interact incorrectly with the other simulator commands?

Collaborator

nickdesaulniers commented Mar 18, 2013

@rpl , the code looks really nice! I have so much to learn about XPCOM! I'll test out that commit to triple check everything.

Collaborator

nickdesaulniers commented Mar 18, 2013

Yeah man looks good!

addon/lib/remote-geolocation-client.js
+ initialize: function initialize(options) {
+ EventTarget.prototype.initialize.call(this, options);
+ this._hookInternalEvents();
+ },
@mykmelez

mykmelez Mar 18, 2013

Owner

Nit: add a line of whitespace before and after the initialize property declaration to make it easier to distinguish from the declarations before and after it.

addon/lib/remote-geolocation-client.js
+ EventTarget.prototype.initialize.call(this, options);
+ this._hookInternalEvents();
+ },
+ _hookInternalEvents: function () {
@mykmelez

mykmelez Mar 18, 2013

Owner

Nit: give this function a name to make it easier to trace in a stack, i.e.:

  _hookInternalEvents: function _hookInternalEvents() {
addon/lib/remote-geolocation-client.js
+ // on kill and send a "listTabs" debug protocol request, finally
+ // emit a clientReady event on "listTabs" reply
+ this.on("clientConnected", function (data) {
+ console.debug("rsc.onClientConnected");
@mykmelez

mykmelez Mar 18, 2013

Owner

Nit: here and elsewhere, change "rsc" to a value that matches the code in question, like "rgc" or "RemoteGeolocationClient".

addon/lib/remote-geolocation-client.js
+ this._clientConnected = true;
+ let client = data.client;
+ this.once("kill", function () client.close());
+ client.request({to: "root", type: "listTabs"}, (function (reply) {
@mykmelez

mykmelez Mar 18, 2013

Owner

Is it really necessary to go through this whole process again for this second connection? Since the geolocation client doesn't connect until the main simulator client is connected, and the main simulator client already determines that the server is ready by telling it to list tabs and waiting for a reply, it seems like the only thing this client should need to do is connect to the server.

(I'm still unsure why we need a separate client at all; but if we do need it, then we should still avoid making it do any unnecessary work.)

prosthesis/components/SimulatorScreen.js
+
+XPCOMUtils.defineLazyServiceGetter(this, "cpmm",
+ "@mozilla.org/childprocessmessagemanager;1",
+ "nsIMessageListenerManager");
@mykmelez

mykmelez Mar 18, 2013

Owner

This doesn't seem to be used anywhere. If it's no longer necessary, we should remove it.

prosthesis/components/SimulatorScreen.js
+ } else if (listener && listener.handleEvent &&
+ typeof listener.handleEvent == "function") {
+ listener.handleEvent(evt);
+ }
@mykmelez

mykmelez Mar 18, 2013

Owner

listener must be a function or an object with a handleEvent method, so this shouldn't check that handleEvent is callable before calling it and ignore it if isn't. It would be better for the code to simply throw, informing the developer about the problem, than to fail silently, i.e.:

      if (typeof listener == "function") {
        listener.call(this, evt);
      } else {
        listener.handleEvent(evt);
      }
prosthesis/components/SimulatorScreen.js
+ listener.handleEvent(evt);
+ }
+ } catch (e) {
+ debug(["EXCEPTION:", e, e.fileName, e.lineNumber].join(' '));
prosthesis/components/SimulatorScreen.js
+ dispatchEvent: function(evt) {
+ if (!this._eventListenersByType) {
+ return;
+ }
@mykmelez

mykmelez Mar 18, 2013

Owner

Instead of testing for this here and in addEventListener/removeEventListener, define it in the constructor, so it's always available.

prosthesis/components/SimulatorScreen.js
+ _updateVisibility: function(evt) {
+ try {
+ this._visibility = evt.target.visibilityState;
+ debug(["update visibility:", this.nodePrincipal.origin, this._visibility].join(' '));
@mykmelez

mykmelez Mar 18, 2013

Owner

Nit: this debug call, and others below, would be easier to read if it were either simple concatenation with the concatenation operator or a simple list of arguments that the debug function concatenates.

prosthesis/components/SimulatorScreen.js
+ this._visibility = evt.target.visibilityState;
+ debug(["update visibility:", this.nodePrincipal.origin, this._visibility].join(' '));
+ } catch(e) {
+ debug(["EXCEPTION:", e, e.fileName, e.lineNumber].join(' '));
@mykmelez

mykmelez Mar 18, 2013

Owner

Use Components.utils.reportError here too (and anywhere else there is exception reporting).

prosthesis/components/SimulatorScreen.js
+ // it's not in fullscreen mode
+ debug("deny mozLockOrientation: " + origin +
+ " is not installed or in fullscreen mode.");
+ return false;
@mykmelez

mykmelez Mar 18, 2013

Owner

This should throw or at least report an error to the console (ideally, whatever the real implementation does).

prosthesis/components/SimulatorScreen.js
+ let appOrigin = this._getOrigin(aWindow.location.href);
+ debug("init called from: " + aWindow.document.nodePrincipal.origin);
+
+ let chromeObject = this._initChild(aWindow);
@mykmelez

mykmelez Mar 18, 2013

Owner

It isn't clear why the content-facing implementation has to move to a separate object. Can you explain the reasoning?

/**
* Creates a SimulatorActor. SimulatorActor provides remote access to the
* FirefoxOS Simulator module.
*/
-function SimulatorActor(aConnection)
+let SimulatorActor = function(aConnection)
@mykmelez

mykmelez Mar 18, 2013

Owner

Even if a function is being assigned to a variable, it's still worth giving it a name so it's easier to debug stacks containing it. The name can be as simple as the name of the variable, i.e.:

let SimulatorActor = function SimulatorActor(aConnection) {
/**
* Creates a SimulatorActor. SimulatorActor provides remote access to the
* FirefoxOS Simulator module.
*/
-function SimulatorActor(aConnection)
+let SimulatorActor = function(aConnection)
{
@mykmelez

mykmelez Mar 18, 2013

Owner

Nit: put this on the same line as the function statement.

@@ -4,26 +4,26 @@
"use strict";
-Components.utils.import("resource://gre/modules/Services.jsm");
+{
@mykmelez

mykmelez Mar 18, 2013

Owner

Why enclose the whole script in a block? It should already be loaded in its own scope by the debugger server.

@@ -176,7 +180,7 @@ SimulatorActor.prototype = {
return true;
}
});
-
+
@mykmelez

mykmelez Mar 18, 2013

Owner

Nit: looks like some extraneous whitespace.

"geolocationResponse": SimulatorActor.prototype.onGeolocationResponse,
};
DebuggerServer.removeGlobalActor(SimulatorActor);
DebuggerServer.addGlobalActor(SimulatorActor,"simulatorActor");
+
+}
@mykmelez

mykmelez Mar 18, 2013

Owner

Nit: add a newline to the end of the file.

prosthesis/content/lockscreen.js
@@ -2,12 +2,21 @@
* 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/. */
+{
@mykmelez

mykmelez Mar 18, 2013

Owner

It looks like the purpose of this block is only to constrain the scope of the log function.

I would prefer to avoid creating such a large block for such a limited purpose, especially since it's so easy to overlook the block, given that its contents aren't indented. That makes it dangerous, because another programmer who modifies this later might declare a variable inside the block without noticing the block and then expect it to be available outside of it as well.

Also, the messages being logged already contain sufficient information to identify their source. The first one, "simulator - LOCKSCREEN ENABLED: "+value, will result in the message "prosthesis: lockscreen.js - simulator - LOCKSCREEN ENABLED: false", when it would be sufficient to log "prosthesis - lockscreen.enabled: false" or even more simply "lockscreen.enabled: false" to inform a reader about the status of the setting and enable them to find the origin of the log message by grepping the code.

So I would instead declare a debug function in the main shell overlay script that all the code loaded into the script can reference. See also #378 for hints on where we're going with logging in the simulator process (although those changes are out of scope for this pull request).

prosthesis/content/lockscreen.js
+});
+
+}
@mykmelez

mykmelez Mar 18, 2013

Owner

Nit: add a newline here too (and anywhere else needed).

prosthesis/content/shell.js
+}
+
+Components.classes["@mozilla.org/file/directory_service;1"]
+ .getService(Components.interfaces.nsIDirectoryService).registerProvider(provider)
@mykmelez

mykmelez Mar 18, 2013

Owner

This could use some explanation. Why do we create a custom provider for coreAppsDir?

Also, this should be in an XPCOM component registered via a component manifest. Otherwise, it might not get registered early enough in the startup process. See references to xpcom-directory-providers for examples.

prosthesis/content/window_manager.js
@@ -0,0 +1,152 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
@mykmelez

mykmelez Mar 18, 2013

Owner

mutation_summary.js may be an exception, since we got it from elsewhere, and it's third-party code, but let's use a hyphen instead of an underscore to separate words in the names of our own files, i.e. call this window-manager.js. (I'd prefer to do this even for mutation_summary.js, i.e. call it mutation-summary.js, especially since we're copying that file rather than using it from a submodule, so renaming the file doesn't fork it.)

prosthesis/content/window_manager.js
+ * 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/. */
+
+try {
@mykmelez

mykmelez Mar 18, 2013

Owner

It shouldn't be necessary to wrap an entire script in a try/catch block just to report an exception. Are there particular exceptions that you expect this code to throw?

prosthesis/content/window_manager.js
+ GlobalSimulatorScreen.height);
+ } catch(e) {
+ debug(["EXCEPTION:", e, e.fileName, e.lineNumber].join(' '));
+ }
@mykmelez

mykmelez Mar 18, 2013

Owner

try/catch blocks like this one, which just reports an exception without attempting to recover from it, should be unnecessary, because an exception in this code should get reported to the console, and the observer service should handle the exception gracefully. So I would very much like to avoid such complexity.

(The situation is different for dispatchEvent in SimulatorScreen.js; there, it makes sense for the implementation to catch and report an exception in an event handler so it doesn't prevent the event from being dispatched to other handlers.)

prosthesis/content/window_manager.js
+ GlobalSimulatorScreen.height);
+ };
+
+ Services.scriptloader.loadSubScript("chrome://prosthesis/content/mutation_summary.js");
@mykmelez

mykmelez Mar 18, 2013

Owner

Could we simply load this from shell.xul?

Owner

mykmelez commented Mar 18, 2013

@rpl Ok, I have finished the review. ;-)

There are some minor issues, but there are also some more significant ones, and there are also some changes for which I don't quite understand the motivation. Perhaps they are all necessary, and it's just a matter of explaining them! I just want to make sure we aren't doing any unnecessary work, since we will pay the cost of unnecessary code complexity every time we make changes.

Also, let's avoid adding any more changes to this pull request besides the ones required to address issues identified by the review. This request is already quite large, and making more changes will make it that much harder to review and merge!

I know sometimes it's easier to make a change on an existing branch than to start a new one, especially when the change depends on the existing branch. And sometimes we can't avoid making significant changes on a single branch, because the work can't easily be split up into multiple discrete steps. But when it's possible to do so, it's better to break up the work, because it's easier to review, which makes it faster to merge! :-)

Member

rpl commented Mar 21, 2013

@mykmelez just pushed another bunch of commits, to address the requested issues (and I cleaned up and simplified window-manager module),
I will proceed to the other tweaks on logging (e.g. unified logging config) in an other pull request.

This pull request now contains 24 commits but I can squash it in a single commit in a second if can be useful,
my apologies for its size and complexity :-(

Why enclose the whole script in a block? It should already be loaded in its own scope by the debugger server.

I believe that different actors are loaded into the same context and the last actor loaded will overwrite the
previous debug helper (e.g. currently dbg-webapps-actors.js define an empty debug helper function),
because DebugServer load actor js files using loadSubscript:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/debugger/server/dbg-server.js#181

Is it really necessary to go through this whole process again for this second connection?
I tried to keep the two lifecycle independent to reduce interaction and to doesn't assume that debugger server implementation will use the same global actor id over different connections,
but I can try to change it to get needed information from the other connection.

Member

rpl commented Mar 21, 2013

This could use some explanation. Why do we create a custom provider for coreAppsDir?

added into a commit by mistake, it was an experiment to fix an error reported into the javascript console,
I've removed it so we can discuss it in a separate pull request.

Member

rpl commented Mar 21, 2013

It looks like the purpose of this block is only to constrain the scope of the log function.

My intention was to enclose window-manager.js and lockscreen.js to prevents unintentional
overwrite of variables defined by scripts loaded in b2g by default (especially when we update b2g
before a release, because we load our scripts into the same context using loadSubScript),
probably I'm a bit too defensive (e.g. for lockscreen.js :-))

Owner

mykmelez commented Mar 25, 2013

On 2013/03/21 16:23, Luca Greco wrote:

This pull request now contains 24 commits but I can squash it in a
single commit in a second if can be useful,

my apologies for its size and complexity :-(

No worries, I actually prefer the separate commits, because they make it
easier to see the changes you make in response to review comments.

I believe that different actors are loaded into the same context and
the last actor loaded will overwrite the
previous debug helper (e.g. currently dbg-webapps-actors.js define an
empty debug helper function),
because DebugServer load actor js files using loadSubscript:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/debugger/server/dbg-server.js#181

Ah, indeed. In that case, we should instead reuse the logging facility
built into dbg-server.js, i.e. the dumpn function, which logs its
unary argument if the devtools.debugger.log preference is set to true.

I know that logging facility isn't ideal, but it's still better to
integrate with the existing facility than create our own and make our
code more complicated to work around the scope issues. In the long run,
the debug server will load each actor in its own context, according to a
DevTools engineer I talked to at the DevTools offsite two weeks ago, and
we won't have this problem.

-myk

Owner

mykmelez commented Mar 25, 2013

My intention was to enclose window-manager.js and lockscreen.js to prevents unintentional
overwrite of variables defined by scripts loaded in b2g by default (especially when we update b2g
before a release, because we load our scripts into the same context using loadSubScript),
probably I'm a bit too defensive (e.g. for lockscreen.js :-))

Note that loadSubScript is just a workaround. Ideally, we'd be loading scripts via <script> tags. Nevertheless, the consequence is the same: the scripts are loaded into the same context as the B2G scripts. But that's ok, provided we minimize the number of globals we create and name them appropriately.

In this case, B2G's shell.js provides a primitive debug function that it doesn't really use. Let's override it in prosthesis's shell.js with another primitive one that adds the ability to log multiple arguments, which is the key difference between the two:

let debug = function debugSimulator() {
  dump(Array.slice(arguments).join(" ") + "\n");
}

Then we can make this facility more sophisticated per issue #378 in a separate changeset.

I realize the function doesn't prepend a prefix to the messages. That isn't necessarily a bad thing, since those prefixes make the messages awfully long, and it's usually easy to figure out the origin of a message. Plus, it's easy to add it to the message itself:

debug("window-manager adjustWindowSize: " + width + "x" + height);

However, I suggest avoiding it unless it seems absolutely necessary. In this example, it'll usually be obvious to developers that adjustWindowSize is in window-manager.js. And when it isn't, f.e. for a new developer, then it's easy to figure out where it is: just grep the code for that string.

An alternative, if you really want the prefixes, is to define a function that is a method of the object doing the logging, and add the prefix there, i.e.:

let WindowManager = {
  debug: function debug() {
    debug.apply(Array.slice(arguments).unshift("WindowManager: "));
  },

  adjustWindowSize: function() {
    this.debug("adjustWindowSize: " + width + "x" + height);
  }
};

Which raises another point: the conventional way for chrome code, including addon code, to encapsulate functionality in a chrome document is to create singleton globals like the WindowManager object in the code sample above. Like an anonymous block, this creates a scope. But it also allows code outside the scope to interact with the code inside it, when appropriate. We should employ this pattern in window-manager.js.

addon/lib/remote-simulator-client.js
@@ -219,14 +227,16 @@ const RemoteSimulatorClient = Class({
},
type: "geolocationResponse"
});
- }).bind(this), function error() {
+ }, function error() {
@mykmelez

mykmelez Mar 28, 2013

Owner

Nit: this is ok, but I actually prefer using Function.bind instead of temporary variables like remote, since it makes the meaning of this consistent within a function that has embedded callbacks, which makes the function easier to read. I.e. something like this:

_registerGeolocationRequest: function(client) {
  client.addListener("geolocationRequest", (function() {
    console.debug("GEOLOCATION REQUEST");
    let geolocation = Cc["@mozilla.org/geolocation;1"].
                      getService(Ci.nsIDOMGeoGeolocation);
    geolocation.getCurrentPosition(
      (function success(position) {
        console.debug("GEOLOCATION RESPONSE", this._remote.simulator);
        this._remote.client.request({
          to: this._remote.simulator,
          message: {
            lat: position.coords.latitude,
            lon: position.coords.longitude,
          },
          type: "geolocationResponse"
        });
      }).bind(this),
      function error() {
        console.error("error getting current position");
      }
    );
  }).bind(this));
},
prosthesis/components/SimulatorScreen.js
+ let origin = nodePrincipal.origin;
+
+ let els = Cc["@mozilla.org/eventlistenerservice;1"]
+ .getService(Ci.nsIEventListenerService);
@mykmelez

mykmelez Mar 28, 2013

Owner

Nit: use this indentation style:

let els = Cc["@mozilla.org/eventlistenerservice;1"].
          getService(Ci.nsIEventListenerService);
+ let tag = Components.stack.caller ?
+ (Components.stack.caller.filename + " L" + Components.stack.caller.lineNumber) :
+ "";
+ dump(" -*- " + tag + ": " + Array.slice(arguments).join(" ") + "\n");
@mykmelez

mykmelez Mar 28, 2013

Owner

Hrm, perhaps we use console messages in different ways. For me, when I read console output, I almost always either 1. already know where a message is located, because I looked it up the first time, or 2. don't need to know, because it only matters whether or not the message appears and what it says.

And when I use the messages like that, then all this additional information is overkill that just makes it harder to read the output. For me, ideally messages wouldn't have any prefixes at all: no file names, no line numbers, no function names, and especially not a meaningless prefix like "-*-", whose only function is to make some messages more prominent (at the expense of others), but only if we restrict their use to a subset of messages.

Note that there's a difference between this console and a console like Firefox's Web Console or Error Console. Those consoles do expose additional information like a timestamp, the log level, and/or a file/line reference. But they're graphical consoles that use visual design to emphasize the message while deemphasizing the other information (via color, placement, abbreviation, etc.). We don't have that option for this console.

So I would really rather not include this information in these console messages. But perhaps you use the console in a different way, and this information is particularly useful to you? If so, please describe how you use it! I'm open to including it if it really makes a difference; I just want to make sure we're doing so for good reasons, not just because we can or because it seems like it might be useful. Since that's a recipe for an overwhelming and underusable interface.

@rpl

rpl Mar 29, 2013

Member

I tried to "auto prefix" the log message because, redefining a debugging helper used by b2g, I feared it might be confusing if a b2g dev try to search the logged message on mxr.mozilla.org
and doesn't find it because its one of our logs.

For almost the same reason I used the same prefix ( "-*-" ) used in the original helper.

I'm not sure how important this could be (I find useful that additional information during
debugging/prototyping but probably it's too much info to leave there normally)

@mykmelez let me know if we prefer to remove (or even comment only the "auto prefix" part)

+ try {
+ Services.obs.notifyObservers(null, "simulator-orientation-lock-change", null);
+ } catch(e) {
+ debug(["EXCEPTION:", e, e.fileName, e.lineNumber].join(' '));
@mykmelez

mykmelez Mar 28, 2013

Owner

I don't know why notifyObservers would ever throw; but if it does, the default exception handler will catch and report it, so there's no reason to do so ourselves. The same is true for the other try/catch blocks.

- _fixSizeInStyle: function(style, width, height) {
- style["width"] = style["min-width"] = style["max-width"] = width;
- style["height"] = style["min-height"] = style["max-height"] = height;
+ debug("notify 'simulator-adjust-window-size'.");
@mykmelez

mykmelez Mar 28, 2013

Owner

Nit: the significance here is not that a notification is about to be sent but rather that the window's size was set (for which the notification, like the console message, is just a symptom), and a message like this would be more informative if it cited the new size of the window, i.e.:

debug("set window size to: " + width + "x" + height);

rpl added some commits Mar 8, 2013

big refactoring and code cleanup:
logging:
- unified logging enabling/disabling and log prefix patterns
- better exception logging

cleanup:
- remove from GlobalSimulatorScreen a lot of DOM manipulation code
  (moved into window_manager.js and notified using Services.obs)
- window_manager.js now manage tweaks on gaia window management,
  shell window resizing and rotate button enabling/disabling
remove unused windowManagerEvents, move remote geolocation client in …
…its own module

NOTE:
geolocation message exchange on the remote debugger protocol doesn't interact correctly
with the other simulator commands over the same connection, "remote-geolocation-client"
module create a new connection to prevents this intection.
FIX: fire geolocationRequest unsolicited geolocationRequest into a ti…
…mer callback

fire geolocationRequest unsolicited geolocationRequest into a timer callback
to prevents debug server bug (as suggested by @myk)
FIX: define a connection.enqueue function to workaround debugger serv…
…er bug

define a connection.enqueue function to workaround debugger server bug
on unsolicited messages sent by an actor handler before return,
because even put it in a timer is not always enough.
Member

rpl commented Mar 29, 2013

@mykmelez I've just pushed:

  • full rebased pull request to resolve merging conflicts in advance
  • detailed log on adjust window size notification
  • fix indentation
  • bind onsuccess callback on geolocation.getCurrentPosition
  • disabled prosthesis chrome logging by default

I only leave open the "remove/disable 'auto prefix' logging" issue,
I'm ready to remove/disable it depending on the decision we take on this.

Owner

mykmelez commented Mar 29, 2013

I only leave open the "remove/disable 'auto prefix' logging" issue,
I'm ready to remove/disable it depending on the decision we take on this.

I'd still like to remove these prefixes when we log to the terminal console, but since this logging is now disabled by default, I'm ok with leaving them in for now and then removing them in a future enhancement to our logging code that uses the SDK's console to log messages that exceed the log level threshold automatically.

And at that point we can also enhance the code to send a structured version of these messages to the remote web console, which can display this additional information in a way that enhances the usability of the message.

mykmelez added a commit that referenced this pull request Mar 29, 2013

@mykmelez mykmelez merged commit 1342ac6 into mozilla:master Mar 29, 2013

Owner

mykmelez commented Mar 29, 2013

Ok, finally merged! I know this changeset turned out to be a long, hard road with many twists and turns. Thanks for the awesome work on this, @rpl!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.