Closed Bug 1250110 Opened 4 years ago Closed 4 years ago

Clean up the path utility functions.

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: ejpbruel, Assigned: jlast)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
> 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]
Assignee: ejpbruel → jlaster
Attached patch path.1.patch (obsolete) — Splinter Review
Attachment #8737262 - Flags: review?(ejpbruel)
The unit tests passed when i removed dirname, now we'll see if all of the integration tests pass as well.
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.
Attachment #8737262 - Flags: review?(ejpbruel) → review-
Attached patch path.2.patch (obsolete) — Splinter Review
Attachment #8737262 - Attachment is obsolete: true
Attached patch path.3.patchSplinter Review
forgot to r? you yesterday
Attachment #8738548 - Attachment is obsolete: true
Attachment #8739138 - Flags: review?(ejpbruel)
Comment on attachment 8739138 [details] [diff] [review]
path.3.patch

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

Looks good to me.
Attachment #8739138 - Flags: review?(ejpbruel) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d93a1bc94513
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.