Bug 685564 - Make page-mod work on Firefox mobile #310

Merged
merged 13 commits into from Dec 30, 2011

Conversation

Projects
None yet
3 participants
@ZER0
Contributor

ZER0 commented Dec 22, 2011

+const {Namespace: NS} = require("./namespace");
+
+const scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
+ getService(Ci.mozIJSSubScriptLoader);

This comment has been minimized.

@Gozala

Gozala Dec 22, 2011

Member

@ZER0 sandbox manipulation low level API has landed to master recently, could you please use that instead of direct use of mozIJSSubScriptLoader and Cu.Sandbox ?

@Gozala

Gozala Dec 22, 2011

Member

@ZER0 sandbox manipulation low level API has landed to master recently, could you please use that instead of direct use of mozIJSSubScriptLoader and Cu.Sandbox ?

+ console.exception(e);
+ }
+ } else {
+ scriptLoader.loadSubScript(uri, sandbox);

This comment has been minimized.

@Gozala

Gozala Dec 22, 2011

Member

Just wanted to mention that sandbox API uses loadSubScript with third argument 'UTF-8', cause otherwise complex unicode chars are not handled properly.

@Gozala

Gozala Dec 22, 2011

Member

Just wanted to mention that sandbox API uses loadSubScript with third argument 'UTF-8', cause otherwise complex unicode chars are not handled properly.

This comment has been minimized.

@ZER0

ZER0 Dec 28, 2011

Contributor

As we discussed in IRC, I merged this code with load function of sandbox module.

@ZER0

ZER0 Dec 28, 2011

Contributor

As we discussed in IRC, I merged this code with load function of sandbox module.

+ return function() {
+ return fn.apply(this, args.concat(Array.slice(arguments)));
+ }
+}

This comment has been minimized.

@Gozala

Gozala Dec 22, 2011

Member

Could you please factor this function out to the utils/function module ? In fact I have a pending pull request which defines this function there as I needed in more then one place, so with this it's one more use case.

@Gozala

Gozala Dec 22, 2011

Member

Could you please factor this function out to the utils/function module ? In fact I have a pending pull request which defines this function there as I needed in more then one place, so with this it's one more use case.

+ */
+const curry = function curry(fn) {
+ if (arguments.length < 2)
+ return fn;

This comment has been minimized.

@Gozala

Gozala Dec 22, 2011

Member

Unless, you have strong feeling about this, I'd remove this check. I had a case once or twice where I used curry just to wrap original function to hide it's properties.

@Gozala

Gozala Dec 22, 2011

Member

Unless, you have strong feeling about this, I'd remove this check. I had a case once or twice where I used curry just to wrap original function to hide it's properties.

+}
+
+Object.freeze(MessageManager);
+Object.freeze(MessageManager.prototype);

This comment has been minimized.

@Gozala

Gozala Dec 22, 2011

Member

I'd encourage you to use Base.extend for defining a MessageManager as it will reduce noise and will make sure that everything that requires freezing etc will be frozen without manually steps.

@Gozala

Gozala Dec 22, 2011

Member

I'd encourage you to use Base.extend for defining a MessageManager as it will reduce noise and will make sure that everything that requires freezing etc will be frozen without manually steps.

This comment has been minimized.

@ZER0

ZER0 Dec 28, 2011

Contributor

I'm not comfortable yet with Base.extend approach, I personally think that in case of regular constructors it adds more additional steps (even if they are hidden) and became a sort of overkill. If it's okay to you, as you suggested also in one of your comment, I will leave as is this part until we are able to have have a talk about it, and I will modify later in case of needed.

@ZER0

ZER0 Dec 28, 2011

Contributor

I'm not comfortable yet with Base.extend approach, I personally think that in case of regular constructors it adds more additional steps (even if they are hidden) and became a sort of overkill. If it's okay to you, as you suggested also in one of your comment, I will leave as is this part until we are able to have have a talk about it, and I will modify later in case of needed.

@Gozala

This comment has been minimized.

Show comment
Hide comment
@Gozala

Gozala Dec 22, 2011

Member

It feels to me that MessageManager has a big overlap with EventEmitter in terms of listeners registration, dispatching events etc... It would be nice to reuse the same code. As a matter of fact I have a branch that implements event emitters using namespaces and Base that eventually should replace an old API. I think this change would have benefit from it.

Member

Gozala commented Dec 22, 2011

It feels to me that MessageManager has a big overlap with EventEmitter in terms of listeners registration, dispatching events etc... It would be nice to reuse the same code. As a matter of fact I have a branch that implements event emitters using namespaces and Base that eventually should replace an old API. I think this change would have benefit from it.

@Gozala

This comment has been minimized.

Show comment
Hide comment
@Gozala

Gozala Dec 22, 2011

Member

I don't consider any of my comments to be critical or blocking this from landing, as they can be addressed in a follow up patch. In such case could you please fill a bug report for that and CC me there.

Member

Gozala commented Dec 22, 2011

I don't consider any of my comments to be critical or blocking this from landing, as they can be addressed in a follow up patch. In such case could you please fill a bug report for that and CC me there.

+// `Namespace` is a e4x function in the scope, so we import the function as `NS`
+// to avoid clashing, until we don't have a better candidate to replace
+// `Namespace` in namespace module.
+const {Namespace: NS} = require("./namespace");

This comment has been minimized.

@Gozala

Gozala Dec 22, 2011

Member

Nit: according to the convention it shoud be (spaces between curlyies) :

const { Namespace: NS } = require("./namespace");

@Gozala

Gozala Dec 22, 2011

Member

Nit: according to the convention it shoud be (spaces between curlyies) :

const { Namespace: NS } = require("./namespace");

This comment has been minimized.

@Gozala

Gozala Dec 22, 2011

Member

I have used ns as an alias instead, and I think I saw Alex used something else. I'd suggest export ns from namespace module as an alias to Namespace and put the comment regarding e4x conflict there.

@Gozala

Gozala Dec 22, 2011

Member

I have used ns as an alias instead, and I think I saw Alex used something else. I'd suggest export ns from namespace module as an alias to Namespace and put the comment regarding e4x conflict there.

+ if (typeof response === "undefined")
+ responses.push(response);
+ else
+ responses.push(JSON.parse(JSON.stringify(response, jsonFixer)));

This comment has been minimized.

@Gozala

Gozala Dec 22, 2011

Member

Alternatively you could just JSON.parse(JSON.stringify(responses, jsonFixer)) all the responses.

@Gozala

Gozala Dec 22, 2011

Member

Alternatively you could just JSON.parse(JSON.stringify(responses, jsonFixer)) all the responses.

This comment has been minimized.

@ZER0

ZER0 Dec 29, 2011

Contributor

In that case JSON.stringify will censored the undefined values to null, and that is not the expected behavior.

@ZER0

ZER0 Dec 29, 2011

Contributor

In that case JSON.stringify will censored the undefined values to null, and that is not the expected behavior.

+ let sandbox = frame(this).receiver;
+
+ if (uri.indexOf("data:") === 0) {
+ let source = uri.replace(/^data:[^,]*,/, "");

This comment has been minimized.

@Gozala

Gozala Dec 22, 2011

Member

Nit: I think let source = uri.substr(uri.indexOf(',') + 1) is faster and easier to read.

@Gozala

Gozala Dec 22, 2011

Member

Nit: I think let source = uri.substr(uri.indexOf(',') + 1) is faster and easier to read.

This comment has been minimized.

@Gozala

Gozala Dec 23, 2011

Member

I think you should decodeURIComponent otherwise it may misbehave when data: uri's are encoded as they should.

@Gozala

Gozala Dec 23, 2011

Member

I think you should decodeURIComponent otherwise it may misbehave when data: uri's are encoded as they should.

+
+ mm.removeMessageListener(topic, listenerB);
+
+ assert.deepEqual(listeners[topic], [listenerA, listenerC],

This comment has been minimized.

@mykmelez

mykmelez Dec 23, 2011

Member

Are listeners supposed to be called in the order they were registered, or is the order in which they are called undetermined? If the latter, it might be worth sorting these arrays, to guard against failure if the underlying implementation fails. Otherwise, we shouldn't sort them, since then the test should be checking that the listeners are in the expected order.

@mykmelez

mykmelez Dec 23, 2011

Member

Are listeners supposed to be called in the order they were registered, or is the order in which they are called undetermined? If the latter, it might be worth sorting these arrays, to guard against failure if the underlying implementation fails. Otherwise, we shouldn't sort them, since then the test should be checking that the listeners are in the expected order.

This comment has been minimized.

@ZER0

ZER0 Dec 29, 2011

Contributor

I didn't find any explicit information about the order in which the listeners are called in the Message Manager APIs, so for this implementation I just followed the easy way and they are called in the order they were registered.

@ZER0

ZER0 Dec 29, 2011

Contributor

I didn't find any explicit information about the order in which the listeners are called in the Message Manager APIs, so for this implementation I just followed the easy way and they are called in the order they were registered.

@mykmelez

This comment has been minimized.

Show comment
Hide comment
@mykmelez

mykmelez Dec 23, 2011

Member

@ZER0 asked me to look at the way tests are written, so I read through test-message-manager.js, and it looks great; I have no problem at all with the way the tests are written. r=@mykmelez on it!

Member

mykmelez commented Dec 23, 2011

@ZER0 asked me to look at the way tests are written, so I read through test-message-manager.js, and it looks great; I have no problem at all with the way the tests are written. r=@mykmelez on it!

+
+ let sandbox = Cu.Sandbox(systemPrincipal, { wantXrays : false });
+
+ Object.defineProperties(sandbox, {

This comment has been minimized.

@Gozala

Gozala Dec 23, 2011

Member

You should probably use merge:

merge(sandbox, {
  addMessageListener: addMessageListener.bind(sandbox),
  ....
})

It will methods configurable & writable, but they are in normal massegaManager anyway.

@Gozala

Gozala Dec 23, 2011

Member

You should probably use merge:

merge(sandbox, {
  addMessageListener: addMessageListener.bind(sandbox),
  ....
})

It will methods configurable & writable, but they are in normal massegaManager anyway.

This comment has been minimized.

@ZER0

ZER0 Dec 30, 2011

Contributor

If you don't have hard feeling about it, I'd like to keep the descriptors.

@ZER0

ZER0 Dec 30, 2011

Contributor

If you don't have hard feeling about it, I'd like to keep the descriptors.

+ * An array with the return values of the listeners if `sync` is `true`,
+ * otherwise `undefined`.
+ */
+function sendMessage(sync, name, data, sender) {

This comment has been minimized.

@Gozala

Gozala Dec 23, 2011

Member

After discussing it on IRC I understood that this is a magic ingredient of this function :) More seriously, I think it's hard to follow and would be much cleaner if this sendMessage was always synchronous. Then you could wrap it into Enqueued for sendMessageAsync.

PS: I was also thinking about renaming Enqueued to delay to match _.delay

@Gozala

Gozala Dec 23, 2011

Member

After discussing it on IRC I understood that this is a magic ingredient of this function :) More seriously, I think it's hard to follow and would be much cleaner if this sendMessage was always synchronous. Then you could wrap it into Enqueued for sendMessageAsync.

PS: I was also thinking about renaming Enqueued to delay to match _.delay

This comment has been minimized.

@ZER0

ZER0 Dec 29, 2011

Contributor

To me, Enqueued is more similar to _.defer than _.delay

@ZER0

ZER0 Dec 29, 2011

Contributor

To me, Enqueued is more similar to _.defer than _.delay

This comment has been minimized.

@ZER0

ZER0 Dec 30, 2011

Contributor

I used invoke to create my own defer function in message-manager. Unfortunately, the Enqueued function returns a value, the timeout's id, and in message-manager I need undefined. I didn't modify Enqueued directly because I don't know if this value is returned on purpose.

@ZER0

ZER0 Dec 30, 2011

Contributor

I used invoke to create my own defer function in message-manager. Unfortunately, the Enqueued function returns a value, the timeout's id, and in message-manager I need undefined. I didn't modify Enqueued directly because I don't know if this value is returned on purpose.

+
+ removeMessageListener : removeMessageListener,
+
+ sendAsyncMessage : curry(sendMessage, false),

This comment has been minimized.

@Gozala

Gozala Dec 23, 2011

Member

with a proposal above it will become:

sendAsyncMessage : curry(delay(sendMessage), false),
@Gozala

Gozala Dec 23, 2011

Member

with a proposal above it will become:

sendAsyncMessage : curry(delay(sendMessage), false),
+
+
+exports["test MessageManager addMessageListener"] =
+ function(assert) {

This comment has been minimized.

@Gozala

Gozala Dec 23, 2011

Member

I would suggest to create message manager and load data URI into it, which registers listener. Dispatch to the listener and see if it was registered by sending message back.

@Gozala

Gozala Dec 23, 2011

Member

I would suggest to create message manager and load data URI into it, which registers listener. Dispatch to the listener and see if it was registered by sending message back.

This comment has been minimized.

@ZER0

ZER0 Dec 29, 2011

Contributor

I did it later in the test. This is one of the unit testing possible approaches, where we test every single method before use it, and at the end we test the flows using the methods already tested. Never use in a method's test or flow's test a method is not already tested, basically that is the principle.
As we discussed in the IRC, we will have a meeting with @mykmelez in order to figure out the pros and the cons about different unit testing approaches. I have no particular hard feelings about it, I'd like have a good an consistent way to test our code that we can use for all tests in jetpack as guidelines.

@ZER0

ZER0 Dec 29, 2011

Contributor

I did it later in the test. This is one of the unit testing possible approaches, where we test every single method before use it, and at the end we test the flows using the methods already tested. Never use in a method's test or flow's test a method is not already tested, basically that is the principle.
As we discussed in the IRC, we will have a meeting with @mykmelez in order to figure out the pros and the cons about different unit testing approaches. I have no particular hard feelings about it, I'd like have a good an consistent way to test our code that we can use for all tests in jetpack as guidelines.

+
+
+exports["test MessageManager addMessageListener with duplicates"] =
+ function(assert) {

This comment has been minimized.

@Gozala

Gozala Dec 23, 2011

Member

same here, just see how many times listeners were called.

@Gozala

Gozala Dec 23, 2011

Member

same here, just see how many times listeners were called.

This comment has been minimized.

@ZER0

ZER0 Dec 30, 2011

Contributor

see my previous reply.

@ZER0

ZER0 Dec 30, 2011

Contributor

see my previous reply.

@ZER0

This comment has been minimized.

Show comment
Hide comment
@ZER0

ZER0 Dec 30, 2011

Contributor

@Gozala about the overlap with EventEmitter, as we discussed on IRC there are some slightly difference that makes not possible reuse the same code (e.g. I need to collect the responses for sync calls).

Contributor

ZER0 commented Dec 30, 2011

@Gozala about the overlap with EventEmitter, as we discussed on IRC there are some slightly difference that makes not possible reuse the same code (e.g. I need to collect the responses for sync calls).

@mykmelez

This comment has been minimized.

Show comment
Hide comment
@mykmelez

mykmelez Dec 30, 2011

Note that you could also have consumers work around the namespace collision at require time via:

let { Namespace: anything } = require("api-utils/namespace");

(where anything can be any non-conflicting name)

Nevertheless, exporting ns directly gives consumers the simplest possible workaround, so it seems worthwhile.

Note that you could also have consumers work around the namespace collision at require time via:

let { Namespace: anything } = require("api-utils/namespace");

(where anything can be any non-conflicting name)

Nevertheless, exporting ns directly gives consumers the simplest possible workaround, so it seems worthwhile.

@mykmelez

This comment has been minimized.

Show comment
Hide comment
@mykmelez

mykmelez Dec 30, 2011

გამარჯობა, welcome, nice!

I wonder if we should factor out this string at some point, now that it's being used in three places. Hmm... it probably isn't useful enough to do so yet.

გამარჯობა, welcome, nice!

I wonder if we should factor out this string at some point, now that it's being used in three places. Hmm... it probably isn't useful enough to do so yet.

@mykmelez

This comment has been minimized.

Show comment
Hide comment
@mykmelez

mykmelez Dec 30, 2011

Member

@Gozala reviewed this pull request and gave it an r+, but @ZER0 made substantive enough changes that he wanted a followup review, so I did that today, as @Gozala is away; and from what I can tell, this patch is ready to land, so r=@mykmelez.

Member

mykmelez commented Dec 30, 2011

@Gozala reviewed this pull request and gave it an r+, but @ZER0 made substantive enough changes that he wanted a followup review, so I did that today, as @Gozala is away; and from what I can tell, this patch is ready to land, so r=@mykmelez.

mykmelez added a commit that referenced this pull request Dec 30, 2011

Merge pull request #310 from ZER0/fennec-birch
fix Bug 685564 - Make page-mod work on Firefox mobile; r=@Gozala, @mykmelez

@mykmelez mykmelez merged commit 6eccac8 into mozilla:master Dec 30, 2011

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