Last Comment Bug 641215 - Improve the SDK support for XUL-based extensions
: Improve the SDK support for XUL-based extensions
Status: RESOLVED WONTFIX
:
Product: Add-on SDK
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
P2 normal (vote)
: ---
Assigned To: Nickolay_Ponomarev
:
:
Mentors:
Depends on: 627209 693421
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-12 07:39 PST by Nickolay_Ponomarev
Modified: 2012-12-13 16:00 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/124# (358 bytes, text/html)
2011-03-18 09:15 PDT, Dietrich Ayala (:dietrich)
dietrich: review-
Details

Description User image Nickolay_Ponomarev 2011-03-12 07:39:31 PST

    
Comment 1 User image Nickolay_Ponomarev 2011-03-12 08:08:57 PST
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 User image Myk Melez [:myk] [@mykmelez] 2011-03-16 10:19:56 PDT
I can review the API changes, and Dietrich can review the code changes.

Dietrich: can you take on this code review?
Comment 3 User image Dietrich Ayala (:dietrich) 2011-03-16 10:24:11 PDT
Yep!
Comment 4 User image Dietrich Ayala (:dietrich) 2011-03-18 09:15:26 PDT
Created attachment 520224 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/124#

Pointer to Github pull-request
Comment 5 User image Dietrich Ayala (:dietrich) 2011-04-25 08:51:32 PDT
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 User image Nickolay_Ponomarev 2011-04-25 10:40:09 PDT
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 User image Nickolay_Ponomarev 2011-04-28 16:44:27 PDT
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 User image Dietrich Ayala (:dietrich) 2011-05-12 13:10:55 PDT
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 User image Nickolay_Ponomarev 2011-05-13 11:33:49 PDT
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	git@github.com:nickolay/addon-sdk.git (fetch)
origin	git@github.com: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 User image Dietrich Ayala (:dietrich) 2011-05-17 18:24:15 PDT
(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 User image Myk Melez [:myk] [@mykmelez] 2011-06-15 12:57:57 PDT
(automatic reprioritization of 1.0 bugs)
Comment 12 User image Nickolay_Ponomarev 2011-07-19 17:38:19 PDT
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 User image Wes Kocher (:KWierso) 2011-09-08 11:18:36 PDT
(Pushing all open bugs to the --- milestone for the new triage system)
Comment 14 User image Irakli Gozalishvili [:irakli] [:gozala] [@gozala] 2012-12-13 16:00:21 PST
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.

Note You need to log in before you can comment on or make changes to this bug.