668493 - Create subdirectory for devtools under browser/

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

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

Most of our devtools are currently living under browser/base/content. We should create a subdirectory for our source code to keep it separated. Individual tools should be ported to jsms and will likely require separate bugs.

Comment 1

[Cedric Vivier [:cedricv]]

Nice. Would it make sense to have it for tests, locales and skin as well?

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Let's not put it under browser/base/content, that's already far too cluttered (running browser-chrome tests there takes forever). browser/devtools ?

Comment 3

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

(In reply to comment #2)
> Let's not put it under browser/base/content, that's already far too
> cluttered (running browser-chrome tests there takes forever).
> browser/devtools ?

OK!

(In reply to comment #1)
> Nice. Would it make sense to have it for tests, locales and skin as well?

we should migrate tests under the /devtools directory. I think locales and skin should probably continue to live where they do now.

Comment 4

[Dão Gottwald [:dao]]

(In reply to comment #2)
> Let's not put it under browser/base/content, that's already far too
> cluttered (running browser-chrome tests there takes forever).

Is there a reason not to have e.g. content/tabview/test/ instead of content/test/tabview/?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

No; we should do that.

Comment 6

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

Attachment: create browser/devtools

first attempt. No contents yet. Just creates the directory and adds a sub-directory, for tests.

Comment 7

[Dão Gottwald [:dao]]

As long as some of this stuff is hardwired in base/content/browser.xul/js/css, I don't think we want the code to live in browser/devtools.

Comment 8

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

(In reply to comment #7)
> As long as some of this stuff is hardwired in
> base/content/browser.xul/js/css, I don't think we want the code to live in
> browser/devtools.

Why not? Too separate from the rest of its contents? I would think that we'd want to keep theme contents in the theme directories as they are, but js code could live here.

I also think, for browser integration, it makes sense to add the markup to browser.xul as we do now for most front-end features. This means we still get a browser peer review for adding front-facing features.

For the inspector code, I'm going to create a separate bug to move that into this directory as well.

See bug 579909 for an example of how we're going to move the browser (or in this case, toolkit) related code into the new directory.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I think that it should be clear that tools are to be moved into JSMs. If the code lives in JSMs I don't see any problem with moving them all to the devtools folder.

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #8)
> (In reply to comment #7)
> > As long as some of this stuff is hardwired in
> > base/content/browser.xul/js/css, I don't think we want the code to live in
> > browser/devtools.
> 
> Why not? Too separate from the rest of its contents?

Yes. I don't want to include ../../devtools/inspector.js in browser.js. It would be ok to include devtools/inspector.js.

(In reply to comment #9)
> I think that it should be clear that tools are to be moved into JSMs. If the
> code lives in JSMs I don't see any problem with moving them all to the
> devtools folder.

This seems to follow the current web console setup, which seems rather broken to me.

As a rule of thumb, any code running some specific UI should live in the same context as the DOM. Please don't move as much code as possible to modules. JS code powering some XUL or HTML window can very well live in that window. (The web console should be such a window and reside in an iframe.)

Comment 11

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

for first pass migration, I'm not going to rewrite existing code as JSMs unless I absolutely have to.

As for moving the browser/devtools directory under base/content, I was already asked to move it to the top of browser/ by gavin. You guys should duke it out and let me know where to land it.

Comment 12

[Dão Gottwald [:dao]]

I agree that we don't want to run all devtools tests as part of browser/base/content/test, but that's a non-issue given comment 4 and 5. I'm not sure if Gavin considers browser/base/content cluttered in some other way.

Comment 13

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

I interpreted comment 2 as browser/base/content had "too much stuff already".

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 546439 [details] [diff] [review]
create browser/devtools

I'd rather review this as part of a patch to actually move things - don't really think there's any value in adding the empty directory. I support the idea of this directory, though!

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Sorry, I didn't see the latest comments when I made comment 14.

I need to look in more detail at which code we're thinking of moving. A patch that actually moves things you plan on moving would help with that, I guess.

I don't think including ../../devtools/inspector.js in browser.js is particularly problematic, but perhaps Dao's right that some of that code should stay closer to the rest of the UI code.

Comment 16

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

We've got some patches incoming that are expecting a browser/devtools directory based on the earlier comments.

bug 669999 and bug 670002 for examples.

See bug 579909 for an example of a patch to move code here.

Maybe inspector.js should continue living in browser/base/content. Some of the related jsms could probably move under browser/devtools though.

Comment 17

[Dão Gottwald [:dao]]

(In reply to comment #16)
> We've got some patches incoming that are expecting a browser/devtools
> directory based on the earlier comments.
> 
> bug 669999 and bug 670002 for examples.

Easily fixable. I don't think this is a good enough reason to push for one particular direction.

Comment 18

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

(In reply to comment #17)
> (In reply to comment #16)
> > We've got some patches incoming that are expecting a browser/devtools
> > directory based on the earlier comments.
> > 
> > bug 669999 and bug 670002 for examples.
> 
> Easily fixable. I don't think this is a good enough reason to push for one
> particular direction.

no, but we need to pick one so we know where to target those patches.

Comment 19

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

fixed with landing of bug 579909.