Last Comment Bug 751226 - Refactor all the existing browser actor implementations to eliminate duplication
: Refactor all the existing browser actor implementations to eliminate duplication
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: Firefox 17
Assigned To: Panos Astithas [:past]
:
: James Long (:jlongster)
Mentors:
Depends on:
Blocks: 779462
  Show dependency treegraph
 
Reported: 2012-05-02 09:50 PDT by Panos Astithas [:past]
Modified: 2012-08-01 04:54 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (11.96 KB, patch)
2012-07-10 11:47 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v2 (30.44 KB, patch)
2012-07-11 07:18 PDT, Panos Astithas [:past]
rcampbell: review+
mark.finkle: review+
21: review+
Details | Diff | Splinter Review

Description Panos Astithas [:past] 2012-05-02 09:50:00 PDT
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 [:past] 2012-07-10 11:47:56 PDT
Created attachment 640702 [details] [diff] [review]
Patch v1

Work-in-progress that seems to work for B2G.
Comment 2 Panos Astithas [:past] 2012-07-11 07:18:48 PDT
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
Comment 3 Panos Astithas [:past] 2012-07-11 09:05:43 PDT
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 [:past] 2012-07-12 10:40:51 PDT
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) 2012-07-18 07:50:23 PDT
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 [:past] 2012-07-18 08:47:40 PDT
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?) 2012-07-18 14:50:07 PDT
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] 2012-07-25 11:48:49 PDT
https://hg.mozilla.org/integration/fx-team/rev/316c7d02fa82
Comment 9 Paul Rouget [:paul] 2012-07-25 15:10:51 PDT
https://hg.mozilla.org/mozilla-central/rev/316c7d02fa82
Comment 10 Panos Astithas [:past] 2012-08-01 04:54:42 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.