706590 - cross add-on modules

Status: RESOLVED WONTFIX

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

There are cases where having application wide modules would be way more convenient / efficient then copies of same modules shipped with each add-on. One good example is described under Bug 644595. Such modules also have a potential of defining clear path for moving parts of sdk to a firefox once they are ready.

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/290

Pointer to Github pull-request

Comment 2

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

This is work in progress patch!

Comment 3

[Alexandre Poirot [:ochameau]]

So it is not ready for review, right?

In current state, it looks like a major rewrite of all codebase in order to use base/namespace ? Such patch is going to be quite huge and scary! 
Do you plan to split such work ?

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Alexandre Poirot (:ochameau) from comment #3)
> So it is not ready for review, right?
> 
> In current state, it looks like a major rewrite of all codebase in order to
> use base/namespace ? Such patch is going to be quite huge and scary! 
> Do you plan to split such work ?

It's actually not as big as it currently looks as it includes changes from the other patches that have their own reviews. Once those land change will be much smaller. That being said, yes I did rewrote part that were depending on the xpcom module I've changed but I tried to keep those changes small.

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Alexandre Poirot (:ochameau) from comment #3)
> So it is not ready for review, right?

Yes I'm still working on it. It does not yet does what description of this bug says. That being said if you'd like to reduce review scope, we could separate required XPCOM module changes into separate bug and review that individually.

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/305

Pointer to Github pull-request

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/305#

Pointer to Github pull-request

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 582096 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/305#

This patch is smaller part of the complete change that should be easier to review.

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 582096 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/305#

I put various comments in the pull request that may or may not need modifications

But something is wrong with Factory.createInstance method that needs to be fixed:
https://github.com/mozilla/addon-sdk/pull/305#r309212

Otherwise you forgot to update xpcom documentation.
I'm not against modifying it in a next step as soon as it lands in the same release.

Comment 10

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/305

Pointer to Github pull-request

Comment 11

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 588831 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/305

Ready for another round!

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 581333 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/290

We had multiple discussions on the pull request,
there is still some items to address:
https://github.com/mozilla/addon-sdk/pull/290/files#r315436
https://github.com/mozilla/addon-sdk/pull/290/files#r315440
https://github.com/mozilla/addon-sdk/pull/290/files#r315443
https://github.com/mozilla/addon-sdk/pull/290/files#r315449

Comment 13

[Alexandre Poirot [:ochameau]]

Comment on attachment 588831 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/305

If you decide to go with solution described here, r+ whatever name you end up choosing  ("from", "for", ...):
https://github.com/mozilla/addon-sdk/pull/305#issuecomment-3579300

Otherwise, please request another review if you use another pattern.

Comment 14

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/aae9630495046e064173948933b9165b38c1dfc3
Merge pull request #305 from Gozala/bug/xpcom-706590

fix bug 706590 - API for implementing XPCOM interfaces r=@ochameau

Comment 15

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

It's not fixed yet, it's just part of this code has landed.