Closed Bug 675371 Opened 8 years ago Closed 8 years ago

load/unload chrome.manifest files for bootstrapped addons automatically

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: davemgarrett, Assigned: darktrojan)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 2 obsolete files)

Bug 564667 added the capability to load/unload a chrome.manifest file dynamically for bootstrapped addons. This follow up bug is for implementing a system to automatically detect chrome.manifest and load/unload as needed it to avoid unnecessary boilerplate code to do so in bootstrap.js files.
I can take this one, I'll have a look on Monday.
Assignee: nobody → colmeiro
Thanks. If possible, it would be nice to have this land before the next milestone so addons that want to use this and support Gecko 8 & 9 won't have to do things differently for each.
I'd use this to make all our langpacks bootstrapped?
(In reply to Axel Hecht [:Pike] from comment #3)
> I'd use this to make all our langpacks bootstrapped?

That is one option, the other is to treat langpacks as a special case like we plan to for dictionaries in bug 591780
(In reply to Dave Townsend (:Mossop) from comment #4)
> (In reply to Axel Hecht [:Pike] from comment #3)
> > I'd use this to make all our langpacks bootstrapped?
> 
> That is one option, the other is to treat langpacks as a special case like
> we plan to for dictionaries in bug 591780

Better still, filed bug 677092 on that.
Hernán, have you had a chance to look at this yet? The next merge is approaching and without this using chrome with bootstrapped addons will have a backwards compatibility issue once this is eventually done.
(In reply to Dave Garrett from comment #6)
> Hernán, have you had a chance to look at this yet? The next merge is
> approaching and without this using chrome with bootstrapped addons will have
> a backwards compatibility issue once this is eventually done.

No, I couldn't look at this properly, I had a busy week. I'm taking a look now and I'll hope to get it before the next merge :)
Attached patch patch (obsolete) — Splinter Review
I've created functions for startup and shutdown of bootstrapped addons. This will also be useful for fixing bug 542385, bug 675372 and friends.
Assignee: colmeiro → geoff
Attachment #564355 - Flags: review?(dtownsend)
What is the need for creating the new methods like that? I'm not sure I see how it will help the bugs you mention.
I actually meant bug 564675, not 542387. And I'm not sure about bug 675372. My theory was that it'd be better to make any required changes for them in one place instead of several. I'm probably just overthinking things again.
Perhaps. I think for now though we can just keep this simple by testing for "startup" and "shutdown" in callBootstrapMethod. You'll find that will not bitrot from bug 591780 quite so badly either ;)

Make that change and I'll go over it again to read the test in more detail but otherwise this looks good.
Attached patch patch v2 (obsolete) — Splinter Review
So, back to my original plan then.
Attachment #564355 - Attachment is obsolete: true
Attachment #564355 - Flags: review?(dtownsend)
Attachment #565400 - Flags: review?(dtownsend)
Comment on attachment 565400 [details] [diff] [review]
patch v2

Review of attachment 565400 [details] [diff] [review]:
-----------------------------------------------------------------

I'd really rather you just added a test for the method name in callBootstrapMethod and called addBootstrappedManifestLocation or removeBootstrappedManifestLocation as appropriate there so we don't have to remember to do it at all call sites
Attachment #565400 - Flags: review?(dtownsend) → review-
Attached patch patch v3Splinter Review
(In reply to Dave Townsend (:Mossop) from comment #13)
> I'd really rather you just added a test for the method name in
> callBootstrapMethod and called addBootstrappedManifestLocation or
> removeBootstrappedManifestLocation as appropriate there so we don't have to
> remember to do it at all call sites

*facepalm* That would be the logical thing to do, and what you asked for. I am an idiot.
Attachment #565400 - Attachment is obsolete: true
Attachment #567278 - Flags: review?(dtownsend)
Oh, and I should say that my new patch was created with -w, for ease of reading.
Attachment #567278 - Flags: review?(dtownsend) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5d9b31e32970
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.