This bug is for a discussion about *possibly* moving the WebConsole (former HeadsUpDisplay) code to browser. bsmedberg thinks this is a good idea as we really have a lot of Fx-specific code and no tests for any other gecko apps: <bsmedberg> ddahl: regarding bug 568647, why does the HUD code live in toolkit/ at all? <ddahl> bsmedberg: we have always wanted to support any app that uses toolkit <bsmedberg> ddahl: why? <bsmedberg> ddahl: do we have tests for that configuration? <ddahl> bsmedberg: after talking to mfinkle and others we thought it would be handy for fennec etc <bsmedberg> ddahl: it seems to me that the better solution might be to give each app that cares a copy of the code <ddahl> bsmedberg: we do not have the tests for that <bsmedberg> ddahl: in any case, I'm trying to triage that blocker request <bsmedberg> ddahl: and given the lack of tests, I think you'd be better off moving the code entirely to browser, rather than taking the split-up patch

Pros: * easier to implement if we don't genericize about additional consumers * current code tests browser only, no external platforms are using the tests * can likely extract a minimal "hudservice" from browser code later if needed Cons: * requires a bunch of hg moves and some makefile updates * promotes copy and paste reuse in other platforms who want to use these features * potential for code-drift and forking (maybe a pro?) in consumers who copy browser code What am I missing?

* Duplication of effort. * Someone on our side will have to keep a watch and keep porting patches over regularly when they can better use their development effort elsewhere. Multiply for each app that wants to clone this feature.

perhaps we can create a ac_add_option that turns off the browser tests? or maybe we move this into browser until we figure out all of the changes needed to make it a real toolkit component?

I spoke with rob strong about this as well, he concurs with bsmedberg. The second iteration with more of a focus on being app-agnostic will result in a cleaner implementation.

So we should figure out if this is possible (and a good idea to do) before release. Need to cc some browser owners as well.

As I know doing this in the rush to getting a release ready is probably asking too much, I filed bug 593328 for the easy intermediate step to only build it for FF.

first attempt, work in progress. One test failing after renaming the tests subdirectory to test (to be more inline with the rest of browser) and copying the test files into tests/ instead of browser to make the paths a little better. failing test is here: browser/devtools/hudservice/test/browser/browser_webconsole_bug_595223_file_uri.js TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/hudservice/test/browser/browser_webconsole_bug_595223_file_uri.js | Exception thrown - [Exception... "File error: Not found" nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)" location: "JS frame :: chrome://mochitests/content/browser/browser/devtools/hudservice/test/browser/head.js :: addTab :: line 65" data: no]

this'll probably need review from a browser peer as well due to the change in browser/build.mk.

Another potential issue, I've left the webConsole.css files in toolkit/themes/global. We'll probably want those moved to browser/themes as well.

(In reply to comment #9) > Another potential issue, I've left the webConsole.css files in > toolkit/themes/global. We'll probably want those moved to browser/themes as > well. Yep. We should do this move wholesale.

Comment on attachment 546440 [details] [diff] [review] move hudservice to browser/devtools >+tier_app_dirs += browser/devtools/hudservice Yuck! Don't want a "hudservice" in browser. Let's please use a reasonable name for this. This is the right opportunity to clean this up.

(In reply to comment #11) > Comment on attachment 546440 [details] [diff] [review] [review] > move hudservice to browser/devtools > > >+tier_app_dirs += browser/devtools/hudservice > > Yuck! Don't want a "hudservice" in browser. Let's please use a reasonable > name for this. This is the right opportunity to clean this up. I resisted the urge to try to make this patch less invasive than it already is. mxr returns 384 matching instances of "hudservice" in 107 files. This patch will be huge with all of those. Should I change all of them as well? It should be a fairly mechanical search and replace, but it going to be hell on any downstream patches. There are at least a few of them in play.

(In reply to comment #12) > > Yuck! Don't want a "hudservice" in browser. Let's please use a reasonable > > name for this. This is the right opportunity to clean this up. > mxr returns 384 matching instances of "hudservice" in 107 files. This patch > will be huge with all of those. I think Dao's just referring to the directory name - since we're moving everything anyways, it doesn't hurt to rename the directory. We can work on the code itself in separate steps.

ok, that's much easier. Dão, correct us if wrong.

Yes, there can be separate steps. > It should be a fairly mechanical search and replace, but it going to be hell on > any downstream patches. There are at least a few of them in play. If a mechanical search and replace is feasible, maybe it could be executed on existing diffs as well? People would need to clear their mercurial queues and reapply the patches.

Comment on attachment 546440 [details] [diff] [review] move hudservice to browser/devtools Review of attachment 546440 [details] [diff] [review]: ----------------------------------------------------------------- I'm fine with the moves, but yeah get a browser peer to sign off on it too

no, existing diffs don't have all of the references to it. Need full file contents of every test, etc. I'd prefer to do that in a follow-up.

(In reply to comment #17) > no, existing diffs don't have all of the references to it. Need full file > contents of every test, etc. I don't understand this comment.

(In reply to comment #18) > (In reply to comment #17) > > no, existing diffs don't have all of the references to it. Need full file > > contents of every test, etc. > > I don't understand this comment. Oh, nevermind. I misread your comment 15. You were referring to "other people's patches" when I thought you meant applying the search and replace to the patch in this bug. If still confused, disregard my comment. :)

a new patch. Incorporated creation of browser/devtools into this patch. renamed browser/devtools/hudservice to browser/devtools/webconsole.

Comment on attachment 546440 [details] [diff] [review] move hudservice to browser/devtools We can do a followup renaming patch (yay). do we have a bug for that?

Comment on attachment 546932 [details] [diff] [review] move hudservice to browser/devtools, rename to webconsole Looks very mechanical:)

Nick Fitzgerald says this won't apply for him against fx-team. It turns out, I wrote it against the devtools repo which I thought was closer to mozilla-central than it was. I'll have to migrate this patch to fx-team and will post the update tomorrow. Thanks for the review, David!

Comment on attachment 546932 [details] [diff] [review] move hudservice to browser/devtools, rename to webconsole >diff --git a/browser/base/jar.mn b/browser/base/jar.mn >++ content/browser/NetworkPanel.xhtml (/browser/devtools/webconsole/NetworkPanel.xhtml) This doesn't really belong here any more (not an override). You could just add a jar.mn under browser/devtools. >diff --git a/browser/build.mk b/browser/build.mk >-tier_app_dirs += toolkit/components/console/hudservice >+tier_app_dirs += browser/devtools/webconsole This isn't needed anymore - just remove it (seems like it would currently cause browser/devtools/webconsole to be rebuilt twice for a full build?)

here's an updated patch. Nick, could you let me know if this applies for you, then I'll re-ask for review.

Applies perfectly, thanks Rob!

Updated, comments addressed.

Comment on attachment 547685 [details] [diff] [review] move hudservice to browser/devtools 3 don't see a jar.mn added for NetworkPanel.xhtml, which seems like it'd break stuff?

whups. forgot to hg add it. Updated. Thanks!

Comment on attachment 547700 [details] [diff] [review] move hud 4 >diff --git a/browser/devtools/jar.mn b/browser/devtools/jar.mn >+devtools.jar: Shouldn't this be browser.jar? Does this patch actually work as-is?

oh geez. It seemed to, though I could have had some leftover build gunk. Let me verify.

(In reply to comment #31) > Comment on attachment 547700 [details] [diff] [review] [review] > move hud 4 > > >diff --git a/browser/devtools/jar.mn b/browser/devtools/jar.mn > > >+devtools.jar: > > Shouldn't this be browser.jar? Does this patch actually work as-is? yeah, it does work. In the Nightly.app bundle, there's a devtools/browser... directory under chrome which should get jarred up. Want me to move into browser.jar anyway? Probably don't need a separate jar file for one entry, though later on we might want to split them. (then again, maybe that'd hurt startup time)

Nothing registers the content package for devtools.jar, so I don't see how it could work (are you sure it isn't just working because browser.jar still has a leftover copy from previous patches?). But yeah, either way it should be browser.jar.

pretty sure I just ran from a full rebuild, but who knows? So, just changing the devtools.jar to browser.jar will put it in the right jar file?

Comment on attachment 547700 [details] [diff] [review] move hud 4 yes, that should work. the path to the file should be relative, too ("webconsole/NetworkPanel.xhtml"). I don't think the MODULE in devtools/webconsole/test/Makefile.in actually does anything useful r=me with those addressed.

pushed to try. green tree. http://tbpl.mozilla.org/?tree=Try&rev=0f75ef7a4fb6

final patch, suitable for pushing.

Comment on attachment 548466 [details] [diff] [review] [in-fx-team] move-hud-final http://hg.mozilla.org/integration/fx-team/rev/fe060dc4ee3d

Verified by checking out mozilla-central and verifying webconsole code is in place.