Closed Bug 1316396 Opened 3 years ago Closed 3 years ago

Reorganize and refactor parent and child helper code

Categories

(WebExtensions :: General, defect)

51 Branch
defect
Not set

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(6 files)

A lot of this code is spread out and organized in a way that isn't easy to follow or reason about. It, and some of the code that predates it, needs to be reorganized into modules that make more sense for the current architecture.
Comment on attachment 8809120 [details]
Bug 1316396: Part 1 - Reorganize parent, child, common, and test code into more appropriate modules.

https://reviewboard.mozilla.org/r/91764/#review92058

This looks like a big improvement, thanks.  I know we haven't been doing this consistently, but I think adding a comment at or near the top of each file briefly summarizing what it is intended to include would be helpful.

::: toolkit/components/extensions/Extension.jsm:612
(Diff revision 3)
> -   * and an nsIFile object pointing to it is returned.
> -   *
> -   * @param {object} data
> -   * @returns {nsIFile}
> -   */
>    static generateXPI(data) {

Is the intention to eventually have callers use ExtensionTestCommon directly and remove these?
Attachment #8809120 - Flags: review?(aswan) → review+
(In reply to Andrew Swan [:aswan] from comment #12)
> Comment on attachment 8809120 [details]
> Bug 1316396: Part 1 - Reorganize parent, child, common, and test code into
> more appropriate modules.
> 
> https://reviewboard.mozilla.org/r/91764/#review92058
> 
> This looks like a big improvement, thanks.  I know we haven't been doing
> this consistently, but I think adding a comment at or near the top of each
> file briefly summarizing what it is intended to include would be helpful.
> 
> ::: toolkit/components/extensions/Extension.jsm:612
> (Diff revision 3)
> > -   * and an nsIFile object pointing to it is returned.
> > -   *
> > -   * @param {object} data
> > -   * @returns {nsIFile}
> > -   */
> >    static generateXPI(data) {
> 
> Is the intention to eventually have callers use ExtensionTestCommon directly
> and remove these?

Yeah, or ideally to merge ExtensionTextUtils, ExtensionXPCShellUtils, and ExtensionTestCommon into one module, and get rid of the duplicated code.
Comment on attachment 8809147 [details]
Bug 1316396: Part 2 - Rename contexts to make it clearer how and where they're used.

https://reviewboard.mozilla.org/r/91784/#review92068

+1000000
Attachment #8809147 - Flags: review?(aswan) → review+
Comment on attachment 8809148 [details]
Bug 1316396: Part 3 - Clean up the native messaging Port logic.

https://reviewboard.mozilla.org/r/91786/#review92070
Attachment #8809148 - Flags: review?(aswan) → review+
Comment on attachment 8809159 [details]
Bug 1316396: Part 4 - Refactor ChildAPIManager and sub-classes.

https://reviewboard.mozilla.org/r/91792/#review92072

::: toolkit/components/extensions/ExtensionChild.jsm:546
(Diff revision 2)
>  let nextId = 1;
>  
>  // We create one instance of this class for every extension context
>  // that needs to use remote APIs. It uses the message manager to
>  // communicate with the ParentAPIManager singleton in
>  // Extension.jsm. It handles asynchronous function calls as well as

ExtensionParent.jsm
Attachment #8809159 - Flags: review?(aswan) → review+
Comment on attachment 8809120 [details]
Bug 1316396: Part 1 - Reorganize parent, child, common, and test code into more appropriate modules.

https://reviewboard.mozilla.org/r/91764/#review92058

+1
Comment on attachment 8809504 [details]
Bug 1316396: Part 5 - Move MessageManagerProxy to ExtensionUtils, and add support for proxied listeners.

https://reviewboard.mozilla.org/r/92068/#review92080

This looks good, are users of the proxied listeners capability coming in part 7?
Attachment #8809504 - Flags: review?(aswan) → review+
Comment on attachment 8809505 [details]
Bug 1316396: Part 6 - Use MessageManagerProxy in parent proxy contexts.

https://reviewboard.mozilla.org/r/92070/#review92082
Attachment #8809505 - Flags: review?(aswan) → review+
Comment on attachment 8809504 [details]
Bug 1316396: Part 5 - Move MessageManagerProxy to ExtensionUtils, and add support for proxied listeners.

https://reviewboard.mozilla.org/r/92068/#review92080

I was planning for them to come in part 6, but I changed my mind. :) I think 6 parts are probably enough for this bug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e31a2cb26bdc0147dcf5b6cf9e104ab0a48c80d
Bug 1316396: Part 1 - Reorganize parent, child, common, and test code into more appropriate modules. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/d921a38dd17b69a5dea2bad3366b79576f3751c3
Bug 1316396: Part 2 - Rename contexts to make it clearer how and where they're used. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/1731b48caa40c1753400d6867a6b2fb99dafd60d
Bug 1316396: Part 3 - Clean up the native messaging Port logic. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5fa01b12572b777103c15f55ef09281215f917e
Bug 1316396: Part 4 - Refactor ChildAPIManager and sub-classes. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2aa5d3af1089e05650634b8f24e675ae99a902f
Bug 1316396: Part 5 - Move MessageManagerProxy to ExtensionUtils, and add support for proxied listeners. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/6469ada96bff9afb7f40a7c2762f253fcff49a49
Bug 1316396: Part 6 - Use MessageManagerProxy in parent proxy contexts. r=aswan
Duplicate of this bug: 1314903
Depends on: 1326044
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.