Strip invalid characters (line breaks, tabs), javascript:schemes from the copied text while pasti...

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=112923

[SanjoyPal, 2011-11-25 09:32:57]

Previous patch broke OmniboxViewTest.AcceptKeywordBySpace. Updated the patch.
Please review one more time.

[SanjoyPal, 2011-11-25 11:23:50]

On 2011/11/25 09:32:57, SanjoyPal wrote:
> Previous patch broke OmniboxViewTest.AcceptKeywordBySpace. Updated the patch.
> Please review one more time.

Patch Set 1 broke OmniboxViewTest.BackspaceDeleteHalfWidthKatakana, i could not
find the root cause. So, i uploaded patch set 2.

[Peter Kasting, 2011-11-28 00:04:02]

On 2011/11/25 11:23:50, SanjoyPal wrote:
> Patch Set 1 broke OmniboxViewTest.BackspaceDeleteHalfWidthKatakana, i could
not
> find the root cause. So, i uploaded patch set 2.

The patch sets should work identically.  Could the root cause have simply been
flakiness?  Did you run the tests several times?

I think you should restore the precise original patch (I mean the one from the
previous CL) and then explain more about why AcceptKeywordBySpace was broken. 
It shouldn't have been.  We don't accept keywords on pasting spaces and we
weren't collapsing whitespace on typing keypresses.

[SanjoyPal, 2011-11-28 05:24:18]

On 2011/11/28 00:04:02, Peter Kasting wrote:
> On 2011/11/25 11:23:50, SanjoyPal wrote:
> > Patch Set 1 broke OmniboxViewTest.BackspaceDeleteHalfWidthKatakana, i could
> not
> > find the root cause. So, i uploaded patch set 2.
> 
> The patch sets should work identically.  Could the root cause have simply been
> flakiness?  Did you run the tests several times?

Yes, I tested several times. It was not falky.

> 
> I think you should restore the precise original patch (I mean the one from the
> previous CL) and then explain more about why AcceptKeywordBySpace was broken. 
> It shouldn't have been.  We don't accept keywords on pasting spaces and we
> weren't collapsing whitespace on typing keypresses.

Say, we have a text like "text with trailing 5 spaces     ", now if we paste
this string then it becomes "text with trailing 5 spaces " with the previous CL
http://codereview.chromium.org/8513002/. But existing omnibox shows "text with
trailing 5 spaces     " after pasting. Thats why AcceptKeywordBySpace test is
failing. Is the current omnibox behavior correct or we need to modify the test
case?

[Peter Kasting, 2011-11-28 18:41:47]

On 2011/11/28 05:24:18, SanjoyPal wrote:
> On 2011/11/28 00:04:02, Peter Kasting wrote:
> > I think you should restore the precise original patch (I mean the one from
the
> > previous CL) and then explain more about why AcceptKeywordBySpace was
broken. 
> > It shouldn't have been.  We don't accept keywords on pasting spaces and we
> > weren't collapsing whitespace on typing keypresses.
> 
> Say, we have a text like "text with trailing 5 spaces     ", now if we paste
> this string then it becomes "text with trailing 5 spaces " with the previous
CL
> http://codereview.chromium.org/8513002/. But existing omnibox shows "text with
> trailing 5 spaces     " after pasting. Thats why AcceptKeywordBySpace test is
> failing. Is the current omnibox behavior correct or we need to modify the test
> case?

Pasting "xyz     " on Linux should result in "xyz" as it does on Windows and Mac
already.  If we have a test relying on the old behavior the test is wrong.

[SanjoyPal, 2011-11-29 07:03:48]

On 2011/11/28 18:41:47, Peter Kasting wrote:
> On 2011/11/28 05:24:18, SanjoyPal wrote:
> > On 2011/11/28 00:04:02, Peter Kasting wrote:
> > > I think you should restore the precise original patch (I mean the one from
> the
> > > previous CL) and then explain more about why AcceptKeywordBySpace was
> broken. 
> > > It shouldn't have been.  We don't accept keywords on pasting spaces and we
> > > weren't collapsing whitespace on typing keypresses.
> > 
> > Say, we have a text like "text with trailing 5 spaces     ", now if we paste
> > this string then it becomes "text with trailing 5 spaces " with the previous
> CL
> > http://codereview.chromium.org/8513002/. But existing omnibox shows "text
with
> > trailing 5 spaces     " after pasting. Thats why AcceptKeywordBySpace test
is
> > failing. Is the current omnibox behavior correct or we need to modify the
test
> > case?
> 
> Pasting "xyz     " on Linux should result in "xyz" as it does on Windows and
Mac
> already.  If we have a test relying on the old behavior the test is wrong.

Restored the original patch from previous CL and disabled the
AcceptKeywordBySpace test case. Now the behavior is same as Windows and Mac.

[Peter Kasting, 2011-11-29 19:07:28]

I don't see this code you said tried pasting text with a whole bunch of trailing
whitespace.

http://codereview.chromium.org/8702002/diff/9001/chrome/browser/ui/omnibox/om...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

http://codereview.chromium.org/8702002/diff/9001/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:760: // Keyword shouldn't
be accepted by pasting.
This block should be removed (since no platforms support pasting a space) and
instead just the SetWindowTextAndCaretPos() call should be moved down to the
beginning of the next block.

http://codereview.chromium.org/8702002/diff/9001/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1387: // This test is
DISABLED because of the behavioral modifications of omnibox.
It's wrong to disable the test.  If the test isn't passing it needs to be fixed,
not disabled.  The test obviously passes on Windows and Mac already so there's
no obvious reason it shouldn't work on Windows.

[SanjoyPal, 2011-11-30 06:49:26]

Modified the test case as per your suggestion. PTAL.

http://codereview.chromium.org/8702002/diff/9001/chrome/browser/ui/omnibox/om...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

http://codereview.chromium.org/8702002/diff/9001/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:760: // Keyword shouldn't
be accepted by pasting.
On 2011/11/29 19:07:28, Peter Kasting wrote:
> This block should be removed (since no platforms support pasting a space) and
> instead just the SetWindowTextAndCaretPos() call should be moved down to the
> beginning of the next block.

Done.

http://codereview.chromium.org/8702002/diff/9001/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1387: // This test is
DISABLED because of the behavioral modifications of omnibox.
On 2011/11/29 19:07:28, Peter Kasting wrote:
> It's wrong to disable the test.  If the test isn't passing it needs to be
fixed,
> not disabled.  The test obviously passes on Windows and Mac already so there's
> no obvious reason it shouldn't work on Windows.

Done.

[Peter Kasting, 2011-12-01 19:31:32]

If this works, then LGTM.

Not sure why the old test would have failed on GTK with the patched code.  The
block you removed should have mostly no-oped.  What was actually failing and
why?

[SanjoyPal, 2011-12-02 06:39:56]

On 2011/12/01 19:31:32, Peter Kasting wrote:
> If this works, then LGTM.
> 
> Not sure why the old test would have failed on GTK with the patched code.  The
> block you removed should have mostly no-oped.  What was actually failing and
> why?

Its failing at ASSERT_EQ(text + char16(' '), omnibox_view->GetText());

The value of (text + char16(' ')) is [foo ] and omnibox_view->GetText() is [foo]
because we are collapsing whitespace on pasting.

[Peter Kasting, 2011-12-02 19:32:07]

On 2011/12/02 06:39:56, SanjoyPal wrote:
> On 2011/12/01 19:31:32, Peter Kasting wrote:
> > If this works, then LGTM.
> > 
> > Not sure why the old test would have failed on GTK with the patched code. 
The
> > block you removed should have mostly no-oped.  What was actually failing and
> > why?
> 
> Its failing at ASSERT_EQ(text + char16(' '), omnibox_view->GetText());
> 
> The value of (text + char16(' ')) is [foo ] and omnibox_view->GetText() is
[foo]
> because we are collapsing whitespace on pasting.

After more digging I understand why this doesn't work with the new GTK code but
does work for Mac and Linux.

On Mac and Linux SetWindowTextAndCaretPos() will effectively set the text
directly and ignore pasting state, so the test won't accurately simulate a real
paste.  On GTK, because HandleInsertText() is reached for all cases, and that
now checks the paste state, we strip whitespace like a real paste would.

In theory I guess we could add some sort of checks to the Mac/Linux
implementations, but that would only have a practical effect on tests.  Any test
we'd need this fix for would be one trying to accurately simulate pasting by
using SetWindowTextAndCaretPos(), which fundamentally doesn't accurately
simulate pasting, so I'm OK not dealing with this case.

[commit-bot: I haz the power, 2011-12-03 13:08:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncj674@motorola.com/8702002/14001

[commit-bot: I haz the power, 2011-12-03 13:08:22]

Presubmit check for 8702002-14001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: base/utf_string_conversion_utils.h

Presubmit checks took 1.0s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Nico, 2011-12-03 21:55:13]

base lgtm stamp

[commit-bot: I haz the power, 2011-12-04 00:54:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncj674@motorola.com/8702002/14001

[commit-bot: I haz the power, 2011-12-04 03:14:59]

Change committed as 112923