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