J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Description • 10 years ago

Many of the paths in Loader.jsm are redundant and unnecessary.

Let's reduce them down where possible!

Also, let's add giant warnings to avoid adding more lines. :D

	Brian Grinstead [:bgrins]
	Comment 1 • 10 years ago

I also wouldn't mind moving some of the js modules that are in toolkit/devtools into the new toolkit/devtools/shared folder just to clean up that directory a bit

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 2 • 10 years ago

(In reply to Brian Grinstead [:bgrins] from comment #1)
> I also wouldn't mind moving some of the js modules that are in
> toolkit/devtools into the new toolkit/devtools/shared folder just to clean
> up that directory a bit

Yeah, sounds like a great idea!

The one thing this work needs to be careful of though is DevTools addons in the wild.  It's likely this work would change the require paths need to get various things.  We can fix in-tree, but not add-ons.

So, either there needs to be some kind of legacy mapping, or we make the add-ons use different paths per Firefox version.  Not sure yet how bad the damage would be.

	Alexandre Poirot [:ochameau]
	Comment 3 • 9 years ago

Ouch!

gcli is doing some complex file <> module mapping:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/gcli/moz.build#7

Both files from toolkit/devtools/gcli/commands and toolkit/devtools/gcli/source/lib/gcli/commands get merged into require("gcli/commands/*") :-/
This is fine with a build system, but won't work with `tools srcdir` feature...

	Alexandre Poirot [:ochameau]
	Comment 4 • 9 years ago

Joe, may we split them (back?) to two distinct folders and require paths?
  http://mxr.mozilla.org/mozilla-central/search?string=gcli/commands

I can easily rewrite all these occurences to require the files like this:
  toolkit/devtools/gcli/source/lib/gcli/commands => require(devtools/toolkit/gcli/commands)
  toolkit/devtools/gcli/source/lib/gcli/commands => require(gcli/commands)

	Joe Walker [:jwalker]
	Comment 5 • 9 years ago

Do you mean:

  toolkit/devtools/gcli/commands => require(devtools/toolkit/gcli/commands)
  toolkit/devtools/gcli/source/lib/gcli/commands => require(gcli/commands)

?

	Alexandre Poirot [:ochameau]
	Comment 6 • 9 years ago

Yes, I messed up with my copy paste.
Would that work for you?

	Joe Walker [:jwalker]
	Comment 7 • 9 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #6)
> Yes, I messed up with my copy paste.
> Would that work for you?

Yes, sounds good.

	Alexandre Poirot [:ochameau]
	Comment 8 • 9 years ago

Simple fix, we were passing path splits to fileURI instead of join...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37beeca647cf

	Alexandre Poirot [:ochameau]
	Comment 9 • 9 years ago

When using gcli and local source mapping, this command keep throwing in the console,
that's because there is a magic build-time mapping that moves resize-commands 
from responsivedesign folder to a top level folder.

Like bug 1188417, we shouldn't do such magic build system mapping.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 10 • 9 years ago

Comment on attachment 8641100 [details] [diff] [review]
Fix broken module paths when mapping to source directory

Review of attachment 8641100 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but please move this to a new bug that blocks this one.

I'd like to keep this bug focused on the step of removing path entries, which may depend on or be done as part of the file move (bug 912121).

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 11 • 9 years ago

Comment on attachment 8641102 [details] [diff] [review]
Fix resize-commands module path when mapping to local sources

Review of attachment 8641102 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but please move this to a new bug that blocks this one.

I'd like to keep this bug focused on the step of removing path entries, which may depend on or be done as part of the file move (bug 912121).

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 12 • 9 years ago

To clarify, the "path entries" I mean here are the require path to file / resource mappings in the Loader.jsm file itself.

	Alexandre Poirot [:ochameau]
	Comment 13 • 9 years ago

Note that I tried to get rid of the magic require(source-map) path,
but that raised some issues as stated in bug 935366 comment 12 and 15.
We may have to keep some of these shortcuts.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 14 • 9 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #13)
> Note that I tried to get rid of the magic require(source-map) path,
> but that raised some issues as stated in bug 935366 comment 12 and 15.
> We may have to keep some of these shortcuts.

Yeah, we'll see.  I am still hopeful most can go away, but we may need a "legacy" mapping of them, especially for add-ons.

I'm planning to test some add-ons to see how bad it is once I've done the move.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 15 • 9 years ago

Assigning this bug for now to acknowledge I intend to do this work, maybe as part of bug 912121.  It would be sad to duplicate efforts when rewriting the whole tree is involved!

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 16 • 9 years ago

I no longer intend to make a change here.  We have simplified the list somewhat already, but there may be a few more cleanups we could make.

	Alexandre Poirot [:ochameau]
	Comment 17 • 9 years ago

What about just getting rid of the srcdir loader now that we have the addon?

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 18 • 9 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #17)
> What about just getting rid of the srcdir loader now that we have the addon?

Yes, I think we do that.  But, it seems like a separate bug from this.  This is about removing duplicate paths, even in the "regular" loader.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 19 • 9 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #18)
> (In reply to Alexandre Poirot [:ochameau] from comment #17)
> > What about just getting rid of the srcdir loader now that we have the addon?
> 
> Yes, I think we do that.  But, it seems like a separate bug from this.  This
> is about removing duplicate paths, even in the "regular" loader.

I think we *should* do that.

	Alexandre Poirot [:ochameau]
	Comment 20 • 9 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #18)
> (In reply to Alexandre Poirot [:ochameau] from comment #17)
> > What about just getting rid of the srcdir loader now that we have the addon?
> 
> Yes, I think we do that.  But, it seems like a separate bug from this.  This
> is about removing duplicate paths, even in the "regular" loader.

Hum. Do you want to converge to have only just "devtools" mapping? (and the magic mapping for promise and xpcshell)
Because I don't see any explicit duplicate entry now:
  http://mxr.mozilla.org/mozilla-central/source/devtools/shared/Loader.jsm#91

I opened bug 1250430 for the SrcDir removal

	Alexandre Poirot [:ochameau]
	Comment 21 • 9 years ago

We still have acorn, which use a magic layout that is different between local and runtime,
otherwise, we just have:
 - "" for SDK modules,
 - devtools to resource://devtools 
 - some compatibility/handy shortcuts for sourcemap and gcli (and ideally also acorn).
 - shortcuts for promise jsm module.
 - something for xpcshell tests

Ryan, do you thing we can close this bug if we fix acorn mapping?

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Comment 22 • 9 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #21)
> We still have acorn, which use a magic layout that is different between
> local and runtime,
> otherwise, we just have:
>  - "" for SDK modules,
>  - devtools to resource://devtools 
>  - some compatibility/handy shortcuts for sourcemap and gcli (and ideally
> also acorn).
>  - shortcuts for promise jsm module.
>  - something for xpcshell tests
> 
> Ryan, do you thing we can close this bug if we fix acorn mapping?

I would like to get away from any random packages like sourcemap and gcli needing entries as well, but this may require more intelligence about how we handle packages.  I mention them more explicitly in bug 1201710 which this depends on.

At the same time, I don't think this is a big issue at this time, so I'd say keep this open for now, but there are probably other problems worth focusing on instead.

	Joe Walker [:jwalker]
	Comment 23 • 9 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #22)
> ...
> I would like to get away from any random packages like sourcemap and gcli
> needing entries as well

For what it's worth all GCLI require statements are relative (e.g. [1]).

So it's possible that if we change references to GCLI from outside /devtools/shared/gcli/source/lib (e.g. [2]) to point to the full path then that's all we need to do.

It might even be as simple as

s/require\("gcli/require("\/devtools\/shared\/gcli\/source\/lib/g

That might be a bit of a mouthful for a require path, but probably better a require-path-mouthful than the mess in the loader.

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/gcli/source/lib/gcli/cli.js#19
[2]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/developer-toolbar.js#30