Closed Bug 699575 Opened 13 years ago Closed 13 years ago

move browser modules to browser/modules

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 11

People

(Reporter: Gavin, Assigned: Gavin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

updated patch
9.28 KB, patch
Details | Diff | Splinter Review
I'd like to encourage the use of modules for Firefox code. Some existing modules have been put in browser/base/content, I think mostly for convenience (it's an existing directory), but there's no reason they need to be there, and it'd be nice to make that directory a little less of a all-purpose dumping ground. I'm only moving the modules in browser/base/content because they're not particularly tied to UI or other larger components (as with the other modules that live under browser/components with other related code). The idea is for browser/modules to be a place to put standalone modules that don't have any particular dependencies to other large pieces of code, like the modules I'm proposing in bug 699573.
Attached patch patch (obsolete) — — Splinter Review
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #571774 - Flags: review?(dao)
One downside of browser/modules/ is that it may look like any browser module should go there, whereas I think the intention is that e.g. devtools modules remain in browser/devtools/. One way to address this could be to use browser/base/modules/ instead. browser/components/wintaskbar/WindowsJumpLists.jsm and browser/components/wintaskbar/WindowsPreviewPerTab.jsm should probably move as well, although this could be a followup bug.
(In reply to Dão Gottwald [:dao] from comment #2) > One downside of browser/modules/ is that it may look like any browser module > should go there, whereas I think the intention is that e.g. devtools modules > remain in browser/devtools/. One way to address this could be to use > browser/base/modules/ instead. I'm not so worried about that. The reviewers all know where the code goes, and we educate via review and other communication. I favor as shallow and uncomplicated a filesystem layout as possible.
My point was that browser/base/modules/ would probably be more self-explanatory and avoid getting people confused; requiring peers to tell people how it's supposed to be done isn't ideal. What may initially seem uncomplicated could in a way be more complicated.
Yeah, and I'm saying I disagree :) I think putting things behind base unnecessarily hides where the modules are. My point about reviewers is that if a patch comes in that puts modules in there that shouldn't go there, the reviewer will catch it.
(In reply to Dietrich Ayala (:dietrich) from comment #5) > Yeah, and I'm saying I disagree :) Do you disagree that browser/base/modules/ is more self-explanatory? > I think putting things behind base > unnecessarily hides where the modules are. Well, I guess this was kind of intentional in my proposal. There's no "the modules". There are some modules that fit in there and others that don't. > My point about reviewers is that > if a patch comes in that puts modules in there that shouldn't go there, the > reviewer will catch it. Reviewers may or may not catch things and the group of reviewers is expanding.
(In reply to Dão Gottwald [:dao] from comment #6) > (In reply to Dietrich Ayala (:dietrich) from comment #5) > > Yeah, and I'm saying I disagree :) > > Do you disagree that browser/base/modules/ is more self-explanatory? yes. i think "base" is ambiguous and really isn't that different than having /browser/modules because of that, so is unnecessarily obfuscating. > Well, I guess this was kind of intentional in my proposal. There's no "the > modules". There are some modules that fit in there and others that don't. i like having a central point for as many modules as possible. the names should explain what they're for. i guess i'm not clear on why specific modules in /browser wouldn't belong here, so maybe that's actually where we're diverging... > Reviewers may or may not catch things and the group of reviewers is > expanding. that's a problem with our reviewers, if they don't know. also, they can ask anyone. and if something lands in the wrong place, we can just move it.
> i like having a central point for as many modules as possible. the names > should explain what they're for. i guess i'm not clear on why specific > modules in /browser wouldn't belong here, Because we group pieces of code primarily based on what feature they belong to. See the various devtools modules, none of which belong in browser/modules/, as they are bundled with code that isn't a module.
I think the "base" in browser/base/modules would be kind of redundant. I don't think it's likely that people will be tempted to put modules tied to other code there solely because the directory is called "modules" (particularly when there are other examples of modules that don't live there). Good point re: the wintaskbar modules, I didn't even realize those were standalone. I can move them as well.
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9) > I think the "base" in browser/base/modules would be kind of redundant. I > don't think it's likely that people will be tempted to put modules tied to > other code there solely because the directory is called "modules" > (particularly when there are other examples of modules that don't live > there). I would have believed that if Dietrich's repeated misinterpretation ("where the modules are", "i'm not clear on why specific modules in /browser wouldn't belong here") hadn't proven the opposite :/
This problem can exist no matter where we put the "modules" directory, so I don't think it makes sense to block on it. We can always move modules and deal with any issues after the fact.
Comment on attachment 571774 [details] [diff] [review] patch (In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9) > I think the "base" in browser/base/modules would be kind of redundant. I > don't think it's likely that people will be tempted to put modules tied to > other code there solely because the directory is called "modules" > (particularly when there are other examples of modules that don't live > there). Dietrich was tempted, and not coincidentally -- his reasoning seems to perfectly confirm my original concern. I think we can count on contributors who aren't as close to the code getting confused even more easily. Sure, we can r- those patches and move stuff when it slips through, but we could also try to avoid it from the beginning. I already said all this, it's your call at this point.
Attachment #571774 - Flags: review?(dao) → review+
Oops, I thought comment 9 was new, when it's in fact the same comment I replied to earlier. (In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #11) > This problem can exist no matter where we put the "modules" directory Yes, it can. My proposal was meant to mitigate this; it's not the silver bullet.
Using "browser/modules" WFM, I also don't think "base" helps explain things. I would, in fact, support just moving everything under /browser/base to /browser. [Which is just content/, a Makefile, and a jar.mn.] All your base are belong to me.
> I also don't think "base" helps explain things. Well, sure. "base" itself doesn't explain much. By "more self-explanatory" I meant "more self-explanatory in that it's not as wrongly self-explanatory as browser/modules/". It's a counter measure -- or obfuscation as Dietrich put it -- against browser/modules/ saying "put any module in here" (similarly to browser/components/, browser/themes/, browser/locales/ etc. *rightfully* conveying such meanings).
(In reply to Justin Dolske [:Dolske] from comment #14) > I would, in fact, support just moving everything under /browser/base to > /browser. +1, I never understood why we have this base/ subfolder... what about filing a bug to do that?
So if you really want a top-level "type of file" hierarchy, I'd suggest we follow it through and do what Dietrich thought this bug was about. E.g. we should then split browser/devtools into browser/content/devtools and browser/modules/devtools.
(In reply to Dão Gottwald [:dao] from comment #17) > So if you really want a top-level "type of file" hierarchy, I'd suggest we > follow it through and do what Dietrich thought this bug was about. E.g. we > should then split browser/devtools into browser/content/devtools and > browser/modules/devtools. I think that 1) shallower filesystems will always be more accessible to contribution and 2) disambiguation is solved by good file-naming. What I was alluding to in comment #7 is that I think all modules of any kind could go into browser/modules and browser/content, etc.
(In reply to Dietrich Ayala (:dietrich) from comment #18) I'm not sure if you just supported what I said or suggested something slightly different. Are you saying that you'd prefer moving devtools modules to browser/modules rather than browser/modules/devtools? And that we should get rid of the content sub folders as well? This sounds like a huge mess to me.
(In reply to Marco Bonardo [:mak] from comment #16) > +1, I never understood why we have this base/ subfolder... what about filing > a bug to do that? Good idea, please CC me!
(In reply to Justin Dolske [:Dolske] from comment #20) > (In reply to Marco Bonardo [:mak] from comment #16) > > > +1, I never understood why we have this base/ subfolder... what about filing > > a bug to do that? > > Good idea, please CC me! It can't hurt to get that bug filed. However, fwiw, I won't hesitate to r- a patch when it comes up as long as the above questions aren't answered. We're in no rush to do this and people should be on the same page as to how these directories should be used.
fwiw, I already expressed my doubt regarding devtools positioning, the fact they are at the same level of the base browser code makes no sense when you have a components/ folder and those are components (even if not in the classic xpcom components meaning). In browser/ there should just be base browser stuff, additional functionality should be in components (components/hightlighter, components/scratchpad ...). At that point I would hardly see someone putting stuff in browser/modules or browser/content unless it's for base browser functionality since there is no "doubts" where additional components are.
PS: the same is valid for fuel, I have no hate vs devtools clearly :)
Attached patch updated patch — — Splinter Review
Includes the move of the wintaskbar modules.
Attachment #571774 - Attachment is obsolete: true
(In reply to Dão Gottwald [:dao] from comment #19) > (In reply to Dietrich Ayala (:dietrich) from comment #18) > I'm not sure if you just supported what I said or suggested something > slightly different. > Are you saying that you'd prefer moving devtools modules to browser/modules > rather than browser/modules/devtools? And that we should get rid of the > content sub folders as well? This sounds like a huge mess to me. i would prefer all modules in browser/modules as files. so we clearly differ in opinion there. that said, browser/modules/devtools (for example) would be a significant improvement on modules being scattered throughout. imo browser/content, browser/components, etc all seem to be out of scope of this bug, since there's not really questions of re-use with the content code.
(In reply to Dietrich Ayala (:dietrich) from comment #25) > (In reply to Dão Gottwald [:dao] from comment #19) > > (In reply to Dietrich Ayala (:dietrich) from comment #18) > > I'm not sure if you just supported what I said or suggested something > > slightly different. > > Are you saying that you'd prefer moving devtools modules to browser/modules > > rather than browser/modules/devtools? And that we should get rid of the > > content sub folders as well? This sounds like a huge mess to me. > > i would prefer all modules in browser/modules as files. so we clearly differ > in opinion there. that said, browser/modules/devtools (for example) would be > a significant improvement on modules being scattered throughout. > > imo browser/content, browser/components, etc all seem to be out of scope of > this bug, since there's not really questions of re-use with the content code. I'd be happy with a policy saying "use a module when you need to reuse code or share data across documents." We don't have such a policy, though, and the trend is to introduce a module whenever you have a self-contained chunk of code. There are devtools modules where the question of reuse is the same as for content code, since they are basically just single-purpose UI code thrown at a prototype object.
I think the following guidelines are useful: - use modules more, for sharing and encapsulating code - if your module doesn't "fit" in anywhere else, put it in browser/modules I don't think it's useful to enforce that all modules must live in browser/modules. I don't think putting devtools modules in browser/modules/devtools would be beneficial, since those modules are closely tied to the rest of the code in browser/devtools. Sorting files solely by type isn't generally useful (we wouldn't introduce browser/javascript and browser/xul!).
This feels like "browser/components", which I think was a mistake originally. I think that in general, all our sources (modules, components, chrome, whatever) should be organized by function. Of course if we need a catchall "no good directory fits" location, then browser/modules is just as good as any other, but I'm hoping that we don't need that much, if ever.
(In reply to Benjamin Smedberg [:bsmedberg] from comment #28) > Of course if we need a catchall "no good directory fits" location, then > browser/modules is just as good as any other, but I'm hoping that we don't need > that much, if ever. We do need that, clearly, since the alternative is the current solution of sticking a bunch of random code in browser/base (which was supposed to contain just the code for our primary UI). We need somewhere for small, self-contained pieces of code to live. I think that place should be browser/modules, and that those pieces should be JS modules wherever possible. I think we've bikeshedded this enough - I don't think our source tree layout matters nearly enough to warrant the amount of discussion we've had in this bug. I'm going to land this patch, since I think it's an improvement over the current status quo, and we can revisit this if it somehow ends up being a problem!
https://hg.mozilla.org/mozilla-central/rev/e27edbae265e
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 716168
See Also: → 1692826
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: