Closed Bug 559306 Opened 14 years ago Closed 11 years ago

add support for chrome.manifest and chrome dirs for main jetpack package into cfx run and cfx xpi

Categories

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

Product:
Add-on SDK Graveyard
Component:
General
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: rpl, Assigned: evold)

References

(
URL
)

Details

Attachments

(2 files, 2 obsolete files)

optional chrome.manifest, chrome dirs and default preferences support
7.79 KB, patch
myk
: review-
myk
: feedback+
Details | Diff | Splinter Review
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/964
355 bytes, text/html
irakli
: review+
ochameau
: review+
Details 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6
Build Identifier: 

With minimal impact on the current implementation, we can add support
to chrome stuff (only in the main package) copying chrome.manifest and
chrome directories into the testing profile and into the generated
xpi:

* http://github.com/rpl/jetpack-sdk/compare/prototype_mainpkg_chromesupport

This is enough to create XUL applications and extensions without any
restrictions (XUL overlay and locales could be used as in today's XUL
application/extensions),
using a thin layer on old stuff to hook all the rest of code,
organized in jetpack modules.

To reach jetpack module code, developers can use the harness component
as described by Nickolay's documentation page, as in the following
example from a firefox mobile extension I've coded using this patch:

* http://github.com/rpl/whereismycar-firefoxmobile/blob/master/chrome/content/overlayFennec.js
* http://github.com/rpl/whereismycar-firefoxmobile/blob/master/chrome/jetpack/loader.jsm

I think that (with some code cleanup) this patch could help
jetpack-sdk to reach all XUL developers right now, and real world
experiences of a great number of XUL developers could help jetpack to
better cover and extend the existent XULRunner platform.

Reproducible: Always
Comment on attachment 438970 [details] [diff] [review]
jetpack-sdk + chrome (urls/dirs) integration prototype 

Looks good. Luca, can you add a test for this change?

Hm, actually I'm not familiar enough with the cuddlefish core yet to know if the infrastructure is there yet to build a test for this. Maybe check out the existing tests and see if it's possible. If not, the patch looks low-risk, so could probably move ahead and have Atul review it test-less.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Yeah, the Jetpack platform only has convenient testing infrastructure for code in CommonJS modules, and some of the cfx Python code, in part because "easy and fast testing" is one of the foremost features of the developer platform.

But we don't yet have much testing infrastructure outside of CommonJS-land. For this kind of full-on integration testing, I suspect the best solution will be to start using Mozmill to run through the process of installing, using, and uninstalling Jetpack-based XPIs.

I bet Heather Arthur in QA can help us with this. Should I file a bug for it?
Also, it should be noted that the reason Jetpack doesn't build extensions with chrome.manifest files "by default" is because things like XUL overlays and chrome: URL registration isn't "undoable", i.e. one can't make them go away without restarting the process.

This is part of the reason the harness.js XPCOM component is so complicated--it's basically doing some of the work that a chrome.manifest file can do, like mapping resource: URIs to directory paths, but in a way that we know to be undoable.

This means that any Jetpack-based extensions which include chrome.manifest files are automatically ineligible for uninstall-sans-restart (see bug 542385).
(In reply to comment #4)
> Also, it should be noted that the reason Jetpack doesn't build extensions with
> chrome.manifest files "by default" is because things like XUL overlays and
> chrome: URL registration isn't "undoable", i.e. one can't make them go away
> without restarting the process.

I'm aware of this, I prefer a future of undoable extensions, but I've designed
this little patch to simplify the workflow described by Nickolay's documentation page on jetpack/xul integration.

So, XUL developers could start to refactor their extensions into jetpack
modules (and reusable jetpack-only packages if applicable) and use their own chrome stuff in the transition.
  
> This is part of the reason the harness.js XPCOM component is so
> complicated--it's basically doing some of the work that a chrome.manifest file
> can do, like mapping resource: URIs to directory paths, but in a way that we
> know to be undoable.
> 
> This means that any Jetpack-based extensions which include chrome.manifest
> files are automatically ineligible for uninstall-sans-restart (see bug 542385).

This is ok, all extensions with a chrome.manifest will need a restart,
when a developer will remove all chrome stuff from its extension, it could be
finally installed/uninstalled without restarts as any full jetpack extension.
Woot! Good that we're on the same page.

Enabling this kind of "gradual migration" was actually one of my intentions in this reboot of the Jetpack platform, so I'm really glad you're running with it! Thanks for the contribution!

I'll review your code as soon as I can, but right now I need to focus on reviewing the blockers for the upcoming 0.3 release and work on my own blockers, so it'll probably be a few days before I get back to this bug. I hope that's okay.
(In reply to comment #6)
> Woot! Good that we're on the same page.
> 
> Enabling this kind of "gradual migration" was actually one of my intentions in
> this reboot of the Jetpack platform, so I'm really glad you're running with it!
> Thanks for the contribution!
> 
> I'll review your code as soon as I can, but right now I need to focus on
> reviewing the blockers for the upcoming 0.3 release and work on my own
> blockers, so it'll probably be a few days before I get back to this bug. I hope
> that's okay.

sure! no problem, I'm on the "radar" :-)
In addition to chrome.manifest and chrome dirs, this patch may also want to copy legitimate 'defaults' directories as mentioned in bug 561503 comment 19.
I've rebased and tweaked the previous patch and added support to an optional default preferences.

I've implemented a very simple scenario to support default preferences in a jetpack package:

* only the main jetpack package can contain a default preferences file (so we don't have complex [and negative] interactions using other jetpack packages)
* only a file preferences can be defined (default_prefs.js into the main package dir)

If we want to cover complex scenarios we need to evaluate the feature more.

In this first iterations this simple scenario could be enough :-) 

P.S. now I clone app-extension and later I copy chrome dirs and other stuff, so at exit we can remove this temporary copy without any risk.
Attachment #438970 - Attachment is obsolete: true
Attachment #443419 - Flags: review?(avarma)
The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Attachment #443419 - Flags: review?(avarma)
What's the status of this bug? I'd like to be able to load a bundled HTML file into a browser tab and have it run as chrome, preferably with access to the addon-kit and api-utils modules.
Priority: -- → P3
Target Milestone: --- → Future
Attachment #443419 - Flags: review?(myk)
Comment on attachment 443419 [details] [diff] [review]
optional hybrid extensions (jetpack-sdk + chrome urls integration prototype + support of a default_prefs.js file)

I looks like we dropped the ball on this patch. :-(  Unfortunately, it no longer applies.

Luca: by any chance are you still interested in working on this?  If so, and you can update your patch, I would love to review and land it!

This functionality is an important part of helping XUL addon developers migrate to the SDK, so it'd be great to have it in the tree.
Attachment #443419 - Flags: review?(myk)
(In reply to comment #12)
> Luca: by any chance are you still interested in working on this?  If so, and
> you can update your patch, I would love to review and land it!

Sure, I'm trying to figure how much is changed in the context.
At a first look it seems to be possible to "fix the patch" in no time,
but I could be wrong.

I'll post some news here asap
I wasn't wrong ;-)

I've just updated the patch to work on the current head on github 
(https://github.com/rpl/addon-sdk/compare/master...prototype_mainpkg_chromesupport)

I don't enable the "chrome legacy" behaviour on "develop-mode" commands
'cause I'm not sure how to handle this case (and I'm not sure it makes sense,
'cause "chrome legacy" extensions need a xulrunner reload to be fully refreshed)
Assignee: nobody → luca.greco
Attachment #443419 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #522324 - Flags: review?(myk)
As far as I understand, this patch adds support for copying a few items if they exist in the root of a jetpack package ('chrome.manifest', 'defaults/preferences/default_prefs.js', and a 'chrome' directory) to the extension template <https://github.com/mozilla/addon-sdk/tree/master/python-lib/cuddlefish/app-extension> before running the application or building an XPI.

That's not quite a complete list of what an extension can contain (see https://developer.mozilla.org/en/Bundles ), and it makes the developer put all the extra files in 'chrome/' subdirectory (updating the chrome.manifest to point into chrome/ when converting an existing extension). Also copying some of the files, but not others, is unintuitive.

Compare this with the approach I've taken in bug 641215: specify the name of a whole directory containing the template in the templatedir key of package.json.

My implementation in bug 641215 is not at all perfect since I force the developer to copy harness.js into their template (and to keep it up to date with future versions of the SDK), but I think it's a step in the right direction.
(In reply to comment #15)
> That's not quite a complete list of what an extension can contain (see
> https://developer.mozilla.org/en/Bundles ), and it makes the developer put all
> the extra files in 'chrome/' subdirectory (updating the chrome.manifest to
> point into chrome/ when converting an existing extension).

It's ok for the patch to solve only a subset of the overall problem.  And it isn't clear to me why it makes developers put all extra files in chrome/.  It seems to me that what it does, rather, is allow developers who have chrome/ files, a chrome.manifest file, and default preferences in a XUL addon to use the SDK to build a XPI for it.


> Also copying some of
> the files, but not others, is unintuitive.

Yes, there's more work to do here, but let's not let the perfect be the enemy of the good.


> Compare this with the approach I've taken in bug 641215: specify the name of a
> whole directory containing the template in the templatedir key of package.json.

These seem like different features, even if they can accomplish the same goal, since the purpose of a template is to provide scaffolding for multiple addons, while the purpose of Luca's patch is to provide a way to bundle a specific addon.
Comment on attachment 522324 [details] [diff] [review]
optional chrome.manifest, chrome dirs and default preferences support

Overall, I like this patch.  The only significant issue I see is the way it treats the copying of default preferences.  The primary use case for this functionality is to help XUL addon developers migrate to the SDK, and XUL addons typically store default prefs in the defaults/preferences/ directory, which they have been doing for many years.

Rather than establish a new location and filename for that file and make developers with existing XUL addons move the file to that location, which changes longstanding convention and requires developers to learn a new set of habits, this patch should handle that task on their behalf by copying the defaults/preferences/ directory and all the files it contains to the same location in the XPI.

A minor issue is that addons that use this functionality aren't explicitly identified.  It would make sense to add a boolean flag such as "xul" to the package manifest that identifies a package as representing a hybrid XUL/SDK addon and only invoking this functionality when that flag is present.

Also note that chrome/ is a typical location for chrome files, but it isn't the only one.  In particular, some XUL addons (like Snowl <http://hg.mozilla.org/labs/snowl/file/>) put their chrome files in top-level content/, locale/, and skin/ directories.

So I wonder if it would make more sense to copy all directories that aren't SDK-managed (when the "xul" flag is specified in the package manifest, anyway) instead of chrome/ specifically (which is still different from Nickolay's patch to permit specification of a template directory).

I'd also like to get Brian's review on the integration with the packaging code, as he is more familiar with it.
Attachment #522324 - Flags: review?(warner-bugzilla)
Attachment #522324 - Flags: review?(myk)
Attachment #522324 - Flags: review-
Attachment #522324 - Flags: feedback+
> since the purpose of a template is to provide scaffolding for multiple addons

This may be the original design, but in bug 641215 it's just a directory serving as the extension's skeleton installed into extensions/<id>/ (along with SDK-managed files from 'lib', 'data', etc). This is not intended to be a different feature from what this patch is providing.

The only difference between that approach and the one you suggest here ("copy all directories that aren't SDK-managed") is where this "template" lives - in a directory of its own or interspersed (no negative connotation intended) with other SDK files. The approach with a separate directory results in a more complex directory tree, but has simpler logic ("copy everything inside the template directory" vs "copy chrome.manifest and all directories, except lib, tests, docs, ...").
(In reply to comment #18)

After digging into Nickolay's pull request, I have to say I mostly agreed with him:
We're trying to solve similar problems

I propose this "merge":
* copy all the content of a "template directory" as in the Nickolay 
  solution
* add a "xul" attribute to the package.json, its value will be the name 
  of this "template dir", or this "xul" attribute is a flag and the "template 
  directory" name will be "xul" (fixed by implementation)
Blocks: 656110
This might be easier with some Fx8 platform fixes, so this might not happen for a while.
Whiteboard: [milestone:1.4]
Whiteboard: [milestone:1.4]
Target Milestone: Future → 1.4
(Pushing all open bugs to the --- milestone for the new triage system)
Target Milestone: 1.4 → ---
Comment on attachment 522324 [details] [diff] [review]
optional chrome.manifest, chrome dirs and default preferences support

Sadly, this no longer applies.
Attachment #522324 - Flags: review?(warner-bugzilla)
Can this be closed in favor something like https://gist.github.com/3493210 ? @Irakli
CCing Irakli so he sees Erik's question from a month ago.
Flags: needinfo?(rFobic)
I don't think it's a solution we can recommend I'm afraid. Writing files to register chrome url's is really dirty solution, although I guess Luca should tell us weather it solves this for him.

Paul also has expressed high interest in having this during work week. I think what we can do is add entry `permissions.chrome` to `package.json` that points to a directory for which chrome.manifest will be generated.

We should also make sure message that this is not something we'll going to support and not something users should use.

I'm also removing priority so we can triage it again and take into account devtools requirements.
Flags: needinfo?(rFobic)
Priority: P3 → --
Attachment #741066 - Flags: review?(rFobic)
Comment on attachment 741066 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/964

I'm blessing the python part of this patch with comments being addressed.
And also giving feedback+++ for such feature implementing that easily!

I let Irakli review JS code and also accept this new user feature in product.
Attachment #741066 - Flags: review+
(In reply to Alexandre Poirot (:ochameau) from comment #30)
> Comment on attachment 741066 [details]
> Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/964
> 
> I'm blessing the python part of this patch with comments being addressed.
> And also giving feedback+++ for such feature implementing that easily!
> 
> I let Irakli review JS code and also accept this new user feature in product.

Thanks for the review Alex! :)

I think I've got those py issues sorted now.
By the way, if we need to use a chrome.manifest for some reason in the future we can hide ours in some other folder to avoid conflicts.
Comment on attachment 741066 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/964

r+ with comments in github addressed.
Attachment #741066 - Flags: review?(rFobic) → review+
crossing fingers.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Doesn't look like this actually landed?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #34)
> crossing fingers.

Oops meant this for bug 858402 ..
Waiting for a response from Irakli in the pull request.
Flags: needinfo?(rFobic)
We talked about this in the meeting today, and decided to land without the permission flag for now, we can add it later if we discover that people have problems with these folders being automatically copied.
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/5544971e4923050be200f36760991c027ed01848
Bug 559306: Allowing chrome.manifest file, and chrome folder for jetpacks

(cherry picked from commit 4922f171502f91a6ac14d5f5a67264d35c84b625)

https://github.com/mozilla/addon-sdk/commit/79995579f165438ec008bae7c1e14c9350ecf17f
Merge pull request #964 from erikvold/559306

Bug 559306: Allow adding a chrome.manifest file and chrome folder r=@gozala r=@ochameau
This doesn't depend on an uplift, so I'm marking it closed now.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Oh, snap, movement on this ticket that I wanted a month ago!  Great to see this land!
Assignee: luca.greco → evold
Flags: needinfo?(rFobic)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: