1250110 - Clean up the path utility functions.

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Eddy Bruel [:ejpbruel]]

We currently have a number of path utility functions in devtools/shared/path.js. However, the dirname function doesn't do what it name suggests: first, it creates a URL with the given path. Second, it creates a second URL with the path "." and the previous URL as the base URL. This step seems redundant, since the result is the same URI. Finally, it returns the spec property of the URL, which is a string representation of the entire URL. As far as I can tell, that makes dirname the identity function.

The joinURI function is implemented redundantly in TabSources.js as _normalize. This function does exactly the same thing, so we should be able to replace _normalize with a call to joinURI.

Finally, I'd like to keep the platform API surface that the debugger needs as small as possible, so having a dependency on the URL constructor just so we can join paths together is not something I'm happy with. Jan suggested that we might be able to use the functions in toolkit/components/osfile/modules/ospath_unix.jsm as a replacement.

Comment 1

[Jim Blandy :jimb]

> I'd like to keep the platform API surface that the debugger needs as small as possible

I don't know anything about this code, but: [raises glass]

Comment 2

[Jason Laster [:jlast]]

Attachment: path.1.patch

Comment 3

[Jason Laster [:jlast]]

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

Comment 4

[Jason Laster [:jlast]]

The unit tests passed when i removed dirname, now we'll see if all of the integration tests pass as well.

Comment 5

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8737262 [details] [diff] [review]
path.1.patch

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

Patch looks good. Please remove _normalize as well, and I'll r+ it.

::: devtools/server/actors/utils/TabSources.js
@@ +792,5 @@
>     * Normalize multiple relative paths towards the base paths on the right.
>     */
>    _normalize: function (...aURLs) {
>      assert(aURLs.length > 1, "Should have more than 1 URL");
> +    return joinURI(aURLs.pop(), aURLs);

This is the right idea, but the logical next step would then be to remove _normalize altogether, no? ;-)

::: devtools/shared/path.js
@@ -8,5 @@
>  
>  /*
> - * Returns the directory name of the path
> - */
> -exports.dirname = path => {

I seem to recall dirname had at least one consumer left somewhere. Please make sure that all the tests still pass with this change.

Comment 6

[Jason Laster [:jlast]]

Attachment: path.2.patch

Comment 7

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=646310ffa4c2

Comment 8

[Jason Laster [:jlast]]

Attachment: path.3.patch

forgot to r? you yesterday

Comment 9

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8739138 [details] [diff] [review]
path.3.patch

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

Looks good to me.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d93a1bc94513

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d93a1bc94513