BrowserRootActor and BrowserTabActor share code in a confusing way

RESOLVED FIXED in Firefox 24

Status

()

Firefox
Developer Tools: Debugger
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

Trunk
Firefox 24
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 738114 [details] [diff] [review]
Clarify sharing of _createExtraActors and _appendExtraActors between BrowserRootActor and BrowserTabActor.

BrowserTabActor grabs BrowserRootActor's _createExtraActors and _appendExtraActors methods and uses them for itself in a confusing way. It's not clear from looking at their definitions in BrowserRootActor that they're going to be used on other actors as well.

The attached patch pulls those methods out, documents their expectations of the actors on which they are used, and attaches them to the actors' prototypes in a way that (hopefully) makes it clearer what's going on. It should have no effect on behavior.

(Try server is unhappy at the moment; will push when possible.)
(Assignee)

Updated

5 years ago
Attachment #738114 - Flags: review?(past)
Comment on attachment 738114 [details] [diff] [review]
Clarify sharing of _createExtraActors and _appendExtraActors between BrowserRootActor and BrowserTabActor.

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

This looks fine to me, but did you make sure that it doesn't break b2g or fennec? Your try run didn't include xpcshell tests, which would have provided an answer for b2g (fennec barely runs any xpcshell tests AFAICT). Can you do fennec builds or do you want me to try it for you?
Attachment #738114 - Flags: review?(past) → review+
(Assignee)

Comment 3

5 years ago
Okay, that try server run seems clean.
(Assignee)

Comment 4

5 years ago
(In reply to Panos Astithas [:past] from comment #2)
> Comment on attachment 738114 [details] [diff] [review]
> Clarify sharing of _createExtraActors and _appendExtraActors between
> BrowserRootActor and BrowserTabActor.
> 
> Review of attachment 738114 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine to me, but did you make sure that it doesn't break b2g or
> fennec? Your try run didn't include xpcshell tests, which would have
> provided an answer for b2g (fennec barely runs any xpcshell tests AFAICT).
> Can you do fennec builds or do you want me to try it for you?

I'll push again with a full set of tests. Shouldn't that cover b2g and fennec?
(Assignee)

Comment 6

5 years ago
(In reply to Panos Astithas [:past] from comment #2)
> Your try run didn't include xpcshell tests, which would have
> provided an answer for b2g

b2g uses dbg-browser-actors.js in xpcshell tests???
(In reply to Jim Blandy :jimb from comment #6)
> (In reply to Panos Astithas [:past] from comment #2)
> > Your try run didn't include xpcshell tests, which would have
> > provided an answer for b2g
> 
> b2g uses dbg-browser-actors.js in xpcshell tests???

Sorry, you are right, we need to do manual testing for both products.
(In reply to Panos Astithas [:past] from comment #7)
> Sorry, you are right, we need to do manual testing for both products.

...and the reason for it is that the only test for the extension API AFAIR is the browser_dbg_globalactor-01.js browser mochitest, which only runs on desktop.
Status: NEW → ASSIGNED
Priority: -- → P2
(Assignee)

Updated

5 years ago
Blocks: 870081
Created attachment 753136 [details] [diff] [review]
Patch v2

Rebased.
Attachment #738114 - Attachment is obsolete: true
Attachment #753136 - Flags: review+
Tested this and bug 863936 on both Fennec and b2g desktop without any issues.
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a330eb80bc3
Flags: in-testsuite-
Target Milestone: --- → Firefox 24
https://hg.mozilla.org/mozilla-central/rev/9a330eb80bc3
Assignee: nobody → jimb
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.