Add the user trash directory to OS.Constants.Path

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: marco, Assigned: lynn_tran, Mentored)

Tracking

Trunk
mozilla34
All
macOS
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++][good first bug])

Attachments

(1 attachment, 5 obsolete attachments)

Reporter

Description

5 years ago
We'd need it in the test added in bug 981251.
Is there a trash directory under non-MacOS X platforms?
Flags: needinfo?(mar.castelluccio)
Reporter

Comment 2

5 years ago
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?" - I'll be away on April 9th-16th) from comment #1)
> Is there a trash directory under non-MacOS X platforms?

Yes.

On Linux, there's a user trash directory (~/.local/share/Trash) and a trash directory for each external volume. The Trash directory contains a "expunged" directory, a "files" directory (that contains the files moved to the trash), a "info" directory that contains files with a ".trashinfo" extension (these files contain metadata about the files moved to the trash, like the original path and the date of removal).

On Windows, I'm not sure if there's a single trash directory or one for each volume.
Flags: needinfo?(mar.castelluccio)
I'm almost sure that there is one per volume under Windows.

Under Linux, is that standardized across distros and environments?

Also, wherever there isn't a single trash can/recycle bin, what behavior would you expect?
Reporter

Comment 4

5 years ago
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?" - I'll be away on April 9th-16th) from comment #3)
> Under Linux, is that standardized across distros and environments?

I guess so, but I'm not sure.

> Also, wherever there isn't a single trash can/recycle bin, what behavior
> would you expect?

Actually, I'm not sure.
In the directory service we don't have a cross-platform key to specify the trash directory.
On Windows we have NS_WIN_BITBUCKET_DIR, that uses CSIDL_BITBUCKET to get the "virtual" recycle bin directory.
On Mac we have NS_MAC_TRASH_DIR.
On Linux we don't have anything.

In bug 981251 I'd just need the trash dir on Mac.

In general, given that the trash behaves in different ways on different platforms, I guess we could just add a macTrashDir (like we did with other OS specific directories).
Let's implement winTrashDir and macTrashDir.

The source code lives here: 
http://dxr.mozilla.org/mozilla-central/source/dom/system/OSFileConstants.cpp?from=OSFileConstants.cpp

The objective of this bug is to add constants `winTrashDir` and `macTrashDir`, representing the path to the trash directory under Windows and MacOS X, much as there is a constant `winAppDataDir` representing the path to the AppData directory under Windows.

These directories are obtained from keys, by calling function `GetPathToSpecialDir`.

The directory for `winTrashDir` must be obtained from key `NS_WIN_BITBUCKET_DIR`, while the directory for `macTrashDir` must be obtained from key `NS_MAC_TRASH_DIR`.
Mentor: dteller
Whiteboard: [lang=c++][good first bug]
Assignee

Comment 6

5 years ago
Posted patch bug-999748-fix.patch (obsolete) — Splinter Review
Attachment #8455065 - Flags: review?(dteller)
Assignee: nobody → lynn_tran
Comment on attachment 8455065 [details] [diff] [review]
bug-999748-fix.patch

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

The code looks good, with a minor typo in the position of a comment.

I would like some tests before we can mark this as r+. There are already some tests on OS.Constants.Paths in file test_desktop_paths.js. You can add your tests to function* `test_desktop_paths`. Use the key "Buckt" under Windows and the key "Trsh" under MacOS X.

::: dom/system/OSFileConstants.cpp
@@ +141,5 @@
>     * The Application directory, that stores applications installed in the
>     * system.
>     */
> +  /**
> +   * The user's trash directory.

Comment is in the wrong place.
Attachment #8455065 - Flags: review?(dteller) → feedback+
Assignee

Comment 8

5 years ago
Posted patch bug-999748-fix.patch (obsolete) — Splinter Review
Attachment #8455065 - Attachment is obsolete: true
Attachment #8456261 - Flags: feedback?(dteller)
Comment on attachment 8456261 [details] [diff] [review]
bug-999748-fix.patch

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

This looks good to me.
Let's give it a try: https://tbpl.mozilla.org/?tree=Try&rev=2408b95d99e2
Attachment #8456261 - Flags: feedback?(dteller) → feedback+
Assignee

Comment 10

5 years ago
So 2 builds are busted and I can't really tell the reason why. The tests for Windows builds failed because of a check, but the only reason I could think of is that it's using the wrong key since the checking looks fine to me.
Flags: needinfo?(dteller)
For the MacOS X builds, look at your `if` from line 996. If's never closed.
For the Windows versions, I'm not sure what's going on. According to the test, the key is correct but the value `OS.Constants.Path.winTrashDir` is not defined.

Could you start by fixing the Mac OS X version and we'll see if that one works?
Flags: needinfo?(dteller)
Assignee

Comment 12

5 years ago
Posted patch bug-999748-fix.patch (obsolete) — Splinter Review
Attachment #8456261 - Attachment is obsolete: true
Attachment #8459667 - Flags: review?(dteller)
Assignee

Updated

5 years ago
Attachment #8459667 - Flags: review?(dteller) → feedback?(dteller)
Assignee

Updated

5 years ago
Flags: needinfo?(dteller)
Comment on attachment 8459667 [details] [diff] [review]
bug-999748-fix.patch

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

Looks good. It's not clear to me what went wrong with the previous Try.
Let's hope this one is more informative.
Try: https://tbpl.mozilla.org/?tree=Try&rev=f770d3ab62eb
Attachment #8459667 - Flags: feedback?(dteller) → feedback+
Flags: needinfo?(dteller)
Assignee

Comment 14

5 years ago
Posted patch bug-999748-fix-v2.patch (obsolete) — Splinter Review
This should get the OSX built this time I hope, but I read up on Windows and just like you said and on Microsoft page, we just need to CSIDL_BITBUCKET to get the virtual folder to the recycle bin. I also tried to look up in the existing code base to see if CSIDL_BITBUCKET is being used elsewhere, but I don't think it exists elsewhere but in a header file for declaring constants.
Attachment #8459667 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Flags: needinfo?(dteller)
I'm just back from vacation. I will try and take a look tomorrow.
I am poking around the code to see if I can find something.
Flags: needinfo?(dteller)
Ok, I toyed with it, and it looks like the Windows recycle bin is a directory without a path, so this could not work.

Lynn Tran, let's get rid of the Windows code and expose the path of the trash only on MacOS X for the moment.
Assignee

Comment 19

5 years ago
Posted patch bug-999748-fix.patch (obsolete) — Splinter Review
Attachment #8463050 - Attachment is obsolete: true
Attachment #8477771 - Flags: review?(dteller)
Assignee

Comment 20

5 years ago
I removed the Window code and the Mac code successfully built and passed all the tests in the latest try
Assignee

Updated

5 years ago
Flags: needinfo?(dteller)
Comment on attachment 8477771 [details] [diff] [review]
bug-999748-fix.patch

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

I believe that you have merged to unrelated patches. Can you just publish the part relevant to this bug?

::: content/base/src/nsDocument.cpp
@@ +4469,5 @@
>        NS_WARNING("Page was shift reloaded, skipping ServiceWorker control");
>        return;
>      }
>  
> +    nsCOMPtr<nsIServiceWorkerManager> swm = mozilla::services::GetServiceWorkerManager();

That looks like a very distinct patch.
Attachment #8477771 - Flags: review?(dteller)
Flags: needinfo?(dteller)
Assignee

Comment 22

5 years ago
Sorry I didn't realize that got mixed up somehow. I checked it before resubmitting it.
Attachment #8477771 - Attachment is obsolete: true
Attachment #8478994 - Flags: review?(dteller)
Assignee

Updated

5 years ago
Flags: needinfo?(dteller)
Comment on attachment 8478994 [details] [diff] [review]
bug-999748-fix.patch

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

Looks good.
Let's run it through Try:  https://tbpl.mozilla.org/?tree=Try&rev=ff1fd7c50be5
Attachment #8478994 - Flags: review?(dteller) → review+
Flags: needinfo?(dteller)
Well, this looks good. Lynn Tran, if you are ready, we can land this.
Flags: needinfo?(lynn_tran)
Assignee

Comment 25

5 years ago
Sure. I think I'm fine with it. Just a bit surprised that this way of doing doesn't work with Windows
Flags: needinfo?(lynn_tran)
In that case, let's land the patch!

Do you wish help finding another bug to work on?
Assignee

Comment 27

5 years ago
If you think there is an interesting one, please tell me, otherwise, I think I could go find another one on bugsahoy. Thanks!
https://hg.mozilla.org/integration/fx-team/rev/d41e0d8d8eba
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [lang=c++][good first bug] → [lang=c++][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d41e0d8d8eba
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [lang=c++][good first bug][fixed-in-fx-team] → [lang=c++][good first bug]
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.