787035 - Document page-mod changes for 1.11

Status: RESOLVED FIXED

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

Description

[Will Bamberg [:wbamberg]]

Several improvements for page-mod are landing in 1.11:
* bug 708190
* bug 684047
* bug 777632

We should make sure all the page-mod documentation is up to date.

Comment 1

[Will Bamberg [:wbamberg]]

Attachment: patch

I've added documentation for `attachTo` (the note about applying only to tab content was already there).

I've not yet added anything on cross-domain scripts, as it still seems uncertain that that will land in 1.11.

I've also substantially rewritten the page-mod API page: before it went into lots of detail about worker multiplicity, without a good overview, and with a very terse API reference with few examples. Instead I've tried to keep the overview short, and pointed at an expanded API reference section for all the non-basic details.

This change is also at https://github.com/wbamberg/jetpack-sdk/tree/787035 is that's an easier way to consume it...

Comment 2

[Will Bamberg [:wbamberg]]

Attachment: include the diagram this time

...and I just realized I forgot to include the diagram. Try this patch instead.

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 664138 [details] [diff] [review]
include the diagram this time

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

Looks good. Thanks again Will for your hard work on exposing our work to the world :)

::: packages/addon-kit/docs/page-mod.md
@@ +12,4 @@
>  
> +* one or more scripts to attach. The SDK calls scripts like this
> +"content scripts". For all the details on content scripts, see the
> +[guide to content scripts](dev-guide/guides/content-scripts/index.html).

I'm not sure it is really worth putting this :
  For all the details on content scripts, see the [guide
  to content scripts](dev-guide/guides/content-scripts
  /index.html).
in such introduction paragraph. It would be better to keep it simple and let the reader find this link later after having a better picture of page-mod itself.

@@ +90,5 @@
> +* `attachTo` controls whether to attach scripts to tabs
> +that were already open when the page-mod was created, and whether to attach
> +scripts to iframes as well as the topmost document.
> +* `contentScriptWhen` controls when the scripts should be attached to
> +the page.

I'm not sure it really is explicit. Shouldn't we specify "page load" in this sentence? With something like this: "... controls when during page load proccess the scripts should be attached"
or "... controles, when, on document's loading, the scripts should be attached"
or anything you can come up with.

@@ +107,2 @@
>  
> +For example, this add-on retrieves the HTML content of specific tags from

this => the following?

@@ +112,2 @@
>  
> +main.js:

I think it would help understanding the lib/data folder story if we specify here an absolute path:
/lib/main.js
and right after:
/data/element-getter.js

@@ +261,3 @@
>  <api name="PageMod">
>  @class
> +A page-mod object. Once activated a page-mod will execute the supplied content

activated => created, instanciated, ....

@@ +272,5 @@
>    @prop include {string,array}
>      A match pattern string or an array of match pattern strings.  These define
> +    the pages to which the page-mod applies. At least one match pattern must
> +    be supplied: `include` is the only mandatory argument to the `PageMod`
> +    constructor.

We repeat that sentence twice really closely. Do you think we can avoid that? And inlude isn't the only one mandatory, you need to pass at least a content script or content style. (I don't know what happens if you omit those, it may or may not throw exception)

@@ +296,5 @@
> +        var pageMod = require("page-mod");
> +        pageMod.PageMod({
> +          include: /.*developer.*/,
> +          contentScript: 'window.alert("Page matches ruleset");'
> +        });

Could you add a quick note about regexp. They have to be "complete". i.e. they have to match the whole URL, so that here, `.*` before and after are really important.
That's a common misunderstanding people have.

@@ +428,5 @@
> +
> +   The values can be any JSON-serializable value: a string, number,
> +   boolean, null, array of JSON-serializable values, or an object whose
> +   property values are themselves JSON-serializable. This means you can't send
> +   functions, and if the object contains methods they won't be usable.

You can't pass cyclic value either. Might be worth specifying that too. Like: var options = {a : options};

@@ +483,5 @@
> +    Dynamically constructing code strings like those assigned to `contentScript`
> +    or `contentStyle` is usually considered an unsafe practice that may cause an
> +    add-on to fail AMO review. In this case it is safe, and should be allowed,
> +    but including a comment like that in the example above will help to
> +    prevent any misunderstanding.

Would it help to put a link to the related bug?
(bug 759190)

@@ +508,3 @@
>  
>    @prop [attachTo] {string,array}
> +   By default, content scripts:

What do you think about that instead:
  If nothing is specified, content scripts will by default:

That's to highlight the ackward behavior where, if you don't pass anything, the default will be [top, frames].
And what about saying, after description of each possible value, that the default value -if nothing is set- is [top, frame] ?

But it might be worth keeping it simple as well.

@@ +582,5 @@
> +   The listener function is passed a
> +   [`worker`](packages/api-utils/content/worker.html) object that you
> +   can use to
> +   [communicate with the content scripts](packages/addon-kit/page-mod.html#Communicating With Content Scripts) your page-mod has
> +   loaded into this particular tab.

this particular tab => a particular page.

Note that your are using `page` everywhere. I'm wondering if it would be better to use `document` instead. As page sounds weird for iframes, whereas document sounds more generic to me.

Comment 4

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/9c26fa35a42ec0476b48615b5f25e34cdfd09b2a
Bug 787035 - Document page-mod changes for 1.11; r=@ochameau

Comment 5

[[github robot]]

Commit pushed to stabilization at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/482f468bf583e3f6a2bb6e0d8d59a39eb5f44610
Bug 787035 - Document page-mod changes for 1.11; r=@ochameau
(cherry picked from commit 9c26fa35a42ec0476b48615b5f25e34cdfd09b2a)