Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
7 years, 2 months ago by scottmg Modified:
7 years, 2 months ago Reviewers:
Peter Kasting, sky CC:
chromium-reviews, tfarina, James Su Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDon't write URLs to clipboard from views omnibox as hyperlinks
This makes the views omnibox match the old Windows omnibox.
See https://chromiumcodereview.appspot.com/10827330#msg3 and 4
for previous discussion on how this fix differs slightly from
the initial bug report in issue 976.
R=pkasting@chromium.org
BUG=976
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227598
Patch Set 1 #
Total comments: 2
Patch Set 2 : . #
Total comments: 6
Patch Set 3 : make behaviour consistent across platforms #
Total comments: 2
Patch Set 4 : fix comments #
Created: 7 years, 2 months ago
Messages
Total messages: 15 (0 generated)
Is there a test where we can check the types of objects placed on the clipboard during a copy? LGTM otherwise. https://codereview.chromium.org/25894003/diff/1/chrome/browser/ui/views/omnib... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/25894003/diff/1/chrome/browser/ui/views/omnib... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:739: &selected_text, &url, &write_url); Can we kill write_url entirely?
> Is there a test where we can check the types of objects placed on the clipboard > during a copy? Hmm, apparently there is already, and it tests for the converse (except for Mac). It was added here: https://chromiumcodereview.appspot.com/11789003/ So, I'm not sure what the "correct" behaviour is then. I don't particularly like the omnibox copying the font settings, but apparently at least one person disagrees: https://code.google.com/p/chromium/issues/detail?id=168583 https://codereview.chromium.org/25894003/diff/1/chrome/browser/ui/views/omnib... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/25894003/diff/1/chrome/browser/ui/views/omnib... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:739: &selected_text, &url, &write_url); On 2013/10/03 23:46:43, Peter Kasting wrote: > Can we kill write_url entirely? No, we use it e.g. below when determining if the drag type should be DRAG_LINK.
On 2013/10/04 00:07:12, scottmg wrote: > > Is there a test where we can check the types of objects placed on the > clipboard > > during a copy? > > Hmm, apparently there is already, and it tests for the converse (except for > Mac). It was added here: https://chromiumcodereview.appspot.com/11789003/ > > So, I'm not sure what the "correct" behaviour is then. I don't particularly like > the omnibox copying the font settings, but apparently at least one person > disagrees: https://code.google.com/p/chromium/issues/detail?id=168583 There are several functions here: ScopedClipboardWriter::WriteBookmark(), ScopedClipboardWriter::WriteHyperlink(), and BookmarkNodeData::WriteToClipboard(). It seems like your change a year ago eliminated one of these calls, while the change you link just above added/changed to the other two. I'm not at all clear on the differences among these. I can't tell if your two changes are directly fighting each other or not. Is there some middle ground where we will paste as a hyperlink, but not change subsequent fonts, in gmail? If no, and we have to pick a set of behaviors, I'd prefer writing to the clipboard just as plain text. It seems kind of ridiculous that this can't be perfect, though.
On 2013/10/04 00:15:15, Peter Kasting wrote: > On 2013/10/04 00:07:12, scottmg wrote: > > > Is there a test where we can check the types of objects placed on the > > clipboard > > > during a copy? > > > > Hmm, apparently there is already, and it tests for the converse (except for > > Mac). It was added here: https://chromiumcodereview.appspot.com/11789003/ > > > > So, I'm not sure what the "correct" behaviour is then. I don't particularly > like > > the omnibox copying the font settings, but apparently at least one person > > disagrees: https://code.google.com/p/chromium/issues/detail?id=168583 > > There are several functions here: ScopedClipboardWriter::WriteBookmark(), > ScopedClipboardWriter::WriteHyperlink(), and > BookmarkNodeData::WriteToClipboard(). It seems like your change a year ago > eliminated one of these calls, while the change you link just above > added/changed to the other two. I'm not at all clear on the differences among > these. I can't tell if your two changes are directly fighting each other or > not. > > Is there some middle ground where we will paste as a hyperlink, but not change > subsequent fonts, in gmail? If no, and we have to pick a set of behaviors, I'd > prefer writing to the clipboard just as plain text. It seems kind of ridiculous > that this can't be perfect, though. Quoting from previous change: """As far as I can tell, when pasting HTML without a font setting, what happens is that it's pasted in "default style" in rich apps. So, if you've changed your font explicitly, then it'll change the font back to the "normal body text" settings for the paste and then maintain that when you continue typing (which I believe is what people are reporting in the bug). Conversely, when there's only plain text, it's effectively as if you'd typed those characters, so any explicit font setting is maintained. The only drawback, as you pointed out is that the linky-ness is lost. Most rich editors detect and add a link back though (I tried Word, Google Docs, Outlook, VS, Chat). One notable exception though is Gmail's rich editor. For whatever reason, it doesn't have linkifying behaviour. I don't know if that's a good enough reason not to change this.""" So, I believe the only options (at least on Windows) are: 1. Link which implies some formatting information; 2. Plain text. So, I think bug 976 and bug 168583 are in direct opposition.
On 2013/10/04 20:27:46, scottmg wrote: > So, I think bug 976 and bug 168583 are in direct opposition. :( OK, let's make all Windows platforms write plain text, and add a comment as to why we're explicitly not writing hypertext.
On 2013/10/04 20:38:16, Peter Kasting wrote: > On 2013/10/04 20:27:46, scottmg wrote: > > So, I think bug 976 and bug 168583 are in direct opposition. > > :( > > OK, let's make all Windows platforms write plain text, and add a comment as to > why we're explicitly not writing hypertext. Done, and updated text. I moved to changing BookmarkNodeData as it seemed like the correct place to decide on what formats are written. The bookmark format is still written, so an application that wanted to retrieve that could still do so (I don't know of any to test with), and the text format is non-HTML.
On 2013/10/05 00:34:06, scottmg wrote: > On 2013/10/04 20:38:16, Peter Kasting wrote: > > On 2013/10/04 20:27:46, scottmg wrote: > > > So, I think bug 976 and bug 168583 are in direct opposition. > > > > :( > > > > OK, let's make all Windows platforms write plain text, and add a comment as to > > why we're explicitly not writing hypertext. > > Done, and updated text. I moved to changing BookmarkNodeData as it seemed like > the correct place to decide on what formats are written. The bookmark format is > still written, so an application that wanted to retrieve that could still do so > (I don't know of any to test with), and the text format is non-HTML. er, "updated test", not "updated text".
https://codereview.chromium.org/25894003/diff/14001/chrome/browser/bookmarks/... File chrome/browser/bookmarks/bookmark_node_data.cc (right): https://codereview.chromium.org/25894003/diff/14001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_node_data.cc:146: #if !defined(OS_WIN) This makes me wonder if we should also avoid writing this format on other platforms. If the problem is e.g. the gmail text editor, isn't that a cross-platform problem? I feel like OS_WIN is probably wrong here. https://codereview.chromium.org/25894003/diff/14001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_node_data.cc:149: // pasting into other programs. Nit: I generally try to avoid bug links, since they can be confusing to find the relevant information. How about: On Windows, we avoid writing hyperlink data, since some rich text editors will change fonts when such data is pasted in; besides, most such editors auto-linkify at some point anyway. https://codereview.chromium.org/25894003/diff/14001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/25894003/diff/14001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1618: #if !defined(OS_MACOSX) && !defined(OS_WIN) Seems like on these platforms we should EXPECT_FALSE instead (2 places).
https://codereview.chromium.org/25894003/diff/14001/chrome/browser/bookmarks/... File chrome/browser/bookmarks/bookmark_node_data.cc (right): https://codereview.chromium.org/25894003/diff/14001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_node_data.cc:146: #if !defined(OS_WIN) On 2013/10/05 00:42:27, Peter Kasting wrote: > This makes me wonder if we should also avoid writing this format on other > platforms. If the problem is e.g. the gmail text editor, isn't that a > cross-platform problem? I feel like OS_WIN is probably wrong here. I confirmed that the behaviour is same (font-preserving) on CrOS, so yes, it seems reasonable to me not to do this on CrOS & Linux too. Done. https://codereview.chromium.org/25894003/diff/14001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_node_data.cc:149: // pasting into other programs. On 2013/10/05 00:42:27, Peter Kasting wrote: > Nit: I generally try to avoid bug links, since they can be confusing to find the > relevant information. How about: > > On Windows, we avoid writing hyperlink data, since some rich text editors will > change fonts when such data is pasted in; besides, most such editors > auto-linkify at some point anyway. Done. https://codereview.chromium.org/25894003/diff/14001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/25894003/diff/14001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1618: #if !defined(OS_MACOSX) && !defined(OS_WIN) On 2013/10/05 00:42:27, Peter Kasting wrote: > Seems like on these platforms we should EXPECT_FALSE instead (2 places). Done.
LGTM https://codereview.chromium.org/25894003/diff/26001/chrome/browser/bookmarks/... File chrome/browser/bookmarks/bookmark_node_data.cc (right): https://codereview.chromium.org/25894003/diff/26001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_node_data.cc:147: // Previously scw.WriteHyperlink was called here. Purposfully don't write Nit: Shorter and more direct: "Don't call scw.WriteHyperlink() here, since..." https://codereview.chromium.org/25894003/diff/26001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/25894003/diff/26001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1617: // Make sure HTML format isn't written. http://crbug.com/976. Nit: Don't refer to a bug number here; if any more explanation is needed than the first sentence, either explain inline, or refer readers to the comment in BookmarkNodeData::WriteToClipboard() for details. (2 places)
Oops, the implementation morphed to a different file. +sky for chrome/browser/bookmarks OWNERS.
I suspect this is going to cause some grief. LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/25894003/32001
Message was sent while issue was closed.
Change committed as 227598 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
