Closed Bug 577558 Opened 11 years ago Closed 11 years ago

Tutorial should have an advanced tutorial option.

Categories

(Add-on SDK Graveyard :: General, enhancement, P3)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: KWierso, Assigned: wbamberg)

Details

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100707 Firefox/3.7a5pre RTSE/1.2.0.20100320
Build Identifier: 

It would be nice to have an additional tutorial that shows how to do somewhat more advanced things while building an extension.

For example, how to write (and use) multiple modules within a single extension.

Reproducible: Always
This will be addressed in a release sometime after 1.0a6
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Target Milestone: -- → 0.7
Target Milestone: 0.7 → 0.8
After thinking about this further, the tutorial requested here seems more like something that would be in the actual SDK docs, and consumed by the Builder as it does with the current documentation.
Component: FlightDeck → Jetpack SDK
QA Contact: flightdeck → jetpack-sdk
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
Target Milestone: 0.8 → ---
(In reply to comment #2)
> After thinking about this further, the tutorial requested here seems more like
> something that would be in the actual SDK docs, and consumed by the Builder as
> it does with the current documentation.

Daniel: I think you're right, this is about a tutorial for building an addon generally using either the SDK or Builder, and it isn't specific to the builder.

Bug 624607 is about creating the kind of example addon that could serve as the basis for a tutorial like this, if I understand it correctly, so cc:ing Will, who is working on that bug.
QA Contact: jetpack-sdk → general
Yes, that's exactly right. Once bug 624607 has landed I'm planning to build a tutorial round it covering slightly more advanced stuff than the current translator tutorial, and including most of the modules in addon-kit.
Assignee: nobody → wbamberg
Status: NEW → ASSIGNED
I know I've asked you for a few reviews lately, and that you have FF4 stuff too. So please let me know if you don't have the time to look at this. 

The code has been reviewed, but I think it would not hurt to have more eyes on it as it should be not just functional but exemplary, and I'm not very familiar with JS/jQuery.

I'm not able, yet, to get the syntax highlighter to work with the HTML and CSS snippets - so I pretended it was JS, with some slightly odd results. I'll continue to work on that, but didn't want it to block the review.

I'm a bit conflicted about whether the last chapter - 'displaying' should be included, or not. It doesn't add anything new in terms of learning about the SDK - in fact it all gets a bit repetitive at that point. On the other hand, the annotator seems like a more useful thing when that last part is implemented.

It would be nice to be able to trigger 'OverQuota' so we can demo it (and the corresponding notification). I smoke tested it when I implemented it by hacking simple-storage, but that's not very good. I suppose probably what I should do here is create some unit tests for storage, and run them as part of the tutorial to demo it...

This code is also at: https://github.com/wbamberg/jetpack-sdk/tree/577558
Attachment #510881 - Flags: review?(adw)
(In reply to comment #5)
> I know I've asked you for a few reviews lately, and that you have FF4 stuff
> too. So please let me know if you don't have the time to look at this.

No problem, looking right now.

> It would be nice to be able to trigger 'OverQuota' so we can demo it (and the
> corresponding notification). I smoke tested it when I implemented it by hacking
> simple-storage, but that's not very good. I suppose probably what I should do
> here is create some unit tests for storage, and run them as part of the
> tutorial to demo it...

I haven't looked at the patch in depth yet so I don't know the context of this comment, but simple-storage does have two unit tests for OverQuota, this function here and the one after:

https://github.com/mozilla/addon-sdk/blob/master/packages/addon-kit/tests/test-simple-storage.js#L154

Let me know if you have any questions about them.
Will, I'll try to have this reviewed by the end of the day.
Comment on attachment 510881 [details] [diff] [review]
Annotator tutorial

Nice job, Will.  Sorry for the delay in review.

My comments are on the code samples in the prose, but they apply to the code itself in the examples directory too.  Also, some code samples are repeated (and tweaked) on multiple pages, and while I may have commented on only one instance of a sample, my comments apply to all its instances.  TBH I didn't look closely at code not listed in the prose.

r+ with the changes below, unless you disagree with something, in which case let me know.

(In reply to comment #5)
> I'm a bit conflicted about whether the last chapter - 'displaying' should be
> included, or not. It doesn't add anything new in terms of learning about the
> SDK - in fact it all gets a bit repetitive at that point. On the other hand,
> the annotator seems like a more useful thing when that last part is
> implemented.

Yeah, it's nice to button it up, and the screenshots are helpful.

> It would be nice to be able to trigger 'OverQuota' so we can demo it (and the
> corresponding notification).

Hmm, I think it's OK to leave that to the reader.  (The quota is controlled by a pref, so if you'd like, you could tell people to set it to a really low number and test it manually.)


>diff --git a/examples/annotator/data/matcher.js b/examples/annotator/data/matcher.js

>+That's considered a match: then we:

?  "That's considered a match.  Then we:"?

>+For all annotated elements:
>+- bind 'mouseenter' and 'mouseleave' events to the element, to send 'show'
>+and 'hide' messages back to the add-on.

Nit: indent the second line of this bullet point so it starts under the B in "bind" in the first line


>diff --git a/examples/annotator/lib/main.js b/examples/annotator/lib/main.js

> // Import the modules we need.
>-const contextMenu = require('context-menu');
>-const panels = require('panel');
> const widgets = require('widget');
> const data = require('self').data;
>-const simpleStorage = require('simple-storage');
> const pageMod = require('page-mod');
>+const panels = require('panel');
>+const simpleStorage = require('simple-storage');
> const privateBrowsing = require('private-browsing');
> const notifications = require("notifications");

Nit: require("notifications") uses double quotes but everything else uses single quotes.  Could you make sure these files are using one or the other consistently?


>diff --git a/static-files/index.html b/static-files/index.html

>+            <li><h4 class="link" id="addon-development/annotator/annotator">Annotator: a More Complex Add-on</h4>

This new tutorial needs to be added to guide/addon-development/tutorials.md.


>diff --git a/static-files/md/dev-guide/addon-development/annotator/annotator.md b/static-files/md/dev-guide/addon-development/annotator/annotator.md

>+notes. Whenever the user loads a page containing annotated elements these
>+elements are highlighted, and if the user moves their mouse over an annotated

Nit: "user" is singular, so "... if the user moves his mouse..." (or "her").

>+### [Design Overview](#guide/addon-development/annotator/overview) ###
>+
>+### [Implementing the Widget](#guide/addon-development/annotator/widget) ###
>+
>+### [Creating Annotations](#guide/addon-development/annotator/creating) ###
>+
>+### [Storing Annotations](#guide/addon-development/annotator/storing) ###
>+
>+### [Displaying Annotations](#guide/addon-development/annotator/displaying) ###

Nit: headings usually head a section, i.e., they usually have text under them.  A list (<ol> or <ul>) would probably be better here.


>diff --git a/static-files/md/dev-guide/addon-development/annotator/creating.md b/static-files/md/dev-guide/addon-development/annotator/creating.md

>+It listens for [mouseenter](http://api.jquery.com/mouseenter/) events.

I was confused about why this links to a jQuery doc until I realized that the page-mod uses jQuery.  It would be good to briefly mention that first and also to stress that mouseenter is a jQuery event (and not a Jetpack event).

>+When a mouseenter event is triggered the selector checks whether the element
>+is eligible for annotation. An event is eligible if it, or one of its ancestors

s/event/element/ in the second line

>+in the DOM tree, has an ID attribute. The idea here is to make it more likely

Would be clearer to s/has an ID attribute/has an attribute named `"id"`/

>+If the page element is eligible for annotation, then the selector highlights
>+that element and binds a click handler to it. The click handler sends a message
>+called `show` back to the main add-on code. The `show` message contains: the URL
>+for the page, the ID attribute value, and the content of the page element.

s/content/text content/ in the last line

I tried out the add-on on mozilla.com.  Lots of elements were highlighted yellow when I moused over them, but when I clicked nothing happened.  Is that right?

>+Finally, the selector listens for the window's `unload` event and sends the
>+main add-on code a message called `detach` when unload occurs. This is so the
>+main add-on can remove the worker associated with this page (eventually,
>+`page-mod` should have its own `unload` event, but this is the workaround for
>+the present).

Nit: Since the parenthetical is a full sentence, end the previous sentence after "page" and capitalize and punctuate the parenthetical.  e.g., "... page.  (Eventually, ... present.)"

My comment below about the widget click hack applies here too.

The code that follows this text is uncommented and pretty dense.  I find it helpful when documentation breaks longer code samples into small chunks and explains each chunk separately (as you've done elsewhere).  Here you've done the opposite: explained the entire listing in one go and then shown it.  What do you think?

>+    function resetMatchedElement() {
>+      if (matchedElement) {
>+        $(matchedElement).css('background-color', originalBgColor);
>+        $(matchedElement).unbind('click.annotator');

Again, if you're not familiar with jQuery it's totally unclear where $ and all these functions come from, so the use of jQuery should be mentioned somewhere.

>+    self.on('message', function onMessage(activation) {
>+      active = activation;
>+      if (!active) {
>+        resetMatchedElement();
>+      }
>+    })

semicolon on last line

>+    $('*').mouseenter(function() {

>+        postMessage(['show', [document.location.toString(),
>+                    $(ancestor).attr("id"),
>+                    $(matchedElement).text()]]);
>+      });
>+    })

semicolon on last line

It's a little hard to follow all the postMessage arguments, especially with the nested array.  How about:

        postMessage(
          ['show',
            [document.location.toString(),
             $(ancestor).attr("id"),
             $(matchedElement).text()]]);

Alternatively and I think better would be to pass objects with named properties as messages rather than arrays.  Like:

  postMessage({
    kind: "show",
    anchor: [
      document.location.toString(),
      $(ancestor).attr("id"),
      $(matchedElement).text()
    ]
  })

Then it's clearer when you're using these things because you have to use the names of the various properties (message.kind, message.anchor) rather than cryptic array indices.

>+Because this code uses jQuery, you'll need to
>+[download](http://docs.jquery.com/Downloading_jQuery) that as well, and save it in
>+`data`.

Oh.  This should be mentioned upfront rather than at the end.

Also, it's kind of weird that we're asking people to download and copy and paste code when it's already bundled in the examples directory.  Or do you think walking through that process improves learning?  If so, I'm cool with that.

>+### Updating main.js ###
>+
>+Go back to `main.js` and add the code to create the selector into the `main`
>+function:
>+
>+    selector = pageMod.PageMod({

var selector = ...

>+The page-mod matches anything: so each time the user loads a page the page-mod

s/anything/all pages/

Nit: a comma is more appropriate here than a colon

>+emits the `attach` event, which will call the handler function we've assigned

s/handler/listener/ here and elsewhere, since "listener" is the word we've used in other docs for event callback functions

>+In the attach handler we do three things:
>+
>+* send the content script a message with the current activation status
>+* add the worker to an array called `selectors` so we can send it messages
>+later on
>+* assign a message handler for messages from this worker. If the message is
>+`show` we will just log the content for the time being. If the message is
>+`detach` we remove the worker from the `selectors` array

Nit: Punctuate the end of the last sentence.

>+Save the file and execute `cfx run` again. Activate the annotator by clicking
>+the widget and load a page. You should see the highlight appearing when you

Would be great to link to the page you're showing in the screenshot so people can easily replicate it.

>+<img src="media/annotator/highlight.png" alt="Annotator Highlighting">

Nit: There's an arrow-looking icon in the location bar of these screenshots; are you running some unrelated add-on?  Ideally screenshots would use a stock Firefox (other than the add-on being demonstrated of course) so people don't get confused when their experiences don't match up.

>+## Annotation Editor Panel ##
>+
>+Next, create a subdirectory under `data` called `editor`. This will contain
>+two files: the HTML content, and the content script.

At this point we're a long way from the introductory text at the top of this page, so a sentence here briefly restating the purpose of the editor panel would be good.  It would also be good to state the connection between the HTML content, content script, and panel.

>+### Annotation Editor HTML ###

>+  textarea {
>+    width: 180px;
>+    height: 180px;
>+    margin: 10px;
>+    padding:0px;

Nit: add a space between "padding:" and "0px;"

>+### Annotation Editor Content Script ###
>+
>+In the corresponding content script we do two things:
>+
>+* handle a message from the add-on code by giving the text area focus
>+* listen for the return key and when it is pressed, send the contents of the
>+text area to the add-on.
>+
>+<br>

Why the <br>?  It adds vertical space that seems unnecessary.

>+### Updating main.js again ###

s/again/Again/ (title case)

>+Then add the following code to the `main` function:
>+
>+    annotationEditor = panels.Panel({

var annotationEditor = ...

>+      onMessage: function(message) {
>+        if (message) {
>+          console.log(this.anchor);
>+          console.log(message);
>+        }

It would be much much clearer to s/message/annotationText/ or something.

>+The only thing left is to link the editor to the selector. So edit the message
>+handler assigned to the selector so that on receiving the `show` message we
>+assign the content of the message to the panel, and show the panel:
>+
>+    selector = pageMod.PageMod({
>+      include: ['*'],
>+      contentScriptWhen: 'ready',
>+      contentScriptFile: [data.url('jquery-1.4.2.min.js'),
>+                          data.url('selector.js')],
>+      onAttach: function(worker) {
>+        worker.postMessage(annotatorIsOn);
>+        worker.on('message', function(message) {
>+          switch(message[0]) {
>+            case 'show':
>+              annotationEditor.anchor = message[1];

Panels have an `anchor` property as part of their API whose value is a "a handle to a DOM node in a page" (and handles are currently unimplemented AFAIK).  Are you storing some arbitrary data there?  It looks like it.  If so, two things: 1) don't use the `anchor` property, and 2) you should call out that the property is not part of the API and you're just attaching some task-specific data to it.


>diff --git a/static-files/md/dev-guide/addon-development/annotator/displaying.md b/static-files/md/dev-guide/addon-development/annotator/displaying.md

>+# Displaying Annotations #
>+
>+In this chapter we'll use a page-mod to locate elements of Web pages that have

Nit: s/Web/web/ since it's an adjective here

>+In the `main` function, add the code to create the matcher:
>+
>+    matcher = pageMod.PageMod({

var matcher = ...

>+Then in the module's scope implement a function to update the matcher's
>+workers, and edit `handleNewAnnotation` to call this new function when the
>+user enters a new annotation:
>+
>+    function updateMatchers() {
>+      matchers.forEach(function (matcher) {
>+        matcher.postMessage(simpleStorage.storage.array);
>+      });
>+    }
>+
>+<br>
>+
>+    function handleNewAnnotation(annotationText, anchor) {
>+      var newAnnotation = new Annotation(annotationText, anchor);
>+      simpleStorage.storage.array.push(newAnnotation);
>+      updateMatchers();
>+    }
>+<br>

Nit: I think the <br>s here are unnecessary, and you haven't used them in other places with adjacent code listings or code listings followed by headers.

>+## Annotation panel ##
>+
>+The annotation panel just shows the content of the annotation.

*The* annotation?  Which one?  Maybe "an annotation" is better?

>+### Annotation panel Content Script ###
>+
>+The annotation panel has a minimal content script, that sets the text:

nit: unncecessary comma

>+Finally, update `main.js` with the code to construct the annotation panel:
>+
>+    annotation = panels.Panel({

var annotation = ...

>+Execute `cfx run` one last time. Activate the annotator and enter an
>+annotation. You should see a yellow border around the item you annotated:

It's a bit of a pain loading the same web page over and over again after cfx run, since it starts you with a new profile.  Thinking out loud: Using `cfx develop` is one option but it's "experimental"; you could tell people to create a test profile and use the -p cfx option, so at least they'd have bookmarks, history, and the awesomebar; you could programatically open a tab in main.js just for development; or simply suggest that the user copy a URL to his clipboard or somewhere (or you could link to a URL in the docs everytime you ask him test something) so he can easily copy and paste it after cfx run.


>diff --git a/static-files/md/dev-guide/addon-development/annotator/overview.md b/static-files/md/dev-guide/addon-development/annotator/overview.md

>+## User Interface ##
>+
>+The annotator's main user interface consists of a widget and three panels.
>+
>+* The widget is used to switch the annotator on and off, and to display a list
>+of all the stored annotations.
>+
>+* The **annotation-editor** panel enables the user to enter a new annotation.
>+
>+* The **annotation-list** panel shows a list of all stored annotations.
>+
>+* The **annotation** panel displays a single annotation.

Nit: The items in this particular list are rendered with a lot of space between them.  I think it's because you've included blank lines between them, which translates to <p>s inside the <li>s.  (The other list on this page is the same, but I think that's fine since its items are pretty long.)

>+## Working with the DOM ##
>+
>+We'll use two page-mods:

A bit more explanation here, like, "We'll use two page-mods to interact with the DOMs of pages that the user has opened."

>+Next, we will implement the
>+[widget](#guide/addon-development/annotator/widget).
>\ No newline at end of file

Please add a new line.


>diff --git a/static-files/md/dev-guide/addon-development/annotator/storing.md b/static-files/md/dev-guide/addon-development/annotator/storing.md

>+## Storing new annotations ##

title case

>+In the module scope, initialize the array:
>+
>+    if (!simpleStorage.storage.array)
>+      simpleStorage.storage.array = [];

`array` is not a very descriptive name.  Array of what?  And the text above doesn't explain either.  What will be stored in it?

>+'annotation anchor', which consists of the URL, element ID and element content:

Nit: use double quotes

>+Now we need to link this code to the annotation editor, so that when the user
>+presses 'return' in the editor, we create and store the new annotation:

Nit: no need to quote "return", would be clearer to say "the return key"

>+The user will be able to click links in the panel, but we want to open them in
>+the main browser window rather than the panel. So the content script binds a
>+click handler to the links which will send the URL to the add-on.

Ugh, we should really make it easier to do this.

>+Here's the code to create the panel, which can go in the `main` function.
>+
>+    annotationList = panels.Panel({

var annotationList = ...

>+Finally we need to connect this to the widget's `right-click` message:
>+
>+    widget = widgets.Widget({

var widget = ...

>+This time execute `cfx xpi` to build the XPI for the add-on, and install it in
>+Firefox. Activate the add-on, add an annotation, and then right-click the
>+widget. You should see something like this:
>+
>+<div align="center">
>+<img src="media/annotator/annotation-list.png" alt="Annotation List">
>+</div>
>+<br>
>+
>+Restart Firefox, right-click the widget again, and check that the annotation
>+is still there.

Ugh, cfx run should easily support persistent data and profiles.  Even though I had read the part above about using cfx xpi, I still did cfx run and was surprised when my annotations didn't persist.  So you may want to add briefly, "If the annotation is not still there, make sure you installed and tested the XPI and didn't simply `cfx run`."

>+## Responding To OverQuota events ##

s/To/to/
s/events/Events/

>+    privateBrowsing.on('start', function() {
>+      widget.contentURL=data.url('widget/pencil-off.jpg');
>+      activateSelectors();
>+    });
>+
>+    privateBrowsing.on('stop', function() {
>+      if (canEnterAnnotations()) {
>+        widget.contentURL=data.url('widget/pencil-on.jpg');

Nit: add spaces between "contentURL", "=", and "data"


>diff --git a/static-files/md/dev-guide/addon-development/annotator/widget.md b/static-files/md/dev-guide/addon-development/annotator/widget.md

>+## The Widget's Content Script ##
>+
>+The widget's content script just listens for left- and right- mouse clicks and
>+posts the corresponding message to the add-on code:
>+
>+    this.addEventListener('click', function(event) {
>+      if(event.button == 0 && event.shiftKey == false)
>+        postMessage('left-click');
>+
>+      if(event.button == 2 || (event.button == 0 && event.shiftKey == true))
>+        postMessage('right-click');
>+        event.preventDefault();
>+    }, true);

This is cool, and it demonstrates Jetpack's flexibility, but it's a hack.  Widgets emit click events as part of their API, and I feel a twinge of pain when our documented examples circumvent our APIs.  If we could go back in time, we should have used the annotator example as an impetus for augmenting the widget API with right-clicks, like a "RightClick" event.

Also, event.preventDefault() doesn't seem to actually prevent the add-on bar's context menu from showing.  (Guess that's bug 626326?)

>+## The Widget's Icons ##
>+
>+You can copy the widget's icons from here:
>+
>+<img style="margin-left:40px; margin-right:20px;" src="media/annotator/pencil-on.jpg" alt="Active Annotator">
>+<img style="margin-left:20px; margin-right:20px;" src="media/annotator/pencil-off.jpg" alt="Inactive Annotator">

Hmm, is there a best or recommended size for widget icons?  (The widget doc should mention it.  Obviously they're scaled -- widget doc should mention that too -- but people who want polish won't be satisfied with scaling.)  Anyhow, for now, could you make these PNGs rather than jpegs?  jpeg compression looks awful for CGI.

>+## main.js ##
>+
>+Now in the `lib` directory open `main.js` and replace its contents with this:
>+
>+    const widgets = require('widget');
>+    const data = require('self').data;
>+
>+    var annotatorIsOn = false;
>+
>+    function toggleActivation() {
>+      annotatorIsOn = !annotatorIsOn;
>+      return annotatorIsOn;
>+    }
>+
>+    exports.main = function(options, callbacks) {

Where options and callbacks are not used, we should be consistent in docs and examples about using main() or not.  I think so far we have omitted main() in that case.

>+      widget = widgets.Widget({
>+        label: 'Annotator',
>+        contentURL: data.url('widget/pencil-off.jpg'),
>+        contentScriptWhen: 'ready',
>+        contentScriptFile: data.url('widget/widget.js'),
>+        onMessage: function(message) {
>+          if (message == 'left-click') {
>+            console.log('activate/deactivate');
>+            toggleActivation()?
>+                              widget.contentURL=data.url('widget/pencil-on.jpg')
>+                             :widget.contentURL=data.url('widget/pencil-off.jpg');

Don't treat the ternary operator expression as a (assignment) statement.  Use if statements instead, or turn it into a proper assignment statement using the lvalue in the branches and the ternary as its rvalue, like this:

  widget.contentURL = toggleActivation() ?
                      data.url('widget/pencil-on.jpg') :
                      data.url('widget/pencil-off.jpg');

Please fix in the actual code too.

>+Next we'll add the code to
>+[create annotations](#guide/addon-development/annotator/creating).
>\ No newline at end of file

Please add a new line.
Attachment #510881 - Flags: review?(adw) → review+
Thanks Drew!

> I tried out the add-on on mozilla.com.  Lots of elements were highlighted
> yellow when I moused over them, but when I clicked nothing happened.  Is that
> right?

You tried out the complete add-on, from /examples/annotator? That is not right, no. It's more reliable on some pages than on others, but it works pretty reliably for me on mozilla.com. But if it's not working for you it sounds as if there's a problem that needs fixing... 

But in the incomplete version we're at when we add the selector page-mod but haven't plumbed it into the editor panel, then yes (although you should see the console message, too).

> >+Finally, the selector listens for the window's `unload` event and sends the
> >+main add-on code a message called `detach` when unload occurs. This is so the
> >+main add-on can remove the worker associated with this page (eventually,
> >+`page-mod` should have its own `unload` event, but this is the workaround for
> >+the present).
> 
> 
> My comment below about the widget click hack applies here too.
> 

I think there might be a bug for this, and the widget hack. I'll find them, or raise a new one, and refer to it.

> Also, it's kind of weird that we're asking people to download and copy and
> paste code when it's already bundled in the examples directory.  Or do you
> think walking through that process improves learning?  If so, I'm cool with
> that.

That is the idea. Otherwise there's a lot of talking and not very much doing.

> Also, event.preventDefault() doesn't seem to actually prevent the add-on bar's
> context menu from showing.  (Guess that's bug 626326?)

Yup.
 
> Where options and callbacks are not used, we should be consistent in docs and
> examples about using main() or not.  I think so far we have omitted main() in
> that case.

So for my education: is getting the options and callbacks args the only practical difference between using main() and just running your script in module scope?
(In reply to comment #9)
> You tried out the complete add-on, from /examples/annotator?

Yes.  Today I fetched the latest SDK tip (ab41edc378595fc30623b9129d2176c48667a3ac at the time), git-apply'ed your patch, and cfx-run'ed from examples/annotator.  There are no errors in the error console or terminal when I click a highlighted element or otherwise.  In case it matters, by mozilla.com I mean http://www.mozilla.com/en-US/firefox/.  When I try with http://blog.mozilla.com/addons/2011/02/04/overview-amo-review-process/, which is in your screenshots, everything works as advertised.  I'm running the latest Firefox nightly: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.0b12pre) Gecko/20110215 Firefox/4.0b12pre

> So for my education: is getting the options and callbacks args the only
> practical difference between using main() and just running your script in
> module scope?

Yeah, pretty much (except I'd say "doing your stuff" instead of "running your script").  You can see what happens here, in the harness when your add-on starts up:

https://github.com/mozilla/addon-sdk/blob/master/python-lib/cuddlefish/app-extension/components/harness.js#L314

It does a require("main"), which evaluates your main.js.  The next thing it does is call main() if you've exported it, passing options and callbacks.  main() used to be required, but one day Atul noticed there's no fundamental need for it (there's a bug comment somewhere), so he added that if-statement and made it optional.  Hypothetically we could move the call to main() someplace else, e.g., so it's deferred.

(This kind of question is a great candidate for a doc on scope and such that I mentioned in bug 633860 comment 7.)
(In reply to comment #10)
> (In reply to comment #9)
> > You tried out the complete add-on, from /examples/annotator?
> 
> Yes.  Today I fetched the latest SDK tip
> (ab41edc378595fc30623b9129d2176c48667a3ac at the time), git-apply'ed your
> patch, and cfx-run'ed from examples/annotator.  There are no errors in the
> error console or terminal when I click a highlighted element or otherwise.  In
> case it matters, by mozilla.com I mean http://www.mozilla.com/en-US/firefox/.

Ha. http://www.mozilla.com/en-US/about/ works for me, but yes, http://www.mozilla.com/en-US/firefox/ doesn't. Nice.
Will has addressed review comments with subsequent commits on his branch for this work, for which he has issued this pull request:

https://github.com/mozilla/addon-sdk/pull/111

Although we're past the freeze, I haven't had a chance to spin an RC yet, and I'd like to take this work as an exception to our normal rules for changes during a freeze, as it complements the other docs work that landed in this release cycle, and the risk seems relatively well-contained.

Thus a=myk for late landing.

Will: please land this at your earliest opportunity.
Landed as: https://github.com/mozilla/addon-sdk/commit/9b9d8b8946f19cebe7b3b54b7763001a1a76b351

There were a couple of nits that I did not fix in this patch: I didn't regenerate the screenshots, and I didn't address the <br>'s - which are there to make the Markdown converter understand that it's terminating a code block. I will address these, but didn't have time yesterday, and really thought it better to land the patch in this release than not to land it because of those problems.
Will, I was looking over the commits and noticed simpleStorage.storage.array is still used in the OverQuota handler (while simpleStorage.storage.annotations is used elsewhere) in both the doc and add-on code:

https://github.com/mozilla/addon-sdk/blame/master/static-files/md/dev-guide/addon-development/annotator/storing.md
(line 261)

https://github.com/mozilla/addon-sdk/blob/master/examples/annotator/lib/main.js#L217
(In reply to comment #14)
> Will, I was looking over the commits and noticed simpleStorage.storage.array is
> still used in the OverQuota handler (while simpleStorage.storage.annotations is
> used elsewhere) in both the doc and add-on code:

Thanks for catching that. What's the best way to get this fix in?
I checked with Myk in #jetpack and landed here:

https://github.com/mozilla/addon-sdk/commit/34cbc22737ed31eaedf91a8c53afa6316305c235

Also resolving this bug fixed.  Nice work Will!
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0b3
Thanks Drew :-)
You need to log in before you can comment on or make changes to this bug.