Create subdirectory for devtools under browser/

RESOLVED FIXED

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: rc, Assigned: rc)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.
Nice. Would it make sense to have it for tests, locales and skin as well?
Let's not put it under browser/base/content, that's already far too cluttered (running browser-chrome tests there takes forever). browser/devtools ?
(Assignee)

Comment 3

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

Updated

6 years ago
Summary: Create subdirectory for devtools under browser/base/content → Create subdirectory for devtools under browser/
(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/?
No; we should do that.
(Assignee)

Comment 6

6 years ago
Created attachment 546439 [details] [diff] [review]
create browser/devtools

first attempt. No contents yet. Just creates the directory and adds a sub-directory, for tests.
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Attachment #546439 - Flags: review?(gavin.sharp)
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.
(Assignee)

Comment 8

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

Comment 11

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

Comment 13

6 years ago
I interpreted comment 2 as browser/base/content had "too much stuff already".
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!
Attachment #546439 - Flags: review?(gavin.sharp)
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.
(Assignee)

Comment 16

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

Comment 18

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

Comment 19

6 years ago
fixed with landing of bug 579909.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Blocks: 703275
You need to log in before you can comment on or make changes to this bug.