Use the new API addition to Document portal allowing clients to get real
path to the exported document. This allows to still use the same path as
provided by the document portal, but display the path as exists on the
host side.
Details
Details
Diff Detail
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
- Branch
- default
- Lint
-
No Lint Coverage Severity Location Code Message Error xpcom/io/nsLocalFileUnix.cpp:796 clang-diagnostic-error Build Error - Unit
-
No Test Coverage
| Time | Test | |
|---|---|---|
| 0 ms | code-review::mercurial
WARNING: The base revision of your patch is not available in the current repository. Your patch has been rebased on central (revision None): issues may be positioned on the wrong lines.
|
Event Timeline
Comment Actions
Code analysis found 1 defect in diff 827073:
- 1 defect found by clang-tidy
1 defect unresolved and 10 defects closed compared to the previous diff 827072.
WARNING: Found 1 defect (warning level) that can be dismissed.
You can run this analysis locally with:
- ./mach static-analysis check --outgoing (C/C++)
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 827073.
Comment Actions
It's Work In Progress, it depends on unmerged xdg-desktop-portal change and I'm still a bit unsure about the DBus call being sync, but I have no idea how to make it async instead (not the DBus call, but I mean nsIFile::GetDisplayName()).
Comment Actions
Then the patch should hold until final API is merged. We may not land WIP patches.
and I'm still a bit unsure about the DBus call being sync, but I have no idea how to make it async instead (not the DBus call, but I mean nsIFile::GetDisplayName()).
nsIFile doesn't have async API, it has to be sync.
Comment Actions
That's why I marked it as WIP so it doesn't get merged yet :).
and I'm still a bit unsure about the DBus call being sync, but I have no idea how to make it async instead (not the DBus call, but I mean nsIFile::GetDisplayName()).
nsIFile doesn't have async API, it has to be sync.
I know, that's why I made it sync, it's just I don't like it being sync as in theory this can block UI. Async is always better.
Comment Actions
You can use Promises in xpidl - https://searchfox.org/mozilla-central/search?q=promise&path=.idl%24&case=false®exp=false
That would allow making this async.
An alternative would be moving/copying the displayName stuff into IOUtils ( https://searchfox.org/mozilla-central/source/dom/system/IOUtils.cpp / https://searchfox.org/mozilla-central/source/dom/chrome-webidl/IOUtils.webidl ) which supports proxying nsIFile calls through something other than the main UI thread.
Comment Actions
Code analysis found 3 defects in diff 832676:
- 2 build errors and 1 defect found by clang-tidy
1 defect closed compared to the previous diff 827073.
WARNING: Found 1 defect (warning level) that can be dismissed.
IMPORTANT: Found 2 defects (error level) that must be fixed before landing.
You can run this analysis locally with:
- ./mach static-analysis check --outgoing (C/C++)
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 832676.
Comment Actions
Code analysis found 4 defects in diff 833040:
- 1 defect found by clang-format
- 1 defect found by clang-tidy
- 2 defects found by clang-format (Mozlint)
1 defect unresolved and 2 defects closed compared to the previous diff 832676.
WARNING: Found 4 defects (warning level) that can be dismissed.
You can run this analysis locally with:
- ./mach lint --warnings --outgoing
- ./mach static-analysis check --outgoing (C/C++)
- ./mach clang-format -p xpcom/io/nsLocalFileUnix.cpp
For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply -p0).
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 833040.
Comment Actions
4 defects closed compared to the previous diff 833040.
If you see a problem in this automated review, please report it here.
Comment Actions
This is pretty close but I think the settings integration may need to be tweaked.
| browser/components/preferences/main.js | ||
|---|---|---|
| 3646–3665 |
Unfortunately there are a few issues here. First, file can clearly be null/undefined, and right now in that case accessing file.path inside the added Linux-specific if condition will throw, which can potentially just break all of settings. Second, the LTR-forcing should probably still happen in this case, in case the path has RTL elements etc. - not just in the case where we don't get a hostPath. Third, we're skipping the logic that checks if the directory corresponds to the OS downloads or desktop folder (at the top of this function). That is, I'm guessing that flatpak gives us a "custom" path that maps to the downloads folder, if that's the folder the user has selected, which won't match the result of _getDownloadsFolder? (if it's not, and somehow this all works because the portal references are identical, then that would be good to document a bit better). To address some of this, can we overwrite file with a new nsIFile instance based out of the host path, or does that fail for some reason? IOW, can we update the code inside the if (folderIndex == 2 && currentDirPref.value) { check to include the logic you've added here, and then do: file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile); file.initWithPath(hostPath); and then let the rest of the code run as before? Or does that fall down in other ways? |
|
| browser/components/preferences/main.js | ||
|---|---|---|
| 3646–3665 |
I don't think we are doing that, because in such case we just return early and don't even get to try the part I'm adding with this patch. It even says that at the bottom of this function: // If we get here, attempts to use a "pretty" name failed. Just display the full path |
|
| browser/components/preferences/main.js | ||
|---|---|---|
| 3646–3665 |
So unfortunately I'm not in a position to test this on a custom flatpak'd build myself (ie I'm not sure how to both apply your patch _and_ run the resulting thing in a flatpak environment). But running a "standard" flatpak build, if I select the Downloads path from the filepicker, then the filepicker returns /home/<user>/Downloads as a file instance and indeed the equality would match and we would have returned early. However, for Desktop and my homedir, it returns /run/user/10000/doc/{hash}/{foldername}. This means that the comparison of the two files fails (they are not equal) - even though in fact, the "host path" _is_ equal. This is all despite the Desktop invocation of getDownloadsFolder returning a normal /home/<user>/ path (somehow I have a Desktop folder but it thinks that $HOME is my desktop folder; I don't really know why flatpak/ubuntu gets those confused, but either way I see those /run/... paths for both folders and so the comparison always fails). So I think we need to resolve the file that the filepicker has returned (and that we've stored in the prefs) to a "real" (host) path _first_, and then compare with the downloads/desktop/whatever folders. This also makes me wonder... do we not want to store those host paths? If we store the /run/ path, and then we restart, will the path still be valid? |
|
Comment Actions
@grulja curious if you are still hoping to continue work here and/or if there is anything I can do to help? :-)
Comment Actions
Yes, I just got sidetracked with other work and since this is blocked by xdg-desktop-portal change, that hasn't been merged yet, I was not giving this priority. I will definitely fix what you mentioned as soon as possible.
| browser/components/preferences/main.js | ||
|---|---|---|
| 3646–3665 |
I'm not sure this will work, because when we first resolve the file to real host path first and this will get resolved to "Downloads" or "Desktop" or whatever and saved this way, how do we retrieve back the original path, which will be the only one accessible from the sandbox? That's also a reason why we don't want to store those host paths instead, because from the sandbox you don't have access to the host path, but only to the mounted file (original path) provided by the document portal. Basically the path as exists on the host is not visible from the sandbox. We just resolve it to the host path so the user is not confused, but under the hood we work with the /run/.... path. |
|
Comment Actions
Code analysis found 1 defect in diff 864767:
- 1 build error found by clang-tidy
IMPORTANT: Found 1 defect (error level) that must be fixed before landing.
You can run this analysis locally with:
- ./mach static-analysis check --outgoing (C/C++)
For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply -p0).
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 864767.
| browser/components/preferences/main.js | ||
|---|---|---|
| 3646–3665 |
Hm, how does that work today, then, before this patch? The default is already "Downloads", right? How do we retrieve that? |
|
| browser/components/preferences/main.js | ||
|---|---|---|
| 3658–3663 |
Assuming we can't do what I suggested in the other comment thread (apologies for my ongoing confusion there re: default download dirs etc.), I'd probably change this a little to amalgamate the return conditions by doing something like: let displayName = file.path; if (/* linux */) { ... if (hostPath) { displayName = hostPath; } } // Force the left-to-right direction when displaying a custom path. return { file, folderDisplayName: `\u2066${displayName}\u2069` }; which means the comment stays with the LTR-enforcing and that logic isn't duplicated. |
|