Don't call AutocompleteInput::Parse() on a background thread, part 1.

Don't call AutocompleteInput::Parse() on a background thread, part 1.

Instead of creating the exact match in HistoryURLProvider::DoAutocomplete(),
pass it in in the HistoryURLProviderParams struct.

This also uses this match as the "can't find DB, fallback" match in Start(); I
don't see any disadvantage to creating that match on the results of running
fixup, instead of creating a pre-fixup match like before.

This also splits out CanPromoteMatchForInlineAutocomplete() from
PromoteMatchForInlineAutocomplete().  I'll be converting
PromoteMatchForInlineAutocomplete() into a different function in the next part
of this change, and this happens to make the calling convention a little more
sane anyway.

Also removes a few comments from the .cc file that are in the .h file already.

BUG=376199
TEST=none
R=mpearson@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277886

[Mark P, 2014-06-17 17:43:25]

https://codereview.chromium.org/336173005/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/history_url_provider.cc (left):

https://codereview.chromium.org/336173005/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_url_provider.cc:797: // Called on the main
thread when the query is complete.
Is this incorrect?  Why did you remove it?

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

https://codereview.chromium.org/336173005/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_url_provider.cc:470: return;
This makes me nervous.  Previously we'd create a a what-you-typed match on the
original text and use it if there was not DB.  I think the previous order was
conscious (otherwise we would've added the Fixup code earlier in this function).
 Now we only get such a match if the text is successfully fixed up.  A UWYT
match is really an important fallback, esp. if the user is in a screwed up
state.

Worse, these types of bugs are temperamental / hard-to-reproduce, meaning that
if this new approach causes problems, they're not likely to be reported.

https://codereview.chromium.org/336173005/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_url_provider.cc:535:
params->what_you_typed_match = what_you_typed_match;
I don't understand why this is necessary.  Didn't you set this in the
constructor above?

https://codereview.chromium.org/336173005/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_url_provider.cc:562:
!content::BrowserThread::IsMessageLoopValid(content::BrowserThread::UI));
elsewhere we usually use
IsThreadInitialized
rather than
IsMessageLoopValid

https://codereview.chromium.org/336173005/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_url_provider.cc:841: DCHECK(matches !=
NULL);
Perhaps DCHECK(params!=NULL)

https://codereview.chromium.org/336173005/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/history_url_provider.h (right):

https://codereview.chromium.org/336173005/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_url_provider.h:229: // Given a |match|
containing the "what you typed" suggestion created by
This comment needs to be slightly revised.

[Peter Kasting, 2014-06-17 20:39:48]

https://codereview.chromium.org/336173005/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/history_url_provider.cc (left):

https://codereview.chromium.org/336173005/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_url_provider.cc:797: // Called on the main
thread when the query is complete.
On 2014/06/17 17:43:25, Mark P wrote:
> Is this incorrect?  Why did you remove it?

This comment is already present in the header.

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

https://codereview.chromium.org/336173005/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_url_provider.cc:470: return;
On 2014/06/17 17:43:25, Mark P wrote:
> This makes me nervous.  Previously we'd create a a what-you-typed match on the
> original text and use it if there was not DB.  I think the previous order was
> conscious (otherwise we would've added the Fixup code earlier in this
function).

It wasn't conscious.  The two pieces were written (both by me) at completely
different times and I wasn't thinking about one when I wrote the other.

>  Now we only get such a match if the text is successfully fixed up.  A UWYT
> match is really an important fallback, esp. if the user is in a screwed up
> state.

If FixupUserInput() fails, the WYT match wouldn't have been navigable anyway. 
We're not losing anything.

I'm not worried about this at all.

https://codereview.chromium.org/336173005/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_url_provider.cc:535:
params->what_you_typed_match = what_you_typed_match;
On 2014/06/17 17:43:25, Mark P wrote:
> I don't understand why this is necessary.  Didn't you set this in the
> constructor above?

Running an autocomplete pass changes the What You Typed match.  I want to pass
the same match to both passes to guarantee they both act the same way on it.

https://codereview.chromium.org/336173005/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_url_provider.cc:562:
!content::BrowserThread::IsMessageLoopValid(content::BrowserThread::UI));
On 2014/06/17 17:43:25, Mark P wrote:
> elsewhere we usually use
> IsThreadInitialized
> rather than
> IsMessageLoopValid

Done.

https://codereview.chromium.org/336173005/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_url_provider.cc:841: DCHECK(matches !=
NULL);
On 2014/06/17 17:43:25, Mark P wrote:
> Perhaps DCHECK(params!=NULL)

I could, but I don't think it adds much.  I intend to remove the |matches|
DCHECK in the next patch, so I didn't bother to add a new one for |params| here.

https://codereview.chromium.org/336173005/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/history_url_provider.h (right):

https://codereview.chromium.org/336173005/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_url_provider.h:229: // Given a |match|
containing the "what you typed" suggestion created by
On 2014/06/17 17:43:25, Mark P wrote:
> This comment needs to be slightly revised.

Done.

[Mark P, 2014-06-17 21:35:35]

lgtm
barring one comment below

--mark

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

https://codereview.chromium.org/336173005/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_url_provider.cc:470: return;
On 2014/06/17 20:39:47, Peter Kasting wrote:
> If FixupUserInput() fails, the WYT match wouldn't have been navigable anyway. 
> We're not losing anything.

Good point.

https://codereview.chromium.org/336173005/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/history_url_provider.cc:535:
params->what_you_typed_match = what_you_typed_match;
On 2014/06/17 20:39:48, Peter Kasting wrote:
> On 2014/06/17 17:43:25, Mark P wrote:
> > I don't understand why this is necessary.  Didn't you set this in the
> > constructor above?
> 
> Running an autocomplete pass changes the What You Typed match.  I want to pass
> the same match to both passes to guarantee they both act the same way on it.

This is not an obvious side effect of DoAutocomplete.  Please put this
explanation in the code.

[Peter Kasting, 2014-06-17 23:02:33]

Committed patchset #3 manually as r277886 (presubmit successful).