1316396 - Reorganize and refactor parent and child helper code

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Kris Maglione [:kmag]]

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 1

[Kris Maglione [:kmag]]

Attachment: Bug 1316396: Part 1 - Reorganize parent, child, common, and test code into more appropriate modules.

Review commit: https://reviewboard.mozilla.org/r/91764/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91764/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1316396: Part 2 - Rename contexts to make it clearer how and where they're used.

Review commit: https://reviewboard.mozilla.org/r/91784/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91784/

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1316396: Part 3 - Clean up the native messaging Port logic.

Review commit: https://reviewboard.mozilla.org/r/91786/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91786/

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8809120 [details]
Bug 1316396: Part 1 - Reorganize parent, child, common, and test code into more appropriate modules.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91764/diff/1-2/

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1316396: Part 4 - Refactor ChildAPIManager and sub-classes.

Review commit: https://reviewboard.mozilla.org/r/91792/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91792/

Comment 6

[Kris Maglione [:kmag]]

Attachment: Bug 1316396: Part 5 - Move MessageManagerProxy to ExtensionUtils, and add support for proxied listeners.

Review commit: https://reviewboard.mozilla.org/r/92068/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92068/

Comment 7

[Kris Maglione [:kmag]]

Attachment: Bug 1316396: Part 6 - Use MessageManagerProxy in parent proxy contexts.

Review commit: https://reviewboard.mozilla.org/r/92070/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92070/

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8809120 [details]
Bug 1316396: Part 1 - Reorganize parent, child, common, and test code into more appropriate modules.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91764/diff/2-3/

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8809147 [details]
Bug 1316396: Part 2 - Rename contexts to make it clearer how and where they're used.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91784/diff/1-2/

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8809148 [details]
Bug 1316396: Part 3 - Clean up the native messaging Port logic.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91786/diff/1-2/

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8809159 [details]
Bug 1316396: Part 4 - Refactor ChildAPIManager and sub-classes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91792/diff/1-2/

Comment 12

[Andrew Swan [:aswan]]

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?

Comment 13

[Kris Maglione [:kmag]]

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

[Andrew Swan [:aswan]]

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

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 8809148 [details]
Bug 1316396: Part 3 - Clean up the native messaging Port logic.

https://reviewboard.mozilla.org/r/91786/#review92070

Comment 16

[Andrew Swan [:aswan]]

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

Comment 17

[Kris Maglione [:kmag]]

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 18

[Andrew Swan [:aswan]]

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?

Comment 19

[Andrew Swan [:aswan]]

Comment on attachment 8809505 [details]
Bug 1316396: Part 6 - Use MessageManagerProxy in parent proxy contexts.

https://reviewboard.mozilla.org/r/92070/#review92082

Comment 20

[Kris Maglione [:kmag]]

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.

Comment 21

[Kris Maglione [:kmag]]

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

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4e31a2cb26bd
https://hg.mozilla.org/mozilla-central/rev/d921a38dd17b
https://hg.mozilla.org/mozilla-central/rev/1731b48caa40
https://hg.mozilla.org/mozilla-central/rev/d5fa01b12572
https://hg.mozilla.org/mozilla-central/rev/e2aa5d3af108
https://hg.mozilla.org/mozilla-central/rev/6469ada96bff