Last Comment Bug 668493 - Create subdirectory for devtools under browser/
: Create subdirectory for devtools under browser/
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Rob Campbell [:rc] (:robcee)
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on:
Blocks: 703275
  Show dependency treegraph
 
Reported: 2011-06-30 06:31 PDT by Rob Campbell [:rc] (:robcee)
Modified: 2011-11-17 08:42 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
create browser/devtools (2.48 KB, patch)
2011-07-17 14:04 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review

Description User image Rob Campbell [:rc] (:robcee) 2011-06-30 06:31:28 PDT
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 User image Cedric Vivier [:cedricv] 2011-06-30 08:44:01 PDT
Nice. Would it make sense to have it for tests, locales and skin as well?
Comment 2 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-02 07:55:47 PDT
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 User image Rob Campbell [:rc] (:robcee) 2011-07-05 12:29:39 PDT
(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 User image Dão Gottwald [:dao] 2011-07-08 06:31:48 PDT
(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 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-09 19:58:20 PDT
No; we should do that.
Comment 6 User image Rob Campbell [:rc] (:robcee) 2011-07-17 14:04:54 PDT
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.
Comment 7 User image Dão Gottwald [:dao] 2011-07-18 01:13:20 PDT
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 User image Rob Campbell [:rc] (:robcee) 2011-07-18 06:41:43 PDT
(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 User image Michael Ratcliffe [:miker] [:mratcliffe] 2011-07-18 07:20:03 PDT
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 User image Dão Gottwald [:dao] 2011-07-18 10:11:38 PDT
(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 User image Rob Campbell [:rc] (:robcee) 2011-07-18 12:04:18 PDT
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 User image Dão Gottwald [:dao] 2011-07-18 12:14:49 PDT
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 User image Rob Campbell [:rc] (:robcee) 2011-07-18 14:32:04 PDT
I interpreted comment 2 as browser/base/content had "too much stuff already".
Comment 14 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-18 14:42:41 PDT
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 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-18 14:49:08 PDT
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 User image Rob Campbell [:rc] (:robcee) 2011-07-18 14:55:14 PDT
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 User image Dão Gottwald [:dao] 2011-07-18 17:38:33 PDT
(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 User image Rob Campbell [:rc] (:robcee) 2011-07-19 15:49:20 PDT
(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 User image Rob Campbell [:rc] (:robcee) 2011-07-28 05:35:47 PDT
fixed with landing of bug 579909.

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