|
|
Created:
9 years, 1 month ago by SanjoyPal Modified:
9 years ago CC:
chromium-reviews, James Su Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionStrip 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 : '' #
Messages
Total messages: 40 (0 generated)
PTAL.
On 2011/11/10 09:39:06, SanjoyPal wrote: > PTAL. I think we'd rather replace these characters with spaces on paste, similar to: http://codereview.chromium.org/8502027/
On 2011/11/10 13:43:23, Alexei Svitkine wrote: > On 2011/11/10 09:39:06, SanjoyPal wrote: > > PTAL. > > I think we'd rather replace these characters with spaces on paste, similar to: > > http://codereview.chromium.org/8502027/ Done. PTAL.
On 2011/11/11 07:54:00, SanjoyPal wrote: > On 2011/11/10 13:43:23, Alexei Svitkine wrote: > > On 2011/11/10 09:39:06, SanjoyPal wrote: > > > PTAL. > > > > I think we'd rather replace these characters with spaces on paste, similar to: > > > > http://codereview.chromium.org/8502027/ > > Done. PTAL. Can you do it at the place where the paste actually takes place?
PTAL.
On 2011/11/11 14:51:09, Alexei Svitkine wrote: > On 2011/11/11 07:54:00, SanjoyPal wrote: > > On 2011/11/10 13:43:23, Alexei Svitkine wrote: > > > On 2011/11/10 09:39:06, SanjoyPal wrote: > > > > PTAL. > > > > > > I think we'd rather replace these characters with spaces on paste, similar > to: > > > > > > http://codereview.chromium.org/8502027/ > > > > Done. PTAL. > > Can you do it at the place where the paste actually takes place? Doing ReplaceChars before pasting in omnibox. PTAL
> Doing ReplaceChars before pasting in omnibox. PTAL This looks better. Is the problem Linux-specific? If not, maybe a better place to do this would be in |model_->CanPasteAndGo()|.
On 2011/11/14 14:18:21, Alexei Svitkine wrote: > > Doing ReplaceChars before pasting in omnibox. PTAL > > This looks better. Is the problem Linux-specific? If not, maybe a better place > to do this would be in |model_->CanPasteAndGo()|. Also, please update the name of the CL.
Please handle this issue like Windows does in the sense of stripping whitespace rather than replacing it with spaces, in order to correctly handle pasting links that have linebreaks in the middle of them. Windows and Mac also do this in the paste handling code itself but this may be impossible to duplicate on GTK due to the asynchronous nature of pasting. When you add the handling code here please also consider implementing the "strip javascript: schemes off pasted text" anti-spoofing feature that we have for Windows and Mac but not Linux ( http://crbug.com/82181 )
On 2011/11/14 23:15:13, Peter Kasting wrote: > Please handle this issue like Windows does in the sense of stripping whitespace > rather than replacing it with spaces, in order to correctly handle pasting links > that have linebreaks in the middle of them. > > Windows and Mac also do this in the paste handling code itself but this may be > impossible to duplicate on GTK due to the asynchronous nature of pasting. > > When you add the handling code here please also consider implementing the "strip > javascript: schemes off pasted text" anti-spoofing feature that we have for > Windows and Mac but not Linux ( http://crbug.com/82181 ) Done. Please review.
I don't think this actually fixes the problem. First, it doesn't look like this will have any effect on normal pasting, just on paste-and-go. Second, I don't even think the paste-and-go stuff works quite right because it looks to me as if CanPasteAndGo(), which updates the edit model, is called _after_ is_paste_and_search(). It looks like maybe somebody refactored this code at some point without realizing what effect moving that call would have. http://codereview.chromium.org/8513002/diff/14002/chrome/browser/ui/gtk/omnib... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/14002/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1449: CollapseWhitespace(StripJavascriptSchemas(text_wstr), true); You need to reverse the CollapseWhitespace and StripJavascriptSchemas calls as in omnibox_view_{mac.mm,win.cc}. Nit: Combine this with the line above as: string16 sanitized_text(text ? StripJavascriptSchemas(CollapseWhitespace(UTF8ToUTF16(text))) : string16());
PTAL. http://codereview.chromium.org/8513002/diff/14002/chrome/browser/ui/gtk/omnib... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/14002/chrome/browser/ui/gtk/omnib... 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: > You need to reverse the CollapseWhitespace and StripJavascriptSchemas calls as > in omnibox_view_{mac.mm,win.cc}. > > Nit: Combine this with the line above as: > > string16 sanitized_text(text ? > StripJavascriptSchemas(CollapseWhitespace(UTF8ToUTF16(text))) : > string16()); Done.
This would fix a crash-on-right-click but I am still concerned about the actual paste case. Where do we sanitize strings that are pasted in (not paste-and-go, this will address that)? http://codereview.chromium.org/8513002/diff/10002/chrome/browser/ui/gtk/omnib... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/10002/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1461: model_->CanPasteAndGo(sanitized_text)); This call still needs to be moved up before the call to is_paste_and_search() above, meaning it needs to be pulled out into a temp.
On 2011/11/15 07:26:12, Peter Kasting wrote: > This would fix a crash-on-right-click but I am still concerned about the actual > paste case. Where do we sanitize strings that are pasted in (not paste-and-go, > this will address that)? > There is no crash while pasting on the omnibox. Crash occurs when creating popup on right click on omnibox. I have updated the CL description. > http://codereview.chromium.org/8513002/diff/10002/chrome/browser/ui/gtk/omnib... > File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): > > http://codereview.chromium.org/8513002/diff/10002/chrome/browser/ui/gtk/omnib... > chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1461: > model_->CanPasteAndGo(sanitized_text)); > This call still needs to be moved up before the call to is_paste_and_search() > above, meaning it needs to be pulled out into a temp.
http://codereview.chromium.org/8513002/diff/10002/chrome/browser/ui/gtk/omnib... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/10002/chrome/browser/ui/gtk/omnib... 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 call still needs to be moved up before the call to is_paste_and_search() > above, meaning it needs to be pulled out into a temp. Done.
reading the cl description, I have no idea what bug you're trying to fix. Your TEST= cases don't really describe what should happen, or what does happen without this patch.
On 2011/11/15 12:38:46, SanjoyPal wrote: > On 2011/11/15 07:26:12, Peter Kasting wrote: > > This would fix a crash-on-right-click but I am still concerned about the > actual > > paste case. Where do we sanitize strings that are pasted in (not > paste-and-go, > > this will address that)? > > There is no crash while pasting on the omnibox. Crash occurs when creating popup > on right click on omnibox. I have updated the CL description. Why did you remove the BUG= number? Restore it and add 82181 to the list also. If we don't crash when pasting, how are we fixing up the user's input? Are we stripping the javascript scheme from it too? Where does this happen? http://codereview.chromium.org/8513002/diff/10004/chrome/browser/ui/gtk/omnib... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/10004/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1449: string16()); Nit: Indent 4, not 8
On 2011/11/15 18:27:26, Peter Kasting wrote: > On 2011/11/15 12:38:46, SanjoyPal wrote: > > On 2011/11/15 07:26:12, Peter Kasting wrote: > > > This would fix a crash-on-right-click but I am still concerned about the > > actual > > > paste case. Where do we sanitize strings that are pasted in (not > > paste-and-go, > > > this will address that)? > > > > There is no crash while pasting on the omnibox. Crash occurs when creating > popup > > on right click on omnibox. I have updated the CL description. > > Why did you remove the BUG= number? Restore it and add 82181 to the list also. > > If we don't crash when pasting, how are we fixing up the user's input? Are we > stripping the javascript scheme from it too? Where does this happen? Done. > > http://codereview.chromium.org/8513002/diff/10004/chrome/browser/ui/gtk/omnib... > File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): > > http://codereview.chromium.org/8513002/diff/10004/chrome/browser/ui/gtk/omnib... > chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1449: string16()); > Nit: Indent 4, not 8 Done. PTAL. Thanks.
http://codereview.chromium.org/8513002/diff/10004/chrome/browser/ui/gtk/omnib... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/10004/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1449: string16()); On 2011/11/15 18:27:26, Peter Kasting wrote: > Nit: Indent 4, not 8 Done.
http://codereview.chromium.org/8513002/diff/20001/chrome/browser/ui/gtk/omnib... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/20001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:881: string16 data_string; Nit: This whole block can be collapsed to just: data.GetString(&text); ...since OnPerformDropImpl() does the whitespace collapsing already. http://codereview.chromium.org/8513002/diff/20001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1448: CollapseWhitespace(StripJavascriptSchemas(UTF8ToUTF16(text)), true) : Why did you reverse the order of the calls here? The previous patch had them correct. http://codereview.chromium.org/8513002/diff/20001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1461: model_->CanPasteAndGo(sanitized_text)); Why did you move this call back down here since the last patch? It needed to happen above the is_paste_and_search() call. http://codereview.chromium.org/8513002/diff/20001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1660: if (c != L'\n' && c != L'\r' && c != L'\t' && c != 0x200B) Seems like we should remove the first three checks from here and instead add after the while loop: if (model_.is_pasting()) filtered_text = CollapseWhitespace(filtered_text, true); Then to autocomplete_edit.h, right after on_paste() is declared, add: bool is_pasting() const { return paste_state_ == PASTING; } (You should double-check that this block is hit during a paste operation and does not trigger when pressing single keys.) http://codereview.chromium.org/8513002/diff/20001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1668: if (sanitized_text.length()) { Nit: Change to using !empty() http://codereview.chromium.org/8513002/diff/20001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1802: if (model_->CanPasteAndGo(CollapseWhitespace(text, true))) { Nit: This needs a StripJavaScriptSchemas() call inserted so that we block dragging javascript: URLs onto the omnibox.
Addressed the comments in the previous patch. Please review. http://codereview.chromium.org/8513002/diff/20001/chrome/browser/ui/gtk/omnib... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/20001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:881: string16 data_string; On 2011/11/21 21:23:43, Peter Kasting wrote: > Nit: This whole block can be collapsed to just: > > data.GetString(&text); > > ...since OnPerformDropImpl() does the whitespace collapsing already. Done. http://codereview.chromium.org/8513002/diff/20001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1448: CollapseWhitespace(StripJavascriptSchemas(UTF8ToUTF16(text)), true) : On 2011/11/21 21:23:43, Peter Kasting wrote: > Why did you reverse the order of the calls here? The previous patch had them > correct. Done. http://codereview.chromium.org/8513002/diff/20001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1461: model_->CanPasteAndGo(sanitized_text)); On 2011/11/21 21:23:43, Peter Kasting wrote: > Why did you move this call back down here since the last patch? It needed to > happen above the is_paste_and_search() call. Done. http://codereview.chromium.org/8513002/diff/20001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1660: if (c != L'\n' && c != L'\r' && c != L'\t' && c != 0x200B) On 2011/11/21 21:23:43, Peter Kasting wrote: > Seems like we should remove the first three checks from here and instead add > after the while loop: > > if (model_.is_pasting()) > filtered_text = CollapseWhitespace(filtered_text, true); > > Then to autocomplete_edit.h, right after on_paste() is declared, add: > > bool is_pasting() const { return paste_state_ == PASTING; } > > (You should double-check that this block is hit during a paste operation and > does not trigger when pressing single keys.) It was hitting when pressing single keys. Done. http://codereview.chromium.org/8513002/diff/20001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1668: if (sanitized_text.length()) { On 2011/11/21 21:23:43, Peter Kasting wrote: > Nit: Change to using !empty() Done. http://codereview.chromium.org/8513002/diff/20001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1802: if (model_->CanPasteAndGo(CollapseWhitespace(text, true))) { On 2011/11/21 21:23:43, Peter Kasting wrote: > Nit: This needs a StripJavaScriptSchemas() call inserted so that we block > dragging javascript: URLs onto the omnibox. Done.
http://codereview.chromium.org/8513002/diff/29001/chrome/browser/ui/gtk/omnib... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/29001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1668: CollapseWhitespace(UTF8ToUTF16(filtered_text), true))); Nit: Can all the text - including |filtered_text| - as UTF16 (string16) and only convert to UTF8 right before calling klass->insert_text()? This would avoid the excessive intermediary conversions here and make the code a bit cleaner.
> Nit: Can all the text - including |filtered_text| - as UTF16 (string16) and only Sorry, that have said be "Can you keep all the text"...
On Tue, Nov 22, 2011 at 11:27 PM, <asvitkine@chromium.org> wrote: > Nit: Can all the text - including |filtered_text| - as UTF16 (string16) >> and >> > only > > Sorry, that have said be "Can you keep all the text"... > Wow, I can't type tonight. "That should have said "Can you keep all the text"..." Sigh.
Done. PTAL. http://codereview.chromium.org/8513002/diff/29001/chrome/browser/ui/gtk/omnib... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/29001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1668: CollapseWhitespace(UTF8ToUTF16(filtered_text), true))); On 2011/11/23 04:26:41, Alexei Svitkine wrote: > Nit: Can all the text - including |filtered_text| - as UTF16 (string16) and only > convert to UTF8 right before calling klass->insert_text()? This would avoid the > excessive intermediary conversions here and make the code a bit cleaner. Done.
http://codereview.chromium.org/8513002/diff/29001/chrome/browser/ui/gtk/omnib... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/29001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1659: filtered_text.append(p, next); Now, this isn't right anymore - since it doesn't make sense to append utf8 to a utf16 string. You need to use: WriteUnicodeCharacter(c, &filtered_text); (from base/utf_string_conversion_utils.h)
LGTM other than the WriteUnicodeCharacter() issue, which needs to be fixed. http://codereview.chromium.org/8513002/diff/33001/chrome/browser/ui/gtk/omnib... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/33001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1450: // Paste and Go menu item. Nit: To prevent future problems, maybe we should add "Note that CanPasteAndGo() needs to be called before is_paste_and_search() in order to set up the paste-and-go state." http://codereview.chromium.org/8513002/diff/33001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1659: filtered_text.append(p, next); What asvitkine said about using WriteUnicodeCharacter(). Note that if you do this, you can eliminate |next| above and convert this while() loop to a simple for() loop: for (const gchar* p = text; *p && ((p - text) < len); p = g_utf8_next_char(p)) { ... http://codereview.chromium.org/8513002/diff/33001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1664: string16 sanitized_text; Nit: Simpler: if (model_->is_pasting()) { filtered_text = StripJavascriptSchemas(CollapseWhitespace(filtered_text, true)); } ...and use |filtered_text| directly below. http://codereview.chromium.org/8513002/diff/33001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1807: (CollapseWhitespace(text, true)))) { Nit: Put '(' on previous line.
PTAL. Thanks http://codereview.chromium.org/8513002/diff/33001/chrome/browser/ui/gtk/omnib... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8513002/diff/33001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1450: // Paste and Go menu item. On 2011/11/23 19:02:11, Peter Kasting wrote: > Nit: To prevent future problems, maybe we should add "Note that CanPasteAndGo() > needs to be called before is_paste_and_search() in order to set up the > paste-and-go state." Done. http://codereview.chromium.org/8513002/diff/33001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1659: filtered_text.append(p, next); On 2011/11/23 19:02:11, Peter Kasting wrote: > What asvitkine said about using WriteUnicodeCharacter(). > > Note that if you do this, you can eliminate |next| above and convert this > while() loop to a simple for() loop: > > for (const gchar* p = text; *p && ((p - text) < len); > p = g_utf8_next_char(p)) { > ... Note that WriteUnicodeCharacter() was not exposed, i had to export it. Done. http://codereview.chromium.org/8513002/diff/33001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1664: string16 sanitized_text; On 2011/11/23 19:02:11, Peter Kasting wrote: > Nit: Simpler: > > if (model_->is_pasting()) { > filtered_text = > StripJavascriptSchemas(CollapseWhitespace(filtered_text, true)); > } > > ...and use |filtered_text| directly below. Done. http://codereview.chromium.org/8513002/diff/33001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1807: (CollapseWhitespace(text, true)))) { On 2011/11/23 19:02:11, Peter Kasting wrote: > Nit: Put '(' on previous line. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ncj674@motorola.com/8513002/35003
Presubmit check for 8513002-35003 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change requires manual test instructions to QA team, add TEST=[instructions]. ** Presubmit ERRORS ** Missing LGTM from an OWNER for: base/utf_string_conversion_utils.h Presubmit checks took 2.0s to calculate.
Can you change the description line to list the two bugs on the same line? Like: BUG=82181,103703 Also, please change the TEST line to be non-empty. (And you need someone from base/OWNERS to approve the export.) -Alexei
> > Also, please change the TEST line to be non-empty. > (In other words, start your test message on the same line as TEST=)
Hi Darin, Could you please review the change in base/utf_string_conversion_utils.h. Thanks.
base change lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ncj674@motorola.com/8513002/35003
Change committed as 111548
Previous patch broke OmniboxViewTest.AcceptKeywordBySpace. Updated the patch. Please one more time.
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.
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/ |