Bug 641215: Improve the SDK support for XUL-based extensions #215

Closed
wants to merge 17 commits into from

5 participants

@nickolay

See https://bugzilla.mozilla.org/show_bug.cgi?id=641215
(a rebased version of #124, since I got fed up with git's merging)

nickolay added some commits Jul 20, 2011
@nickolay nickolay Add an optional --template parameter to 'cfx init', refactor template…
…s.py to store more information about the template, including paths of the files to create
03493d5
@nickolay nickolay Don't load http://www.mozilla.org/ in a tab, since it's slow and prin…
…ts a lot of CSS warnings to the console.
4a910ad
@nickolay nickolay Test sample addons generated by 'cfx init' in cfx testex 2da1430
@nickolay nickolay Refactor code into findTargetCfg, eliminating the need to restart cfx…
… when package.json is updated with a generated id

Also add stderr redirection to findTargetCfg for future tests.
1e344c6
@nickolay nickolay Make 'cfx init' generate the id in package.json.
Nice for regular jetpacks, necessary for non-bootstrapped ones.
320bc16
@nickolay nickolay Add 'templatedir' property to package.json (for use in XUL extensions)
(link to xul-extensions.html added, the file itself is restored in a later commit)
c98c649
@nickolay nickolay Add a XUL-based extension template to cfx init (since we're running '…
…cfx test' on these templates, this also serves as a unit-test for XUL extensions support in the SDK), also test that XUL features work in the XUL extension template by adding including a jsm module in the template.
f7a3e75
@nickolay nickolay Since we support 'old-style' em:id values in package.json 'id's, migh…
…t support UUID-style em:id as well. This helps with switching existing extensions to using the SDK.
4738507
@nickolay nickolay Don't overwrite em:bootstrap value with 'true' in the template's inst…
…all.rdf: the default template has it set to true anyway, and non-bootstrapped extensions need to set it to false.
12e537e
@nickolay nickolay Restore the xul-extensions document (not linked from the sidebar) b0dffbf
@nickolay nickolay Change the name of a method per Dietrich's suggestion. bdf9b4c
@nickolay nickolay Don't overwrite em:bootstrap value with 'true' pt.2:
restore the change to __init__.py.
60ece92
@mykmelez

Our convention for command-line flags that refer to a directory is to end them with "dir", i.e. profiledir, pkgdir, keydir, and the existing templatedir (for the run and xpi commands). In this case, I don't see a simple way to overload the existing templatedir while marking this experimental, and since it is experimental, this name is fine for now. But if we eventually decide to support it, we'll need to address this naming issue.

Mozilla member
@mykmelez

It took me a bit to realize this function opens the target file for writing. It might be worth being more explicit in its name, even though that'll make it really like, i.e. open_target_file_for_writing or perhaps shortenable to open_file_for_writing.

@mykmelez

I suppose I would have figured it out more quickly if I had paid closer attention to this code comment. ;-) I swear I read it!

@mykmelez

Nit: our convention for word delimitation in function names in Python code seems to be underscores rather than CamelCase, so this should be find_target_cfg.

@mykmelez

Nits:

  • addon -> add-on
  • "to -> " to (to put a space between "add-on" and "to")
  • specify the template for the add-on to be created -> the template for the add-on to be created (since this option represents a noun like "pkgdir" rather than a verb like "strip", its description should be a noun phrase instead of a sentence expressing an action).
@mykmelez

Shouldn't --templatedir simply override package.json::templatedir? I could imagine users specifying a templatedir in package.json but then wanting to see what an addon would look like with a different template applied as a one-off using --templatedir.

@mykmelez
Mozilla member

This feature should be marked experimental to give us space to evaluate and modify it before we decide if/when to support it. (In particular, I'm interested in investigating whether we can merge the template and package directories, so developers only have to manage a single directory of files per addon instead of two different directories, and so we don't have two different things in our conceptual model that are both called "templates").

@mykmelez
Mozilla member

Nit: just noticed this now, but templates.py uses two-space indent, whereas our convention for Python files is four-space indent.

@mykmelez
Mozilla member

Aha! Looks like Dietrich had the same idea I did earlier. This name is great. :-)

@mykmelez
Mozilla member

Nit: extension -> add-on (we changed this globally recently, standardizing on a single term.)

Using the SDK With XUL Extensions should identify the functionality it describes as experimental, to give us the space to continue to develop it without prematurely making promises about its behavior and the level of support we're giving it.

@mykmelez
Mozilla member

Sorry for the extreme delay getting to this! I reviewed this code, and overall it's a bunch of useful fixes, including a number that are useful beyond just XUL addon support. Review comments are above, and they're fairly minor issues, so this doesn't seem too hard to land.

However, I'd like @warner to review this too, since it's mostly Python code, and he's our resident snake charmer (@warner: I found it much easier to review this as individual commits than as a diff against the branches' common ancestor).

I'd also like @wbamberg to review the documentation bits, namely changes c98c649 and b0dffbf.

@nickolay
@nickolay

OK, I think I addressed all of your comments, except the --templatedir one (#215 (comment)) and this comment (#215 (comment)). Let me know if there's anything else.

@wbamberg

I made some changes to the docs, mostly formatting changes, and added a short worked example. I also updated the cfx docs, and pushed it all to: https://github.com/wbamberg/jetpack-sdk/tree/xul-addons-doc.

I think it'll be simplest if we land this pull request as it is, then I raise a separate bug for my subsequent documentation edits. Does that work for you too, Nickolay?

@nickolay

Does that work for you too, Nickolay?

Sure! (I couldn't figure out how to see the changes, hope it'll be easier with a proper pull request.)

@warner
Mozilla member
  • initializer(): It might be cleaner if we move initializer() out into
    templates.py altogether, and make it responsible for populating a given
    target directory with a template, passing in a dictionary of parameters
    like name= . (in general, I'm trying to move code out of __init__.py,
    because it's grown into a 750-line monster that's impossible to test, so
    any new code goes into a new file, and anything that gets changed
    significantly gets moved out too)

  • initializer() could be written a bit differently. open_for_writing() is
    a bit uncertain whether it wants to be a separate function (it accepts
    root_path instead of capturing it from the lexical scope) or a closure
    (it captures out), and also whether it exists to write to files or to
    create directories. Maybe something like: https://gist.github.com/1288827
    could be clearer.

  • also EMPTY_FOLDER could be a singleton object (that's usually how I see
    markers and sentinels implemented, with EMPTY_FOLDER = object(). Then you
    check to see if that's what you have with if content is EMPTY_FOLDER).
    Also, it's perfectly reasonable to use None here: for those cases (like
    data/), there are no contents, and the only purpose of the entry is to
    create the parent directories.

  • looks like there's some merge work to be done: we changed __init__.py to
    always build an XPI, and the handling of "jid" changed as a result, so some
    of the code towards the end of that file may need to be rewritten. I might
    suggest taking your current one-big-delta (git diff master...HEAD) and
    applying/reworking each hunk to current master (it might get hard to think
    of this as a basic merge, because of the magnitude of changes on trunk).

  • options.templatedir: we got rid of the OS-X -specific "Test App.app"
    directory on trunk, so it's probably safe to set options.templatedir=
    early, like:

    if "templatedir" in target_cfg:
        if options.templatedir:
            print >>sys.stderr, "The --templatedir option can not be used " \
                                "when package.json specifies 'templatedir'."
            sys.exit(1)
        options.templatedir = os.path.join(target_cfg["root_dir"],
                                           target_cfg["templatedir"])
    else:
        mydir = os.path.dirname(os.path.abspath(__file__))
        options.templatedir = os.path.join(mydir, "app-extension")
  • it'd be nice to remove some of the conditionals that were in __init__.py, for example if we could require an ID all the time

Otherwise looks pretty good. I'm uncertain if, in the long run, I want to see
the templates expressed as code (like in this patch) or as pre-populated
directories (like bug 613630).
The real-directory-on-disk route makes it harder to substitute in things like
name and id, as well as marking empty folders, but we could probably set
up some conventions to work (any instances of the string "SAMPLE_ADDON_NAME"
in the package.json would be replaced, and any file named EMPTY_FOLDER would
cause a directory to be created but not actually get copied over). The
advantage is that 1: the slightly gnarly code added to test_all_examples
could be simpler, and 2: it'd be easier to modify the templates (just edit
files in-place) or set up new ones (just make a copy of your working addon).

@Gozala
Mozilla member

@nickolay Now that sdk loader is in Firefox most of the SDK apis can be easily consumed via standard add-ons, which makes me believe this is obsolete. Feel free to reopen if you think otherwise.

@Gozala Gozala closed this Dec 13, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment