Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(246)

Issue 8513002: Strip invalid characters (line breaks, tabs), javascript:schemes from the copied text before pasting (Closed)

Created:
9 years, 1 month ago by SanjoyPal
Modified:
9 years ago
CC:
chromium-reviews, James Su
Visibility:
Public.

Description

Strip invalid characters (line breaks, tabs), javascript:schemes from the copied text while pasting text, droping text and creating right click popup for omnibox. BUG=82181, 103703 . TEST=Copy a string with line breaks "\n" or tabs "\t". Then right click on omnibox. Chromium should not trigger DCHECKS. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111548

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Total comments: 2

Patch Set 5 : '' #

Patch Set 6 : '' #

Total comments: 2

Patch Set 7 : '' #

Total comments: 2

Patch Set 8 : '' #

Total comments: 12

Patch Set 9 : '' #

Total comments: 3

Patch Set 10 : '' #

Total comments: 8

Patch Set 11 : '' #

Patch Set 12 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+29 lines, -25 lines) Patch
M chrome/browser/autocomplete/autocomplete_edit.h View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc View 1 2 3 4 5 6 7 8 9 10 11 7 chunks +26 lines, -25 lines 0 comments Download

Messages

Total messages: 40 (0 generated)
SanjoyPal
PTAL.
9 years, 1 month ago (2011-11-10 09:39:06 UTC) #1
Alexei Svitkine (slow)
On 2011/11/10 09:39:06, SanjoyPal wrote: > PTAL. I think we'd rather replace these characters with ...
9 years, 1 month ago (2011-11-10 13:43:23 UTC) #2
SanjoyPal
On 2011/11/10 13:43:23, Alexei Svitkine wrote: > On 2011/11/10 09:39:06, SanjoyPal wrote: > > PTAL. ...
9 years, 1 month ago (2011-11-11 07:54:00 UTC) #3
Alexei Svitkine (slow)
On 2011/11/11 07:54:00, SanjoyPal wrote: > On 2011/11/10 13:43:23, Alexei Svitkine wrote: > > On ...
9 years, 1 month ago (2011-11-11 14:51:09 UTC) #4
SanjoyPal
PTAL.
9 years, 1 month ago (2011-11-14 12:44:12 UTC) #5
SanjoyPal
On 2011/11/11 14:51:09, Alexei Svitkine wrote: > On 2011/11/11 07:54:00, SanjoyPal wrote: > > On ...
9 years, 1 month ago (2011-11-14 12:45:28 UTC) #6
Alexei Svitkine (slow)
> Doing ReplaceChars before pasting in omnibox. PTAL This looks better. Is the problem Linux-specific? ...
9 years, 1 month ago (2011-11-14 14:18:21 UTC) #7
Alexei Svitkine (slow)
On 2011/11/14 14:18:21, Alexei Svitkine wrote: > > Doing ReplaceChars before pasting in omnibox. PTAL ...
9 years, 1 month ago (2011-11-14 23:09:29 UTC) #8
Peter Kasting
Please handle this issue like Windows does in the sense of stripping whitespace rather than ...
9 years, 1 month ago (2011-11-14 23:15:13 UTC) #9
SanjoyPal
On 2011/11/14 23:15:13, Peter Kasting wrote: > Please handle this issue like Windows does in ...
9 years, 1 month ago (2011-11-15 06:40:22 UTC) #10
Peter Kasting
I don't think this actually fixes the problem. First, it doesn't look like this will ...
9 years, 1 month ago (2011-11-15 06:52:22 UTC) #11
SanjoyPal
PTAL. http://codereview.chromium.org/8513002/diff/14002/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/14002/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc#newcode1449 chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1449: CollapseWhitespace(StripJavascriptSchemas(text_wstr), true); On 2011/11/15 06:52:22, Peter Kasting wrote: ...
9 years, 1 month ago (2011-11-15 07:11:14 UTC) #12
Peter Kasting
This would fix a crash-on-right-click but I am still concerned about the actual paste case. ...
9 years, 1 month ago (2011-11-15 07:26:12 UTC) #13
SanjoyPal
On 2011/11/15 07:26:12, Peter Kasting wrote: > This would fix a crash-on-right-click but I am ...
9 years, 1 month ago (2011-11-15 12:38:46 UTC) #14
SanjoyPal
http://codereview.chromium.org/8513002/diff/10002/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/10002/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc#newcode1461 chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1461: model_->CanPasteAndGo(sanitized_text)); On 2011/11/15 07:26:12, Peter Kasting wrote: > This ...
9 years, 1 month ago (2011-11-15 12:42:37 UTC) #15
Evan Stade
reading the cl description, I have no idea what bug you're trying to fix. Your ...
9 years, 1 month ago (2011-11-15 17:21:04 UTC) #16
Peter Kasting
On 2011/11/15 12:38:46, SanjoyPal wrote: > On 2011/11/15 07:26:12, Peter Kasting wrote: > > This ...
9 years, 1 month ago (2011-11-15 18:27:26 UTC) #17
SanjoyPal
On 2011/11/15 18:27:26, Peter Kasting wrote: > On 2011/11/15 12:38:46, SanjoyPal wrote: > > On ...
9 years, 1 month ago (2011-11-21 06:38:29 UTC) #18
SanjoyPal
http://codereview.chromium.org/8513002/diff/10004/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/10004/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc#newcode1449 chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1449: string16()); On 2011/11/15 18:27:26, Peter Kasting wrote: > Nit: ...
9 years, 1 month ago (2011-11-21 06:38:43 UTC) #19
Peter Kasting
http://codereview.chromium.org/8513002/diff/20001/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/20001/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc#newcode881 chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:881: string16 data_string; Nit: This whole block can be collapsed ...
9 years, 1 month ago (2011-11-21 21:23:43 UTC) #20
SanjoyPal
Addressed the comments in the previous patch. Please review. http://codereview.chromium.org/8513002/diff/20001/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/20001/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc#newcode881 chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:881: ...
9 years, 1 month ago (2011-11-22 07:02:13 UTC) #21
Alexei Svitkine (slow)
http://codereview.chromium.org/8513002/diff/29001/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/29001/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc#newcode1668 chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1668: CollapseWhitespace(UTF8ToUTF16(filtered_text), true))); Nit: Can all the text - including ...
9 years, 1 month ago (2011-11-23 04:26:41 UTC) #22
Alexei Svitkine (slow)
> Nit: Can all the text - including |filtered_text| - as UTF16 (string16) and only ...
9 years, 1 month ago (2011-11-23 04:27:30 UTC) #23
Alexei Svitkine (slow)
On Tue, Nov 22, 2011 at 11:27 PM, <asvitkine@chromium.org> wrote: > Nit: Can all the ...
9 years, 1 month ago (2011-11-23 04:31:29 UTC) #24
SanjoyPal
Done. PTAL. http://codereview.chromium.org/8513002/diff/29001/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/29001/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc#newcode1668 chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1668: CollapseWhitespace(UTF8ToUTF16(filtered_text), true))); On 2011/11/23 04:26:41, Alexei Svitkine ...
9 years, 1 month ago (2011-11-23 05:56:11 UTC) #25
Alexei Svitkine (slow)
http://codereview.chromium.org/8513002/diff/29001/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/29001/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc#newcode1659 chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1659: filtered_text.append(p, next); Now, this isn't right anymore - since ...
9 years, 1 month ago (2011-11-23 14:35:47 UTC) #26
Peter Kasting
LGTM other than the WriteUnicodeCharacter() issue, which needs to be fixed. http://codereview.chromium.org/8513002/diff/33001/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): ...
9 years, 1 month ago (2011-11-23 19:02:11 UTC) #27
SanjoyPal
PTAL. Thanks http://codereview.chromium.org/8513002/diff/33001/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/33001/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc#newcode1450 chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1450: // Paste and Go menu item. On ...
9 years, 1 month ago (2011-11-24 07:36:10 UTC) #28
Alexei Svitkine (slow)
LGTM
9 years, 1 month ago (2011-11-24 13:33:06 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ncj674@motorola.com/8513002/35003
9 years, 1 month ago (2011-11-24 17:34:32 UTC) #30
commit-bot: I haz the power
Presubmit check for 8513002-35003 failed and returned exit status 1. Running presubmit commit checks ...
9 years, 1 month ago (2011-11-24 17:34:36 UTC) #31
Alexei Svitkine (slow)
Can you change the description line to list the two bugs on the same line? ...
9 years, 1 month ago (2011-11-24 17:43:24 UTC) #32
Alexei Svitkine (slow)
> > Also, please change the TEST line to be non-empty. > (In other words, ...
9 years, 1 month ago (2011-11-24 17:44:17 UTC) #33
SanjoyPal
Hi Darin, Could you please review the change in base/utf_string_conversion_utils.h. Thanks.
9 years, 1 month ago (2011-11-24 17:59:51 UTC) #34
Nico
base change lgtm
9 years, 1 month ago (2011-11-24 18:11:12 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ncj674@motorola.com/8513002/35003
9 years, 1 month ago (2011-11-24 18:14:22 UTC) #36
commit-bot: I haz the power
Change committed as 111548
9 years, 1 month ago (2011-11-24 19:19:32 UTC) #37
SanjoyPal
Previous patch broke OmniboxViewTest.AcceptKeywordBySpace. Updated the patch. Please one more time.
9 years ago (2011-11-25 08:06:53 UTC) #38
SanjoyPal
On 2011/11/25 08:06:53, SanjoyPal wrote: > Previous patch broke OmniboxViewTest.AcceptKeywordBySpace. Updated the patch. > Please ...
9 years ago (2011-11-25 08:07:19 UTC) #39
SanjoyPal
9 years ago (2011-11-25 09:31:36 UTC) #40
On 2011/11/25 08:07:19, SanjoyPal wrote:
> On 2011/11/25 08:06:53, SanjoyPal wrote:
> > Previous patch broke OmniboxViewTest.AcceptKeywordBySpace. Updated the
patch.
> > Please one more time.
> 
> Please review one more time.

Created a new CL http://codereview.chromium.org/8702002/

Powered by Google App Engine
This is Rietveld 408576698