The default bug view has changed. See this FAQ.

Move WebConsole code to browser

VERIFIED FIXED in Firefox 8

Status

()

Firefox
Developer Tools
VERIFIED FIXED
7 years ago
3 years ago

People

(Reporter: ddahl, Assigned: rc)

Tracking

Trunk
Firefox 8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [console-2][qa-][testday-20110930])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Assignee)

Comment 1

7 years ago
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

7 years ago
* 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.
(Reporter)

Comment 3

7 years ago
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?
(Reporter)

Comment 4

7 years ago
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.

Updated

7 years ago
Whiteboard: [kd4b4]
(Reporter)

Comment 5

7 years ago
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.

Updated

7 years ago
Whiteboard: [kd4b4] → [kd4b6]

Comment 6

7 years ago
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.

Updated

7 years ago
Whiteboard: [kd4b6]
Blocks: 601890
(Assignee)

Updated

6 years ago
Blocks: 568647
(Reporter)

Updated

6 years ago
Whiteboard: [console-2]
(Assignee)

Comment 7

6 years ago
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]
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Attachment #546440 - Flags: review?(dtownsend)
Attachment #546440 - Flags: review?(ddahl)
(Assignee)

Comment 8

6 years ago
this'll probably need review from a browser peer as well due to the change in browser/build.mk.
(Assignee)

Comment 9

6 years ago
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.
(Reporter)

Comment 10

6 years ago
(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.
Attachment #546440 - Flags: review-
(Assignee)

Comment 12

6 years ago
(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.
(Assignee)

Comment 14

6 years ago
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
Attachment #546440 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 17

6 years ago
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.
(Assignee)

Comment 19

6 years ago
(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. :)
(Assignee)

Comment 20

6 years ago
Created attachment 546932 [details] [diff] [review]
move hudservice to browser/devtools, rename to webconsole
Attachment #546932 - Flags: review?(gavin.sharp)
Attachment #546932 - Flags: review?(ddahl)
Attachment #546932 - Flags: review?(dao)
(Assignee)

Comment 21

6 years ago
a new patch. Incorporated creation of browser/devtools into this patch.

renamed browser/devtools/hudservice to browser/devtools/webconsole.
(Reporter)

Comment 22

6 years ago
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?
Attachment #546440 - Flags: review?(ddahl) → review+
(Reporter)

Comment 23

6 years ago
Comment on attachment 546932 [details] [diff] [review]
move hudservice to browser/devtools, rename to webconsole

Looks very mechanical:)
Attachment #546932 - Flags: review?(ddahl) → review+
(Assignee)

Comment 24

6 years ago
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?)

Updated

6 years ago
Attachment #546932 - Flags: review?(dao)
(Assignee)

Comment 26

6 years ago
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.
Attachment #546932 - Attachment is obsolete: true
Attachment #547209 - Flags: feedback?(nfitzgerald)
Attachment #546932 - Flags: review?(gavin.sharp)
Applies perfectly, thanks Rob!
(Assignee)

Updated

6 years ago
Attachment #547209 - Flags: review?(gavin.sharp)
Attachment #547209 - Flags: feedback?(nfitzgerald) → feedback+
(Assignee)

Comment 28

6 years ago
Created attachment 547685 [details] [diff] [review]
move hudservice to browser/devtools 3

Updated, comments addressed.
Attachment #547209 - Attachment is obsolete: true
Attachment #547685 - Flags: review?(gavin.sharp)
Attachment #547209 - Flags: review?(gavin.sharp)
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?
(Assignee)

Comment 30

6 years ago
Created attachment 547700 [details] [diff] [review]
move hud 4

whups. forgot to hg add it. Updated. Thanks!
Attachment #547685 - Attachment is obsolete: true
Attachment #547700 - Flags: review?(gavin.sharp)
Attachment #547685 - Flags: review?(gavin.sharp)
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?
(Assignee)

Comment 32

6 years ago
oh geez.

It seemed to, though I could have had some leftover build gunk. Let me verify.
(Assignee)

Comment 33

6 years ago
(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.
(Assignee)

Comment 35

6 years ago
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.
Attachment #547700 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 37

6 years ago
pushed to try. green tree.

http://tbpl.mozilla.org/?tree=Try&rev=0f75ef7a4fb6
(Assignee)

Comment 38

6 years ago
Created attachment 548466 [details] [diff] [review]
[in-fx-team] move-hud-final

final patch, suitable for pushing.
Attachment #546440 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Whiteboard: [console-2] → [console-2][fixed-in-devtools]
(Assignee)

Comment 39

6 years ago
Comment on attachment 548466 [details] [diff] [review]
[in-fx-team] move-hud-final

http://hg.mozilla.org/integration/fx-team/rev/fe060dc4ee3d
Attachment #548466 - Attachment description: move-hud-final → [in-fx-team] move-hud-final
http://hg.mozilla.org/mozilla-central/rev/eee7a94c44e2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Hardware: x86 → All
Resolution: --- → FIXED
Whiteboard: [console-2][fixed-in-devtools] → [console-2]
Target Milestone: --- → Firefox 8
Blocks: 582596

Updated

6 years ago
Whiteboard: [console-2] → [console-2][qa-]
Verified by checking out mozilla-central and verifying webconsole code is in place.
Status: RESOLVED → VERIFIED
Whiteboard: [console-2][qa-] → [console-2][qa-][testday-20110930]
You need to log in before you can comment on or make changes to this bug.