675371 - load/unload chrome.manifest files for bootstrapped addons automatically

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Garrett]

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.

Comment 1

[Hernán Rodriguez Colmeiro (:peregrino)]

I can take this one, I'll have a look on Monday.

Comment 2

[Dave Garrett]

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.

Comment 3

[Axel Hecht [:Pike]]

I'd use this to make all our langpacks bootstrapped?

Comment 4

[Dave Townsend [:mossop]]

(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

Comment 5

[Axel Hecht [:Pike]]

(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.

Comment 6

[Dave Garrett]

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.

Comment 7

[Hernán Rodriguez Colmeiro (:peregrino)]

(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 :)

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: patch

I've created functions for startup and shutdown of bootstrapped addons. This will also be useful for fixing bug 542385, bug 675372 and friends.

Comment 9

[Dave Townsend [:mossop]]

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.

Comment 10

[Geoff Lankow (:darktrojan)]

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.

Comment 11

[Dave Townsend [:mossop]]

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.

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: patch v2

So, back to my original plan then.

Comment 13

[Dave Townsend [:mossop]]

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

Comment 14

[Geoff Lankow (:darktrojan)]

Attachment: patch v3

(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.

Comment 15

[Geoff Lankow (:darktrojan)]

Oh, and I should say that my new patch was created with -w, for ease of reading.

Comment 16

[Geoff Lankow (:darktrojan)]

Attachment: patch v3 for checkin

Comment 17

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5d9b31e32970

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/5d9b31e32970

Comment 19

[Geoff Lankow (:darktrojan)]

Updated:

https://developer.mozilla.org/en/Extensions/Bootstrapped_extensions#Adding_user_interface_with_a_chrome.manifest
https://developer.mozilla.org/en/Firefox_10_for_developers