Bug 757612 - page-worker postMessage has a timing issue #852

Merged
merged 1 commit into from Mar 26, 2013

Conversation

Projects
None yet
3 participants
Member

jsantell commented Mar 13, 2013

No description provided.

lib/sdk/content/worker.js
@@ -591,6 +584,7 @@ const Worker = EventEmitter.compose({
this._windowID = null;
observers.remove("inner-window-destroyed", this._documentUnload);
this._earlyEvents.slice(0, this._earlyEvents.length);
+ this._earlyMessages.slice(0, this._earlyMessages.length);
@ZER0

ZER0 Mar 22, 2013

Contributor

You can use this._earlyMessages.length = 0 to clear an array.

lib/sdk/content/worker.js
+ this._earlyMessages = [];
+ return this._earlyMessages;
+ },
+
@ZER0

ZER0 Mar 22, 2013

Contributor

As said before, we should try to uniform events and messages to have one array, and therefore simplify the code above about queue, removing the curry.

@ZER0

ZER0 Mar 25, 2013

Contributor

You removed _earlyEvents in favour of _earlyMessages, I wonder if is there any specific reason? In my mind "messages" are "events" with type "message", so sounds more clear to me have _earlyEvents. However I know that internally we do some mess, that's why I'm asking what makes you decided to use _earlyMessages instead.

Notice also that _earlyEvents was already used, and we should avoid to replace code if it's not really needed. Of course if it's needed – e.g. it's more self-explained – it's welcome :)

@jsantell

jsantell Mar 25, 2013

Member

Agreed -- 'message' is considered a type of 'event' -- changing back to only _earlyEvents

lib/sdk/content/worker.js
-
- this._contentWorker.emit("message", data);
+ postMessage: function (data) {
+ // Overwrite `postMessage` because of Traits
@ZER0

ZER0 Mar 22, 2013

Contributor

The comment should be a bit more explicative.

test/test-page-worker.js
+ "});",
+ contentURL: 'data:text/html;charset=utf-8,',
+ });
+ page.postMessage('ping');
@ZER0

ZER0 Mar 22, 2013

Contributor

Don't mix single quotes with double quotes: we should be consistent in the file, so use what the file use (most).

Contributor

ZER0 commented Mar 22, 2013

I think it's fine, I reject the review mainly because as you said we can probably make the code easier to read, removing some complexity. But the approach is good.

lib/sdk/content/worker.js
this._port = EventEmitterTrait.create({
- emit: function () self._emitEventToContent(Array.slice(arguments))
+ emit: (this._emitEventToContent).bind(this)
@ZER0

ZER0 Mar 25, 2013

Contributor

Why the parenthesis?

@jsantell

jsantell Mar 25, 2013

Member

Good call, this was from a previous change

Contributor

ZER0 commented Mar 25, 2013

r+ if all test passes – please, have a check on fennec too.
A couple of comments, but nothing special, more curiosity than other, it's up to you decide to use _earlyMessages rather than _earlyEvents if you think there is a good reason to change the name of the previous array.

jsantell added a commit that referenced this pull request Mar 26, 2013

Merge pull request #852 from jsantell/page-worker-postMessage-queue-7…
…57612

Fix Bug 757612 - page-worker postMessage has a timing issue, a=@ZER0

@jsantell jsantell merged commit 4ca340c into mozilla:master Mar 26, 2013

@jsantell jsantell deleted the jsantell:page-worker-postMessage-queue-757612 branch Apr 5, 2013

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