642335 - Some Markdown links are broken

Status: RESOLVED FIXED

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

Description

[Will Bamberg [:wbamberg]]

Some Markdown 'reference links' omit the second pair of brackets, i.e.:

[link name]
...
[link name]http://example.com 

rather than

[link name][]
...
[link name]http://example.com 

This isn't valid Markdown as far as I can see (http://daringfireball.net/projects/markdown/syntax#link).

These are all the links I've found that are broken by this (mostly in Internals):

troubleshoooting
    [project mailing list]
tabs
    [app tab]
globals
    [JavaScript 1.8.1]
    [CommonJS Module Specification]
    [CommonJS Asynchronous Module Proposal]
low-level modules
    [xhr module documentation]
    [xhr module source code]
    [xhr module test suite]
api-utils
    [nose]
content
    [Loader]
    [Worker]
    [Symbiont]
loader
    [EventEmitter]
symbiont
    [panel]
    [Worker]
worker
    [Worker]
    [EventEmitter] 
e10s
    [MDN Handle Documentation]
    [Out-of-Process Add-ons]
es5
    [ES5 specification]
    [introduction]
    [changes to JavaScript]
    [custom iterators]
observer-service
    [Observer Notifications]
unload
    [Narwhal]
harness
    [code]
internals->globals
    [Package Specification]
internals->glossary
    [Low-Level Module Best Practices]

Comment 1

[Will Bamberg [:wbamberg]]

Attachment: Fixed those links, and some other formatting/language problems

While I was in there I fixed some other things that have been bugging me. In particular, the passwords documentation.

Comment 2

[Drew Willcoxon :adw]

Comment on attachment 522751 [details] [diff] [review]
Fixed those links, and some other formatting/language problems

Thanks for improving the passwords doc.  It's too bad somebody let that land with all its grammar and spelling mistakes.

I'm r-'ing this patch because I think the passwords doc fixes don't go far enough.  The other parts of the patch look OK, so you can either A) post a new patch with only those parts, I'll r+ (with the xhr.md change below), and you can file a new bug for the passwords doc and I'll be happy to review there, or B) do another big patch here with the passwords doc changes described below.

>diff --git a/packages/api-utils/docs/xhr.md b/packages/api-utils/docs/xhr.md

>+## Constructors ##
>+
>+<api name="XMLHttpRequest">
>+@class

The Constructors heading isn't needed anymore since you fixed up apidoc to automatically print headings.  (If you view this page in the browser, "Constructors" is immediately followed by "API Reference".)

-----

Passwords doc comments follow.

We've been trending away from using terms like "application" and "host application" in favor of "Firefox" for addon-kit.  Please change that.

Also, Firefox's password manager is routinely referred to as the "password manager", not the "login manager" or the "login management system", so please change that, too.

The doc uses three related terms, "password", "login", and "credential", without ever explaining the distinctions between them.  If there are no distinctions meaningful to the reader, the doc should pick one and stick with it.  I don't think that's the case though, so there needs to be some discussion about the terms, and the doc should then use them in a way consistent with that discussion.

>-        // credentials is an array of all credentials with a given `realm`.
>+        // credentials is an array of all credentials
>+        // with a given `realm`.

While you're here, could you capitalize all these comments, since they're complete sentences?

>-        // credentials is an array of all credentials associated with given url.
>+        // credentials is an array of all credentials
>+        // associated with the supplied url.

The other comments backtick-quote the properties they mention, so please be consistent: either backtick-quote "url" here or don't backtick-quote any of the others.

>-        // credentials is an array of all credentials associated with given url
>-        // and given realm for useres with given username. 
>+        // credentials is an array of all credentials
>+        // associated with given url, realm, and user name

Same here, and please end this comment with a period like the others.

>         credentials.forEach(function(credential) {

What is this "credential" object?  Is it the same or similar to the options object passed to search() and store()?  The doc doesn't say.

>+An object containing fields associated with a credential being searched. It may
>+contain any combination of the following fields:

Please use the word "properties" instead of "fields", here and everywhere else.

>+* `username`
>+* `password`,
>+* `realm`
>+* `url`
>+* `usernameField`
>+* `passwordField`

"password" has a stray comma.

>+All these fields are described in detail under the `store` section. Any

The "store section" appears in the browser as "store(options)" -- maybe it would be better to refer to it as "the `store` function below"?

>+<api name="store">
>+@function
> 
>-@prop  [onComplete] {function}
>-The callback function that is called once credential is stored.
>+This function is used to store credentials in the host
>+application's built-in login manager.
> 
>-@prop [onError] {function}
>-The callback function that is called if storing a credential failed. Function is
>-passed an `error` containing a reason of a failure.
>+It takes an `options` object as an
>+argument: this contains the containing fields needed to create a login
>+credential.

"this contains the containing fields"?

>-## Storing an add-on associated credential ##
>+** Storing a credential associated with an add-on**

This section is a little oblique: what I want to know as a consumer of this API is how to store a credential associated with *my* add-on, not some arbitrary add-on.  So could you talk about "your add-on" instead of add-ons in general?

>-Add-ons also may store credentials that are not associated with any web sites.
>-In such case `realm` string briefly denotes the login's purpose.
>+Add-ons may store credentials that are not associated with any web sites.
>+In this case the `realm` string briefly denotes the login's purpose.

"In this case, use the `realm` string to briefly denote the login's purpose."

>-## Storing a web page associated credential ##
>+** Storing a credential associated with a web page **
> 
>-Most sites use HTML form login based authentication. Following example stores
>-credentials for such a web site:
>+Most sites use HTML form login based authentication. The following example
>+stores a credential for such a web site:

"HTML form login based authentication" is a mouthful.  "HTML form-based authentication"?

>-## Storing a site authentication login ##
>+** Storing a site authentication login **
> 
>-Some web sites use HTTP/FTP authentication mechanism and associated credentials
>+Some web sites use HTTP/FTP authentication. The associated credentials
> contain different fields:
> 
>     require("passwords").store({
>@@ -195,20 +153,80 @@ sends a reply such as:
>     Server: Apache/1.3.27
>     WWW-Authenticate: Basic realm="ExampleCo Login"
> 
>-If website does not sends `realm` string with `WWW-Authenticate` header then
>-`realm` property must be omitted.
>+If the website does not include the `realm` string in the `WWW-Authenticate`
>+header, then the `realm` property must be omitted.

"web site" and "website" are used inconsistently throughout this doc.  Please consistently use one or the other.

This section is confusing.  It seems to expect me, the consumer of this API, to examine HTTP responses.  How do I do that?  Do I have to do that to use this API?  Or am I supposed to do that manually with some tool and then use that info when writing my add-on?  Also, according to this example response, the WWW-Authenticate header includes a "Basic realm" string, not a "`realm` string".

>+Which properties are valid for `options` depends on the type of
>+authentication, but regardless of that there are two optional callback
>+`onComplete` and `onError` properties that may be passed to observe the
>+success or failure of the operation.

"... there are two optional callback properties, `onComplete` and `onError`, ..."

>+@param options {object}
>+An object containing fields associated with a credential being created and
>+stored. Some fields are necessary for one type of authentication and not
>+for another. Please see examples to find more details.

This paragraph is paraphrasing the paragraph above it.  The paragraph above should simply replace this one.

>+@prop username {string}
>+The user name for the login.

Since the property's name is "username" and not "userName", s/user name/username/.

>+@prop [url] {string}
>+The `url` to which the login applies, formatted as a URL (for example,
>+"http://www.site.com"). A port number (":123") may be appended.

"The url... formatted as a URL"?  WTF?  And why is the first "url" lowercased and backticked?

>+_Please note: `http`, `https` and `ftp` URLs should not include path from the
>+full URL, it will be stripped out if included._

What does "should not include path from the full URL" mean?  These URLs should not include paths at all?  Clear examples of what to do and what not to do, separate from the prose, would be good.  Also, this sentence is a comma splice.

>+@prop [formSubmitURL] {string}
>+The URL a form-based login was submitted to.

This use of past tense here and elsewhere in the store() description is confusing.  I'm storing a credential; I didn't submit anything anywhere, did I?  I think this means you were supposed to have looked at how some web site's authentication works and written down the form's URL or something.

>+Forms with no `action` attribute default to submitting to their origin URL, so
>+that should be stored here. (`formSubmitURL` should not include the path from
>+the full URL, it will be stripped out if included).

Same comment here as in the "Please note" sentence above.

>+@prop [realm] {string}
>+The HTTP Realm for which the login was requested.

Why is Realm capitalized here?

>+@prop [usernameField] {string}
>+The `name` attribute for the user name input in a form. Non-form logins
>+must omit this field.

s/user name/username/

>+@prop [onError] {function}
>+The callback function that is called if storing a credential failed. The
>+function is passed an `error` containing a reason of a failure.

"an `error`"?  What does that mean?  Why is it in backticks?  What kind of object is it?

> <api name="remove">
> @function
>-@param options {object}
>-When removing a credentials the specified `options` object must exactly match
>-what was stored (including a `password`). For this reason it is recommended to
>-chain this function with `search` to make sure that only necessary matches are
>-removed.
> 
>-## Removing a credential ##
>+Remove a stored credential.

s/Remove/Removes/

>+@param options {object}
>+When removing a credential the specified `options` object must exactly match
>+what was stored (including a `password`). For this reason it is recommended to
>+chain this function with `search` to make sure that only necessary matches are
>+removed.

"necessary matches" is weird.  "stored credentials"?

"chain" is ambiguous.  It would be clearer to say that `search` should be used beforehand to retrieve the target credential, and to point to the first example in this section.

Comment 3

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

(In reply to comment #2)

> The doc uses three related terms, "password", "login", and "credential",
> without ever explaining the distinctions between them.  If there are no
> distinctions meaningful to the reader, the doc should pick one and stick with
> it.  I don't think that's the case though, so there needs to be some discussion
> about the terms, and the doc should then use them in a way consistent with that
> discussion.

We should use the word "password" to refer to the `password` field in web forms and the `password` property in this API.

We should use the word "credential" to refer to the set of user-provided information necessary to authenticate a user with a service, which is either a password or the combination of a username and password.  We should also use it to refer to a record or object representing a credential (along with information used to identify its service).

Where it seems reasonable, we may also use the word "password" to describe the things the password manager stores and the Passwords API exposes, given that the password is the sole required member of a credential.

We shouldn't use the word "login" at all.


> >-        // credentials is an array of all credentials associated with given url.
> >+        // credentials is an array of all credentials
> >+        // associated with the supplied url.
> 
> The other comments backtick-quote the properties they mention, so please be
> consistent: either backtick-quote "url" here or don't backtick-quote any of the
> others.

Also, the acronym "URL" should be capitalized (except when referring to a `url` property of an API object).


> >-        // credentials is an array of all credentials associated with given url
> >-        // and given realm for useres with given username. 
> >+        // credentials is an array of all credentials
> >+        // associated with given url, realm, and user name
> 
> Same here, and please end this comment with a period like the others.

But keep "username" a closed compound word, which is consistent with usage elsewhere as well as in the API itself (otherwise the `username` property would be `userName`, as Drew noted).

Comment 4

[Will Bamberg [:wbamberg]]

> >-## Storing an add-on associated credential ##
> >+** Storing a credential associated with an add-on**
> 
> This section is a little oblique: what I want to know as a consumer of this API
> is how to store a credential associated with *my* add-on, not some arbitrary
> add-on.  So could you talk about "your add-on" instead of add-ons in general?
> 
> >-Add-ons also may store credentials that are not associated with any web sites.
> >-In such case `realm` string briefly denotes the login's purpose.
> >+Add-ons may store credentials that are not associated with any web sites.
> >+In this case the `realm` string briefly denotes the login's purpose.
> 
> "In this case, use the `realm` string to briefly denote the login's purpose."
> 

So it also seems to me as if a bit more explanation of how add-on can manage their own credentials would be useful.

First: if an add-on is storing its own credential, then `realm` is just free text 'to briefly denote the login's purpose'. The use of '{{add-on}}' in the realm string in the example isn't anything special (as I had previously assumed). 

However: if add-on 1 creates a credential like "realm":"login", "username":"joe", and then add-on 2 does the same, we will get 2 credentials stored under the same realm/username combination, and when add-on 1 tries to retrieve its credential using those search terms, then 2 credentials are returned. Like this sequence:

1) add-on_1: create credential("login", "joe") -> OK
2) add-on_2: create credential("login", "joe") -> OK
3) add-on_1: search credentials("login", "joe") -> 2 found

This doesn't happen if the same add-on tries it:

1) add-on_1: create credential("login", "joe") -> OK
2) add-on_1: create credential("login", "joe") -> Error, credential already exists

At least, that's what seems to happen from playing with the code. If this is right, should it be a recommendation to prefix `realm`

So just to check I understand this: there's no segregation of stored credentials by add-on going on here? So if add-on 1 creates a credential called "Login"

> >-## Storing a web page associated credential ##
> >+** Storing a credential associated with a web page **
> > 
> >-Most sites use HTML form login based authentication. Following example stores
> >-credentials for such a web site:
> >+Most sites use HTML form login based authentication. The following example
> >+stores a credential for such a web site:
> 
> "HTML form login based authentication" is a mouthful.  "HTML form-based
> authentication"?
> 
> >-## Storing a site authentication login ##
> >+** Storing a site authentication login **
> > 
> >-Some web sites use HTTP/FTP authentication mechanism and associated credentials
> >+Some web sites use HTTP/FTP authentication. The associated credentials
> > contain different fields:
> > 
> >     require("passwords").store({
> >@@ -195,20 +153,80 @@ sends a reply such as:
> >     Server: Apache/1.3.27
> >     WWW-Authenticate: Basic realm="ExampleCo Login"
> > 
> >-If website does not sends `realm` string with `WWW-Authenticate` header then
> >-`realm` property must be omitted.
> >+If the website does not include the `realm` string in the `WWW-Authenticate`
> >+header, then the `realm` property must be omitted.
> 
> "web site" and "website" are used inconsistently throughout this doc.  Please
> consistently use one or the other.
> 
> This section is confusing.  It seems to expect me, the consumer of this API, to
> examine HTTP responses.  How do I do that?  Do I have to do that to use this
> API?  Or am I supposed to do that manually with some tool and then use that
> info when writing my add-on?  Also, according to this example response, the
> WWW-Authenticate header includes a "Basic realm" string, not a "`realm`
> string".
> 
> >+Which properties are valid for `options` depends on the type of
> >+authentication, but regardless of that there are two optional callback
> >+`onComplete` and `onError` properties that may be passed to observe the
> >+success or failure of the operation.
> 
> "... there are two optional callback properties, `onComplete` and `onError`,
> ..."
> 
> >+@param options {object}
> >+An object containing fields associated with a credential being created and
> >+stored. Some fields are necessary for one type of authentication and not
> >+for another. Please see examples to find more details.
> 
> This paragraph is paraphrasing the paragraph above it.  The paragraph above
> should simply replace this one.
> 
> >+@prop username {string}
> >+The user name for the login.
> 
> Since the property's name is "username" and not "userName", s/user
> name/username/.
> 
> >+@prop [url] {string}
> >+The `url` to which the login applies, formatted as a URL (for example,
> >+"http://www.site.com"). A port number (":123") may be appended.
> 
> "The url... formatted as a URL"?  WTF?  And why is the first "url" lowercased
> and backticked?
> 
> >+_Please note: `http`, `https` and `ftp` URLs should not include path from the
> >+full URL, it will be stripped out if included._
> 
> What does "should not include path from the full URL" mean?  These URLs should
> not include paths at all?  Clear examples of what to do and what not to do,
> separate from the prose, would be good.  Also, this sentence is a comma splice.
> 
> >+@prop [formSubmitURL] {string}
> >+The URL a form-based login was submitted to.
> 
> This use of past tense here and elsewhere in the store() description is
> confusing.  I'm storing a credential; I didn't submit anything anywhere, did I?
>  I think this means you were supposed to have looked at how some web site's
> authentication works and written down the form's URL or something.
> 
> >+Forms with no `action` attribute default to submitting to their origin URL, so
> >+that should be stored here. (`formSubmitURL` should not include the path from
> >+the full URL, it will be stripped out if included).
> 
> Same comment here as in the "Please note" sentence above.
> 
> >+@prop [realm] {string}
> >+The HTTP Realm for which the login was requested.
> 
> Why is Realm capitalized here?
> 
> >+@prop [usernameField] {string}
> >+The `name` attribute for the user name input in a form. Non-form logins
> >+must omit this field.
> 
> s/user name/username/
> 
> >+@prop [onError] {function}
> >+The callback function that is called if storing a credential failed. The
> >+function is passed an `error` containing a reason of a failure.
> 
> "an `error`"?  What does that mean?  Why is it in backticks?  What kind of
> object is it?
> 
> > <api name="remove">
> > @function
> >-@param options {object}
> >-When removing a credentials the specified `options` object must exactly match
> >-what was stored (including a `password`). For this reason it is recommended to
> >-chain this function with `search` to make sure that only necessary matches are
> >-removed.
> > 
> >-## Removing a credential ##
> >+Remove a stored credential.
> 
> s/Remove/Removes/
> 
> >+@param options {object}
> >+When removing a credential the specified `options` object must exactly match
> >+what was stored (including a `password`). For this reason it is recommended to
> >+chain this function with `search` to make sure that only necessary matches are
> >+removed.
> 
> "necessary matches" is weird.  "stored credentials"?
> 
> "chain" is ambiguous.  It would be clearer to say that `search` should be used
> beforehand to retrieve the target credential, and to point to the first example
> in this section.

Comment 5

[Will Bamberg [:wbamberg]]

Sorry, ignore comment 4. Can I delete comments?

> >-## Storing an add-on associated credential ##
> >+** Storing a credential associated with an add-on**
> 
> This section is a little oblique: what I want to know as a consumer of this API
> is how to store a credential associated with *my* add-on, not some arbitrary
> add-on.  So could you talk about "your add-on" instead of add-ons in general?
> 
> >-Add-ons also may store credentials that are not associated with any web sites.
> >-In such case `realm` string briefly denotes the login's purpose.
> >+Add-ons may store credentials that are not associated with any web sites.
> >+In this case the `realm` string briefly denotes the login's purpose.
> 
> "In this case, use the `realm` string to briefly denote the login's purpose."
> 

So it also seems to me as if a bit more explanation of how add-on can manage
their own credentials would be useful.

First: if an add-on is storing its own credential, then `realm` is just free
text 'to briefly denote the login's purpose'. The use of '{{add-on}}' in the
realm string in the example isn't anything special (as I had previously
assumed). 

However: if add-on 1 creates a credential like "realm":"login",
"username":"joe", and then add-on 2 does the same, we will get 2 credentials
stored under the same realm/username combination, and when add-on 1 tries to
retrieve its credential using those search terms, then 2 credentials are
returned. Like this sequence:

1) add-on_1: create credential("login", "joe") -> OK
2) add-on_2: create credential("login", "joe") -> OK
3) add-on_1: search credentials("login", "joe") -> 2 found

This doesn't happen if the same add-on tries it:

1) add-on_1: create credential("login", "joe") -> OK
2) add-on_1: create credential("login", "joe") -> Error, credential already
exists

At least, that's what seems to happen from playing with the code. If this is
right, should it be a recommendation to prefix `realm` with self.id?

Comment 6

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

(In reply to comment #5)
> If this is
> right, should it be a recommendation to prefix `realm` with self.id?

No, `passwords` should automagically segregate addon-created credentials by addon, using a URL unique to each addon to accomplish this.  If that isn't happening, it's a bug that we must fix.

Comment 7

[Will Bamberg [:wbamberg]]

(In reply to comment #6)
> (In reply to comment #5)
> > If this is
> > right, should it be a recommendation to prefix `realm` with self.id?
> 
> No, `passwords` should automagically segregate addon-created credentials by
> addon, using a URL unique to each addon to accomplish this. 

Well that would make sense. Could you confirm that with this:

add-on-1/lib/main.js:
---------------------
function store_password() {
  require("passwords").store({
    realm: "yarr!!!",
    username: "a pirate",
    password: "SeCrEt123",
    onComplete: function onComplete() {
      console.log(require("self").id);
    }
  });
}

require("widget").Widget({
  id: "store-password",
  label: "Store password",
  content: "Store",
  width: 50,
  onClick: function() {
    store_password();
  }
});


add-on-2/lib/main.js:
---------------------
function show_passwords() {
  require("passwords").search({
    realm: "yarr!!!",
    onComplete: function onComplete(credentials) {
      credentials.forEach(function(credential) {
        console.log(require("self").id);
        console.log("stored:");
        console.log(credential.username);
        console.log(credential.realm);
        console.log(credential.password);
        });
      }
    });
  }

require("widget").Widget({
  id: "show-password",
  label: "Show password",
  content: "Show",
  width: 50,
  onClick: function() {
    show_passwords();
  }
});

...if you install both add-ons, click 'Store', then click 'Show', you get:

info: jid0-04cBe23VEePc3Of5vcLk4yTwjW8
info: jid0-01mBBFyu0ZAXCFuB1JYKooSTKIc
info: stored:
info: a pirate
info: yarr!!!
info: SeCrEt123

Comment 8

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

(In reply to comment #7)
> ...if you install both add-ons, click 'Store', then click 'Show', you get:
> 
> info: jid0-04cBe23VEePc3Of5vcLk4yTwjW8
> info: jid0-01mBBFyu0ZAXCFuB1JYKooSTKIc
> info: stored:
> info: a pirate
> info: yarr!!!
> info: SeCrEt123

My JIDs are different, but otherwise yes, that's what I get.

Comment 9

[Will Bamberg [:wbamberg]]

Attachment: Revert passwords fixes, and remove a couple of stray headings

It looks like the passwords doc needs more extensive work, so let's split it off: bug 646865. I still hope to get that up for review today.

Comment 10

[Will Bamberg [:wbamberg]]

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