Implement XDG file transfer portal [chromium/src : main]
Thomas Anderson (Gerrit)
Thomas Anderson voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Thomas Anderson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Thomas Anderson
| Commit-Queue | +1 |
1 comment
Related details
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
pinging thestig@
The first time I looked, this had red bots. I didn't get a notification that this was ready until now.
Lei Zhang (Gerrit)
Lei Zhang added 20 comments![]( "Open in Gerrit")
friend class dbus_xdg::FileTransferPortal; // http://crbug.com/443355 if (rets.has_value()) {Flip conditional, so the 3 return path are {fail, fail, success}.
for (size_t i = 0; i < files.size(); ++i) {The logic here may be a little easier to understand if it's reorganized as follows:
1) Use spans to break `files` into chunks of up to 16.
2) For each chunk, try to open the FDs.
3) If that chunk has FDs, send.
std::string CreateTempFile(const std::string& name) {std::string_view
EXPECT_EQ(fds.size(), 2u);Can this be ASSERT_EQ(), or use EXPECT_THAT(fds, testing::ElementsAre(...)).
std::vector<std::string> GetPathsFromUriList(const std::string& uri_list);Take a string_view instead? Then the caller in ui/ozone/platform/wayland/host/wayland_clipboard.cc can avoid a copy.
Add yourself as the owner for clipboard_util_linux*?
std::vector<std::string> paths;Also call reserve() here?
if (!dbus_xdg::FileTransferPortal::IsAvailableSync()) {Should this check if `paths` is empty, like line 61?
if (is_linux) {As is - can be merged with line 95. Separately, should these conditionals include `use_dbus`?
if (format_map_.contains(p) && !std::ranges::contains(*targets, p)) {How many times will the for-loop reach this conditional? Is the number small enough that a linear search through `*targets` won't be so bad?
base::as_bytes(base::span(*data)));Use base::as_byte_span().
virtual ui::PlatformClipboard::Data ReadFileTransfer() = 0;Document.
if (mime_type != kMimeTypePortalFileTransfer &&Pull this out into a bool that is always true on other platforms. Then there's no need to duplicate the log message.
if (!paths.empty()) {Flip the conditional here so line 730 becomes adjacent to this check.
std::map<std::string, std::vector<uint8_t>> additional_data_;IWYU
std::map<std::string, std::vector<uint8_t>> additional_data_;Would it be easier to make the map value std::string?
std::map<std::string, std::vector<uint8_t>> additional_data_;Should this be Linux-only?
if (mime_type == ui::kMimeTypePortalFileTransfer ||Should these be Linux-only?
Related details
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Thomas Anderson voted and added 21 comments![]( "Open in Gerrit")
Votes added by Thomas Anderson
| Commit-Queue | +1 |
21 comments
latest patch set also adds a feature flag (disabled by default)
friend class dbus_xdg::FileTransferPortal; // http://crbug.com/443355Use current bugs numbers. https://crbug.com/40398800
Done
Flip conditional, so the 3 return path are {fail, fail, success}.
Done
The logic here may be a little easier to understand if it's reorganized as follows:
1) Use spans to break `files` into chunks of up to 16.
2) For each chunk, try to open the FDs.
3) If that chunk has FDs, send.
Done
std::string CreateTempFile(const std::string& name) {Thomas Andersonstd::string_view
Done
Can this be ASSERT_EQ(), or use EXPECT_THAT(fds, testing::ElementsAre(...)).
ASSERT can only be used in the test body. I added a return after the EXPECT.
std::vector<std::string> GetPathsFromUriList(const std::string& uri_list);Take a string_view instead? Then the caller in ui/ozone/platform/wayland/host/wayland_clipboard.cc can avoid a copy.
Done
Add yourself as the owner for clipboard_util_linux*?
Done
std::vector<std::string> paths;Thomas AndersonAlso call reserve() here?
Done
Should this check if `paths` is empty, like line 61?
Done
As is - can be merged with line 95. Separately, should these conditionals include `use_dbus`?
Done
if (format_map_.contains(p) && !std::ranges::contains(*targets, p)) {How many times will the for-loop reach this conditional? Is the number small enough that a linear search through `*targets` won't be so bad?
Done
base::as_bytes(base::span(*data)));Thomas AndersonUse base::as_byte_span().
Done
virtual ui::PlatformClipboard::Data ReadFileTransfer() = 0;Thomas AndersonDocument.
Done
Pull this out into a bool that is always true on other platforms. Then there's no need to duplicate the log message.
Done
Flip the conditional here so line 730 becomes adjacent to this check.
Done
std::vector<FileInfo> file_infos;Thomas Andersonreserve()
Done
std::map<std::string, std::vector<uint8_t>> additional_data_;Thomas AndersonIWYU
Done
std::map<std::string, std::vector<uint8_t>> additional_data_;Would it be easier to make the map value std::string?
Done
std::map<std::string, std::vector<uint8_t>> additional_data_;Thomas AndersonShould this be Linux-only?
Done
if (mime_type == ui::kMimeTypePortalFileTransfer ||Thomas AndersonShould these be Linux-only?
Done
Related details
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang voted and added 4 comments![]( "Open in Gerrit")
Votes added by Lei Zhang
| Code-Review | +1 |
4 comments
for (size_t i = 0; i < files.size(); i += kBatchSize) {Maybe easier to grab a span outside of the loop, then in each iteration, call files_span.take_first(N) if needed, until files_span is empty.
per-file clipboard_util_linux*=thomasa...@chromium.orgASCII-sort -> move to bottom.
if (mime_type == ui::kMimeTypePortalFileTransfer ||Thomas AndersonShould these be Linux-only?
Done
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Thomas Anderson voted and added 5 comments![]( "Open in Gerrit")
Votes added by Thomas Anderson
| Commit-Queue | +1 |
5 comments
+blundell@ for ui/base/clipboard approval
Maybe easier to grab a span outside of the loop, then in each iteration, call files_span.take_first(N) if needed, until files_span is empty.
Done
ASCII-sort -> move to bottom.
Done
if (mime_type == ui::kMimeTypePortalFileTransfer ||Thomas AndersonShould these be Linux-only?
Lei ZhangDone
Done here or elsewhere?
- Colin Blundell
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
Thanks! Does this need a security review of some kind, and if not, why not? I don't have any experience here, and naively the flow being introduced seems non-trivial.
Related details
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Thomas Anderson added 1 comment![]( "Open in Gerrit")
Thanks! Does this need a security review of some kind, and if not, why not? I don't have any experience here, and naively the flow being introduced seems non-trivial.
Historically security reviews have not been required for interaction with system services (Wayland/X11/D-Bus).
Related details
- Colin Blundell
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell voted and added 1 comment![]( "Open in Gerrit")
Votes added by Colin Blundell
| Code-Review | +1 |
1 comment
SGTM, thanks! I defer to your and Lei's expertise and experience in this space.
Related details
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |