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