Fixed regression with extension omnibox API where whitespace would get trimmed

Fixed regression with extension omnibox API where whitespace would get trimmed
after style markup, introduced by r107746.

BUG=106355
TEST=see bug for repro


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113554

[Alexei Svitkine (slow), 2011-12-07 05:32:31]

If I'm understanding the problem correctly, I think the bug here is that we
trimLeft() every node of text appended, which will incorrectly trim space in the
middle of result.description.

I think the fix is to make the JS sanitizeString() take a |shouldTrim|
parameter, which will control whether it calls trimLeft() or not and then change
the call site to:

var shouldTrim = (result.description.length == 0);
result.description += sanitizeString(child.nodeValue, shouldTrim);

With that, I don't think we need any changes to the C++ code that already trims
only the leading text. Can you verify whether that fixes the issue and if so,
update the CL with that change?

[Matt Perry, 2011-12-07 20:33:53]

OK, I made the change as you described, and it works. Please take a look.

[Alexei Svitkine (slow), 2011-12-07 20:39:08]

LGTM

[Alexei Svitkine (slow), 2011-12-07 20:42:26]

On 2011/12/07 20:39:08, Alexei Svitkine wrote:
> LGTM

Sorry, I take that back. This is wrong, because the style indices will be
incorrect.

Take a look at this code:

 var style = {
              'type': child.nodeName,
              'offset': result.description.length	
            };	 

It sets 'offset' to the current description length. So doing a trim at the end
won't work. (Which is incidentally why I didn't call sanitizeString() on the
description at the end and why it has to be done in Javascript and not at a
different level).

Please do it as I suggested in my comment, instead.

[Matt Perry, 2011-12-07 21:21:10]

Thanks for catching that. I've redone it how you suggested, and verified that it
works for the case of leading spaces.

[Alexei Svitkine (slow), 2011-12-07 21:23:38]

LGTM for realz now. Thanks!

[commit-bot: I haz the power, 2011-12-07 23:12:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpcomplete@chromium.org/8824018/7003

[commit-bot: I haz the power, 2011-12-08 03:38:53]

Change committed as 113554