641215 - Improve the SDK support for XUL-based extensions

Status: RESOLVED WONTFIX

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

Comment 1

[Nickolay_Ponomarev]

Pull request: https://github.com/mozilla/addon-sdk/pull/124

It's about tweaks to the cfx side of the SDK. The functional changes are listed below.

Myk, Brian, Atul, who can review this?

- Add a 'cfx init' mode, which creates a non-restartless ("XUL") extension.
- Make 'cfx testex' test the results of 'cfx init', thus testing that the XUL-based addons work.
- add 'templatedir' key to package.json, so that you don't have to specify it all the time when running cfx with XUL extensions.
- Minor fixes: support the addon id in the UUID form and stop unconditionally writing em:bootstrap=true to install.rdf when generating the XPI.
- Make 'cfx init' always generate the ID instead of deferring the generation until cfx xpi/run and make restarting cfx after the ID is generated unnecessary.
- an update to the XUL extensions page in the docs

The commits are a bit more detailed than that, and even with git reordering patches is hard, so I decided not to do that unless it makes someone's life easier.

Comment 2

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

I can review the API changes, and Dietrich can review the code changes.

Dietrich: can you take on this code review?

Comment 3

[Dietrich Ayala (:dietrich)]

Yep!

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/124#

Pointer to Github pull-request

Comment 5

[Dietrich Ayala (:dietrich)]

Ok, finally getting around to this! Nickolay, is this still valid, or do you have an updated version of the patch?

(FWIW, this would be so much nicer if split into different bugs.)

Comment 6

[Nickolay_Ponomarev]

I don't have an updated patch.

>(FWIW, this would be so much nicer if split into different bugs.)
The patches are dependent on each other, but separate -- you can deal with any first N from the patchset. Feel free to review only the first N patches here, and we can split the rest into a separate bug.

I don't know how to split these changes to make it easier to review, especially since many of these changes only make sense together (even though first N can be applied separately.)

Comment 7

[Nickolay_Ponomarev]

Updated to yesterday's tip, see the pull request.

Thanks for your comments so far (even though I have yet to address them), keep them coming!

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 520224 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/124#

r-, mostly because it needs to be updated for bug 654588. most of the comments on the pull request are minor, so once that stuff gets ironed out, should be ready to go.

also, please ask :warner to review the documentation bits about the best way to load modules. there was some discussion about this on irc today, and iirc brought up some concerns that we might want to note in these docs.

Comment 9

[Nickolay_Ponomarev]

Thanks... I'm not sure if I'll have the time to work on this anytime soon.


By the way, Github confuses me. I see two comments from you on run-tests.js in the pull request (#124), but the patch doesn't make any changes to that file:

$ git remote -v | grep origin
origin	[e-mail]:nickolay/addon-sdk.git (fetch)
origin	[e-mail]:nickolay/addon-sdk.git (push)
$ git diff origin/master origin/bug641215-xul-extensions | grep "+++"
+++ b/python-lib/cuddlefish/__init__.py
+++ b/python-lib/cuddlefish/rdf.py
+++ b/python-lib/cuddlefish/templates.py
+++ b/python-lib/cuddlefish/tests/test_init.py
+++ b/static-files/md/dev-guide/addon-development/package-spec.md
+++ b/static-files/md/dev-guide/addon-development/xul-extensions.md

Did you actually comment on the run-tests.js changes in this pull request?

Comment 10

[Dietrich Ayala (:dietrich)]

(In reply to comment #9)
> Thanks... I'm not sure if I'll have the time to work on this anytime soon.
> 
> 
> By the way, Github confuses me. I see two comments from you on run-tests.js
> in the pull request (#124), but the patch doesn't make any changes to that
> file:

...

> Did you actually comment on the run-tests.js changes in this pull request?

Yeah. Clicking on the "diff" button on Github shows changes to that file in the pull request.

Comment 11

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

(automatic reprioritization of 1.0 bugs)

Comment 12

[Nickolay_Ponomarev]

A rebased version of the pull request is at https://github.com/mozilla/addon-sdk/pull/215

> python-lib/cuddlefish/__init__.py
> @@ -301,17 +310,34 @@ def test_all_examples(env_root, defaults):
>      examples = [dirname for dirname in os.listdir(examples_dir)
>                  if os.path.isdir(os.path.join(examples_dir, dirname))]
>      examples.sort()
> -    fail = False
> -    for dirname in examples:
> -        print "Testing %s..." % dirname
> +
> +    def run_test(pkgdir):

> test pgkdir is not empty?

Not sure why... (Did you mean test that the directory is not empty?)
The directories in examples/ were not checked before the change, and
if the directory we ran 'cfx init' in turns out to be empty, running
'cfx test' will fail, which is the expected outcome.


python-lib/cuddlefish/__init__.py
@@ -388,23 +428,83 @@ def initializer(env_root, args, out=sys.stdout, err=sys.stderr):
     if len(os.listdir(path)) > 0:
         print >>err, 'This command must be run in an empty directory.'
         return 1
-    for d in ['lib','data','tests','docs']:
-        os.mkdir(os.path.join(path,d))
-        print >>out, '*', d, 'directory created'
-    open('README.md','w').write(README_DOC % {'name':addon})
-    print >>out, '* README.md written'
-    open('package.json','w').write(PACKAGE_JSON % {'name':addon})
-    print >>out, '* package.json written'
-    open(os.path.join(path,'tests','test-main.js'),'w').write(TEST_MAIN_JS)
-    print >>out, '* tests/test-main.js written'
-    open(os.path.join(path,'lib','main.js'),'w').write(MAIN_JS)
-    print >>out, '* lib/main.js written'
-    open(os.path.join(path,'docs','main.md'),'w').write(MAIN_JS_DOC)
-    print >>out, '* docs/main.md written'
+
+    def open_target_file(root_path, template_file_path):

> nit: open_for_writing might be a little clearer

done

>> ... keydir ...
> update for bug 654588, here and elsewhere

done

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(Pushing all open bugs to the --- milestone for the new triage system)

Comment 14

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I think this is obsolete now since some of the SDK is already in FF and rest is coming. Feel free to reopen if you think otherwise.