Closed Bug 392251 Opened 17 years ago Closed 17 years ago

Load extensions from appdir/distribution/bundles

Categories

(Toolkit :: Add-ons Manager, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: hello, Assigned: benjamin)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

The goal for these is to "future-proof" for things we might want to do but the ini format won't support. Note that this is *not* meant to be for 'companion' add-ons, those need to end up in the user's profile. I think we should put them in an 'extensions' directory under distribution, just to keep things tidy.
What extension manager features are these going to use (e.g. add-on update, compatibility check, enable / disable, etc.)?
None: they are going to be loaded as extension directories without any compatibility checks and unknown to the EM.
If they aren't going to use any of the EM then why not make this another chrome / components / etc. dir(s)?
That would be fine by me, FWIW. I said 'extensions' in the bug based on my conversation with Benjamin earlier today. However, making them extensions does have the advantage that it'd be easier to drop in e.g. a search plugin.
I talked with Benjamin today about this and his plan is for it to add this to nsXREDirProvider in the same way that enabled extensions are added without these extensions ever touching the Extension Manager. The name of the dir shouldn't be extensions to avoid possible confusion with other extensions directories that are managed by the Extension Manager since people will likely assume that this new directory should behave the same way as EM managed extensions directories. How about bundles for the name of the directory?
(In reply to comment #5) > How about bundles for the name of the directory? Sold. Go go go! ;-)
Attached patch Load directories in distro/bundles, rev. 1 (obsolete) (deleted) — Splinter Review
Attachment #276867 - Flags: review?(mark.finkle)
I don't think I can create a usable testcase for this until we have the XR-based test harness.
Flags: in-testsuite?
Attachment #276867 - Flags: review?(mark.finkle) → review+
reassigning to dmills for actual landing, since I'm away
Assignee: benjamin → thunder
Assignee: thunder → benjamin
Fixed on trunk
Status: NEW → RESOLVED
Closed: 17 years ago
Keywords: dev-doc-needed
Resolution: --- → FIXED
Summary: Load extensions from appdir/distribution/extensions → Load extensions from appdir/distribution/bundles
Backed out due to a Ts regression.
Status: RESOLVED → REOPENED
Keywords: dev-doc-needed
Resolution: FIXED → ---
I found an old bug that I forgot about: we read extensions.ini many times (at least 6 on each startup) and with the first patch we were also enumerating the distribution/bundles directory 5+ times, and enumerating directories is an unpleasant Ts hit. This patch caches the results of reading extensions.ini and enumerating the bundles directory and should improve Ts a bit, rather than hurt it.
Attachment #276867 - Attachment is obsolete: true
Attachment #278637 - Flags: review?(thunder)
Comment on attachment 278637 [details] [diff] [review] Only read extensions.ini once, and only enumerate the directory once, rev. 2 Fix under XRE_EXTENSIONS_DIR_LIST: + LoadDirsIntoArray(mThemeDirectories, + kAppendNothing, directories); Should be: + LoadDirsIntoArray(mExtensionDirectories, + kAppendNothing, directories); Other than that, it looks ok.
Attachment #278637 - Flags: review?(thunder) → review+
Checked in and backed out again due to Ts regression (about 3%). Other than enumerating a nonexistent directory taking 3ms, I can't think of a reason for this. I may try landing rev. 2 without the distribution/bundles chunk sometime tomorrow to see whether that has any effects.
I'm a little worried about this bug. Have you tried doing any profiling locally to see why it's spending more time? It doesn't make sense to me that the latest version is that much more expensive than what was there before.
Oh, check out the version I landed today... especially http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsXREDirProvider.cpp&branch=&root=/cvsroot&subdir=/mozilla/toolkit/xre&command=DIFF_FRAMESET&rev1=1.62&rev2=1.63 ... it introduced a behavior change where appdir/components was being registered twice. I fixed that and the Ts regression went away. I intend to remove the LOAD_DISTRO_BUNDLES ifdefs tomorrow morning.
Ah, I didn't see that go by! Excellent news, thanks :-)
Re-enabled the LOAD_DISTRO_BUNDLES code this morning, no Ts effect. yay.
Status: REOPENED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
Depends on: 408919
Product: Firefox → Toolkit
Keywords: dev-doc-needed
It's not inherently obvious (to me, anyway) where these extensions are being loaded from. I even spent time reading the patch, and still couldn't figure it out. Where in relation to the executable is the bundles directory?
(In reply to comment #19) > It's not inherently obvious (to me, anyway) where these extensions are being > loaded from. I even spent time reading the patch, and still couldn't figure it > out. > > Where in relation to the executable is the bundles directory? If the executable is in <appdir>/firefox.exe then the bundles dir is in <appdir>/distribution/bundles
How about on Mac? in <path to firefox-bin>/distribution/bundles?
(In reply to comment #21) > How about on Mac? in <path to firefox-bin>/distribution/bundles? Should be yes
This is now mentioned here: https://developer.mozilla.org/en/Extension_Packaging#Including_add-ons_in_a_customized_application And on the main Firefox 3.6 for developers page.
(In reply to comment #23) > This is now mentioned here: > > https://developer.mozilla.org/en/Extension_Packaging#Including_add-ons_in_a_customized_application > > And on the main Firefox 3.6 for developers page. FWIW this feature has been there since Firefox 3.0.
Interesting. Wonder why it just came up on my list of 3.6 items. I'll revise the content.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: