657037 - Update all module documentation to use @event

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: Documentation, defect, P2)

Comment 1

[Will Bamberg [:wbamberg]]

Attachment: Make module docs use @event

Changes in tabs are a bit more extensive, as the 'overview' looked a bit sad after I moved the events doc into the API reference.

Comment 2

[Drew Willcoxon :adw]

Comment on attachment 532371 [details] [diff] [review]
Make module docs use @event

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

Thanks Will.  r+, but please address my comments in the code before landing.

But before that: This bug is titled "Update all module documentation to use @event", but there are other high-level modules that use events that aren't covered by this patch.  (It looks like most if not all of them have never called out their events in separate sections.)  I think that's OK, as long as it's understood that that's the case, and that there are follow-up bugs.

Also, FWIW, an unrelated comment on information architecture: Functions and methods in the documentation list their parameters alongside their names, e.g., foo(bar, baz), so when bar and baz are subsequently described it's clear what they're referring to.  But events don't do that, so event descriptions and the event listener parameter descriptions that follow them appear kind of disconnected.  Moreover, event listener parameters aren't described like method parameters: only a type is listed, no name, and it took me a moment to parse that.  I don't have a good solution for the first point: maybe some small "Event Listener Parameters" text above the parameter descriptions?  For the second point, I think it might be worth naming the parameters.  In addition to being consistent with method parameter descriptions, it would help to succinctly describe parameters' purposes.

::: packages/addon-kit/docs/panel.md
@@ +241,5 @@
> +
> +
> +<api name="message">
> +@event
> +Subscribe to this event to receive message events from content scripts

The docs generally say events are "listened to" or "listened for", don't they?  As in, "Listen for this event..." or "Register a listener for this event..."

@@ +246,5 @@
> +associated with this panel. When a content script posts a message using
> +`self.postMessage()`, the message is delivered to the add-on code in the
> +panel's `message` event.
> +
> +@argument {JSON}

I may be wrong, but I'm pretty sure the argument is automatically serialized when it's sent and deserialized when it's received.  So it doesn't need to be JSON, only a JSONable value like a string, number, or object.  In documentation that I've written, I've used the word "value" for a generic argument type.

@@ +248,5 @@
> +panel's `message` event.
> +
> +@argument {JSON}
> +The message sent from the content script is passed as an argument to event
> +listeners. It must be serializable to JSON.

If I'm indeed right, it would be helpful to say something like, "It must be serializable to JSON, but because serialization and deserialization is automatically handled for you, it doesn't actually have to be JSON."  (Or probably this would be best explained in one place, like the Working with Content Scripts guide...)

::: packages/addon-kit/docs/simple-storage.md
@@ +120,5 @@
>  </api>
>  
> +<api name="OverQuota">
> +@event
> +The module emits this event when your add-on's storage is over its quota.

"is over" implies a state, but it would be more accurate to say "goes over", which implies a transition.

::: packages/addon-kit/docs/tabs.md
@@ +40,4 @@
>  
> +Although you can't access the tab's content directly using this module, you can
> +attach a [page-mod](packages/addon-kit/docs/page-mod.html) to a tab, and use
> +that to access and manipulate the content.

I guess it depends on your definition of "directly," but this isn't correct, is it?  tab.attach() is part of this module.  Also, I'm not sure it's right to refer to the tab.attach() mechanism as a page-mod.  I'd prefer not to dilute the importance of the high-level API names.  A page-mod is only the thing you get when you use the page-mod API.  I think a more accurate name for this thing is "worker."

@@ +261,5 @@
> +This event is emitted when a tab is closed. When a window is closed
> +this event will be emitted for each of the open tabs in that window.
> +
> +@argument {Tab}
> +Listeners are passed the `tab` object that has closed.

We should use backticks only for identifiers, e.g., class names, names of variables in code samples, event names.  Since the `tab` here doesn't refer to any variable or other object, it shouldn't use backticks.  (Alternatively, `Tab` would be OK, since then it'd be referring to an instance of the Tab class.)

@@ +274,5 @@
> +A single tab will emit this event every time the DOM is loaded: so it will be
> +emitted again if the tab's location changes or the content is reloaded.
> +
> +After this event has been emitted, all properties relating to the tab's
> +content can be used.

It might be worth mentioning that properties can't be used before this event is fired back when you say, "You can get and set various properties of tabs."

@@ +277,5 @@
> +After this event has been emitted, all properties relating to the tab's
> +content can be used.
> +
> +@argument {Tab}
> +Listeners are passed the `tab` object that has loaded.

Same here about backticks.

@@ +286,5 @@
> +
> +This event is emitted when an inactive tab is made active.
> +
> +@argument {Tab}
> +Listeners are passed the `tab` object that has become active.

Same here about backticks.

@@ +295,5 @@
> +
> +This event is emitted when the active tab is made inactive.
> +
> +@argument {Tab}
> +Listeners are passed the `tab` object that has become inactive.

Same here about backticks.

@@ +303,5 @@
> +
> +<api name="open">
> +@event
> +
> +The event is emitted when a new tab is opened. This does not mean that

The event -> This event

@@ +309,5 @@
> +to the user.
> +
> +Properties relating to the tab's content (for example: `title`, `favicon`,
> +and `url`) will not be correct at this point. If you need to access these
> +properties, use the `ready` event listener:

use the `ready` event listener -> listen for the `ready` event

or

register a `ready` event listener

@@ +319,5 @@
> +      });
> +    });
> +
> +@argument {Tab}
> +Listeners are passed the `tab` object that just opened.

Same here about backticks.

@@ +329,5 @@
> +This event is emitted when a tab is closed. When a window is closed
> +this event will be emitted for each of the open tabs in that window.
> +
> +@argument {Tab}
> +Listeners are passed the `tab` object that has closed.

Same here about backticks.

@@ +335,5 @@
> +
> +<api name="ready">
> +@event
> +
> +This event is emitted when the DOM for the tab's content is ready.

It's not right to speak about "the tab" here, since this event is emitted on the module and not a Tab object.  "A tab"?

@@ +345,5 @@
> +After this event has been emitted, all properties relating to the tab's
> +content can be used.
> +
> +@argument {Tab}
> +Listeners are passed the `tab` object that has loaded.

Same here about backticks.

@@ +354,5 @@
> +
> +This event is emitted when an inactive tab is made active.
> +
> +@argument {Tab}
> +Listeners are passed the `tab` object that has become active.

Same here about backticks.

@@ +363,5 @@
> +
> +This event is emitted when the active tab is made inactive.
> +
> +@argument {Tab}
> +Listeners are passed the `tab` object that has become inactive.

Same here about backticks.

::: packages/api-utils/docs/content/worker.md
@@ +92,5 @@
>  If this worker is attached to a content document, returns the related 
>  [tab](packages/addon-kit/docs/tabs.html).
> +</api>
> +
> +Content workers may emit two types of events:

This text isn't needed anymore, right?  At least the other files your patch modifies don't use it anymore.  If you'd like to keep it, it should omit the word "may," since workers *will* emit those types of events.

@@ +102,5 @@
> +script will asynchronously emit the `message` event on the corresponding
> +worker.
> +
> +@argument {JSON}
> +The event listener is passed the message, which must be serializable to JSON.

Same here about JSON.  The value doesn't have to be JSON, only JSONable.

Comment 3

[Will Bamberg [:wbamberg]]

Thanks for the review.

> But before that: This bug is titled "Update all module documentation to use
> @event", but there are other high-level modules that use events that aren't
> covered by this patch.  (It looks like most if not all of them have never
> called out their events in separate sections.)  I think that's OK, as long
> as it's understood that that's the case, and that there are follow-up bugs.

I think this bug really ought to cover all of them. Looking again, I can see:

hotkeys: 'press'
page-mod: 'attach'
request: 'complete'
selection: 'select'

Do you know of more? 

> Also, FWIW, an unrelated comment on information architecture: Functions and
> methods in the documentation list their parameters alongside their names,
> e.g., foo(bar, baz), so when bar and baz are subsequently described it's
> clear what they're referring to.  But events don't do that, so event
> descriptions and the event listener parameter descriptions that follow them
> appear kind of disconnected.  Moreover, event listener parameters aren't
> described like method parameters: only a type is listed, no name, and it
> took me a moment to parse that.  I don't have a good solution for the first
> point: maybe some small "Event Listener Parameters" text above the parameter
> descriptions?  For the second point, I think it might be worth naming the
> parameters.  In addition to being consistent with method parameter
> descriptions, it would help to succinctly describe parameters' purposes.

This is tricky, on both points. For point 1, I agree with you, but I'm not sure what to do about it, except that I'd much rather not have yet another subheading in there. I'll try to think if something. For point 2, I intentionally didn't include parameter names, because unlike method parameters, these parameters aren't named by the SDK: the handler function can name them whatever it likes. If we do name them, how should we name them? In the example code, they're typically just given the name of the object, like:

tabs.on('open', function(tab){

But then the if we used that then the docs will look like this:

"tab : Tab
The tab that opened"

Which just looks silly. I suppose we should choose better names then:

"openedTab : Tab 
The tab that opened"

Maybe that is better? I'm not too keen, still, because it seems to me that it might mislead people into thinking that they have to give it that name.

> ::: packages/addon-kit/docs/tabs.md
> @@ +40,4 @@
> >  
> > +Although you can't access the tab's content directly using this module, you can
> > +attach a [page-mod](packages/addon-kit/docs/page-mod.html) to a tab, and use
> > +that to access and manipulate the content.
> 
> I guess it depends on your definition of "directly," but this isn't correct,
> is it?  tab.attach() is part of this module. 

Yeah, it does depend on the definition - which makes this not a good word to use.

> Also, I'm not sure it's right
> to refer to the tab.attach() mechanism as a page-mod.  I'd prefer not to
> dilute the importance of the high-level API names.  A page-mod is only the
> thing you get when you use the page-mod API.  I think a more accurate name
> for this thing is "worker."

I'm surprised about this. This description is from bug 622077, which introduced the feature. On reflection, I agree with you that "page-mod" is a bad word to use here. Most of the time the docs use 'worker' to refer to the communications channel between the add-on code and content scripts, not to the thing executing in the page. For example, in the worker doc itself:

"Content workers are message-passing facilities for communication between content scripts and the main add-on code."

So it would at least be more consistent with the other docs to say something like:

"You can attach a [content script](dev-guide/addon-development/web-content.html) to a tab, and use that to access and manipulate the content."

Comment 4

[Drew Willcoxon :adw]

(In reply to comment #3)
> Do you know of more?

Browsing the code, these are the ones I see.  Under each module I've listed the things that emit events followed by the names of those events and event listener parameters (name : type).

context-menu
  Item and Menu objects
    message(msg : value)
  Item and Menu object content scripts
    context(clickedNode : node)
    click(clickedNode : node, itemData : string)

page-mod
  PageMod objects
    attach(worker : worker)
    error(error : Error)
  PageMod object content scripts
    message(msg : value)

page-worker:
  Page objects
    message(msg : value)
    error(error : Error)

panel
  Panel objects
    show()
    hide()
    message(msg : value)
    error(error : Error)
  Panel object content scripts
    message(msg : value)

private-browsing:
  module
    start()
    stop()

request:
  Request objects
    complete(response : object)

selection:
  module
    select()

simple-storage
  module
    OverQuota()

tabs
  I looked at the tabs code and my brain melted.

widget
  Same for widget.

windows
  Same for windows!

hotkeys has onPress, but it doesn't appear that hotkey.on("press") will work, so there is no "press" event.

Is @event intended to describe events emitted in content scripts too?  In other words, content-script events need to be documented too -- how?

For page-mod, the content-script "message" event is used in content script defined on a PageMod object, but the message event is really related to the worker object, not the PageMod.  So I'm not sure whether message should be documented with the PageMod class or left to the worker documentation.

It's problematic that worker is a low-level module, yet it's used by high-level modules, and high-level add-on devs therefore need to read about its events. :(

> But then the if we used that then the docs will look like this:
> 
> "tab : Tab
> The tab that opened"
> 
> Which just looks silly. I suppose we should choose better names then:

(Non-event-listener) function parameter names face the same issue too, no?  And I don't recall it being a big deal.

> Maybe that is better? I'm not too keen, still, because it seems to me that
> it might mislead people into thinking that they have to give it that name.

People who know what they're doing will know that they don't have to use the same names.  People who don't know what they're doing will use the same names, but so what?  If anything they'll benefit because their parameters will be well named (assuming the docs use good names!).

Flipping your argument around, if parameter names aren't given, people who don't know what they're doing might worry about what to name their parameters, since they have to name them something.

Anyhow, I don't want my comments about this to block this bug, since it's a separate issue. :)

> So it would at least be more consistent with the other docs to say something
> like:
> 
> "You can attach a [content
> script](dev-guide/addon-development/web-content.html) to a tab, and use that
> to access and manipulate the content."

Hmm, to my mind, the thing being attached to the tab is the worker (a communications channel in your words), not the content script (the thing executing in the page in your words).  The tab's page is a black box.  You attach a worker to it and through the worker inject a content script, which then executes in the page.

But, I was under the impression that an actual worker object was involved in the tab.attach() API, similar to page-mod's API, but I see that's not the case.  So I'm fine with your suggestion.

One more thing: page-worker.md, panel.md and widget.md all say "See Events above" in various places, so they need to be updated.

Comment 5

[Will Bamberg [:wbamberg]]

Attachment: New patch addressing comments

I know you already r+ed this, but just in case you wanted to see the changes, as there were quite a few. But please, feel free to cancel this re-review if you don't think it's needed.

> Is @event intended to describe events emitted in content scripts too?  In
> other words, content-script events need to be documented too -- how?

For the time being the only events that get sent from content scripts are messages (from all of them(?)) and context-menu's click and context events. Rather than introduce a new API structure for those, it seems to me clearer to do more or less what we do now, and document message events are pretty throughly documented in the content scripts stuff, and context menu events in the context menu docs.

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 533482 [details] [diff] [review]
New patch addressing comments

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

Thanks Will, r+ sustained!  But I have some non-nit comments.

(In reply to comment #5)
> Rather than introduce a new API structure for those, it seems to me clearer
> to do more or less what we do now

OK.

::: packages/addon-kit/docs/context-menu.md
@@ +431,5 @@
> +
> +The message can be any
> +<a href = "dev-guide/addon-development/web-content.html#json_serializable">JSON-serializable value</a>.
> +
> +</api>

Nit: The blank line before </api> is unnecessary, here and elsewhere.

Nit: The paragraph that starts "The message can be..." doesn't need to be a separate paragraph, here and elsewhere.

::: packages/addon-kit/docs/page-worker.md
@@ +101,4 @@
>      This property is optional and defaults to "end".
>  
>    @prop [onMessage] {function}
> +    Use this to add a listener to the page-worker's `message` event.

Page worker objects in this doc (and elsewhere in your patch) are referred to using the two separate words "page worker," not the hyphenated word "page-worker" (which IMO should be reserved for references to the module name).

::: packages/addon-kit/docs/tabs.md
@@ +258,5 @@
>  </api>
>  
> +<api name="close">
> +@event
> +

Nit: This blank line is unnecessary, here and elsewhere.

@@ +260,5 @@
> +<api name="close">
> +@event
> +
> +This event is emitted when a tab is closed. When a window is closed
> +this event will be emitted for each of the open tabs in that window.

This is the "close" event that's emitted on a tab, not the one emitted on the module, so this description and the next several event descriptions need to acknowledge that.

e.g.:

This event is emitted when the tab is closed.  It's also emitted when the tab's window is closed.

@@ +263,5 @@
> +This event is emitted when a tab is closed. When a window is closed
> +this event will be emitted for each of the open tabs in that window.
> +
> +@argument {Tab}
> +Listeners are passed the tab object that has closed.

e.g.:

Listeners are passed the tab object.

(or "this tab object")

@@ +280,5 @@
> +content can be used.
> +
> +@argument {Tab}
> +Listeners are passed the tab object that has loaded.
> +</api>

This "ready" event too.

@@ +289,5 @@
> +This event is emitted when an inactive tab is made active.
> +
> +@argument {Tab}
> +Listeners are passed the tab object that has become active.
> +</api>

This "activate" event too.

@@ +298,5 @@
> +This event is emitted when the active tab is made inactive.
> +
> +@argument {Tab}
> +Listeners are passed the tab object that has become inactive.
> +</api>

This "deactivate" event too.

And the rest of these events are module events, so they're OK.

::: packages/addon-kit/docs/widget.md
@@ +584,5 @@
>  
>  <api name="message">
>  @event
> +If you listen to this event you can receive message events from content
> +scripts associated with this widget. When a content script posts a

Ideally the term "widget view" (or something) would be used here instead of "widget," since this section is the WidgetView API and widgets and widget views are not the same thing and the distinction is pretty important.

The WidgetView intro makes the distinction, but the rest of its description doesn't.  So, while you're here and if you'd like, could you fix that?

::: static-files/md/dev-guide/addon-development/web-content.md
@@ +279,4 @@
>  element in the page
>  * `warning` sends a silly string back to the page-mod
>  
> +### <a name="json_serializable">JSON-Serializable Values</a> ###

Nice, thanks.

@@ +282,5 @@
> +### <a name="json_serializable">JSON-Serializable Values</a> ###
> +
> +The payload for an event can be any JSON-serializable value. The code that
> +sends the event serializes the payload to JSON automatically, and the code
> +that receives the event deserializes the payload automatically. So you don't

Nit: I think it's a little awkward to speak about "the code" in this context.  I'd say something like, "When events are sent their payloads are automatically serialized, and when events are received their payloads are automatically deserialized, so you don't need to worry about serialization."

@@ +288,5 @@
> +
> +However, you _do_ have to ensure that the payload can be serialized to JSON.
> +This means that it needs to be a string, number, boolean, array of
> +JSON-serializable values, or an object whose property values are themselves
> +JSON-serializable. However, you can't send functions, and if the object

"However" isn't appropriate here, since the sentence that follows is an implication of the previous sentence.  It would be better to say something like, "Note that this means you can't send functions..."

Comment 7

[Drew Willcoxon :adw]

@@ +288,5 @@
> +
> +However, you _do_ have to ensure that the payload can be serialized to JSON.
> +This means that it needs to be a string, number, boolean, array of
> +JSON-serializable values, or an object whose property values are themselves
> +JSON-serializable. However, you can't send functions, and if the object

Oh, null is also JSON-serializable.

Comment 8

[Drew Willcoxon :adw]

> @@ +288,5 @@
> > +
> > +However, you _do_ have to ensure that the payload can be serialized to JSON.
> > +This means that it needs to be a string, number, boolean, array of
> > +JSON-serializable values, or an object whose property values are themselves
> > +JSON-serializable. However, you can't send functions, and if the object
> 
> "However" isn't appropriate here

Er, to be clear, the second "however."

Comment 9

[Will Bamberg [:wbamberg]]

Fixed by: https://github.com/mozilla/addon-sdk/commit/9a7f59bd1c3a0e7d0a5acb5cb7c7b58941c0b7e8