Allow setting multiple URLs for text/uri-list drag type [chromium/src : main]
Joone Hur (Gerrit)
Joone Hur added 1 comment![]( "Open in Gerrit")
Related details
- Daniel Cheng
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rakina Zata Amni
| Code-Review | +1 |
1 comment
- Daniel Cheng
- David Trainor
- Joone Hur
- Min Qin
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Trainor (Gerrit)
David Trainor added 2 comments![]( "Open in Gerrit")
chrome/browser changes are minimal - will stamp once ui/base/dragdrop is reviewed!
GURL url() const;
~UrlInfo();
std::vector<GURL> urls;I'm not an owner of this directory so will defer to the right folks - not sure how I feel about having urls get added to this, but "url" and "title" represent the first one. Just wanted to flag it!
Related details
- Daniel Cheng
- Joone Hur
- Min Qin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 18 comments![]( "Open in Gerrit")
My biggest concern is about the platform-specific plumbing. I'm not sure many OSes actually support multiple URLs in their default URL type (see the comments about Windows and Mac). We may need to do something else to make it work well.
My other comment is about the invariants we want to maintain. To make the APIs easier to use, I propose:
- don't allow any invalid GURLs in the various URL lists. So when we're parsing out the URL lists, that means validating that they're actually valid URLs before appending them to the list.
- an empty list means no URLs at all
- the various SetURL()/SetURLs() functions should only be called if there is a non-empty list of URLs
if (data.urls.empty() || !data.urls.front().is_valid() || is_in_split_view) {Should we really have invalid URLs here?
I think we should avoid having invalid URLs in this list; instead, whether or not the list is empty should represent whether or not URLs are present.
NSURL* url = net::NSURLWithGURL(_dropData.url());IIRC, this is the bit that produces the drag data asynchronously. But we only ever produce one URL–so does that mean this only sort of works on Mac?
}It's too bad `base::JoinString()` doesn't take a projection.
You could also save some steps here: if the GURLs are valid, they should only contain ASCII characters. So we can do:
```
std::u16string merged_urls;
for (const auto& url : drop_data.urls) {
DCHECK(base::IsStringASCII(url.spec());
if (!merged_urls.empty()) {
merged_urls += "\r\n";
}
merged_urls.append(url.spec().begin(), url.spec().end());
}
```
But probably we should just change the Mojo type to avoid these conversions. I suppose that's mostly out of scope here though.
std::vector<std::u16string> urls16 = base::SplitString(Use SplitStringPiece() here please, to avoid allocating a new string for each thing.
if (std::optional<ui::OSExchangeData::UrlInfo> url = data.GetURLAndTitle(rename this url_info; that was my oversight when I wrote this originally
url.has_value() && !url->urls.empty()) {I think we should probably guarantee `urls` is non-empty if we return a `url_info` here.
GURL url() const;Remove this, or have it return a `const GURL*`, returning non-null only if `urls` is non-empty.
std::vector<std::u16string> urls16 = base::SplitString(SplitStringPiece() here as well, to avoid allocations.
That being said... I'm not sure if this is really correct. This is used to process some pre-existing formats on the clipboard, which expect to be a certain shape.
// Mozilla URL format or Unicode URLe.g. do we really expect a list of URLs in these clipboard formats? Will other apps expect a list of URLs/incorrectly treat the second URL as the title? If I remember correctly, this format isn't determined solely by Chrome; it's used for interop with other OS apps.
urls = {net::FilePathToFileURL(base::FilePath(filenames[0]))};Since we're using a vector-based approach now, let's avoid even adding the GURL to the list if it's invalid.
std::u16string title;There should be a comment here that states that this is the title for the first URL, if there is any.
GURL url() const;
~UrlInfo();
std::vector<GURL> urls;I'm not an owner of this directory so will defer to the right folks - not sure how I feel about having urls get added to this, but "url" and "title" represent the first one. Just wanted to flag it!
I'm fine with this (modulo the plumbing questions), but I think this should also have a comment/guarantee about not including invalid GURLs in the list.
GURL url = urls.empty() ? GURL() : urls.front();Is it really OK to drop the rest of the URLs on the floor here?
NSArray<NSPasteboardItem*>* items = clipboard_util::PasteboardItemsFromUrls(Do we really want to continue with the rest of this work if there aren't any URLs? I'm not sure we would have called this before... would we?
title_ = title;We should not do any of this if urls is empty (probably we should not call this function at all)
std::string url_list;Same comment here; we probably don't want to allow calling this function with an empty list of URLs.
ClipboardFormatType::UrlType().ToFormatEtc(), storage));I'm not sure that it's valid to pass a URL list here.
https://learn.microsoft.com/en-us/windows/win32/shell/clipboard#cfstr_ineturl
"This format gives the best clipboard representation of a single URL."
Related details
- Joone Hur
- Min Qin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman voted and added 3 comments![]( "Open in Gerrit")
Votes added by Avi Drissman
| Code-Review | -1 |
3 comments
I object to the design. URLs have titles, and if we want to plumb a list of URLs, we need to plumb a parallel list of titles.
GURL url = urls.empty() ? GURL() : urls.front();Is it really OK to drop the rest of the URLs on the floor here?
We have a call to create multiple pasteboard items (`clipboard_util::PasteboardItemsFromUrls`) and a call to put multiple pasteboard items onto the pasteboard (`clipboard_util::AddDataToPasteboard`). It’s more work to drop the extra URLs than to just remove the call to `.firstObject` below.
void SetURLs(const std::vector<GURL>& urls,
std::u16string_view title) override;This API doesn’t make sense. Each URL has a title, so we’d need a parallel array of titles.
Related details
- Joone Hur
- Min Qin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman added 1 comment![]( "Open in Gerrit")
// Holds one or more URLs, such as those from dragging links or images.
// For text/uri-list drag types, this contains multiple URLs.
// https://www.rfc-editor.org/rfc/rfc2483#section-5
std::vector<GURL> urls;
std::u16string url_title; // The title associated with `url`.
Same objection here; each URL should be matched with a title (an optional one, but still).
Avi Drissman (Gerrit)
Avi Drissman added 1 comment![]( "Open in Gerrit")
drop_data.urls = {
GURL(base::SysNSStringToUTF8(urls_and_titles.firstObject.URL))};
drop_data.url_title =
base::SysNSStringToUTF16(urls_and_titles.firstObject.title);
}Please wire up all URLs here. This code dropped all but the first on the floor because `DropData` couldn’t handle more than one. With this CL it can, so we should take advantage of that.
Rakina Zata Amni (Gerrit)
Rakina Zata Amni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rakina Zata Amni
| Code-Review | +0 |
content/ changes LGTM, sorry for the delay!
Ah, I only looked at the changes as a "change URL -> list of URLs" change, but it looks like there are more concerns for this around the titles etc. Since avi@ is also a content/ owner and has concerns about this, let me remove my +1 and move myself to cc (sorry about the confusion, and thanks avi@ and dcheng@!).
Related details
- Joone Hur
- Min Qin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Avi Drissman
| Code-Review | +0 |
Rakina Zata Amnicontent/ changes LGTM, sorry for the delay!
Ah, I only looked at the changes as a "change URL -> list of URLs" change, but it looks like there are more concerns for this around the titles etc. Since avi@ is also a content/ owner and has concerns about this, let me remove my +1 and move myself to cc (sorry about the confusion, and thanks avi@ and dcheng@!).
No problem.
This is a “url → list of urls” change, but if we do that (and I am happy to see such a change!) it should be done fully.
Related details
- Joone Hur
- Min Qin
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
Thanks for the feedback! I will update this CL this week.
Related details
- Min Qin
- Rakina Zata Amni
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joone Hur (Gerrit)
Joone Hur added 20 comments![]( "Open in Gerrit")
I’ve updated the CL based on your suggestions. The major change is the introduction of the ui::ClipboardUrlInfo struct, which holds a URL and its title. This struct is now used to support multiple URLs via a std::vector<ui::ClipboardUrlInfo>, replacing the previous urlInfo in OSExchangeData and the url & url_title properties in DropData.
There are many updates in this CL. Please let me know if I missed anything or if further changes are needed.
if (data.urls.empty() || !data.urls.front().is_valid() || is_in_split_view) {Should we really have invalid URLs here?
I think we should avoid having invalid URLs in this list; instead, whether or not the list is empty should represent whether or not URLs are present.
Done
IIRC, this is the bit that produces the drag data asynchronously. But we only ever produce one URL–so does that mean this only sort of works on Mac?
url() is intended to return a single URL so it should also work on Mac. I'll replace url() with front().url here.
std::vector<std::u16string> urls16 = base::SplitString(Use SplitStringPiece() here please, to avoid allocating a new string for each thing.
Done
if (std::optional<ui::OSExchangeData::UrlInfo> url = data.GetURLAndTitle(rename this url_info; that was my oversight when I wrote this originally
Done
I think we should probably guarantee `urls` is non-empty if we return a `url_info` here.
Done
drop_data.urls = {
GURL(base::SysNSStringToUTF8(urls_and_titles.firstObject.URL))};
drop_data.url_title =
base::SysNSStringToUTF16(urls_and_titles.firstObject.title);
}Please wire up all URLs here. This code dropped all but the first on the floor because `DropData` couldn’t handle more than one. With this CL it can, so we should take advantage of that.
Done
// Holds one or more URLs, such as those from dragging links or images.
// For text/uri-list drag types, this contains multiple URLs.
// https://www.rfc-editor.org/rfc/rfc2483#section-5
std::vector<GURL> urls;
std::u16string url_title; // The title associated with `url`.
Same objection here; each URL should be matched with a title (an optional one, but still).
Done
Remove this, or have it return a `const GURL*`, returning non-null only if `urls` is non-empty.
Done
std::vector<std::u16string> urls16 = base::SplitString(SplitStringPiece() here as well, to avoid allocations.
That being said... I'm not sure if this is really correct. This is used to process some pre-existing formats on the clipboard, which expect to be a certain shape.
Done
e.g. do we really expect a list of URLs in these clipboard formats? Will other apps expect a list of URLs/incorrectly treat the second URL as the title? If I remember correctly, this format isn't determined solely by Chrome; it's used for interop with other OS apps.
According to the spec, they should return a list of URLs, but Chromium doesn't handle multiple URLs properly—this is the core issue. While the Windows clipboard format(CFSTR_INETURL) isn't designed for multiple URLs, we can still set them using a delimiter. On MacOS, this change enables support for multiple URLs.
urls = {net::FilePathToFileURL(base::FilePath(filenames[0]))};Since we're using a vector-based approach now, let's avoid even adding the GURL to the list if it's invalid.
Done
There should be a comment here that states that this is the title for the first URL, if there is any.
With the proposed change to use std::vector<ClipboardUrlInfo>, each UrlInfo object in the vector can hold its own title.
GURL url() const;
~UrlInfo();
std::vector<GURL> urls;Daniel ChengI'm not an owner of this directory so will defer to the right folks - not sure how I feel about having urls get added to this, but "url" and "title" represent the first one. Just wanted to flag it!
I'm fine with this (modulo the plumbing questions), but I think this should also have a comment/guarantee about not including invalid GURLs in the list.
Instead of modifying struct UrlInfo, I'll use std::vector<ClipboardUrlInfo> to hold the URL information.
GURL url = urls.empty() ? GURL() : urls.front();Avi DrissmanIs it really OK to drop the rest of the URLs on the floor here?
We have a call to create multiple pasteboard items (`clipboard_util::PasteboardItemsFromUrls`) and a call to put multiple pasteboard items onto the pasteboard (`clipboard_util::AddDataToPasteboard`). It’s more work to drop the extra URLs than to just remove the call to `.firstObject` below.
can we separate the multiple URL support for Mac into a follow-up change?
NSArray<NSPasteboardItem*>* items = clipboard_util::PasteboardItemsFromUrls(Do we really want to continue with the rest of this work if there aren't any URLs? I'm not sure we would have called this before... would we?
We need to return early if urls is empty here. I'll add that check.
We should not do any of this if urls is empty (probably we should not call this function at all)
Done
Same comment here; we probably don't want to allow calling this function with an empty list of URLs.
Done
ClipboardFormatType::UrlType().ToFormatEtc(), storage));I'm not sure that it's valid to pass a URL list here.
https://learn.microsoft.com/en-us/windows/win32/shell/clipboard#cfstr_ineturl
"This format gives the best clipboard representation of a single URL."
For multiple URLs, we can provide them in CF_TEXT as a standard fallback. For a single URL, we need to continue providing it in CFSTR_INETURL for compatibility with other applications.
void SetURLs(const std::vector<GURL>& urls,
std::u16string_view title) override;This API doesn’t make sense. Each URL has a title, so we’d need a parallel array of titles.
- Avi Drissman
- Daniel Cheng
- David Trainor
- Min Qin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman added 7 comments![]( "Open in Gerrit")
EXPECT_EQ(page_url, url_infos->front().url);
EXPECT_EQ(page_title, url_infos->front().title);
#if !BUILDFLAG(IS_WIN)Check for exactly one value in `url_infos` before checking the url/title?
#if BUILDFLAG(IS_MAC)
// On the Mac, the clipboard can hold multiple items, each with
// different representations. Therefore, when the "write multiple URLs"
// call is made, a full-fledged array of objects and types are written
// to the clipboard, providing rich interoperability with the rest of
// the OS and other apps. Then, when `GetURLAndTitle` is called, it
// looks at the clipboard, sees URL and title data, and returns true.
std::optional<std::vector<ui::ClipboardUrlInfo>> url_infos =
drag_data->GetURLAndTitle(
ui::FilenameToURLPolicy::DO_NOT_CONVERT_FILENAMES);
ASSERT_TRUE(url_infos.has_value());
// The bookmarks are added in order, and the first is retrieved, so
// expect the values from the first bookmark.
EXPECT_EQ(page_title, url_infos->front().title);
EXPECT_EQ(page_url, url_infos->front().url);
#else
// On other platforms, because they don't have the concept of multiple
// items on the clipboard, single URLs are added as a URL, but multiple
// URLs are added as a data blob opaque to the outside world. Then, when
// `GetURLAndTitle` is called, it's unable to extract any single URL,
// and returns false.
EXPECT_FALSE(drag_data
->GetURLAndTitle(
ui::FilenameToURLPolicy::DO_NOT_CONVERT_FILENAMES)
.has_value());
#endif
#if !BUILDFLAG(IS_WIN)This difference in behavior between Mac/non-Mac is due to the exact thing that you’re changing in this CL. For followup, would we need to change this section?
DCHECK(!_dropData.url_infos.empty());
NSURL* url = net::NSURLWithGURL(_dropData.url_infos.front().url);
// If NSURL creation failed, check for a badly-escaped JavaScript URL.
// Strip out any existing escapes and then re-escape uniformly.
if (!url &&
_dropData.url_infos.front().url.SchemeIs(url::kJavaScriptScheme)) {
std::string unescapedUrlString = base::UnescapeBinaryURLComponent(
_dropData.url_infos.front().url.spec());
std::string escapedUrlString =
base::EscapeUrlEncodedData(unescapedUrlString, false);
url = [NSURL URLWithString:base::SysUTF8ToNSString(escapedUrlString)];
}
return url.absoluteString;
}
// URL title.
if ([type isEqualToString:ui::kUTTypeUrlName]) {
return base::SysUTF16ToNSString(_dropData.url_infos.front().title);
}Oh boy.
The problem here is that this code assumes that the webpage is initiating a drag of a single object. We’d have to rethink how to do this if we have to initiate a drag of multiple objects.
This is fine for now but can you file a bug for followup? This is going to be a longer-range refactor of the code.
IPC_STRUCT_TRAITS_BEGIN(ui::ClipboardUrlInfo)Can you define a `ui::` structure here in content? Do we still even use these macros files with mojo?
std::optional<std::vector<ClipboardUrlInfo>> GetURLAndTitle(
FilenameToURLPolicy policy) const;
std::optional<std::vector<ui::ClipboardUrlInfo>> GetURLs(
FilenameToURLPolicy policy) const;
// Return information about the contained files, if any.The bifurcation was “get one url and its title” and “get many urls but no titles”. This is a chance to unify those two, and since we already have to change every caller, can we merge these together as `GetURLs()`? From this interface it seems these are redundant.
std::optional<std::vector<ui::ClipboardUrlInfo>> GetURLs(You’re in the `ui::` namespace so no need to qualify here.
GURL url = urls.empty() ? GURL() : urls.front();Avi DrissmanIs it really OK to drop the rest of the URLs on the floor here?
Joone HurWe have a call to create multiple pasteboard items (`clipboard_util::PasteboardItemsFromUrls`) and a call to put multiple pasteboard items onto the pasteboard (`clipboard_util::AddDataToPasteboard`). It’s more work to drop the extra URLs than to just remove the call to `.firstObject` below.
can we separate the multiple URL support for Mac into a follow-up change?
That sounds fine, as this leaves the Mac impl no worse off than today and this is getting to be a big CL. As a Mac person, happy to advise if you have questions when you follow up.
Related details
- Daniel Cheng
- David Trainor
- Joone Hur
- Min Qin
Joone Hur (Gerrit)
Joone Hur voted and added 6 comments![]( "Open in Gerrit")
Votes added by Joone Hur
| Commit-Queue | +1 |
6 comments
EXPECT_EQ(page_url, url_infos->front().url);
EXPECT_EQ(page_title, url_infos->front().title);
#if !BUILDFLAG(IS_WIN)Check for exactly one value in `url_infos` before checking the url/title?
Done
#if BUILDFLAG(IS_MAC)This difference in behavior between Mac/non-Mac is due to the exact thing that you’re changing in this CL. For followup, would we need to change this section?
Sure, the behavior of handling multiple URLs, similar to Mac, can now be applied to other platforms. For this, we won't need to run the has_single_url() in BookmarkNodeData::Write because other platforms can also support multiple URLs.
DCHECK(!_dropData.url_infos.empty());
NSURL* url = net::NSURLWithGURL(_dropData.url_infos.front().url);
// If NSURL creation failed, check for a badly-escaped JavaScript URL.
// Strip out any existing escapes and then re-escape uniformly.
if (!url &&
_dropData.url_infos.front().url.SchemeIs(url::kJavaScriptScheme)) {
std::string unescapedUrlString = base::UnescapeBinaryURLComponent(
_dropData.url_infos.front().url.spec());
std::string escapedUrlString =
base::EscapeUrlEncodedData(unescapedUrlString, false);
url = [NSURL URLWithString:base::SysUTF8ToNSString(escapedUrlString)];
}
return url.absoluteString;
}
// URL title.
if ([type isEqualToString:ui::kUTTypeUrlName]) {
return base::SysUTF16ToNSString(_dropData.url_infos.front().title);
}Oh boy.
The problem here is that this code assumes that the webpage is initiating a drag of a single object. We’d have to rethink how to do this if we have to initiate a drag of multiple objects.
This is fine for now but can you file a bug for followup? This is going to be a longer-range refactor of the code.
Sure, I will file a bug for this.
Can you define a `ui::` structure here in content? Do we still even use these macros files with mojo?
Chromium still uses the legacy IPC system to transmit DropData on MacOS, and ClipboardUrlInfo is part of that structure. Since other ui:: types like ui::FileInfo are already defined here for the same reason, it makes sense to include ClipboardUrlInfo as well.
std::optional<std::vector<ClipboardUrlInfo>> GetURLAndTitle(
FilenameToURLPolicy policy) const;
std::optional<std::vector<ui::ClipboardUrlInfo>> GetURLs(
FilenameToURLPolicy policy) const;
// Return information about the contained files, if any.The bifurcation was “get one url and its title” and “get many urls but no titles”. This is a chance to unify those two, and since we already have to change every caller, can we merge these together as `GetURLs()`? From this interface it seems these are redundant.
Yes, GetURLs calls GetURLAndTitle so we could merge them into one. However, each platform has different implementation, so this change would be a bit large. Can I separate this for follow-up CL?
std::optional<std::vector<ui::ClipboardUrlInfo>> GetURLs(You’re in the `ui::` namespace so no need to qualify here.
- Avi Drissman
- Daniel Cheng
- David Trainor
- Min Qin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Avi Drissman
| Code-Review | +1 |
1 comment
std::optional<std::vector<ClipboardUrlInfo>> GetURLAndTitle(
FilenameToURLPolicy policy) const;
std::optional<std::vector<ui::ClipboardUrlInfo>> GetURLs(
FilenameToURLPolicy policy) const;
// Return information about the contained files, if any.Joone HurThe bifurcation was “get one url and its title” and “get many urls but no titles”. This is a chance to unify those two, and since we already have to change every caller, can we merge these together as `GetURLs()`? From this interface it seems these are redundant.
Yes, GetURLs calls GetURLAndTitle so we could merge them into one. However, each platform has different implementation, so this change would be a bit large. Can I separate this for follow-up CL?
- Daniel Cheng
- David Trainor
- Joone Hur
- Min Qin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joone Hur (Gerrit)
Joone Hur added 4 comments![]( "Open in Gerrit")
My biggest concern is about the platform-specific plumbing. I'm not sure many OSes actually support multiple URLs in their default URL type (see the comments about Windows and Mac). We may need to do something else to make it work well.
My other comment is about the invariants we want to maintain. To make the APIs easier to use, I propose:
- don't allow any invalid GURLs in the various URL lists. So when we're parsing out the URL lists, that means validating that they're actually valid URLs before appending them to the list.
- an empty list means no URLs at all
- the various SetURL()/SetURLs() functions should only be called if there is a non-empty list of URLs
Done
@dch...@chromium.org could you review the updated CL? I've updated the CL to ensure invalid URLs are not added to the URL lists. Additionally, the SetURL/SetURLs functions are now only called when there is a non-empty list of valid URLs to transfer.
std::optional<std::vector<ClipboardUrlInfo>> GetURLAndTitle(
FilenameToURLPolicy policy) const;
std::optional<std::vector<ui::ClipboardUrlInfo>> GetURLs(
FilenameToURLPolicy policy) const;
// Return information about the contained files, if any.Joone HurThe bifurcation was “get one url and its title” and “get many urls but no titles”. This is a chance to unify those two, and since we already have to change every caller, can we merge these together as `GetURLs()`? From this interface it seems these are redundant.
Avi DrissmanYes, GetURLs calls GetURLAndTitle so we could merge them into one. However, each platform has different implementation, so this change would be a bit large. Can I separate this for follow-up CL?
As long as this is a direct followup, yes.
Sure!
ClipboardFormatType::UrlType().ToFormatEtc(), storage));Joone HurI'm not sure that it's valid to pass a URL list here.
https://learn.microsoft.com/en-us/windows/win32/shell/clipboard#cfstr_ineturl
"This format gives the best clipboard representation of a single URL."
For multiple URLs, we can provide them in CF_TEXT as a standard fallback. For a single URL, we need to continue providing it in CFSTR_INETURL for compatibility with other applications.
For this CL, we're maintaining this text/uri-list approach for multi-URL writes as it works across browsers because it follows the Web standard(text/uri-list). We can create a follow-up bug to investigate non-browser application compatibility more thoroughly if desired.
Related details
- Daniel Cheng
- David Trainor
- Min Qin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Min Qin (Gerrit)
Min Qin voted Code-Review+1![]( "Open in Gerrit")
- Daniel Cheng
- David Trainor
- Joone Hur
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sylvain Defresne (Gerrit)
Sylvain Defresne voted and added 1 comment![]( "Open in Gerrit")
Related details
- Daniel Cheng
- David Trainor
- Joone Hur
Daniel Cheng (Gerrit)
Daniel Cheng added 3 comments![]( "Open in Gerrit")
void SetURLs(const std::vector<ClipboardUrlInfo>& url_infos) override;Change this to take a span; that way callers just passing a single url don't have to construct a vector.
// TODO(joone): we should support multiple URLs, ClipboardFormatType::UrlType().ToFormatEtc(), storage));Joone HurI'm not sure that it's valid to pass a URL list here.
https://learn.microsoft.com/en-us/windows/win32/shell/clipboard#cfstr_ineturl
"This format gives the best clipboard representation of a single URL."
Joone HurFor multiple URLs, we can provide them in CF_TEXT as a standard fallback. For a single URL, we need to continue providing it in CFSTR_INETURL for compatibility with other applications.
For this CL, we're maintaining this text/uri-list approach for multi-URL writes as it works across browsers because it follows the Web standard(text/uri-list). We can create a follow-up bug to investigate non-browser application compatibility more thoroughly if desired.
I don't think I understand this comment. It might happen to work across browsers, but this concretely changes what we write to the clipboard.
Before, we would write out something that was a bunch of URLs concatenated together... but canonicalized to be a valid URL.
After this though, we can write something that another app, trying to interpret as a **single** URL (which is what the type claims to represent) that would now be invalid. It doesn't matter that Chrome itself is updated to handle this case correctly; we need to care about compatibility with non-Chrome apps too
We either need to take the Mac approach (where we drop everything but the first URL for now), or we should figure out how to properly plumb it through. What we should not do is ignore the MSDN docs that `CFSTR_INETURL` represents a single URL and just write out multiple URLs here.
Related details
- David Trainor
- Joone Hur
David Trainor (Gerrit)
David Trainor added 2 comments![]( "Open in Gerrit")
if (data.urls.empty() || !data.urls.front().is_valid() || is_in_split_view) {Joone HurShould we really have invalid URLs here?
I think we should avoid having invalid URLs in this list; instead, whether or not the list is empty should represent whether or not URLs are present.
Done
Just checking - is the plan to move is_valid() checks to the call sites that build the ClipboardUrlInfo instances?
Related details
- Joone Hur
Joone Hur (Gerrit)
Joone Hur added 1 comment![]( "Open in Gerrit")
if (data.urls.empty() || !data.urls.front().is_valid() || is_in_split_view) {Joone HurShould we really have invalid URLs here?
I think we should avoid having invalid URLs in this list; instead, whether or not the list is empty should represent whether or not URLs are present.
David TrainorDone
Just checking - is the plan to move is_valid() checks to the call sites that build the ClipboardUrlInfo instances?
- Daniel Cheng
- David Trainor
Joone Hur (Gerrit)
Joone Hur added 3 comments![]( "Open in Gerrit")
void SetURLs(const std::vector<ClipboardUrlInfo>& url_infos) override;Change this to take a span; that way callers just passing a single url don't have to construct a vector.
Will do.
// TODO(joone): we should support multiple URLs,Instead of a username, use crbug.com/41011768 here.
Will do.
ClipboardFormatType::UrlType().ToFormatEtc(), storage));Joone HurI'm not sure that it's valid to pass a URL list here.
https://learn.microsoft.com/en-us/windows/win32/shell/clipboard#cfstr_ineturl
"This format gives the best clipboard representation of a single URL."
Joone HurFor multiple URLs, we can provide them in CF_TEXT as a standard fallback. For a single URL, we need to continue providing it in CFSTR_INETURL for compatibility with other applications.
Daniel ChengFor this CL, we're maintaining this text/uri-list approach for multi-URL writes as it works across browsers because it follows the Web standard(text/uri-list). We can create a follow-up bug to investigate non-browser application compatibility more thoroughly if desired.
I don't think I understand this comment. It might happen to work across browsers, but this concretely changes what we write to the clipboard.
Before, we would write out something that was a bunch of URLs concatenated together... but canonicalized to be a valid URL.
After this though, we can write something that another app, trying to interpret as a **single** URL (which is what the type claims to represent) that would now be invalid. It doesn't matter that Chrome itself is updated to handle this case correctly; we need to care about compatibility with non-Chrome apps too
We either need to take the Mac approach (where we drop everything but the first URL for now), or we should figure out how to properly plumb it through. What we should not do is ignore the MSDN docs that `CFSTR_INETURL` represents a single URL and just write out multiple URLs here.
This CL addresses a bug by aligning Chromium’s handling of the text/uri-list drag type with Firefox, using the MIME type defined by the web standard. While the Windows CFSTR_INETURL clipboard format is traditionally intended for a single URL, it appears to tolerate multiple URLs without issue—it doesn’t seem to validate them strictly, and arbitrary strings are accepted
That said, I understand the concern: writing multiple URLs under a format that some applications interpret as a single URL could lead to compatibility issues. However, the current behavior already causes problems—by concatenating multiple URLs into a single, invalid URL string.
If this CL introduces issues with non-browser applications, Firefox would likely exhibit the same behavior, since it uses the same MIME type.
I propose we land this CL to fix the existing bug and improve standards alignment. In parallel, we can file a follow-up issue to investigate how non-browser applications interpret multi-URL clipboard content.
David Trainor (Gerrit)
David Trainor added 1 comment![]( "Open in Gerrit")
if (data.urls.empty() || !data.urls.front().is_valid() || is_in_split_view) {Joone HurShould we really have invalid URLs here?
I think we should avoid having invalid URLs in this list; instead, whether or not the list is empty should represent whether or not URLs are present.
David TrainorDone
Joone HurJust checking - is the plan to move is_valid() checks to the call sites that build the ClipboardUrlInfo instances?
Yes
It looks like some of the behavior stayed as DCHECKS or the is_valid() checks weren't added in all the callsites? Sorry if I'm missing something - can you double check and validate all the ClipboardUrlInfo creation paths are omitting !is_valid() URLs? Will defer to Daniel if we're okay with a behavioral change here though.
Related details
- Daniel Cheng
- Joone Hur
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 4 comments![]( "Open in Gerrit")
if (data.urls.empty() || !data.urls.front().is_valid() || is_in_split_view) {Joone HurShould we really have invalid URLs here?
I think we should avoid having invalid URLs in this list; instead, whether or not the list is empty should represent whether or not URLs are present.
David TrainorDone
Joone HurJust checking - is the plan to move is_valid() checks to the call sites that build the ClipboardUrlInfo instances?
David TrainorYes
It looks like some of the behavior stayed as DCHECKS or the is_valid() checks weren't added in all the callsites? Sorry if I'm missing something - can you double check and validate all the ClipboardUrlInfo creation paths are omitting !is_valid() URLs? Will defer to Daniel if we're okay with a behavioral change here though.
yeah, ideally I think the bits in ui/base that parse URLs out of the data object/whatever should not be populating URLs that are invalid. Some platforms do this; some do not.
But in general, the caller should be able to just `DCHECK()` and rely on the abstractions around the platform to handle this.
ClipboardFormatType::UrlType().ToFormatEtc(), storage));Joone HurI'm not sure that it's valid to pass a URL list here.
https://learn.microsoft.com/en-us/windows/win32/shell/clipboard#cfstr_ineturl
"This format gives the best clipboard representation of a single URL."
Joone HurFor multiple URLs, we can provide them in CF_TEXT as a standard fallback. For a single URL, we need to continue providing it in CFSTR_INETURL for compatibility with other applications.
Daniel ChengFor this CL, we're maintaining this text/uri-list approach for multi-URL writes as it works across browsers because it follows the Web standard(text/uri-list). We can create a follow-up bug to investigate non-browser application compatibility more thoroughly if desired.
Joone HurI don't think I understand this comment. It might happen to work across browsers, but this concretely changes what we write to the clipboard.
Before, we would write out something that was a bunch of URLs concatenated together... but canonicalized to be a valid URL.
After this though, we can write something that another app, trying to interpret as a **single** URL (which is what the type claims to represent) that would now be invalid. It doesn't matter that Chrome itself is updated to handle this case correctly; we need to care about compatibility with non-Chrome apps too
We either need to take the Mac approach (where we drop everything but the first URL for now), or we should figure out how to properly plumb it through. What we should not do is ignore the MSDN docs that `CFSTR_INETURL` represents a single URL and just write out multiple URLs here.
This CL addresses a bug by aligning Chromium’s handling of the text/uri-list drag type with Firefox, using the MIME type defined by the web standard. While the Windows CFSTR_INETURL clipboard format is traditionally intended for a single URL, it appears to tolerate multiple URLs without issue—it doesn’t seem to validate them strictly, and arbitrary strings are accepted
That said, I understand the concern: writing multiple URLs under a format that some applications interpret as a single URL could lead to compatibility issues. However, the current behavior already causes problems—by concatenating multiple URLs into a single, invalid URL string.
If this CL introduces issues with non-browser applications, Firefox would likely exhibit the same behavior, since it uses the same MIME type.
I propose we land this CL to fix the existing bug and improve standards alignment. In parallel, we can file a follow-up issue to investigate how non-browser applications interpret multi-URL clipboard content.
Just because Firefox does it doesn't make it correct. Drag and drop is a compatibility nightmare because people didn't read the docs and just write whatever they want into the X selection. We shouldn't match compatibility with Firefox and just assume nothing else will break.
I'm assuming the Firefox snippet in question is https://searchfox.org/mozilla-central/source/widget/windows/nsClipboard.cpp#257, and it's really not clear to me that it's intentional anyway...
I see two paths forward here.
1. Microsoft updates the MSDN docs to specifically state multiple URLs are allowed and dictates a format. Presumably someone will need to do this compatibility testing.
2. We don't break compatibility with other, pre-existing Windows apps and figure out another way to transport this data (probably using another clipboard format–potentially custom–that specifically supports multiple URLs)
if (!test_url.SchemeIsFile() ||We should also probably skip adding here if the URL is invalid?
Related details
- David Trainor
- Joone Hur
Joone Hur (Gerrit)
Joone Hur added 3 comments![]( "Open in Gerrit")
ClipboardFormatType::UrlType().ToFormatEtc(), storage));Joone HurI'm not sure that it's valid to pass a URL list here.
https://learn.microsoft.com/en-us/windows/win32/shell/clipboard#cfstr_ineturl
"This format gives the best clipboard representation of a single URL."
Joone HurFor multiple URLs, we can provide them in CF_TEXT as a standard fallback. For a single URL, we need to continue providing it in CFSTR_INETURL for compatibility with other applications.
Daniel ChengFor this CL, we're maintaining this text/uri-list approach for multi-URL writes as it works across browsers because it follows the Web standard(text/uri-list). We can create a follow-up bug to investigate non-browser application compatibility more thoroughly if desired.
Joone HurI don't think I understand this comment. It might happen to work across browsers, but this concretely changes what we write to the clipboard.
Before, we would write out something that was a bunch of URLs concatenated together... but canonicalized to be a valid URL.
After this though, we can write something that another app, trying to interpret as a **single** URL (which is what the type claims to represent) that would now be invalid. It doesn't matter that Chrome itself is updated to handle this case correctly; we need to care about compatibility with non-Chrome apps too
We either need to take the Mac approach (where we drop everything but the first URL for now), or we should figure out how to properly plumb it through. What we should not do is ignore the MSDN docs that `CFSTR_INETURL` represents a single URL and just write out multiple URLs here.
Daniel ChengThis CL addresses a bug by aligning Chromium’s handling of the text/uri-list drag type with Firefox, using the MIME type defined by the web standard. While the Windows CFSTR_INETURL clipboard format is traditionally intended for a single URL, it appears to tolerate multiple URLs without issue—it doesn’t seem to validate them strictly, and arbitrary strings are accepted
That said, I understand the concern: writing multiple URLs under a format that some applications interpret as a single URL could lead to compatibility issues. However, the current behavior already causes problems—by concatenating multiple URLs into a single, invalid URL string.
If this CL introduces issues with non-browser applications, Firefox would likely exhibit the same behavior, since it uses the same MIME type.
I propose we land this CL to fix the existing bug and improve standards alignment. In parallel, we can file a follow-up issue to investigate how non-browser applications interpret multi-URL clipboard content.
Just because Firefox does it doesn't make it correct. Drag and drop is a compatibility nightmare because people didn't read the docs and just write whatever they want into the X selection. We shouldn't match compatibility with Firefox and just assume nothing else will break.
I'm assuming the Firefox snippet in question is https://searchfox.org/mozilla-central/source/widget/windows/nsClipboard.cpp#257, and it's really not clear to me that it's intentional anyway...
I see two paths forward here.
1. Microsoft updates the MSDN docs to specifically state multiple URLs are allowed and dictates a format. Presumably someone will need to do this compatibility testing.
2. We don't break compatibility with other, pre-existing Windows apps and figure out another way to transport this data (probably using another clipboard format–potentially custom–that specifically supports multiple URLs)
I will investigate whether creating a custom clipboard format with the text/uri-list format enables proper support for multiple URLs.
We should also probably skip adding here if the URL is invalid?
- Daniel Cheng
- David Trainor
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Trainor (Gerrit)
David Trainor added 4 comments![]( "Open in Gerrit")
general lgtm on chrome/browser integration pieces - only question is around the std::optional has_value() and the assumption on empty() on the underlying vector if it's present.
CHECK(!url_infos->empty());Worth still checking has_value() as well?
url_infos.has_value()) {Do you have to check that the vector is not empty? If that's never supposed to happen if the optional has a value, can we CHECK inside of the if statement?
if (url_info.has_value() && prefs_) {Do you have to check that the vector is not empty here as well?
Related details
- Daniel Cheng
- Joone Hur
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joone Hur (Gerrit)
Joone Hur added 3 comments![]( "Open in Gerrit")
Worth still checking has_value() as well?
Yes, a condition will be added to explicitly check url_infos.has_value() to ensure the presence of URLs before further processing.
Do you have to check that the vector is not empty? If that's never supposed to happen if the optional has a value, can we CHECK inside of the if statement?
Done
Do you have to check that the vector is not empty here as well?
- Avi Drissman
- Daniel Cheng
- David Trainor
- Min Qin
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
| 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")
@dch...@chromium.org ClipboardFormatType::MultiUrlType() has been added for the text/uri-list format. GetUrl() (in clipboard_util_win.cc) and OSExchangeDataProviderWin::SetURLs (in os_exchange_data_provider_win.cc) now use this custom clipboard format to hanlde multiple URLs properly.
Sylvain Defresne (Gerrit)
Sylvain Defresne voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sylvain Defresne
| Code-Review | +1 |
- Avi Drissman
- Daniel Cheng
- David Trainor
- Joone Hur
- Min Qin
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman voted Code-Review+1![]( "Open in Gerrit")
Daniel Cheng (Gerrit)
Daniel Cheng added 2 comments![]( "Open in Gerrit")
// Parses a multi-line string from the clipboard to extract URLs andI think it would be better if we didn't change our interpretation of existing types, unless it's shown to improve compatiblity or fix other bugs.
This also changes the behavior; before, if we only had a URL, we'd set the string as both the URL and the title. After this, we no longer do that.
In addition, if we add a new function for parsing text/uri-list, maybe it should match the behavior of reusing the URL's spec as the title if there is no explict title? (I'll note the RFC for that MIME type also specifically states that comment lines are allowed, though it seems OK to punt on that specific part of it for now: https://www.rfc-editor.org/rfc/rfc2483.html#section-5)
return true;We should check that the list is non-empty here right? Is there a reason we shouldn't?
Related details
- David Trainor
- Joone Hur
- Min Qin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Trainor (Gerrit)
David Trainor added 2 comments![]( "Open in Gerrit")
CHECK(!url_infos->empty());Joone HurWorth still checking has_value() as well?
Yes, a condition will be added to explicitly check url_infos.has_value() to ensure the presence of URLs before further processing.
The previous path looked like it worked via:
- std::optional has_value - CHECK
- Empty unchecked.
The current path looks like it works via:
- std::optional has_value - return early
- Empty - CHECK
Is this behavioral change intentional?
CHECK(!url_infos->empty());This looks like a behavioral change as well - if we're pruning out invalid URLs before we get to this point, is the empty check something we should CHECK() on or just if-check? We if-checked' the is_valid() previously, which would be reflected as an empty list post change right?
Related details
- Joone Hur
- Min Qin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
// Parses a multi-line string from the clipboard to extract URLs andI think it would be better if we didn't change our interpretation of existing types, unless it's shown to improve compatiblity or fix other bugs.
This also changes the behavior; before, if we only had a URL, we'd set the string as both the URL and the title. After this, we no longer do that.
In addition, if we add a new function for parsing text/uri-list, maybe it should match the behavior of reusing the URL's spec as the title if there is no explict title? (I'll note the RFC for that MIME type also specifically states that comment lines are allowed, though it seems OK to punt on that specific part of it for now: https://www.rfc-editor.org/rfc/rfc2483.html#section-5)
I tried to update SplitUrlAndTitle to use the URL itself as the title when no explicit title was provided. However, this led to many test failures because the Linux port expects an empty title in this scenario. It appears `title->assign(str)` is never called even if the str doesn't include a title because the string received by SplitUrlAndTitle (from the clipboard storage) has a \n at its end in this case. This is due to how URLs are stored in the clipboard: even if the title is empty, a newline is always appended after the URL. See SetURL code:
```cpp
void OSExchangeDataProviderWin::SetURL(const GURL& url,
std::u16string_view title) {
// Add text/x-moz-url for drags from Firefox
STGMEDIUM storage = CreateStorageForString(
base::StrCat({base::UTF8ToUTF16(url.spec()), u"\n", title}));
data_->contents_.push_back(DataObjectImpl::StoredDataInfo::TakeStorageMedium(
ClipboardFormatType::MozUrlType().ToFormatEtc(), storage));
```
Therefore, this CL doesn't change the existing single-URL behavior while also correctly parsing multiple URLs.
Related details
- Daniel Cheng
- Min Qin
Joone Hur (Gerrit)
Joone Hur added 2 comments![]( "Open in Gerrit")
CHECK(!url_infos->empty());Joone HurWorth still checking has_value() as well?
David TrainorYes, a condition will be added to explicitly check url_infos.has_value() to ensure the presence of URLs before further processing.
The previous path looked like it worked via:
- std::optional has_value - CHECK
- Empty unchecked.
The current path looks like it works via:
- std::optional has_value - return early
- Empty - CHECK
Is this behavioral change intentional?
Yes, this behavioral change is intentional.
We should check that the list is non-empty here right? Is there a reason we shouldn't?
- Daniel Cheng
- David Trainor
- Min Qin
Joone Hur (Gerrit)
Joone Hur added 1 comment![]( "Open in Gerrit")
CHECK(!url_infos->empty());Joone HurThis looks like a behavioral change as well - if we're pruning out invalid URLs before we get to this point, is the empty check something we should CHECK() on or just if-check? We if-checked' the is_valid() previously, which would be reflected as an empty list post change right?
You're right. I will add the empty check in the if condition. Thanks!
Related details
- Daniel Cheng
- David Trainor
- Min Qin
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Trainor (Gerrit)
David Trainor voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Trainor
| Code-Review | +1 |
1 comment
lgtm % remaining reviewers. Thanks!
Related details
- Daniel Cheng
- Joone Hur
- Min Qin
Code-Coverage
Code-Owners
Code-Review
| 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")
// Parses a multi-line string from the clipboard to extract URLs andJoone HurI think it would be better if we didn't change our interpretation of existing types, unless it's shown to improve compatiblity or fix other bugs.
This also changes the behavior; before, if we only had a URL, we'd set the string as both the URL and the title. After this, we no longer do that.
In addition, if we add a new function for parsing text/uri-list, maybe it should match the behavior of reusing the URL's spec as the title if there is no explict title? (I'll note the RFC for that MIME type also specifically states that comment lines are allowed, though it seems OK to punt on that specific part of it for now: https://www.rfc-editor.org/rfc/rfc2483.html#section-5)
I tried to update SplitUrlAndTitle to use the URL itself as the title when no explicit title was provided. However, this led to many test failures because the Linux port expects an empty title in this scenario. It appears `title->assign(str)` is never called even if the str doesn't include a title because the string received by SplitUrlAndTitle (from the clipboard storage) has a \n at its end in this case. This is due to how URLs are stored in the clipboard: even if the title is empty, a newline is always appended after the URL. See SetURL code:
```cpp
void OSExchangeDataProviderWin::SetURL(const GURL& url,
std::u16string_view title) {
// Add text/x-moz-url for drags from Firefox
STGMEDIUM storage = CreateStorageForString(
base::StrCat({base::UTF8ToUTF16(url.spec()), u"\n", title}));
data_->contents_.push_back(DataObjectImpl::StoredDataInfo::TakeStorageMedium(
ClipboardFormatType::MozUrlType().ToFormatEtc(), storage));```
Therefore, this CL doesn't change the existing single-URL behavior while also correctly parsing multiple URLs.
I should have been clearer. I think text/uri-list should have its own parsing function. It's not clear at all that text/x-moz-url or any of the other current formats are intended to handle multiple URLs at all, so the existing parsing helper shouldn't need to handle the multiple URL case at all.
There are clearly some problems/edge cases that aren't handled well for the previous code; it'd be nice to fix them, but the text-uri/list parsing is straightforward and should not inherit its problems.
Related details
- Joone Hur
- Min Qin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 5 comments![]( "Open in Gerrit")
I think the compatibility and interop implications of this CL are quite tricky. Even if we make the plumbing work just within Chrome, the different formats support different subsets of capabilities (text/x-moz-url supports URL + title, text/uri-list supports multiple URLs), and it's not clear how to combine those capabilities in a coherent way that works well for all use cases.
At this point, I think what's needed is to figure out these problems outside this code review, and then update this once we have a design we're happy with. Iterating on this in a code review feels like it's going to be slow and frustrating for everyone involved.
// Parses a multi-line string from the clipboard to extract URLs andJoone HurI think it would be better if we didn't change our interpretation of existing types, unless it's shown to improve compatiblity or fix other bugs.
This also changes the behavior; before, if we only had a URL, we'd set the string as both the URL and the title. After this, we no longer do that.
In addition, if we add a new function for parsing text/uri-list, maybe it should match the behavior of reusing the URL's spec as the title if there is no explict title? (I'll note the RFC for that MIME type also specifically states that comment lines are allowed, though it seems OK to punt on that specific part of it for now: https://www.rfc-editor.org/rfc/rfc2483.html#section-5)
Daniel ChengI tried to update SplitUrlAndTitle to use the URL itself as the title when no explicit title was provided. However, this led to many test failures because the Linux port expects an empty title in this scenario. It appears `title->assign(str)` is never called even if the str doesn't include a title because the string received by SplitUrlAndTitle (from the clipboard storage) has a \n at its end in this case. This is due to how URLs are stored in the clipboard: even if the title is empty, a newline is always appended after the URL. See SetURL code:
```cpp
void OSExchangeDataProviderWin::SetURL(const GURL& url,
std::u16string_view title) {
// Add text/x-moz-url for drags from Firefox
STGMEDIUM storage = CreateStorageForString(
base::StrCat({base::UTF8ToUTF16(url.spec()), u"\n", title}));
data_->contents_.push_back(DataObjectImpl::StoredDataInfo::TakeStorageMedium(
ClipboardFormatType::MozUrlType().ToFormatEtc(), storage));```
Therefore, this CL doesn't change the existing single-URL behavior while also correctly parsing multiple URLs.
I should have been clearer. I think text/uri-list should have its own parsing function. It's not clear at all that text/x-moz-url or any of the other current formats are intended to handle multiple URLs at all, so the existing parsing helper shouldn't need to handle the multiple URL case at all.
There are clearly some problems/edge cases that aren't handled well for the previous code; it'd be nice to fix them, but the text-uri/list parsing is straightforward and should not inherit its problems.
Sorry but one more thing I thought of. Even if we separate the parsing functions, we may still affect what we parse out of the clipboard.
This might matter for things like the bookmark manager, where carrying around a title to associate with the bookmark matters.
if (GetData(data_object, ClipboardFormatType::MozUrlType(), &store) ||text/x-moz-url doesn't actually appear to support multiple URLs (from what I could tell from reading FF source code)
GetData(data_object, ClipboardFormatType::MultiUrlType(), &store)) {But in that case, we have a fundamental tradeoff when reading from the clipboard because:
- text/x-moz-url supports titles
- but text/uri-list does not
We could try to figure out how to integrate data by reading both... but that seems messy too.
{base::UTF8ToUTF16(url_list), u"\n", url_infos.front().title}));i.e. I'm not sure it's actually OK to write out a list of URLs here.
Joone Hur (Gerrit)
Joone Hur added 6 comments![]( "Open in Gerrit")
I think the compatibility and interop implications of this CL are quite tricky. Even if we make the plumbing work just within Chrome, the different formats support different subsets of capabilities (text/x-moz-url supports URL + title, text/uri-list supports multiple URLs), and it's not clear how to combine those capabilities in a coherent way that works well for all use cases.
At this point, I think what's needed is to figure out these problems outside this code review, and then update this once we have a design we're happy with. Iterating on this in a code review feels like it's going to be slow and frustrating for everyone involved.
That makes sense. I agree that the compatibility and interop issues are tricky, and trying to solve them in a code review isn't the best approach. I'll open a design doc to figure out the current issues and solutions.
Thanks for your feedback!
// Parses a multi-line string from the clipboard to extract URLs andJoone HurI think it would be better if we didn't change our interpretation of existing types, unless it's shown to improve compatiblity or fix other bugs.
This also changes the behavior; before, if we only had a URL, we'd set the string as both the URL and the title. After this, we no longer do that.
In addition, if we add a new function for parsing text/uri-list, maybe it should match the behavior of reusing the URL's spec as the title if there is no explict title? (I'll note the RFC for that MIME type also specifically states that comment lines are allowed, though it seems OK to punt on that specific part of it for now: https://www.rfc-editor.org/rfc/rfc2483.html#section-5)
Daniel ChengI tried to update SplitUrlAndTitle to use the URL itself as the title when no explicit title was provided. However, this led to many test failures because the Linux port expects an empty title in this scenario. It appears `title->assign(str)` is never called even if the str doesn't include a title because the string received by SplitUrlAndTitle (from the clipboard storage) has a \n at its end in this case. This is due to how URLs are stored in the clipboard: even if the title is empty, a newline is always appended after the URL. See SetURL code:
```cpp
void OSExchangeDataProviderWin::SetURL(const GURL& url,
std::u16string_view title) {
// Add text/x-moz-url for drags from Firefox
STGMEDIUM storage = CreateStorageForString(
base::StrCat({base::UTF8ToUTF16(url.spec()), u"\n", title}));
data_->contents_.push_back(DataObjectImpl::StoredDataInfo::TakeStorageMedium(
ClipboardFormatType::MozUrlType().ToFormatEtc(), storage));```
Therefore, this CL doesn't change the existing single-URL behavior while also correctly parsing multiple URLs.
Daniel ChengI should have been clearer. I think text/uri-list should have its own parsing function. It's not clear at all that text/x-moz-url or any of the other current formats are intended to handle multiple URLs at all, so the existing parsing helper shouldn't need to handle the multiple URL case at all.
There are clearly some problems/edge cases that aren't handled well for the previous code; it'd be nice to fix them, but the text-uri/list parsing is straightforward and should not inherit its problems.
Sorry but one more thing I thought of. Even if we separate the parsing functions, we may still affect what we parse out of the clipboard.
This might matter for things like the bookmark manager, where carrying around a title to associate with the bookmark matters.
I see. First, I will create a new function specifically for text/uri-list that handles multiple URLs. I will also ensure that the existing parsing logic for single URLs is not changed and continues to correctly extract both the URL and its associated title, preserving the current behavior for features like the bookmark manager. This approach will prevent any unintended side effects from my new code.
if (GetData(data_object, ClipboardFormatType::MozUrlType(), &store) ||text/x-moz-url doesn't actually appear to support multiple URLs (from what I could tell from reading FF source code)
Got it. I'll make sure the CL handles text/x-moz-url with only a single URL, consistent with what you found in the FF code
GetData(data_object, ClipboardFormatType::MultiUrlType(), &store)) {But in that case, we have a fundamental tradeoff when reading from the clipboard because:
- text/x-moz-url supports titles
- but text/uri-list does not
We could try to figure out how to integrate data by reading both... but that seems messy too.
That's exactly why we need to separate the parsing functions. text/x-moz-url can be parsed for a title and URL, while a new function can handle text/uri-list's multiple URLs. The application can choose which data is more useful after reading them.
{base::UTF8ToUTF16(url_list), u"\n", url_infos.front().title}));i.e. I'm not sure it's actually OK to write out a list of URLs here.
I'll undo this change and ensure that text/x-moz-url only handles a single URL.
Related details
- Daniel Cheng
- Min Qin
Code-Coverage
Code-Owners
Code-Review
| 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")
Hi @dch...@chromium.org, I've updated the CL to keep the existing single-URL handling code. This ensures we don't break the ability to get a title for a single URL. I've also separated the handler for the text/uri-list format so that it doesn't affect x-moz-url handling code.
Please check the below code:
ui/base/dragdrop/os_exchange_data_provider_win.cc
ui/base/clipboard/clipboard_util_win.cc
Therefore, I think we can land the current CL. I've also added more details to the design document, as we discussed.
Related details
- Daniel Cheng
- Min Qin
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sylvain Defresne (Gerrit)
Sylvain Defresne voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sylvain Defresne
| Code-Review | +1 |
- Avi Drissman
- Daniel Cheng
- David Trainor
- Joone Hur
- Min Qin
Code-Coverage
Code-Owners
Code-Review
| 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")
Hi @dch...@chromium.org, I've updated the CL to keep the existing single-URL handling code. This ensures we don't break the ability to get a title for a single URL. I've also separated the handler for the text/uri-list format so that it doesn't affect x-moz-url handling code.
Please check the below code:
ui/base/dragdrop/os_exchange_data_provider_win.cc
ui/base/clipboard/clipboard_util_win.ccTherefore, I think we can land the current CL. I've also added more details to the design document, as we discussed.
Hi @dch...@chromium.org, I've updated the CL to keep the existing single-URL handling code. This ensures we don't break the ability to get a title for a single URL. I've also separated the handler for the text/uri-list format so that it doesn't affect x-moz-url handling code.
Please check the below code:
ui/base/dragdrop/os_exchange_data_provider_win.cc
ui/base/clipboard/clipboard_util_win.ccTherefore, I think we can land the current CL. I've also added more details to the design document, as we discussed.
@dch...@chromium.org, I'm sharing the design document: https://docs.google.com/document/d/1zNvn2hQOfMCc9-L9_Wt-Q4rBhmSAQHhUYWMDdNKLCGU/edit?usp=sharing
Related details
- Daniel Cheng
- David Trainor
- Min Qin
Daniel Cheng (Gerrit)
Daniel Cheng added 4 comments![]( "Open in Gerrit")
The design doc helps; however, I should have been specific that the part that is important to focus on is how to translate between the web formats and the platform-specific formats. The actual C++ code isn't that interesting for the design doc; it's how we want to do the transform, and how we can do it in a way that's not surprising and doesn't lose data in edge cases.
RegisterClipboardFormatChecked(L"multi-url/title"));We should either:
- not try to make this MIME-ish and just name this with a name like "PNG" or "HTML Format" or "Web Custom Format Map"
- or this should be something like `chromium/x-source-url`
Personally I would recommend the former here; I don't think there's a strong convention about clipboard foramt names being MIME types on Windows.
It would also be a good courtesy to at least reach out to Apple and Mozilla to see if they want to be able to interoperate here.
if (url_infos.empty()) {I think we should probably `DCHECK()` that this is not the case, but I'm not 100% sure...
if (!url_info.title.empty()) {If we keep the format as-is, we probably want to unconditionally serialize a title, even if it's empty. This simplifies the parsing, which will just have to deal with URL <-> title pairs.
Otherwise, the parsing will have to deal with ambiguity: what happens if the title also happens to be a valid URL?
Also, what happens if the title has an embedded newline? I don't think anything necessarily prevents that.
The lazy answer here is "encode the URL + titles as an array of JSON values", but I'm not sure if that's the best approach either and that's somethign the design doc should probably talk about more.
Related details
- David Trainor
- Joone Hur
- Min Qin
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joone Hur (Gerrit)
Joone Hur added 5 comments![]( "Open in Gerrit")
Thanks for the review!
The design doc helps; however, I should have been specific that the part that is important to focus on is how to translate between the web formats and the platform-specific formats. The actual C++ code isn't that interesting for the design doc; it's how we want to do the transform, and how we can do it in a way that's not surprising and doesn't lose data in edge cases.
I also added more context about the new clipboard format: https://docs.google.com/document/d/1zNvn2hQOfMCc9-L9_Wt-Q4rBhmSAQHhUYWMDdNKLCGU/edit?tab=t.0#heading=h.xy3joqqkfkn3, and I’ll include a section explaining how the data is converted through the clipboard.
RegisterClipboardFormatChecked(L"multi-url/title"));We should either:
- not try to make this MIME-ish and just name this with a name like "PNG" or "HTML Format" or "Web Custom Format Map"
- or this should be something like `chromium/x-source-url`
Personally I would recommend the former here; I don't think there's a strong convention about clipboard foramt names being MIME types on Windows.
It would also be a good courtesy to at least reach out to Apple and Mozilla to see if they want to be able to interoperate here.
How about using "Web Bookmark List"? It fits well with existing Windows clipboard format naming conventions like "HTML Format" or "Web Custom Format Map". I’ll start by updating the format name to this, and we can revisit it later if a better name comes up during discussions with others.
if (url_infos.empty()) {I think we should probably `DCHECK()` that this is not the case, but I'm not 100% sure...
We can remove this condition since url_infos is already validated in PrepareDragData:
```
if (!drop_data.url_infos.empty()) {
provider->SetURLs(drop_data.url_infos);
}
```
if (!url_info.title.empty()) {If we keep the format as-is, we probably want to unconditionally serialize a title, even if it's empty. This simplifies the parsing, which will just have to deal with URL <-> title pairs.
Otherwise, the parsing will have to deal with ambiguity: what happens if the title also happens to be a valid URL?
Also, what happens if the title has an embedded newline? I don't think anything necessarily prevents that.
The lazy answer here is "encode the URL + titles as an array of JSON values", but I'm not sure if that's the best approach either and that's somethign the design doc should probably talk about more.
Okay, I'll simplify the parsing code by skipping the check for empty titles.
I'll also mention "using an array of JSON values" as an alternative format in the design document.
Related details
- Daniel Cheng
- David Trainor
- Min Qin
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
| 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")
The new clipboard format for multiple URLs has been renamed from `MultiUrlTitleType` to `BookmarkListType`:
```
const ClipboardFormatType& ClipboardFormatType::BookmarkListType() {
static base::NoDestructor<ClipboardFormatType> format(
RegisterClipboardFormatChecked(L"Bookmark List"));
return *format;
}
```
The design document has also been updated to include details on how the data is transformed during the drag operation.
Related details
- Daniel Cheng
- David Trainor
- Min Qin
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |