Parse multiple URLs from text/uri-list drag data [chromium/src : main]
Joone Hur (Gerrit)
Joone Hur added 1 comment![]( "Open in Gerrit")
Joone HurPlease rebase this CL now that the others have landed. That should make this much smaller more reviewable.
The biggest issue that remains to resolve is how exactly to serialize and deserialize a list of urls and titles to/from IDataObject. As I noted in a previous comment, the current serialization doesn't handle the case of embedded `\n` in titles gracefully. Whatever we write should handle that case (and other edge cases) gracefully. I'm not sure if there is a standard convention for serializing structured data to `IDataObject` on Windows; if there is, we should use it. Worst-case scenario, we can use `base::Pickle` though I don't love falling back to that as it's fairly Chrome-specific.
How about we use JSON for the `Bookmark List` clipboard format? This will correctly handle any special characters in the titles, like newlines, and isn't Chrome-specific.
We can serialize the `std::vector<ClipboardUrlInfo>` into a JSON string and write that to the clipboard as `Bookmark List` format. On the reading side, the JSON can be parsed back into the vector.
Related details
- Daniel Cheng
- Min Qin
- Sylvain Defresne
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. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
I'm planning on removing myself from clipboard and drag and drop OWNERS. While I have a lot of historical context, I think someone on the desktop team ultimately needs to be involved here.
@dfr...@chromium.org, do you mind helping take over this review for the Windows perspective? Thanks!
Related details
- Dana Fried
- Min Qin
- Sylvain Defresne
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. |
Dana Fried (Gerrit)
Dana Fried added 1 comment![]( "Open in Gerrit")
Since it appears you do not explicitly need my approval, I'm going to tag in folks on the Windows and Linux teams who has a little closer knowledge of this stuff as relates to their specific platforms.
Related details
- David Bienvenu
- Joone Hur
- Min Qin
- Sylvain Defresne
- 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. |
David Bienvenu (Gerrit)
David Bienvenu added 1 comment![]( "Open in Gerrit")
I don't see code looking for \r\n, just \n. Am I missing something?
Related details
Joone Hur (Gerrit)
Joone Hur added 1 comment![]( "Open in Gerrit")
I don't see code looking for \r\n, just \n. Am I missing something?
This CL's parser is designed for compatibility. It splits by `\n` and uses `TRIM_WHITESPACE`, which correctly handles \r\n endings by stripping the `\r`.
Since the parser handles both, the writer just uses `\n`. I'll update the CL description to clarify this. Thanks!
Related details
- David Bienvenu
Thomas Anderson (Gerrit)
Thomas Anderson added 1 comment![]( "Open in Gerrit")
const auto& url_info = url_infos.front();you can set multiple URLs by setting `format_map_[kMimeTypeUriList] = ...`
Related details
- David Bienvenu
- Joone Hur
- Min Qin
- Sylvain Defresne
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. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
Joone HurPlease rebase this CL now that the others have landed. That should make this much smaller more reviewable.
The biggest issue that remains to resolve is how exactly to serialize and deserialize a list of urls and titles to/from IDataObject. As I noted in a previous comment, the current serialization doesn't handle the case of embedded `\n` in titles gracefully. Whatever we write should handle that case (and other edge cases) gracefully. I'm not sure if there is a standard convention for serializing structured data to `IDataObject` on Windows; if there is, we should use it. Worst-case scenario, we can use `base::Pickle` though I don't love falling back to that as it's fairly Chrome-specific.
How about we use JSON for the `Bookmark List` clipboard format? This will correctly handle any special characters in the titles, like newlines, and isn't Chrome-specific.
We can serialize the `std::vector<ClipboardUrlInfo>` into a JSON string and write that to the clipboard as `Bookmark List` format. On the reading side, the JSON can be parsed back into the vector.
That could work, but you need to make sure the platform reviewers are aware of this. I don't feel comfortable making a long-term decision here as I no longer own this code.
Joone Hur (Gerrit)
Joone Hur added 1 comment![]( "Open in Gerrit")
const auto& url_info = url_infos.front();you can set multiple URLs by setting `format_map_[kMimeTypeUriList] = ...`
Sure, setting kMimeTypeUriList is necessary for working with other applications like Firefox. I will create a follow-up CL for this change to track the multi-URL implementation for Linux.
Related details
- David Bienvenu
- Min Qin
- Sylvain Defresne
- Thomas Anderson
David Bienvenu (Gerrit)
David Bienvenu added 1 comment![]( "Open in Gerrit")
Joone HurPlease rebase this CL now that the others have landed. That should make this much smaller more reviewable.
The biggest issue that remains to resolve is how exactly to serialize and deserialize a list of urls and titles to/from IDataObject. As I noted in a previous comment, the current serialization doesn't handle the case of embedded `\n` in titles gracefully. Whatever we write should handle that case (and other edge cases) gracefully. I'm not sure if there is a standard convention for serializing structured data to `IDataObject` on Windows; if there is, we should use it. Worst-case scenario, we can use `base::Pickle` though I don't love falling back to that as it's fairly Chrome-specific.
Daniel ChengHow about we use JSON for the `Bookmark List` clipboard format? This will correctly handle any special characters in the titles, like newlines, and isn't Chrome-specific.
We can serialize the `std::vector<ClipboardUrlInfo>` into a JSON string and write that to the clipboard as `Bookmark List` format. On the reading side, the JSON can be parsed back into the vector.
That could work, but you need to make sure the platform reviewers are aware of this. I don't feel comfortable making a long-term decision here as I no longer own this code.
+1 - if this needs to be rebased, I'm going to hold off on reviewing until it is.
Related details
- Joone Hur
Joone Hur (Gerrit)
Joone Hur removed Sadrul Chowdhury from this change![]( "Open in Gerrit")
- Min Qin
- Sylvain Defresne
- 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. |
Joone Hur (Gerrit)
Joone Hur added 1 comment![]( "Open in Gerrit")
Joone HurPlease rebase this CL now that the others have landed. That should make this much smaller more reviewable.
The biggest issue that remains to resolve is how exactly to serialize and deserialize a list of urls and titles to/from IDataObject. As I noted in a previous comment, the current serialization doesn't handle the case of embedded `\n` in titles gracefully. Whatever we write should handle that case (and other edge cases) gracefully. I'm not sure if there is a standard convention for serializing structured data to `IDataObject` on Windows; if there is, we should use it. Worst-case scenario, we can use `base::Pickle` though I don't love falling back to that as it's fairly Chrome-specific.
Daniel ChengHow about we use JSON for the `Bookmark List` clipboard format? This will correctly handle any special characters in the titles, like newlines, and isn't Chrome-specific.
We can serialize the `std::vector<ClipboardUrlInfo>` into a JSON string and write that to the clipboard as `Bookmark List` format. On the reading side, the JSON can be parsed back into the vector.
David BienvenuThat could work, but you need to make sure the platform reviewers are aware of this. I don't feel comfortable making a long-term decision here as I no longer own this code.
+1 - if this needs to be rebased, I'm going to hold off on reviewing until it is.
@davidb...@chromium.org Could you please review this CL?
The updated CL switches the `Bookmark List` clipboard format to JSON.
Related details
- Daniel Cheng
- David Bienvenu
- Min Qin
- Sylvain Defresne
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joone Hur (Gerrit)
Joone Hur added 1 comment![]( "Open in Gerrit")
Joone HurPlease rebase this CL now that the others have landed. That should make this much smaller more reviewable.
The biggest issue that remains to resolve is how exactly to serialize and deserialize a list of urls and titles to/from IDataObject. As I noted in a previous comment, the current serialization doesn't handle the case of embedded `\n` in titles gracefully. Whatever we write should handle that case (and other edge cases) gracefully. I'm not sure if there is a standard convention for serializing structured data to `IDataObject` on Windows; if there is, we should use it. Worst-case scenario, we can use `base::Pickle` though I don't love falling back to that as it's fairly Chrome-specific.
Daniel ChengHow about we use JSON for the `Bookmark List` clipboard format? This will correctly handle any special characters in the titles, like newlines, and isn't Chrome-specific.
We can serialize the `std::vector<ClipboardUrlInfo>` into a JSON string and write that to the clipboard as `Bookmark List` format. On the reading side, the JSON can be parsed back into the vector.
David BienvenuThat could work, but you need to make sure the platform reviewers are aware of this. I don't feel comfortable making a long-term decision here as I no longer own this code.
Joone Hur+1 - if this needs to be rebased, I'm going to hold off on reviewing until it is.
@davidb...@chromium.org Could you please review this CL?
The updated CL switches the `Bookmark List` clipboard format to JSON.
@davidb...@chromium.org ping?
Related details
- Daniel Cheng
- David Bienvenu
- Min Qin
- Sylvain Defresne
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bienvenu (Gerrit)
David Bienvenu added 1 comment![]( "Open in Gerrit")
The ui/base/clipboard/clipboard_util_win.[cc,h] changes look OK to me, but I'm not an owner of them, or any of the files changed, actually. You'll need to get some owners to review all these files.
Related details
- Daniel Cheng
- Joone Hur
Thomas Anderson (Gerrit)
Thomas Anderson voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Daniel Cheng
- Joone Hur
- Min Qin
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/57082.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Joone Hur (Gerrit)
Joone Hur voted Commit-Queue+2![]( "Open in Gerrit")
- Daniel Cheng
- Min Qin
- Sylvain Defresne
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Parse multiple URLs from text/uri-list drag data
According to RFC 2483, Section 5, the text/uri-list MIME type supports
multiple URLs separated by CRLF(\r\n). However, Chromium was not able to
parse multiple URLs from a single text/uri-list string, treating the
entire string as one invalid URL.
This CL updates the drag-and-drop implementation to correctly support
multiple URLs. It now parses the uri-list string by splitting on the
newline character(\n) and applying base::TRIM_WHITESPACE, which robustly
handles both \n and \r\n for better compatibility.
In addition, this CL introduces the BookmarkList clipboard format for
Windows, which contains a JSON array of URLs and titles. If the URL list
has multiple entries, they are also stored using the new BookmarkList
type on the Windows clipboard.
See this design document for more details:
https://docs.google.com/document/d/1zNvn2hQOfMCc9-L9_Wt-Q4rBhmSAQHhUYWMDdNKLCGU/edit?usp=sharing
The below CLs were split from the original CL:
- https://chromium-review.googlesource.com/c/chromium/src/+/7059146
- https://chromium-review.googlesource.com/c/chromium/src/+/7084017
- M content/browser/web_contents/web_contents_view_aura.cc
- A third_party/blink/web_tests/external/wpt/clipboard-apis/drag-multiple-urls.html
- M ui/base/clipboard/clipboard_format_type.h
- M ui/base/clipboard/clipboard_format_type_win.cc
- M ui/base/clipboard/clipboard_util_win.cc
- M ui/base/clipboard/clipboard_util_win.h
- M ui/base/dragdrop/os_exchange_data.cc
- M ui/base/dragdrop/os_exchange_data_provider.h
- M ui/base/dragdrop/os_exchange_data_provider_mac.h
- M ui/base/dragdrop/os_exchange_data_provider_mac.mm
- M ui/base/dragdrop/os_exchange_data_provider_non_backed.cc
- M ui/base/dragdrop/os_exchange_data_provider_non_backed.h
- M ui/base/dragdrop/os_exchange_data_provider_non_backed_unittest.cc
- M ui/base/dragdrop/os_exchange_data_provider_win.cc
- M ui/base/dragdrop/os_exchange_data_provider_win.h
- M ui/base/x/x11_os_exchange_data_provider.cc
- M ui/base/x/x11_os_exchange_data_provider.h
- M ui/ozone/platform/wayland/host/wayland_exchange_data_provider.cc
- M ui/ozone/platform/x11/test/os_exchange_data_provider_x11_unittest.cc
Code-Review: +1 by Thomas Anderson
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joone Hur (Gerrit)
Joone Hur added 1 comment![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/57082