Closed Bug 482942 Opened 11 years ago Closed 11 years ago

Implement NS_LOCALFILE_DELETE_ON_CLOSE flag for nsILocalFile::OpenNSPRFileDesc

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: roc, Assigned: roc)

References

Details

(Keywords: fixed1.9.1)

Attachments

(3 files, 2 obsolete files)

Attached patch fix (obsolete) — Splinter Review
I need this for bug 475441. I'd be surprised if it's not useful elsewhere. The patch is simple because we already reimplement PR_OpenFile in nsLocalFileWin so we can easily pass the right flag to ::CreateFile.
Attachment #367017 - Flags: review?(benjamin)
Attachment #367017 - Attachment is patch: true
Attachment #367017 - Attachment mime type: application/octet-stream → text/plain
Attachment #367017 - Flags: review?(benjamin) → review+
Comment on attachment 367017 [details] [diff] [review]
fix

>diff --git a/xpcom/io/nsILocalFile.idl b/xpcom/io/nsILocalFile.idl

> %{C++
> #include "prio.h"
> #include "prlink.h"
> #include <stdio.h>
>+
>+// Not added to nsILocalFile since that interface is frozen
>+#define NS_LOCALFILE_DELETE_ON_CLOSE 0x80000000
> %}

I don't see any reason to not put this as a const in nsILocalFile itself: it won't affect the ABI at all.

const unsigned long LOCALFILE_DELETE_ON_CLOSE = 0x80000000;

It would be nice to have tests for this, but I suppose you're stuck with binary tests; not sure whether that's worth it.

r=me with the const instead of the #define
How about
  const unsigned long DELETE_ON_CLOSE = 0x80000000;
? We don't need the LOCALFILE prefix if the constant is in the interface.
(In reply to comment #1)
> It would be nice to have tests for this, but I suppose you're stuck with binary
> tests; not sure whether that's worth it.

It should be pretty easy to do this with CPP_UNIT_TESTS; see TestPipe.cpp and others in xpcom/tests.  I don't see a reason not to require a test here.
Isn't DELETE_ON_CLOSE a define on Windows?
No, that's FILE_DELETE_ON_CLOSE.
Attached patch with tests (obsolete) — Splinter Review
The current nsIFileTests.cpp is not built and basically doesn't work the right way to run automatically (also the tests currently fail because they're wrong). So I rewrote it so it can run automatically and renamed it to TestFile.cpp.

Along the way, I discovered that nsILocalFile::MoveTo implementations are inconsistent about whether they modify the file object to point to the destination. nsLocalFileUnix doesn't but the rest do. So I'm fixing that here.

I sent this to the try-servers for some testing. If that works out I'll get this re-reviewed.
(In reply to comment #6)
> The current nsIFileTests.cpp is not built

Sorry, it is built; it does not run automatically.
(In reply to comment #6)
> Along the way, I discovered that nsILocalFile::MoveTo implementations are
> inconsistent about whether they modify the file object to point to the
> destination. nsLocalFileUnix doesn't but the rest do. So I'm fixing that here.

That's bug 200024. I think changing this behavior might negatively impact code that's already worked around that bug... unfortunately I only have vague recollections of work arounds in front-end code, and MXR doesn't return any hits for that bug number.
Code that depends on the current behaviour would almost certainly have to not be executed on Mac and Windows.

The to-ing and fro-ing in bug 200024 misses the point that it's much more important to be consistent across platforms here than to worry about the philosophy of what an nsIFile means. And it is much more likely that existing code depends on the Mac/Windows behaviour. So we should standardize on that.

So I think we should take this on 1.9.2. On 1.9.1 we can land a version of the patch without the tests or the MoveTo behaviour change.
I'm most likely just thinking of workarounds that would become unnecessary rather than bugs that fixing this would cause. mconnor might remember more, I think he was involved.

(In reply to comment #9)
> So I think we should take this on 1.9.2. On 1.9.1 we can land a version of the
> patch without the tests or the MoveTo behaviour change.

I don't disagree; that sounds like a fine plan. Might be worth making the two changes in separate bugs/changesets rather than bunching together that bug fix and the new feature, though.
I don't remember anything about this, sorry.  But I took a look at all of the browser/toolkit callers that still exist, and they assume the Mac/Window behaviour (or it doesn't matter, because that nsIFile isn't used again) so I'd be inclined to just fix it on 1.9.1.
I really don't think we should be taking compat-breaking changes like that on 1.9.1 at this point, personally.
I think it's a big assumption that it'd break compat.  I don't know that this bug is well-known enough that it's worth leaving moveTo in a state where people will end up with inconsistent behaviour on *nix.  And probably not know why the Linux users are having problems...
Thing is, not changing it means nothing that's not already broken breaks; it's a known quantity.

Changing it means that people who tested on *nix (and maybe even ship a *nix-only extension) get broken.

Maybe I'm being too conservative given our new schedule, but I feel like we need to stop making extension compat changes on 1.9.1, and need to do it yesterday...
Attachment #367017 - Attachment is obsolete: true
Attachment #368189 - Attachment is obsolete: true
Attachment #368652 - Flags: review?(benjamin)
Behaviour change in nsLocalFileUnix to make the object refer to the destination after MoveTo. Also includes the tests.
Attachment #368653 - Flags: review?(benjamin)
Comment on attachment 368652 [details] [diff] [review]
basic DELETE_ON_CLOSE patch

Actually I'll just carry forward r+ here...
Attachment #368652 - Flags: review?(benjamin) → review+
Attachment #368653 - Flags: review?(benjamin) → review+
Comment on attachment 368653 [details] [diff] [review]
MoveTo behaviour change, and tests

Definitely let's take the behavior change now in m-c.

I tend to agree with bz that we should not take the behavior change on branch, minor as it may seem.
Pushed DELETE_ON_CLOSE as http://hg.mozilla.org/mozilla-central/rev/1517b7567b28

Pushed tests and MoveTo behaviour change as http://hg.mozilla.org/mozilla-central/rev/dea4ff139073
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs 191 landing] ONLY the DELETE_ON_CLOSE change
Comment on attachment 368652 [details] [diff] [review]
basic DELETE_ON_CLOSE patch

I guess this needs 191 approval. I need it for the media block cache work.
Attachment #368652 - Flags: approval1.9.1?
Whiteboard: [needs 191 landing] ONLY the DELETE_ON_CLOSE change → [needs 191 approval] ONLY the DELETE_ON_CLOSE change
Can someone please approve this? I'm on the verge of just landing it since I need to land the media-cache on branch for beta.
Comment on attachment 368652 [details] [diff] [review]
basic DELETE_ON_CLOSE patch

bombs away.
Attachment #368652 - Flags: approval1.9.1? → approval1.9.1+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/66db860daa9d
Keywords: fixed1.9.1
Whiteboard: [needs 191 approval] ONLY the DELETE_ON_CLOSE change
Comment on attachment 368653 [details] [diff] [review]
MoveTo behaviour change, and tests

>+static PRBool gFailCount;

Doesn't this need s/PRBool/PRUint32/?
Target Milestone: --- → mozilla1.9.2a1
Attachment #374775 - Flags: review?(benjamin) → review+
Comment on attachment 374775 [details] [diff] [review]
(Cv1) TestHarness.h: s/PRBool/PRUint32/
[Checkin: Comment 27]


http://hg.mozilla.org/mozilla-central/rev/f37d7b5e0493
Attachment #374775 - Attachment description: (Cv1) TestHarness.h: s/PRBool/PRUint32/ → (Cv1) TestHarness.h: s/PRBool/PRUint32/ [Checkin: Comment 27]
Attachment #368653 - Flags: approval1.9.1?
Comment on attachment 368653 [details] [diff] [review]
MoveTo behaviour change, and tests

Benjamin, should this patch go to 1.9.1 too or not?
Attachment #368653 - Flags: approval1.9.1? → approval1.9.1-
(In reply to comment #28)
> (From update of attachment 368653 [details] [diff] [review])
> Benjamin, should this patch go to 1.9.1 too or not?

approval1.9.1-: then patch Cv1 is not needed either.
You need to log in before you can comment on or make changes to this bug.