Last Comment Bug 579909 - Move WebConsole code to browser
: Move WebConsole code to browser
Status: VERIFIED FIXED
[console-2][qa-][testday-20110930]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 8
Assigned To: Rob Campbell [:rc] (:robcee)
:
Mentors:
Depends on:
Blocks: 568647 582596 601890
  Show dependency treegraph
 
Reported: 2010-07-19 07:55 PDT by David Dahl :ddahl
Modified: 2014-04-26 03:29 PDT (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
move hudservice to browser/devtools (134.08 KB, patch)
2011-07-17 14:12 PDT, Rob Campbell [:rc] (:robcee)
dtownsend: review+
bugzilla: review+
dao+bmo: review-
Details | Diff | Review
move hudservice to browser/devtools, rename to webconsole (135.83 KB, patch)
2011-07-19 15:51 PDT, Rob Campbell [:rc] (:robcee)
bugzilla: review+
Details | Diff | Review
move hudservice to browser/devtools, rename to webconsole 2 (134.67 KB, patch)
2011-07-20 12:53 PDT, Rob Campbell [:rc] (:robcee)
nfitzgerald: feedback+
Details | Diff | Review
move hudservice to browser/devtools 3 (134.53 KB, patch)
2011-07-22 07:27 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Review
move hud 4 (134.78 KB, patch)
2011-07-22 08:44 PDT, Rob Campbell [:rc] (:robcee)
gavin.sharp: review+
Details | Diff | Review
[in-fx-team] move-hud-final (134.73 KB, patch)
2011-07-26 08:21 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Review

Description David Dahl :ddahl 2010-07-19 07:55:54 PDT
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
Comment 1 Rob Campbell [:rc] (:robcee) 2010-07-19 08:04:35 PDT
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?
Comment 2 Philip Chee 2010-07-19 08:28:25 PDT
* 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.
Comment 3 David Dahl :ddahl 2010-07-19 08:45:59 PDT
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?
Comment 4 David Dahl :ddahl 2010-07-30 09:29:45 PDT
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.
Comment 5 David Dahl :ddahl 2010-08-09 15:07:17 PDT
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.
Comment 6 Robert Kaiser (not working on stability any more) 2010-09-03 04:56:43 PDT
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.
Comment 7 Rob Campbell [:rc] (:robcee) 2011-07-17 14:12:37 PDT
Created attachment 546440 [details] [diff] [review]
move hudservice to browser/devtools

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]
Comment 8 Rob Campbell [:rc] (:robcee) 2011-07-17 14:13:46 PDT
this'll probably need review from a browser peer as well due to the change in browser/build.mk.
Comment 9 Rob Campbell [:rc] (:robcee) 2011-07-17 14:31:22 PDT
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.
Comment 10 David Dahl :ddahl 2011-07-18 07:30:53 PDT
(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 11 Dão Gottwald [:dao] 2011-07-18 10:02:38 PDT
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.
Comment 12 Rob Campbell [:rc] (:robcee) 2011-07-18 12:00:43 PDT
(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.
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-18 12:04:31 PDT
(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.
Comment 14 Rob Campbell [:rc] (:robcee) 2011-07-18 12:09:22 PDT
ok, that's much easier. Dão, correct us if wrong.
Comment 15 Dão Gottwald [:dao] 2011-07-18 12:11:57 PDT
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 16 Dave Townsend [:mossop] 2011-07-18 14:45:27 PDT
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
Comment 17 Rob Campbell [:rc] (:robcee) 2011-07-18 14:46:46 PDT
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.
Comment 18 Dão Gottwald [:dao] 2011-07-18 17:34:55 PDT
(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.
Comment 19 Rob Campbell [:rc] (:robcee) 2011-07-19 03:24:15 PDT
(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. :)
Comment 20 Rob Campbell [:rc] (:robcee) 2011-07-19 15:51:33 PDT
Created attachment 546932 [details] [diff] [review]
move hudservice to browser/devtools, rename to webconsole
Comment 21 Rob Campbell [:rc] (:robcee) 2011-07-19 15:52:26 PDT
a new patch. Incorporated creation of browser/devtools into this patch.

renamed browser/devtools/hudservice to browser/devtools/webconsole.
Comment 22 David Dahl :ddahl 2011-07-19 16:12:07 PDT
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 23 David Dahl :ddahl 2011-07-19 16:15:51 PDT
Comment on attachment 546932 [details] [diff] [review]
move hudservice to browser/devtools, rename to webconsole

Looks very mechanical:)
Comment 24 Rob Campbell [:rc] (:robcee) 2011-07-19 17:25:23 PDT
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 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-19 18:46:38 PDT
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?)
Comment 26 Rob Campbell [:rc] (:robcee) 2011-07-20 12:53:53 PDT
Created attachment 547209 [details] [diff] [review]
move hudservice to browser/devtools, rename to webconsole 2

here's an updated patch. Nick, could you let me know if this applies for you, then I'll re-ask for review.
Comment 27 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2011-07-20 13:14:02 PDT
Applies perfectly, thanks Rob!
Comment 28 Rob Campbell [:rc] (:robcee) 2011-07-22 07:27:20 PDT
Created attachment 547685 [details] [diff] [review]
move hudservice to browser/devtools 3

Updated, comments addressed.
Comment 29 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-22 07:39:59 PDT
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?
Comment 30 Rob Campbell [:rc] (:robcee) 2011-07-22 08:44:11 PDT
Created attachment 547700 [details] [diff] [review]
move hud 4

whups. forgot to hg add it. Updated. Thanks!
Comment 31 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-22 12:00:23 PDT
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?
Comment 32 Rob Campbell [:rc] (:robcee) 2011-07-22 12:28:49 PDT
oh geez.

It seemed to, though I could have had some leftover build gunk. Let me verify.
Comment 33 Rob Campbell [:rc] (:robcee) 2011-07-22 13:44:58 PDT
(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)
Comment 34 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-22 14:21:27 PDT
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.
Comment 35 Rob Campbell [:rc] (:robcee) 2011-07-22 14:40:04 PDT
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 36 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-23 19:49:09 PDT
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.
Comment 37 Rob Campbell [:rc] (:robcee) 2011-07-25 15:03:36 PDT
pushed to try. green tree.

http://tbpl.mozilla.org/?tree=Try&rev=0f75ef7a4fb6
Comment 38 Rob Campbell [:rc] (:robcee) 2011-07-26 08:21:11 PDT
Created attachment 548466 [details] [diff] [review]
[in-fx-team] move-hud-final

final patch, suitable for pushing.
Comment 39 Rob Campbell [:rc] (:robcee) 2011-07-26 10:33:46 PDT
Comment on attachment 548466 [details] [diff] [review]
[in-fx-team] move-hud-final

http://hg.mozilla.org/integration/fx-team/rev/fe060dc4ee3d
Comment 40 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-28 09:56:19 PDT
http://hg.mozilla.org/mozilla-central/rev/eee7a94c44e2
Comment 41 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-30 15:29:56 PDT
Verified by checking out mozilla-central and verifying webconsole code is in place.

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