[Omnibox] Changing scheme from https:// to http:// results in DCHECK (Always).

[Omnibox] Changing protocol scheme from https:// to http:// results in DCHECK (Always).

With present implementation, for giving scores to the history urls based on supplied term string, it calculates word start offsets for each term (word). However cases where term do not having word-part set to zero (as default). This leads to DCHECK(at_word_boundary) fail in later part execution inside ScoredHistoryMatch::GetTopicalityScore(). For instance, term string "http ://", both the terms {"http", "://"} are offsets to {0, 0}.

This patch, sets the missing word-parts start offset to the size of the term (word), i.e. for terms {"http", "://"} sets start offsets to {0, 3}, avoiding this DCHECK failure.

BUG=495571
R=mpearson@chromium.org
TEST=InMemoryURLIndexTest.AddHistoryMatch

Committed: https://crrev.com/9501f2bcce4f6537edf70bbeeef8ddeea927cd76
Cr-Commit-Position: refs/heads/master@{#334340}

[Pritam Nikam, 2015-06-04 10:43:27]

pritam.nikam@samsung.com changed reviewers:
+ vivek.vg@samsung.com

[Pritam Nikam, 2015-06-04 10:44:46]

Hi Vivek,

I've encountered this DCHECK failure in my local testing. Please glance through
it.

Thanks!

[Peter Kasting, 2015-06-04 21:14:52]

pkasting@chromium.org changed reviewers:
+ mpearson@chromium.org

[Peter Kasting, 2015-06-04 21:15:05]

On 2015/06/04 10:44:46, Pritam Nikam wrote:
> Hi Vivek,
> 
> I've encountered this DCHECK failure in my local testing. Please glance
through
> it.

As an owner of this code: who is Vivek?  It seems like mpearson (added) would
have been the correct reviewer here.

[Mark P, 2015-06-04 22:08:34]

I'm not convinced the current code is wrong.  Can you give an example of the
current values of lower_terms_to_word_starts_offsets_ for an input and the
values your new code produces?

From reading the bug, I would think where something is going wrong is either
where we split by cursor position to produce terms to match
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
or somewhere in chrome/browser/autocomplete/scored_history_match.cc that takes
the resulting terms and searches for their hits in the title and URL.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...

(From the bug, I wouldn't think the problem has to do with setting term
positions in general.)

--mark

[Pritam Nikam, 2015-06-05 12:25:30]

On 2015/06/04 22:08:34, Mark P wrote:
> I'm not convinced the current code is wrong.  Can you give an example of the
> current values of lower_terms_to_word_starts_offsets_ for an input and the
> values your new code produces?
> 
> From reading the bug, I would think where something is going wrong is either
> where we split by cursor position to produce terms to match
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
> or somewhere in chrome/browser/autocomplete/scored_history_match.cc that takes
> the resulting terms and searches for their hits in the title and URL.
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
> 
> (From the bug, I wouldn't think the problem has to do with setting term
> positions in general.)
> 
> --mark

Thanks Peter and Mark to pitch in.

Vivek is my colleague and I've requested his opinion on patch before I'll add
file owner for review.


@Mark,
Presently, I could see once we delete 's' from "https://" in omnibox,
lower_terms_ were kept {"http", "://"} and respective
lower_terms_to_word_starts_offsets_ were {0, 0}. And I agree these splits are
based on cursor position. Moreover, even if we pass "http<white-space>://" to
omnibox we'll end-up getting same lower_terms_ and
lower_terms_to_word_starts_offsets_. So when we calculate topicality score in
ScoredHistoryMatch::GetTopicalityScore, we advance next_word_starts (ref [1]),
and at_word_boundary stays false. This intern results into DCHECK for protocol
scheme at [2]. I can help here with debug logs if needed.

In my opinion, if we take care of adjusting lower_terms_to_word_starts_offsets_
for non-word terms (here "://") to size (or length, here 3) instead of keeping
these to default i.e. 0, this problem shall go off as we'll actually correcting
the at_word_boundary calculation (it may sound odd, but that would be an easy
fix).

[1].
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
[2].
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...

Please correct my understand.

Thanks!

[Mark P, 2015-06-05 17:22:24]

On 2015/06/05 12:25:30, Pritam Nikam wrote:
> On 2015/06/04 22:08:34, Mark P wrote:
> > I'm not convinced the current code is wrong.  Can you give an example of the
> > current values of lower_terms_to_word_starts_offsets_ for an input and the
> > values your new code produces?
> > 
> > From reading the bug, I would think where something is going wrong is either
> > where we split by cursor position to produce terms to match
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
> > or somewhere in chrome/browser/autocomplete/scored_history_match.cc that
takes
> > the resulting terms and searches for their hits in the title and URL.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
> > 
> > (From the bug, I wouldn't think the problem has to do with setting term
> > positions in general.)
> > 
> > --mark
> 
> Thanks Peter and Mark to pitch in.
> 
> Vivek is my colleague and I've requested his opinion on patch before I'll add
> file owner for review.
> 
> 
> @Mark,
> Presently, I could see once we delete 's' from "https://" in omnibox,
> lower_terms_ were kept {"http", "://"} and respective
> lower_terms_to_word_starts_offsets_ were {0, 0}. And I agree these splits are
> based on cursor position. Moreover, even if we pass "http<white-space>://" to
> omnibox we'll end-up getting same lower_terms_ and
> lower_terms_to_word_starts_offsets_. So when we calculate topicality score in
> ScoredHistoryMatch::GetTopicalityScore, we advance next_word_starts (ref [1]),
> and at_word_boundary stays false. This intern results into DCHECK for protocol
> scheme at [2]. I can help here with debug logs if needed.
> 
> In my opinion, if we take care of adjusting
lower_terms_to_word_starts_offsets_
> for non-word terms (here "://") to size (or length, here 3) instead of keeping
> these to default i.e. 0, this problem shall go off as we'll actually
correcting
> the at_word_boundary calculation (it may sound odd, but that would be an easy
> fix).
> 
> [1].
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
> [2].
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
> 
> Please correct my understand.

Thanks for the explanation.  Now I understand what's going on.  Your fix looks
like a reasonable approach.  It would be nice if you can add some tests to
AddHistoryMatch() for lower_terms_to_word_starts_offsets_ while you're touching
this code.  This is the second time we've had a bug / weird thing here, and in
general it'd be nice to know that the offsets makes sense for all sorts of
inputs (starting with punctuation, ending with punctuation, punctuation in
middle, hyphenation, etc.)

I'm happy to be the official reviewer of this changelist.

--mark

[Pritam Nikam, 2015-06-05 18:48:32]

On 2015/06/05 17:22:24, Mark P wrote:
> On 2015/06/05 12:25:30, Pritam Nikam wrote:
> > On 2015/06/04 22:08:34, Mark P wrote:
> > > I'm not convinced the current code is wrong.  Can you give an example of
the
> > > current values of lower_terms_to_word_starts_offsets_ for an input and the
> > > values your new code produces?
> > > 
> > > From reading the bug, I would think where something is going wrong is
either
> > > where we split by cursor position to produce terms to match
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
> > > or somewhere in chrome/browser/autocomplete/scored_history_match.cc that
> takes
> > > the resulting terms and searches for their hits in the title and URL.
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
> > > 
> > > (From the bug, I wouldn't think the problem has to do with setting term
> > > positions in general.)
> > > 
> > > --mark
> > 
> > Thanks Peter and Mark to pitch in.
> > 
> > Vivek is my colleague and I've requested his opinion on patch before I'll
add
> > file owner for review.
> > 
> > 
> > @Mark,
> > Presently, I could see once we delete 's' from "https://" in omnibox,
> > lower_terms_ were kept {"http", "://"} and respective
> > lower_terms_to_word_starts_offsets_ were {0, 0}. And I agree these splits
are
> > based on cursor position. Moreover, even if we pass "http<white-space>://"
to
> > omnibox we'll end-up getting same lower_terms_ and
> > lower_terms_to_word_starts_offsets_. So when we calculate topicality score
in
> > ScoredHistoryMatch::GetTopicalityScore, we advance next_word_starts (ref
[1]),
> > and at_word_boundary stays false. This intern results into DCHECK for
protocol
> > scheme at [2]. I can help here with debug logs if needed.
> > 
> > In my opinion, if we take care of adjusting
> lower_terms_to_word_starts_offsets_
> > for non-word terms (here "://") to size (or length, here 3) instead of
keeping
> > these to default i.e. 0, this problem shall go off as we'll actually
> correcting
> > the at_word_boundary calculation (it may sound odd, but that would be an
easy
> > fix).
> > 
> > [1].
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
> > [2].
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
> > 
> > Please correct my understand.
> 
> Thanks for the explanation.  Now I understand what's going on.  Your fix looks
> like a reasonable approach.  It would be nice if you can add some tests to
> AddHistoryMatch() for lower_terms_to_word_starts_offsets_ while you're
touching
> this code.  This is the second time we've had a bug / weird thing here, and in
> general it'd be nice to know that the offsets makes sense for all sorts of
> inputs (starting with punctuation, ending with punctuation, punctuation in
> middle, hyphenation, etc.)
> 
> I'm happy to be the official reviewer of this changelist.
> 
> --mark

[Pritam Nikam, 2015-06-05 18:51:47]

On 2015/06/05 18:48:32, Pritam Nikam wrote:
> On 2015/06/05 17:22:24, Mark P wrote:
> > On 2015/06/05 12:25:30, Pritam Nikam wrote:
> > > On 2015/06/04 22:08:34, Mark P wrote:
> > > > I'm not convinced the current code is wrong.  Can you give an example of
> the
> > > > current values of lower_terms_to_word_starts_offsets_ for an input and
the
> > > > values your new code produces?
> > > > 
> > > > From reading the bug, I would think where something is going wrong is
> either
> > > > where we split by cursor position to produce terms to match
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
> > > > or somewhere in chrome/browser/autocomplete/scored_history_match.cc that
> > takes
> > > > the resulting terms and searches for their hits in the title and URL.
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
> > > > 
> > > > (From the bug, I wouldn't think the problem has to do with setting term
> > > > positions in general.)
> > > > 
> > > > --mark
> > > 
> > > Thanks Peter and Mark to pitch in.
> > > 
> > > Vivek is my colleague and I've requested his opinion on patch before I'll
> add
> > > file owner for review.
> > > 
> > > 
> > > @Mark,
> > > Presently, I could see once we delete 's' from "https://" in omnibox,
> > > lower_terms_ were kept {"http", "://"} and respective
> > > lower_terms_to_word_starts_offsets_ were {0, 0}. And I agree these splits
> are
> > > based on cursor position. Moreover, even if we pass "http<white-space>://"
> to
> > > omnibox we'll end-up getting same lower_terms_ and
> > > lower_terms_to_word_starts_offsets_. So when we calculate topicality score
> in
> > > ScoredHistoryMatch::GetTopicalityScore, we advance next_word_starts (ref
> [1]),
> > > and at_word_boundary stays false. This intern results into DCHECK for
> protocol
> > > scheme at [2]. I can help here with debug logs if needed.
> > > 
> > > In my opinion, if we take care of adjusting
> > lower_terms_to_word_starts_offsets_
> > > for non-word terms (here "://") to size (or length, here 3) instead of
> keeping
> > > these to default i.e. 0, this problem shall go off as we'll actually
> > correcting
> > > the at_word_boundary calculation (it may sound odd, but that would be an
> easy
> > > fix).
> > > 
> > > [1].
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
> > > [2].
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
> > > 
> > > Please correct my understand.
> > 
> > Thanks for the explanation.  Now I understand what's going on.  Your fix
looks
> > like a reasonable approach.  It would be nice if you can add some tests to
> > AddHistoryMatch() for lower_terms_to_word_starts_offsets_ while you're
> touching
> > this code.  This is the second time we've had a bug / weird thing here, and
in
> > general it'd be nice to know that the offsets makes sense for all sorts of
> > inputs (starting with punctuation, ending with punctuation, punctuation in
> > middle, hyphenation, etc.)
> > 
> > I'm happy to be the official reviewer of this changelist.
> > 
> > --mark

Sure Mark. I'll add tests and update this patch set on Monday.
Thanks!

[Pritam Nikam, 2015-06-08 15:25:48]

Hi Mark,
I've added unit tests. Please have a look.

Thanks!

[Mark P, 2015-06-08 18:13:17]

Generally fine.  Lots of minor comments, nothing major.

--mark

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/in_memory_url_index_unittest.cc (right):

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1197: // Helper
function to get lower case |lower_string| and |lower_terms| (words
get -> set

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1198: // list) based
on supplied |search_string| and |cursor_position|.
It's a bit confusing that we add a space in order to get |lower_string|.  Can
you perhaps add a comment here, something like the one in
url_index_private_data.cc:
   // If cursor position is set and useful (not at either end of the
  // string), allow the search string to be broken at cursor position.
  // We do this by pretending there's a space where the cursor is.
Also, it would help to explain that lower_terms is obtained by splitting
lower_string on whitespace.

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1199: void
StringToTerms(const char* search_string,
nit: Please move this helper function into the anonymous namespace at the top of
this file.

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1218: /* No
punctuations, only cursor position change. */
style nit: you have indented two space too many here and below

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1219: {"ABCD",
base::string16::npos, {0}},
style nit: one space inside { before " and one space before the final }

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1220: {"abcd", 0,
{0}},
optional nit: align the { in the expected_word_starts_offsets lists so it's easy
to read each line
(and also align the cursor_position field.  This is naturally mostly aligned
here because your test strings are usually the same length, but not always.)

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1224: /* Staring
with punctuation. */
Staring -> Starting

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1243: {"ab:cd", 5,
{0}},
Can you add a cursor_position = 3 test in this block too?

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1265: if
(!lower_terms.empty()) {
You shouldn't guard this code in this way.  For instance, suppose the _test_
erroneously returned lower_terms.size() == 0 always.  Then we'd claim that
AddHistoryMatch() is functioning correctly, even though never actually made any
calls to AddHistoryMatch.

I don't think you need the guard at all in this test.  Do you?  If not, please
remove it.

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/url_index_private_data.cc (right):

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/url_index_private_data.cc:1293: // an offset of term
size. For an instance, the offset for "://" should be
nit: For an instance -> For example

[Pritam Nikam, 2015-06-09 10:03:53]

Thanks Mark for review inputs.
I've addressed these along new patch set (#3), ptal.

Thanks!

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/in_memory_url_index_unittest.cc (right):

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1197: // Helper
function to get lower case |lower_string| and |lower_terms| (words
On 2015/06/08 18:13:16, Mark P wrote:
> get -> set

Done.

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1198: // list) based
on supplied |search_string| and |cursor_position|.
On 2015/06/08 18:13:16, Mark P wrote:
> It's a bit confusing that we add a space in order to get |lower_string|.  Can
> you perhaps add a comment here, something like the one in
> url_index_private_data.cc:
>    // If cursor position is set and useful (not at either end of the
>   // string), allow the search string to be broken at cursor position.
>   // We do this by pretending there's a space where the cursor is.
> Also, it would help to explain that lower_terms is obtained by splitting
> lower_string on whitespace.

Done.

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1199: void
StringToTerms(const char* search_string,
On 2015/06/08 18:13:16, Mark P wrote:
> nit: Please move this helper function into the anonymous namespace at the top
of
> this file.

Done.

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1218: /* No
punctuations, only cursor position change. */
On 2015/06/08 18:13:16, Mark P wrote:
> style nit: you have indented two space too many here and below

Done.

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1219: {"ABCD",
base::string16::npos, {0}},
On 2015/06/08 18:13:16, Mark P wrote:
> style nit: one space inside { before " and one space before the final }

Done.

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1220: {"abcd", 0,
{0}},
On 2015/06/08 18:13:16, Mark P wrote:
> optional nit: align the { in the expected_word_starts_offsets lists so it's
easy
> to read each line
> (and also align the cursor_position field.  This is naturally mostly aligned
> here because your test strings are usually the same length, but not always.)

Done.

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1224: /* Staring
with punctuation. */
On 2015/06/08 18:13:16, Mark P wrote:
> Staring -> Starting

Done.

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1243: {"ab:cd", 5,
{0}},
On 2015/06/08 18:13:16, Mark P wrote:
> Can you add a cursor_position = 3 test in this block too?

Done.

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1265: if
(!lower_terms.empty()) {
On 2015/06/08 18:13:16, Mark P wrote:
> You shouldn't guard this code in this way.  For instance, suppose the _test_
> erroneously returned lower_terms.size() == 0 always.  Then we'd claim that
> AddHistoryMatch() is functioning correctly, even though never actually made
any
> calls to AddHistoryMatch.
> 
> I don't think you need the guard at all in this test.  Do you?  If not, please
> remove it.

Done.

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/url_index_private_data.cc (right):

https://codereview.chromium.org/1163963004/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/url_index_private_data.cc:1293: // an offset of term
size. For an instance, the offset for "://" should be
On 2015/06/08 18:13:16, Mark P wrote:
> nit: For an instance -> For example

Done.

[Mark P, 2015-06-09 17:19:09]

lgtm

[Pritam Nikam, 2015-06-10 05:30:24]

On 2015/06/09 17:19:09, Mark P wrote:
> lgtm

Thanks Mark! I'll proceed with commit.

Thanks!

[Pritam Nikam, 2015-06-10 05:30:28]

The CQ bit was checked by pritam.nikam@samsung.com

[commit-bot: I haz the power, 2015-06-10 05:31:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1163963004/40001

[commit-bot: I haz the power, 2015-06-10 05:54:54]

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

[commit-bot: I haz the power, 2015-06-10 05:54:55]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Pritam Nikam, 2015-06-10 10:47:06]

On 2015/06/10 05:54:55, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

Seems mac bot failed with C++11 error about initializing explicit constructor
with {}.

Errors look like this:
../../chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1224:37:
error: chosen constructor is explicit in copy-initialization
    { "ABCD", base::string16::npos, {0} },

Adding explicit vector in initialization we can avoid errors.

However, there are failures on executing unit test for mac -
TEST=InMemoryURLIndexTest.AddHistoryMatch. Analysing it further.

Thanks!

[Pritam Nikam, 2015-06-10 15:12:01]

The CQ bit was checked by pritam.nikam@samsung.com to run a CQ dry run

[Pritam Nikam, 2015-06-10 15:12:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/1163963004/#ps60001
(title: "Fixed breakages on mac trybot.")

[commit-bot: I haz the power, 2015-06-10 15:12:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1163963004/60001

[Pritam Nikam, 2015-06-10 15:44:34]

Hi Mark,

There seems a problem with mac trybot for C++11 (clang) error about initializing
explicit constructor with {}. I've tried one possible solution, i.e. on MAC OS
use size_t array instead of std::vector<size_t>. Please glance through the
changes.

Thanks!

[Mark P, 2015-06-10 16:53:01]

https://codereview.chromium.org/1163963004/diff/60001/chrome/browser/autocomp...
File chrome/browser/autocomplete/in_memory_url_index_unittest.cc (right):

https://codereview.chromium.org/1163963004/diff/60001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1230: const
SizeVector expected_word_starts_offsets;
Instead of this approach, just use a fixed size array, e.g.,
const size_t expected_word_starts_offsets[4]
and use an invalid value (e.g., -1) for the entries that don't need to exist.
This is the usual technique used in unit tests.

Sorry I didn't notice this earlier.

[commit-bot: I haz the power, 2015-06-10 17:04:33]

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

[commit-bot: I haz the power, 2015-06-10 17:04:35]

Dry run: This issue passed the CQ dry run.

[Pritam Nikam, 2015-06-11 03:06:10]

Hi Mark, PTAL.

Thanks!

https://codereview.chromium.org/1163963004/diff/60001/chrome/browser/autocomp...
File chrome/browser/autocomplete/in_memory_url_index_unittest.cc (right):

https://codereview.chromium.org/1163963004/diff/60001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1230: const
SizeVector expected_word_starts_offsets;
On 2015/06/10 16:53:01, Mark P wrote:
> Instead of this approach, just use a fixed size array, e.g.,
> const size_t expected_word_starts_offsets[4]
> and use an invalid value (e.g., -1) for the entries that don't need to exist.
> This is the usual technique used in unit tests.
> 
> Sorry I didn't notice this earlier.

Done.

[Pritam Nikam, 2015-06-11 03:06:28]

The CQ bit was checked by pritam.nikam@samsung.com to run a CQ dry run

[Pritam Nikam, 2015-06-11 03:06:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/1163963004/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-06-11 03:07:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1163963004/80001

[commit-bot: I haz the power, 2015-06-11 04:36:06]

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

[commit-bot: I haz the power, 2015-06-11 04:36:07]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Mark P, 2015-06-11 18:37:06]

https://codereview.chromium.org/1163963004/diff/80001/chrome/browser/autocomp...
File chrome/browser/autocomplete/in_memory_url_index_unittest.cc (right):

https://codereview.chromium.org/1163963004/diff/80001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1289: }
You should either check that match.lower_terms_to_word_starts_offsets_.size() is
the expected length or (equivalently) test that everything from j where the
above loop ends to 4 all have values of -1.

(Otherwise a function that always returns an empty
lower_terms_to_word_starts_offsets_ list will always pass this test.)

[Pritam Nikam, 2015-06-12 06:09:50]

Thanks Mark. I've incorporated review input in new patch set, please have a
look.

Thanks!

https://codereview.chromium.org/1163963004/diff/80001/chrome/browser/autocomp...
File chrome/browser/autocomplete/in_memory_url_index_unittest.cc (right):

https://codereview.chromium.org/1163963004/diff/80001/chrome/browser/autocomp...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1289: }
On 2015/06/11 18:37:06, Mark P wrote:
> You should either check that match.lower_terms_to_word_starts_offsets_.size()
is
> the expected length or (equivalently) test that everything from j where the
> above loop ends to 4 all have values of -1.
> 
> (Otherwise a function that always returns an empty
> lower_terms_to_word_starts_offsets_ list will always pass this test.)

Done.

Now, I'm matching match.lower_terms_to_word_starts_offsets_.size() against
expected length (expected_word_starts_offsets_size).

[Pritam Nikam, 2015-06-12 06:10:28]

The CQ bit was checked by pritam.nikam@samsung.com to run a CQ dry run

[Pritam Nikam, 2015-06-12 06:10:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/1163963004/#ps100001
(title: "Incorporates review.")

[commit-bot: I haz the power, 2015-06-12 06:10:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1163963004/100001

[commit-bot: I haz the power, 2015-06-12 07:03:47]

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

[commit-bot: I haz the power, 2015-06-12 07:03:48]

Dry run: This issue passed the CQ dry run.

[Mark P, 2015-06-12 20:02:26]

lgtm

https://codereview.chromium.org/1163963004/diff/100001/chrome/browser/autocom...
File chrome/browser/autocomplete/in_memory_url_index_unittest.cc (right):

https://codereview.chromium.org/1163963004/diff/100001/chrome/browser/autocom...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1223: const size_t
expected_word_starts_offsets[4];
FYI: this can be [3] if you want to bother touching this changelist again and
shorten the below initializer lists.

[Pritam Nikam, 2015-06-13 10:44:48]

Patchset #7 (id:120001) has been deleted

[Pritam Nikam, 2015-06-13 10:48:16]

Thanks Mark,
I'll go ahead with CQ.

Thanks!

https://codereview.chromium.org/1163963004/diff/100001/chrome/browser/autocom...
File chrome/browser/autocomplete/in_memory_url_index_unittest.cc (right):

https://codereview.chromium.org/1163963004/diff/100001/chrome/browser/autocom...
chrome/browser/autocomplete/in_memory_url_index_unittest.cc:1223: const size_t
expected_word_starts_offsets[4];
On 2015/06/12 20:02:26, Mark P wrote:
> FYI: this can be [3] if you want to bother touching this changelist again and
> shorten the below initializer lists.

Sure, done.

[Pritam Nikam, 2015-06-13 10:50:49]

The CQ bit was checked by pritam.nikam@samsung.com to run a CQ dry run

[Pritam Nikam, 2015-06-13 10:50:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/1163963004/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-06-13 10:50:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1163963004/140001

[commit-bot: I haz the power, 2015-06-13 12:22:02]

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

[commit-bot: I haz the power, 2015-06-13 12:22:03]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Pritam Nikam, 2015-06-13 12:29:12]

The CQ bit was checked by pritam.nikam@samsung.com

[commit-bot: I haz the power, 2015-06-13 12:29:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1163963004/140001

[commit-bot: I haz the power, 2015-06-13 13:20:43]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-06-13 13:21:32]

Patchset 7 (id:??) landed as
https://crrev.com/9501f2bcce4f6537edf70bbeeef8ddeea927cd76
Cr-Commit-Position: refs/heads/master@{#334340}