973931 - [OS.File] makeDir should not fail if the directory is a root in the format "D:"

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[:Paolo Amadini]

Bug 960046 fixed makeDir for the root directory case, but not when the directory is specified without a trailing backslash on Windows.

I guess, but I couldn't verify, that changing this check for "absolute" to a check for "winDrive" can fix the issue:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_win_front.jsm#467

Comment 1

[:Paolo Amadini]

Attachment: The patch

This patch fixes the issue in bug 958899. I also removed a redundant test.

Comment 2

[:Paolo Amadini]

Tracking this bug in addition to bug 958899.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8380332 [details] [diff] [review]
The patch

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

r=me with the comments below addressed

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +461,5 @@
>         // This is always the case with root directories
>         if( splitPath.components[splitPath.components.length - 1].length === 0 ) {
>           splitPath.components.pop();
>         }
>         // One component and an absolute path implies a directory root.

Please fix the comment.

::: toolkit/components/osfile/tests/xpcshell/test_makeDir.js
@@ +63,1 @@
>    }

I'd like to keep the second part of the test, to ensure that we're not breaking another important characteristic of makeDir.

Comment 4

[:Paolo Amadini]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #3)
> I'd like to keep the second part of the test, to ensure that we're not
> breaking another important characteristic of makeDir.

Isn't that already tested a few lines above?

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Right, my bad.

Comment 6

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cc8da2bf746

Comment 7

[:Paolo Amadini]

Comment on attachment 8380332 [details] [diff] [review]
The patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 958899
User impact if declined: Inability to download to a root directory like "D:"
Testing completed (on m-c, etc.): Tested locally, just landed on inbound
Risk to taking this patch (and alternatives if risky): Minimal, has regression tests
String or IDL/UUID changes made by this patch: None

Comment 8

[Sylvestre Ledru [:Sylvestre]]

I am waiting for the patch to land in m-c before approving it.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3cc8da2bf746

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/40c3496f329a
https://hg.mozilla.org/releases/mozilla-beta/rev/3d19db329bdd

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/3d19db329bdd