Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10827330:
Don't copy urls in richtext from omnibox on Windows (Closed)
|
Created:
8 years, 4 months ago by scottmg Modified:
8 years, 4 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, James Su Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDon't copy urls in richtext from omnibox on Windows
BUG=976
TEST=Change font in gmail, type, copy/paste url, and make font settings are preserved.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151808
Patch Set 1 #
Total comments: 2
Patch Set 2 : remove {} #Messages
Total messages: 14 (0 generated)
https://chromiumcodereview.appspot.com/10827330/diff/1/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (left): https://chromiumcodereview.appspot.com/10827330/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1339: scw.WriteHyperlink(net::EscapeForHTML(text), url.spec()); It surprises me that this could somehow preserve formatting (fonts/colors), because all we pass in is two strings. There's no font or color information passed to WriteHyperlink(), so how can that call be the problem here? (I want to understand why the bug happens and the fix works before I OK any changes.)
On 2012/08/15 05:54:28, Peter Kasting wrote: > https://chromiumcodereview.appspot.com/10827330/diff/1/chrome/browser/ui/view... > File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (left): > > https://chromiumcodereview.appspot.com/10827330/diff/1/chrome/browser/ui/view... > chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1339: > scw.WriteHyperlink(net::EscapeForHTML(text), url.spec()); > It surprises me that this could somehow preserve formatting (fonts/colors), > because all we pass in is two strings. There's no font or color information > passed to WriteHyperlink(), so how can that call be the problem here? > > (I want to understand why the bug happens and the fix works before I OK any > changes.) Yeah, it's tricky. 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. I verified that IE and FF only copy plain text using a clipboard viewer, fwiw. I don't know how relevant/important that is.
The original bug reports described the problem as preserving colors etc. from the address bar, which this clearly wouldn't fix. However, all of the more recent comments on the bug seem to instead be complaining about the behavior that you're trying to fix, so maybe this will make people happy. LGTM https://chromiumcodereview.appspot.com/10827330/diff/1/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://chromiumcodereview.appspot.com/10827330/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1337: if (write_url) { Nit: Remove {}
On 2012/08/15 17:21:40, Peter Kasting wrote: > The original bug reports described the problem as preserving colors etc. from > the address bar, which this clearly wouldn't fix. However, all of the more > recent comments on the bug seem to instead be complaining about the behavior > that you're trying to fix, so maybe this will make people happy. Right, I was assuming that that long-ago reports had been fixed at some point along the way and the bug morphed a bit. > > LGTM > > https://chromiumcodereview.appspot.com/10827330/diff/1/chrome/browser/ui/view... > File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): > > https://chromiumcodereview.appspot.com/10827330/diff/1/chrome/browser/ui/view... > chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1337: if (write_url) { > Nit: Remove {} Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/10827330/6001
Try job failure for 10827330-6001 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/10827330/6001
Try job failure for 10827330-6001 on win_rel for step "update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/10827330/6001
Try job failure for 10827330-6001 (retry) on win_rel for step "interactive_ui_tests". It's a second try, previously, steps "interactive_ui_tests, browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
On 2012/08/15 22:26:35, I haz the power (commit-bot) wrote: > Try job failure for 10827330-6001 (retry) on win_rel for step > "interactive_ui_tests". > It's a second try, previously, steps "interactive_ui_tests, browser_tests" > failed. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... (do not appear related, 4th times the charm!)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/10827330/6001
Change committed as 151808 |
