751226 - Refactor all the existing browser actor implementations to eliminate duplication

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v1

Work-in-progress that seems to work for B2G.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: 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

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 5

[Rob Campbell [:rc] (:robcee)]

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.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

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!

Comment 8

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/316c7d02fa82

Comment 9

[Paul Rouget [:paul]]

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

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

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