Omnibox: Fix to Display Bookmarks With Leading Spaces

Omnibox: Fix for Bookmarks With Leading Spaces

These would cause DCHECK failures later in the system.

BUG=403518
Committed: https://crrev.com/7004e38c80048876907398d58c0b8f08c53e773a
Cr-Commit-Position: refs/heads/master@{#291924}

[Mark P, 2014-08-25 19:39:30]

Peter, please take a look.  This should be straightforward.

--mark

[Peter Kasting, 2014-08-25 19:46:44]

LGTM

https://codereview.chromium.org/499343004/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/bookmark_provider.cc (right):

https://codereview.chromium.org/499343004/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/bookmark_provider.cc:33: // Remove leading spaces
from |title| before displaying, otherwise it looks
Nit: Remove -> Removes; correct -> corrects

https://codereview.chromium.org/499343004/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/bookmark_provider.cc:39: base::string16
trimmed_title;
Nit: Saves the assignment later:

  size_t correction = title->length();
  base::TrimWhitespace(*title, base::TRIM_LEADING, title);
  correction -= title->length();

[Mark P, 2014-08-25 19:51:16]

https://codereview.chromium.org/499343004/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/bookmark_provider.cc (right):

https://codereview.chromium.org/499343004/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/bookmark_provider.cc:33: // Remove leading spaces
from |title| before displaying, otherwise it looks
On 2014/08/25 19:46:44, Peter Kasting wrote:
> Nit: Remove -> Removes; correct -> corrects

Both done.

https://codereview.chromium.org/499343004/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/bookmark_provider.cc:39: base::string16
trimmed_title;
On 2014/08/25 19:46:44, Peter Kasting wrote:
> Nit: Saves the assignment later:
> 
>   size_t correction = title->length();
>   base::TrimWhitespace(*title, base::TRIM_LEADING, title);
>   correction -= title->length();

Too much less readable, not worth it to save one short string copy.

[Peter Kasting, 2014-08-25 19:54:22]

On 2014/08/25 19:51:16, Mark P wrote:
>
https://codereview.chromium.org/499343004/diff/1/chrome/browser/autocomplete/...
> chrome/browser/autocomplete/bookmark_provider.cc:39: base::string16
> trimmed_title;
> On 2014/08/25 19:46:44, Peter Kasting wrote:
> > Nit: Saves the assignment later:
> > 
> >   size_t correction = title->length();
> >   base::TrimWhitespace(*title, base::TRIM_LEADING, title);
> >   correction -= title->length();
> 
> Too much less readable, not worth it to save one short string copy.

Eh?  This seems more readable than your version to me, especially if
"correction" gets renamed "leading_whitespace_chars" or something.

[Mark P, 2014-08-25 22:51:12]

On Mon, Aug 25, 2014 at 12:54 PM, <pkasting@chromium.org> wrote:

> On 2014/08/25 19:51:16, Mark P wrote:
>
> https://codereview.chromium.org/499343004/diff/1/chrome/
> browser/autocomplete/bookmark_provider.cc#newcode39
>
>> chrome/browser/autocomplete/bookmark_provider.cc:39: base::string16
>> trimmed_title;
>> On 2014/08/25 19:46:44, Peter Kasting wrote:
>> > Nit: Saves the assignment later:
>> >
>> >   size_t correction = title->length();
>> >   base::TrimWhitespace(*title, base::TRIM_LEADING, title);
>> >   correction -= title->length();
>>
>
>  Too much less readable, not worth it to save one short string copy.
>>
>
> Eh?  This seems more readable than your version to me, especially if
> "correction" gets renamed "leading_whitespace_chars" or something.
>
I still don't agree, but it's not worth discussing further.  Made changes
as you suggested.


> https://codereview.chromium.org/499343004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark P, 2014-08-25 22:51:20]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-08-25 22:53:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mpearson@chromium.org/499343004/40001

[commit-bot: I haz the power, 2014-08-26 01:24:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-08-26 01:37:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-26 01:37:21]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Mark P, 2014-08-26 17:18:49]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-08-26 17:19:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mpearson@chromium.org/499343004/40001

[commit-bot: I haz the power, 2014-08-26 17:20:55]

Committed patchset #3 (40001) as 33917baa645308d255f23042bd89c0cdcd9ceddd

[commit-bot: I haz the power, 2014-09-10 02:43:31]

Patchset 3 (id:??) landed as
https://crrev.com/7004e38c80048876907398d58c0b8f08c53e773a
Cr-Commit-Position: refs/heads/master@{#291924}