653187 - Deprecate use of `on` and `postMessage`from content script globals

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Alexandre Poirot [:ochameau]]

Attachment: Deprecate them by calling console.warn, but still works.

In order to avoid global scope pollution, we are starting to send deprecate message when using `on` and `postMessage` from content scripts globals.

We are suggesting to use their equivalent on `self` object and new EventEmitter interface `self.port`.

We have two migration cases:
1/ events like 'context' in context-menu, where you still need to call self.on
2/ all custom events, where we should highlight events and self.port.{on,emit} instead of postMessage pattern.


Here is a first patch that print a warning message in the console, but functions still work. We still need to remove postMessage call over all our codebase as warning messages make tests fail.
I'd prefere to keep these functions working for at least one release, but we can easily disable them by removing two lines.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Same patch but convert all call to postMessage in our codebase

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 528653 [details] [diff] [review]
Same patch but convert all call to postMessage in our codebase

Review of attachment 528653 [details] [diff] [review]:

Looks good, r=myk with the changes to the statements of deprecation described below.

::: packages/api-utils/lib/content/worker.js
@@ +225,5 @@
+      on: {
+        value: function () {
+          console.warn("Global `on` function is deprecated in content " +
+                       "scripts, instead you have to use events with " +
+                       "`self.port.on`. (or messages with `self.on`)");

Since we aren't deprecating self.on("message"), we can simply suggest developers switch from `on` to `self.on`, which doesn't require as much explanation, since it doesn't require us to present the Port API and explain how to convert message-passing code to event-emission code.

So this should read:

The global `on()` function in content scripts is deprecated in favor of the `self.on()` function, which works the same.  Replace calls to `on()` with calls to `self.on()`.

@@ +235,5 @@
+        value: function () {
+          console.warn("Global `postMessage` function is deprecated in " +
+                       "content scripts, instead you have to use events with " +
+                       "`self.port.emit`. (or messages with " +
+                       "`self.postMessage`)");

Same thing here, this should read:

The global `postMessage()` function in content scripts is deprecated in favor of the `self.postMessage()` function, which works the same.  Replace calls to `postMessage()` with calls to `self.postMessage()`.

::: packages/api-utils/tests/test-content-symbiont.js
@@ +97,1 @@
         onMessage = function onMessage(message) {

Hmm, this reminds me that we should deprecate global onMessage as well in favor of self.on("message").

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Patch on top of previous one, that deprecates onMessage

As previous patch, onMessage is still going to work, but with warning messages displayed in console.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 528653 [details] [diff] [review]
Same patch but convert all call to postMessage in our codebase

a=myk to land this during the freeze.

Comment 5

[Alexandre Poirot [:ochameau]]

(In reply to comment #2)
> Comment on attachment 528653 [details] [diff] [review]
> Same patch but convert all call to postMessage in our codebase
> 
> Review of attachment 528653 [details] [diff] [review]:
> 
> Looks good, r=myk with the changes to the statements of deprecation described
> below.

This patch landed:
https://github.com/mozilla/addon-sdk/commit/2cd756e5b880de4610f8600ba3cdbbec4b687373

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 528842 [details] [diff] [review]
Patch on top of previous one, that deprecates onMessage

Review of attachment 528842 [details] [diff] [review]:

Looks great, modulo a couple minor nits, r=myk, and a=myk for commission during 1.0b5 freeze.

::: packages/api-utils/lib/content/worker.js
@@ +220,5 @@
+        set: function(value) {
+          console.warn("The global `onMessage` function in content scripts " +
+                       "is deprecated in favor of the `self.on()` function. " +
+                       "Replace `onMessage = function (data){}` definitions " +
+                       "with calls to `self.on('message', function (data){})`");

Nit: append a period to the second sentence, i.e.:

    "with calls to `self.on('message', function (data){})`.");

@@ +221,5 @@
+          console.warn("The global `onMessage` function in content scripts " +
+                       "is deprecated in favor of the `self.on()` function. " +
+                       "Replace `onMessage = function (data){}` definitions " +
+                       "with calls to `self.on('message', function (data){})`");
+          self._onMessage = value

Nit: append a semi-colon to the end of this line.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

P1 for 1.0, although we'll take for 1.0b5, it just needs to land!

Comment 8

[Drew Willcoxon :adw]

Hey guys, sorry if this has been covered (patches coming fast and furious recently :), but like the self.on() change (bug 653148), the docs need to use self.postMessage() so people following them don't get deprecation warnings.  Is that work being done too?

Comment 9

[Alexandre Poirot [:ochameau]]

Landed:
https://github.com/mozilla/addon-sdk/commit/dd446fb9011c1495d1b97f545272fbbd97aeb4cc

Comment 10

[Alexandre Poirot [:ochameau]]

(In reply to comment #8)
> Hey guys, sorry if this has been covered (patches coming fast and furious
> recently :), but like the self.on() change (bug 653148), the docs need to use
> self.postMessage() so people following them don't get deprecation warnings.  Is
> that work being done too?

Yes, this work is being done by Will in bug 653109.

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Fix merge with bug 630962, replace on/postMessage

Current test are failing as I didn't replace these calls to on/postMessage in my branch.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 529004 [details] [diff] [review]
Fix merge with bug 630962, replace on/postMessage

r+a=myk, but note: bustage fixes don't need review/approval!

Comment 13

[Alexandre Poirot [:ochameau]]

Landed:
https://github.com/mozilla/addon-sdk/commit/b243c4bc5697540278524cae9d3baa3fd697604d