Refactor all the existing browser actor implementations to eliminate duplication

RESOLVED FIXED in Firefox 17

Status

()

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

People

(Reporter: past, Assigned: past)

Tracking

Trunk
Firefox 17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 attachment, 1 obsolete attachment)

We now have functional browser actor implementations for desktop Firefox, Fennec and B2G. My plan was to get these working first, gain some experience, and then see what parts are the same or reusable. We have now reached that point and my current thinking is that we should factor all common parts into a base browser actor object that lives in toolkit/, with additional product-specific bits that live in browser/, mobile/ and b2g/ respectively. This should facilitate building, say SeaMonkey browser actors, if anyone is so inclined. The refactoring planned in bug 740551 should be taken into account as well.
Assignee: nobody → past
Status: NEW → ASSIGNED
Created attachment 640702 [details] [diff] [review]
Patch v1

Work-in-progress that seems to work for B2G.
Created attachment 641049 [details] [diff] [review]
Patch v2

Tested on B2G desktop and device, and desktop Firefox. Still fighting with Fennec build.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=6022b0f66312

The patch removes a sizable chunk of the browser actors for B2G and Fennec, as evidenced by these patch statistics:

 b2g/chrome/content/dbg-browser-actors.js               |  350 +++++++++++---------------------------------
 b2g/chrome/content/shell.js                            |    1 +
 mobile/android/chrome/content/browser.js               |    1 +
 mobile/android/chrome/content/dbg-browser-actors.js    |  454 ++++++++-------------------------------------------------
 toolkit/devtools/debugger/server/dbg-browser-actors.js |   17 +-
 5 files changed, 172 insertions(+), 651 deletions(-)

The changes are mostly these:
- root actors inherit from BrowserRootActor
- tab actors inherit from BrowserTabActor
- added a getTabContainer method to allow inheritance of watch/unwatchWindow
- removed now redundant methods
Attachment #640702 - Attachment is obsolete: true
Comment on attachment 641049 [details] [diff] [review]
Patch v2

This patch breaks fennec debugging. I'll investigate tomorrow, but a quick overview can't hurt.
Attachment #641049 - Flags: feedback?(rcampbell)
Comment on attachment 641049 [details] [diff] [review]
Patch v2

So apparently I wasn't tapping the button in the test page as I thought, and Fennec works fine: pauses, breakpoints, everything. Rob, you can take a more thorough look now, and I'll ping Fennec and B2G people when you're satisfied with it.
Attachment #641049 - Flags: review?(rcampbell)
Attachment #641049 - Flags: feedback?(rcampbell)
Attachment #641049 - Flags: feedback-
Attachment #641049 - Flags: feedback-
Comment on attachment 641049 [details] [diff] [review]
Patch v2

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

I'm ok with these changes, but we should probably have some review for the android and b2g bits.
Attachment #641049 - Flags: review?(rcampbell) → review+
Comment on attachment 641049 [details] [diff] [review]
Patch v2

(In reply to Rob Campbell [:rc] (:robcee) from comment #5)
> I'm ok with these changes, but we should probably have some review for the
> android and b2g bits.

I know just the right people.
Gents, see comment 2 for a short description of the changes in the patch.
Attachment #641049 - Flags: review?(mark.finkle)
Attachment #641049 - Flags: review?(21)
Comment on attachment 641049 [details] [diff] [review]
Patch v2

Just some nits

>diff --git a/mobile/android/chrome/content/dbg-browser-actors.js b/mobile/android/chrome/content/dbg-browser-actors.js

> function createRootActor(aConnection) {
>-  return new BrowserRootActor(aConnection);
>+  return new FennecRootActor(aConnection);

We stay away from "Fennec" prefixed code if possible. I see you use "Device" for b2g. That's fine for us too. Sometimes we use "Mobile" but I think "Device" works better.

Nice code reduction!
Attachment #641049 - Flags: review?(mark.finkle) → review+
Attachment #641049 - Flags: review?(21) → review+
Whiteboard: [land-in-fx-team]

Comment 8

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/316c7d02fa82
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/316c7d02fa82

Updated

5 years ago
Target Milestone: --- → Firefox 17

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 779462
(In reply to Mark Finkle (:mfinkle) from comment #7)
> Comment on attachment 641049 [details] [diff] [review]
> Patch v2
> 
> Just some nits
> 
> >diff --git a/mobile/android/chrome/content/dbg-browser-actors.js b/mobile/android/chrome/content/dbg-browser-actors.js
> 
> > function createRootActor(aConnection) {
> >-  return new BrowserRootActor(aConnection);
> >+  return new FennecRootActor(aConnection);
> 
> We stay away from "Fennec" prefixed code if possible. I see you use "Device"
> for b2g. That's fine for us too. Sometimes we use "Mobile" but I think
> "Device" works better.

This nit was forgotten when landing the patch, so I'll take care of it in bug 779462.
You need to log in before you can comment on or make changes to this bug.