Improve the SDK support for XUL-based extensions

RESOLVED WONTFIX

Status

Add-on SDK
General
P2
normal
RESOLVED WONTFIX
6 years ago
4 years ago

People

(Reporter: Nickolay_Ponomarev, Assigned: Nickolay_Ponomarev)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
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.
Depends on: 627209
I can review the API changes, and Dietrich can review the code changes.

Dietrich: can you take on this code review?
Yep!
Created attachment 520224 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/124#

Pointer to Github pull-request
Attachment #520224 - Flags: review?(dietrich)
Priority: -- → P3
Target Milestone: --- → 1.0
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.)
(Assignee)

Comment 6

6 years ago
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.)
(Assignee)

Comment 7

6 years ago
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 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.
Attachment #520224 - Flags: review?(dietrich) → review-
(Assignee)

Comment 9

6 years ago
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?
(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.
(automatic reprioritization of 1.0 bugs)
Priority: P3 → P2
Target Milestone: 1.0 → 1.1
(Assignee)

Comment 12

6 years ago
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
(Pushing all open bugs to the --- milestone for the new triage system)
Target Milestone: 1.1 → ---
(Assignee)

Updated

6 years ago
Depends on: 693421
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.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.