Add title to current page in zero suggest.

Add title to current page in zero suggest.

TBR=jochen
for mechanical changes to chrome/browser/extensions/api/omnibox/* and chrome/browser/android/omnibox/*

BUG=697624
Review-Url: https://codereview.chromium.org/2738003002
Cr-Commit-Position: refs/heads/master@{#467762}
Committed: https://chromium.googlesource.com/chromium/src/+/8cabc77f6204866cbcdc8d581e130f287acaeb06

[gcomanici, 2017-03-08 20:15:36]

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

[Mark P, 2017-03-09 04:51:59]

Have you checked this?  I don't think this approach will generally work.  The
in-memory history database only contains URLs with typed_counts > 0, i.e., URLs
that have been the result of an omnibox navigation (so called "typed" visits). 
This is probably a small subset of the URLs that people want to use zero suggest
from.

I think we need to plumb the real page title in from the web contents somehow. 
I'm not sure off the top of my head how to do that, though I know it's possible.
 (We already are getting the real page title out of the web contents to plumb
into the name of the tab on the tab strip.)

--mark

[Mark P, 2017-03-09 17:04:48]

On 2017/03/09 04:51:59, Mark P wrote:
> Have you checked this?  I don't think this approach will generally work.  The
> in-memory history database only contains URLs with typed_counts > 0, i.e.,
URLs
> that have been the result of an omnibox navigation (so called "typed" visits).

> This is probably a small subset of the URLs that people want to use zero
suggest
> from.
> 
> I think we need to plumb the real page title in from the web contents somehow.

> I'm not sure off the top of my head how to do that, though I know it's
possible.
>  (We already are getting the real page title out of the web contents to plumb
> into the name of the tab on the tab strip.)

I realized that last night I wasn't helpful :-(, so let me try to be more
constructive.
Apologies!  It's a busy week so I'm having trouble finding time to dive into
this more.

I'd check two places:

(1) All providers are given an AutocompleteProviderClient that provides some
access
to relevant information.
https://cs.chromium.org/chromium/src/components/omnibox/browser/autocomplete_...
Maybe one of the accessors there provides something that can help find the
current page title.

(2) ZeroSuggest provider currently gets the current URL from AutocompleteInput.
https://cs.chromium.org/chromium/src/components/omnibox/browser/autocomplete_...
Perhaps we can simply add a title field to that structure?  Hopefully it'll be
easily available in
the place that creates the AutocompleteInput.

If these pointers don't help, let me know and I'll try to look closer on Friday
or early next week.

cheers,
mark

[gcomanici, 2017-03-09 17:20:15]

On 2017/03/09 17:04:48, Mark P wrote:
> On 2017/03/09 04:51:59, Mark P wrote:
> > Have you checked this?  I don't think this approach will generally work. 
The
> > in-memory history database only contains URLs with typed_counts > 0, i.e.,
> URLs
> > that have been the result of an omnibox navigation (so called "typed"
visits).
> 
> > This is probably a small subset of the URLs that people want to use zero
> suggest
> > from.
> > 
> > I think we need to plumb the real page title in from the web contents
somehow.
> 
> > I'm not sure off the top of my head how to do that, though I know it's
> possible.
> >  (We already are getting the real page title out of the web contents to
plumb
> > into the name of the tab on the tab strip.)
> 
> I realized that last night I wasn't helpful :-(, so let me try to be more
> constructive.
> Apologies!  It's a busy week so I'm having trouble finding time to dive into
> this more.
> 
> I'd check two places:
> 
> (1) All providers are given an AutocompleteProviderClient that provides some
> access
> to relevant information.
>
https://cs.chromium.org/chromium/src/components/omnibox/browser/autocomplete_...
> Maybe one of the accessors there provides something that can help find the
> current page title.
> 
> (2) ZeroSuggest provider currently gets the current URL from
AutocompleteInput.
>
https://cs.chromium.org/chromium/src/components/omnibox/browser/autocomplete_...
> Perhaps we can simply add a title field to that structure?  Hopefully it'll be
> easily available in
> the place that creates the AutocompleteInput.
> 
> If these pointers don't help, let me know and I'll try to look closer on
Friday
> or early next week.
> 
> cheers,
> mark

Hi Mark, 

Thanks for your suggestions. Indeed I only tested it on typed URLs, so I was
under the wrong impression that it works. It doesn't. I was looking into your
second suggestion today. I believe it is the best choice as a channel through
which the title can be communicated to the autocomplete service. We use this for
the url of the current page.

best,
Gheorghe

[gcomanici, 2017-03-09 18:41:35]

Patchset #2 (id:20001) has been deleted

[gcomanici, 2017-03-09 18:46:18]

Hi Mark,

PTAL at the new patch, which retrieves the title from the omnibox client. I have
tested it and it works for non-typed URLs as well. 

It is not the prettiest change, as I use a setter for the title instead of
adding it to the list of parameters of the constructor. Still, I find it good
for the purpose, as the title is only used once. If the title becomes a really
important piece of info for autocomplete, then some refactoring is due to make
sure that the constructor for AutoCompleteInput does not take 10+ params.

Thanks,
Gheorghe

[Mark P, 2017-03-13 23:16:32]

On 2017/03/09 18:46:18, gcomanici wrote:
> Hi Mark,
> 
> PTAL at the new patch, which retrieves the title from the omnibox client. I
have
> tested it and it works for non-typed URLs as well. 
> 
> It is not the prettiest change, as I use a setter for the title instead of
> adding it to the list of parameters of the constructor. Still, I find it good
> for the purpose, as the title is only used once. If the title becomes a really
> important piece of info for autocomplete, then some refactoring is due to make
> sure that the constructor for AutoCompleteInput does not take 10+ params.

Yeah, that constructor is a mess.  We should change to default arguments, I
think.  But that's outside the scope of this changelist.

I actually think you should add a new parameter, or at least add a new parameter
then run "git cl try".  You'll hit a lot of compile errors and you'll have to
fix them.  But those compile errors will point out that this change doesn't
cause setting the title on zero suggest on Android (you didn't touch that file).
 Maybe there's other places that needs to be changed too...?

Also, please guard this change by a separate feature flag.  I can imagine that
we want to try swapping with and without a title.  After all, on desktop, if
there's a long title, it's possible the current URL may be truncated in the
dropdown (based on how the omnibox allocated space between URL and title), and
that could be negative to the user.  I'd like to be able to toggle whether this
can happen or not independent of whether we're swapping the title of the rest of
the entries.

--mark

[Mark P, 2017-03-15 23:33:03]

I am okay if you want to make this not work on Android, only desktop.  Is that
the direction you want to take?

Regardless, I'm still not sure about using a setter as opposed to putting things
in the constructor.  Do you expect this to be a short-term experiment (i.e., run
it for a week on dev, learn something, then rip it out) or something that might
remain in the code base for the longer term?  I.e., something you may want to
launch?  If the latter, I think adding to the constructor is more future-proof,
as that makes sure that people sending input to the AutocompleteController set
everything that's appropriate.

In any case, please add a new feature flag for this.

Let me know when it's ready for review again.

cheers,
mark

[gcomanici, 2017-04-03 19:18:48]

Hi Mark, 

I would like to get back to this. I believe it is reasonable to have the title
of the current page when this is given as a suggestion along others that have
both title and URL, especially when the title is shown to the left of
zero-suggest URL suggestions. Since contextual zero-suggestions always provide
titles, I don't see this as an experimental feature. I followed on your
suggestion to add it as a field to the constructor. Additionally, I added a new
feature specific to this change.

I would keep it for Desktop only and add it to Android in a different CL.

[Mark P, 2017-04-10 22:38:02]

Gheorghe,

Sorry for the delay.  This change generally looks pretty good.  (I reviewed
everything except the the test file changes.)

If you don't want to add this to Android right now, that's fine as I said
before.  But please file a bug so we don't forget.  And, regardless, you'll have
to make changes to
chrome/browser/android/omnibox/autocomplete_controller_android.cc
otherwise it won't compile.

And when I found the above link, I discovered you probably also have to touch
chrome/browser/android/vr_shell/vr_omnibox.cc
which is likely not compiled by default in your current build.  (I wonder if the
try bots would've caught this.)

git cl try / "CQ dry run" are your friends. :-)

cheers,
mark

https://codereview.chromium.org/2738003002/diff/80001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2738003002/diff/80001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2519:
{"include-title-to-autocomplete-match-for-current-url",
Please revise the command line flag in according with the revised feature flag
name.

https://codereview.chromium.org/2738003002/diff/80001/chrome/browser/flag_des...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2738003002/diff/80001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.cc:2914: "Include title to current URL
suggestion in omnibox";
nit: to -> for the
optional nit: omit "suggestion"; it seems odd to me to call this a suggestion.

https://codereview.chromium.org/2738003002/diff/80001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.cc:2918: "typing a URL or a search query, the
URL of the current page is provided "
Please consider a revised phrasing, like this:
In the event that omnibox provides suggestions on-focus, the URL of the current
page is provided as the first suggestion with a title.  Enabling this flag
causes the title to be displayed.

https://codereview.chromium.org/2738003002/diff/80001/chrome/browser/flag_des...
File chrome/browser/flag_descriptions.h (right):

https://codereview.chromium.org/2738003002/diff/80001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.h:3159: // Name for the about: flag for
including the title to auto complete match for
nit: "for the" -> "of the"
(for consistency with the rest of this file)

https://codereview.chromium.org/2738003002/diff/80001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.h:3161: extern const char
kIncludeTitleToACMatchForCurrentUrlName[];
Please revise this variable name and comment to be similar to the (revised)
feature name.
But also include "omnibox" in there somewhere for easy searching.
ditto below

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
File components/omnibox/browser/autocomplete_input.h (right):

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
components/omnibox/browser/autocomplete_input.h:75: const base::string16&
current_title,
Please revise the function comment.

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:553: client_->GetURL(),
base::string16(), ClassifyPage(), false, false, true,
There is no reason not to use the real title here.

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
File components/omnibox/browser/omnibox_field_trial.cc (right):

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:31: // Feature used for the
Zero Suggest Redirect to Chrome Field Trial.
Copy-paste error?  Please write a real comment.

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
File components/omnibox/browser/omnibox_field_trial.h (right):

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.h:27: extern const base::Feature
kIncludeTitleToACMatchForCurrentUrl;
nit: please add to the bottom.  (We've been working sequentially; no reason to
change now.)

When moving, be sure to move the instantiation in the .cc file too.

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.h:27: extern const base::Feature
kIncludeTitleToACMatchForCurrentUrl;
ACMatch seems unnecessary here.  Perhaps
kDisplayTitleForCurrentUrl
?

And remember to revise the feature name string in the .cc file too.

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
File components/omnibox/browser/zero_suggest_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
components/omnibox/browser/zero_suggest_provider.cc:486:
VerbatimMatchForURL(client(), tmp, GURL(current_query_),
Instead of this, please pass the title all the way into MatchForCurrentURL() and
from there into SuggestExactInput(). Don't revise it after the fact.

This will fix the other cases where other on-focus providers construct a URL
(such as physical web provider and clipboard provider).

[gcomanici, 2017-04-11 17:54:13]

The CQ bit was checked by gcomanici@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 17:54:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 18:06:46]

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

[commit-bot: I haz the power, 2017-04-11 18:06:47]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gcomanici, 2017-04-11 18:29:51]

The CQ bit was checked by gcomanici@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 18:30:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gcomanici, 2017-04-11 18:33:16]

The CQ bit was checked by gcomanici@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 18:34:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 18:56:19]

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

[commit-bot: I haz the power, 2017-04-11 18:56:20]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[gcomanici, 2017-04-11 19:26:17]

The CQ bit was checked by gcomanici@chromium.org to run a CQ dry run

[gcomanici, 2017-04-11 19:26:33]

Patchset #6 (id:120001) has been deleted

[gcomanici, 2017-04-11 19:26:47]

Patchset #6 (id:140001) has been deleted

[commit-bot: I haz the power, 2017-04-11 19:26:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 19:43:53]

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

[commit-bot: I haz the power, 2017-04-11 19:43:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gcomanici, 2017-04-11 20:15:17]

Patchset #6 (id:160001) has been deleted

[gcomanici, 2017-04-11 20:15:28]

The CQ bit was checked by gcomanici@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 20:16:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gcomanici, 2017-04-11 20:44:29]

Patchset #5 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-04-11 20:50:42]

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

[commit-bot: I haz the power, 2017-04-11 20:50:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gcomanici, 2017-04-11 20:51:41]

Hi Mark,

Can I instead keep Issue 697624 open until this is added to Android? Is it a
common practice, or is it better to specifically file a separate bug with the
same description but for a different platform?

I followed your advice and fixed the compilation errors that `git cl try`
figured out. 

PTAL for the other changes that you suggested.

Thanks!
Gheorghe

https://codereview.chromium.org/2738003002/diff/80001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2738003002/diff/80001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2519:
{"include-title-to-autocomplete-match-for-current-url",
On 2017/04/10 22:38:01, Mark P wrote:
> Please revise the command line flag in according with the revised feature flag
> name.

Done.

https://codereview.chromium.org/2738003002/diff/80001/chrome/browser/flag_des...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2738003002/diff/80001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.cc:2914: "Include title to current URL
suggestion in omnibox";
On 2017/04/10 22:38:01, Mark P wrote:
> nit: to -> for the
> optional nit: omit "suggestion"; it seems odd to me to call this a suggestion.

Done.

https://codereview.chromium.org/2738003002/diff/80001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.cc:2918: "typing a URL or a search query, the
URL of the current page is provided "
On 2017/04/10 22:38:02, Mark P wrote:
> Please consider a revised phrasing, like this:
> In the event that omnibox provides suggestions on-focus, the URL of the
current
> page is provided as the first suggestion with a title.  Enabling this flag
> causes the title to be displayed.

Done.

https://codereview.chromium.org/2738003002/diff/80001/chrome/browser/flag_des...
File chrome/browser/flag_descriptions.h (right):

https://codereview.chromium.org/2738003002/diff/80001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.h:3159: // Name for the about: flag for
including the title to auto complete match for
On 2017/04/10 22:38:02, Mark P wrote:
> nit: "for the" -> "of the"
> (for consistency with the rest of this file)

Done.

https://codereview.chromium.org/2738003002/diff/80001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.h:3161: extern const char
kIncludeTitleToACMatchForCurrentUrlName[];
On 2017/04/10 22:38:02, Mark P wrote:
> Please revise this variable name and comment to be similar to the (revised)
> feature name.
> But also include "omnibox" in there somewhere for easy searching.
> ditto below

Done.

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
File components/omnibox/browser/autocomplete_input.h (right):

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
components/omnibox/browser/autocomplete_input.h:75: const base::string16&
current_title,
On 2017/04/10 22:38:02, Mark P wrote:
> Please revise the function comment.

Done.

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:553: client_->GetURL(),
base::string16(), ClassifyPage(), false, false, true,
On 2017/04/10 22:38:02, Mark P wrote:
> There is no reason not to use the real title here.

Fixed.

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
File components/omnibox/browser/omnibox_field_trial.cc (right):

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:31: // Feature used for the
Zero Suggest Redirect to Chrome Field Trial.
On 2017/04/10 22:38:02, Mark P wrote:
> Copy-paste error?  Please write a real comment.

Done.

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
File components/omnibox/browser/omnibox_field_trial.h (right):

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.h:27: extern const base::Feature
kIncludeTitleToACMatchForCurrentUrl;
On 2017/04/10 22:38:02, Mark P wrote:
> ACMatch seems unnecessary here.  Perhaps
> kDisplayTitleForCurrentUrl
> ?
> 
> And remember to revise the feature name string in the .cc file too.

Done.

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.h:27: extern const base::Feature
kIncludeTitleToACMatchForCurrentUrl;
On 2017/04/10 22:38:02, Mark P wrote:
> ACMatch seems unnecessary here.  Perhaps
> kDisplayTitleForCurrentUrl
> ?
> 
> And remember to revise the feature name string in the .cc file too.

Done.

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
File components/omnibox/browser/zero_suggest_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/80001/components/omnibox/brow...
components/omnibox/browser/zero_suggest_provider.cc:486:
VerbatimMatchForURL(client(), tmp, GURL(current_query_),
On 2017/04/10 22:38:02, Mark P wrote:
> Instead of this, please pass the title all the way into MatchForCurrentURL()
and
> from there into SuggestExactInput(). Don't revise it after the fact.
> 
> This will fix the other cases where other on-focus providers construct a URL
> (such as physical web provider and clipboard provider).

Done.

[Mark P, 2017-04-12 05:07:57]

Getting there.  Most of the comments below are nits, except for one substantive
issue regarding SuggestExactMatch versus VerbatimMatchForUrl.

I thoroughly reviewed everything except the test files.

--mark

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/about_f...
chrome/browser/about_flags.cc:2598: {"omnibox-display-title-for-current-url",
nit: blank line above, following convention of this file

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/about_f...
chrome/browser/about_flags.cc:2605: // histograms.xml and don't forget to run
AboutFlagsHistogramTest unit test.
Friendly reminder to remember to do this before attempting to submit, else the
submit will fail.

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_omnibox.cc (right):

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_omnibox.cc:47: text, cursor_pos, desired_tld,
current_url, base::string16(),
Following the conventions of this outline-like file, please create a temporary
variable current_title and use that.

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/flag_de...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:2972: "Include title for current URL in
omnibox";
nit: for -> for the
nit: in -> in the

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:2975: "In the event that omnibox provides
suggestions on-focus, the URL of the "
nit: that -> that the

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:2976: "current page is provided as the first
suggestion with a title.  Enabling "
nit: with -> without
(yes, I know I'm editing the suggestion I gave you :p)  I had a typo/thinko in
the suggestion...  (See, we both make mistakes!)

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:2977: "this flag causes the title to be
displayed.";
nit: blank line between end of your variable definition and the end of the
namespace, per previous file convention.

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/flag_de...
File chrome/browser/flag_descriptions.h (right):

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.h:3221: // Name of the about: flag for
displaying the title to omnibox auto complete
I'm not sure what you meant here by this sentence (there might be a typo or
two).  Did you mean:
// Name of the about: flag for displaying the title of the omnibox match for
current URL.
?

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.h:3225: // Description of the about: flag g for
displaying the title to omnibox auto
Another typo "g" :-P  , plus a copy and paste error.
How about taking the corrected sentence above and swapping "Title" for
"Description"?

https://codereview.chromium.org/2738003002/diff/180001/components/omnibox/bro...
File components/omnibox/browser/autocomplete_input.cc (right):

https://codereview.chromium.org/2738003002/diff/180001/components/omnibox/bro...
components/omnibox/browser/autocomplete_input.cc:518: current_url_ = GURL();
Please clear the title here too.

https://codereview.chromium.org/2738003002/diff/180001/components/omnibox/bro...
File components/omnibox/browser/autocomplete_input.h (right):

https://codereview.chromium.org/2738003002/diff/180001/components/omnibox/bro...
components/omnibox/browser/autocomplete_input.h:41: // the omnibox.
Please add why this variable is needed, otherwise this is just stating the
obvious.  Perhaps something like:
Sometimes set as the description if returning a URL-what-you-typed match for the
current URL.

https://codereview.chromium.org/2738003002/diff/180001/components/omnibox/bro...
File components/omnibox/browser/history_url_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/180001/components/omnibox/bro...
components/omnibox/browser/history_url_provider.cc:620: if
(base::FeatureList::IsEnabled(omnibox::kDisplayTitleForCurrentUrl)) {
I don't this this is right.  SuggestExactInput can be called on things like
user input = "http://foo.com", even if the user is currently on
"http://bar.com".  We don't want to display bar.com's title on foo.com".

I think I was wrong when I suggested pushing this code all the way into
SuggestExactInput.  VerbatimMatchForURL() I guess is the right place in the
hierarchy.  (It must be, as we've now looked at two of the three possibilities!)
 Sorry for giving bad advice on my previous review.

https://codereview.chromium.org/2738003002/diff/180001/components/omnibox/bro...
File components/omnibox/browser/zero_suggest_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/180001/components/omnibox/bro...
components/omnibox/browser/zero_suggest_provider.cc:120: std::string url_string
= GetContextualSuggestionsUrl();
nit: Please restore the blank line you deleted here.  It helps separate two
conceptually separate blocks.

https://codereview.chromium.org/2738003002/diff/180001/components/omnibox/bro...
components/omnibox/browser/zero_suggest_provider.cc:194: current_query_.clear();
Please clear the title here as well.

[gcomanici, 2017-04-12 16:15:58]

The CQ bit was checked by gcomanici@chromium.org to run a CQ dry run

[gcomanici, 2017-04-12 16:16:31]

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/about_f...
chrome/browser/about_flags.cc:2598: {"omnibox-display-title-for-current-url",
On 2017/04/12 05:07:56, Mark P wrote:
> nit: blank line above, following convention of this file

Done.

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_omnibox.cc (right):

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_omnibox.cc:47: text, cursor_pos, desired_tld,
current_url, base::string16(),
On 2017/04/12 05:07:56, Mark P wrote:
> Following the conventions of this outline-like file, please create a temporary
> variable current_title and use that.

Done.

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/flag_de...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:2972: "Include title for current URL in
omnibox";
On 2017/04/12 05:07:56, Mark P wrote:
> nit: for -> for the
> nit: in -> in the

Done.

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:2975: "In the event that omnibox provides
suggestions on-focus, the URL of the "
On 2017/04/12 05:07:56, Mark P wrote:
> nit: that -> that the

Done.

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:2976: "current page is provided as the first
suggestion with a title.  Enabling "
On 2017/04/12 05:07:56, Mark P wrote:
> nit: with -> without
> (yes, I know I'm editing the suggestion I gave you :p)  I had a typo/thinko in
> the suggestion...  (See, we both make mistakes!)

Done.

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:2977: "this flag causes the title to be
displayed.";
On 2017/04/12 05:07:56, Mark P wrote:
> nit: blank line between end of your variable definition and the end of the
> namespace, per previous file convention.

Done.

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/flag_de...
File chrome/browser/flag_descriptions.h (right):

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.h:3221: // Name of the about: flag for
displaying the title to omnibox auto complete
On 2017/04/12 05:07:56, Mark P wrote:
> I'm not sure what you meant here by this sentence (there might be a typo or
> two).  Did you mean:
> // Name of the about: flag for displaying the title of the omnibox match for
> current URL.
> ?

Done.

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.h:3225: // Description of the about: flag g for
displaying the title to omnibox auto
On 2017/04/12 05:07:56, Mark P wrote:
> Another typo "g" :-P  , plus a copy and paste error.
> How about taking the corrected sentence above and swapping "Title" for
> "Description"?
> 

Done. Sorry about that. We should have some linting to figure out awkward
grammar and spelling errors in comments.

https://codereview.chromium.org/2738003002/diff/180001/components/omnibox/bro...
File components/omnibox/browser/autocomplete_input.cc (right):

https://codereview.chromium.org/2738003002/diff/180001/components/omnibox/bro...
components/omnibox/browser/autocomplete_input.cc:518: current_url_ = GURL();
On 2017/04/12 05:07:56, Mark P wrote:
> Please clear the title here too.

Done.

https://codereview.chromium.org/2738003002/diff/180001/components/omnibox/bro...
File components/omnibox/browser/autocomplete_input.h (right):

https://codereview.chromium.org/2738003002/diff/180001/components/omnibox/bro...
components/omnibox/browser/autocomplete_input.h:41: // the omnibox.
On 2017/04/12 05:07:56, Mark P wrote:
> Please add why this variable is needed, otherwise this is just stating the
> obvious.  Perhaps something like:
> Sometimes set as the description if returning a URL-what-you-typed match for
the
> current URL.

Done.

https://codereview.chromium.org/2738003002/diff/180001/components/omnibox/bro...
File components/omnibox/browser/history_url_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/180001/components/omnibox/bro...
components/omnibox/browser/history_url_provider.cc:620: if
(base::FeatureList::IsEnabled(omnibox::kDisplayTitleForCurrentUrl)) {
On 2017/04/12 05:07:56, Mark P wrote:
> I don't this this is right.  SuggestExactInput can be called on things like
> user input = "http://foo.com", even if the user is currently on
> "http://bar.com".  We don't want to display bar.com's title on foo.com".
> 
> I think I was wrong when I suggested pushing this code all the way into
> SuggestExactInput.  VerbatimMatchForURL() I guess is the right place in the
> hierarchy.  (It must be, as we've now looked at two of the three
possibilities!)
>  Sorry for giving bad advice on my previous review.

Done.

https://codereview.chromium.org/2738003002/diff/180001/components/omnibox/bro...
File components/omnibox/browser/zero_suggest_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/180001/components/omnibox/bro...
components/omnibox/browser/zero_suggest_provider.cc:120: std::string url_string
= GetContextualSuggestionsUrl();
On 2017/04/12 05:07:56, Mark P wrote:
> nit: Please restore the blank line you deleted here.  It helps separate two
> conceptually separate blocks.

Done.

https://codereview.chromium.org/2738003002/diff/180001/components/omnibox/bro...
components/omnibox/browser/zero_suggest_provider.cc:194: current_query_.clear();
On 2017/04/12 05:07:57, Mark P wrote:
> Please clear the title here as well.

Done.

[commit-bot: I haz the power, 2017-04-12 16:16:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 17:10:19]

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

[commit-bot: I haz the power, 2017-04-12 17:10:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mark P, 2017-04-12 17:58:46]

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/flag_de...
File chrome/browser/flag_descriptions.h (right):

https://codereview.chromium.org/2738003002/diff/180001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.h:3225: // Description of the about: flag g for
displaying the title to omnibox auto
On 2017/04/12 16:16:30, gcomanici wrote:
> On 2017/04/12 05:07:56, Mark P wrote:
> > Another typo "g" :-P  , plus a copy and paste error.
> > How about taking the corrected sentence above and swapping "Title" for
> > "Description"?
> > 
> 
> Done. Sorry about that. We should have some linting to figure out awkward
> grammar and spelling errors in comments.

What a fun idea.  "lint" runs a grammar checker. :-)

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
File components/omnibox/browser/history_url_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
components/omnibox/browser/history_url_provider.cc:619: return match;
nit: might as well keep the blank line here
(partially because then history will correctly show that you didn't touch this
file)

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
File components/omnibox/browser/verbatim_match.cc (right):

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
components/omnibox/browser/verbatim_match.cc:43: if
(base::FeatureList::IsEnabled(omnibox::kDisplayTitleForCurrentUrl)) {
nit: include-what-you-use (omnibox_field_trial.h defines this const)

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
components/omnibox/browser/verbatim_match.cc:44: match.description =
input.current_title();
|destination_url| does not necessarily correspond to input.current_url or
input.current_title.  Please add a new variable into VerbatimMatchForURL().

[Mark P, 2017-04-12 18:06:38]

Oh, and I keep forgetting to mention, yes keeping bug 697624 open until there's
an Android implementation is fine.  After this change is submitted, just
remember to clarify the bug so it's clear what remaining work needs to be done.

--mark

[gcomanici, 2017-04-12 19:24:48]

The CQ bit was checked by gcomanici@chromium.org to run a CQ dry run

[gcomanici, 2017-04-12 19:25:00]

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
File components/omnibox/browser/history_url_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
components/omnibox/browser/history_url_provider.cc:619: return match;
On 2017/04/12 17:58:46, Mark P wrote:
> nit: might as well keep the blank line here
> (partially because then history will correctly show that you didn't touch this
> file)

Done.

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
File components/omnibox/browser/verbatim_match.cc (right):

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
components/omnibox/browser/verbatim_match.cc:43: if
(base::FeatureList::IsEnabled(omnibox::kDisplayTitleForCurrentUrl)) {
On 2017/04/12 17:58:46, Mark P wrote:
> nit: include-what-you-use (omnibox_field_trial.h defines this const)

I couldn't find it there. Could you be more specific?

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
components/omnibox/browser/verbatim_match.cc:44: match.description =
input.current_title();
On 2017/04/12 17:58:46, Mark P wrote:
> |destination_url| does not necessarily correspond to input.current_url or
> input.current_title.  Please add a new variable into VerbatimMatchForURL().

I made a change based on your suggestion but without adding a new variable. What
do you mean by `a new variable`?

[commit-bot: I haz the power, 2017-04-12 19:26:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 19:37:03]

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

[commit-bot: I haz the power, 2017-04-12 19:37:05]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mark P, 2017-04-12 19:38:01]

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
File components/omnibox/browser/verbatim_match.cc (right):

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
components/omnibox/browser/verbatim_match.cc:43: if
(base::FeatureList::IsEnabled(omnibox::kDisplayTitleForCurrentUrl)) {
On 2017/04/12 19:25:00, gcomanici wrote:
> On 2017/04/12 17:58:46, Mark P wrote:
> > nit: include-what-you-use (omnibox_field_trial.h defines this const)
> 
> I couldn't find it there. Could you be more specific? 

Please include components/omnibox/browser/omnibox_field_trial.h here.

kDisplayTitleForCurrentUrl is a constant that's defined there.  The only reason
this code currently compiles is because you're getting it via a transitive
include somewhere.

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
components/omnibox/browser/verbatim_match.cc:44: match.description =
input.current_title();
On 2017/04/12 19:25:00, gcomanici wrote:
> On 2017/04/12 17:58:46, Mark P wrote:
> > |destination_url| does not necessarily correspond to input.current_url or
> > input.current_title.  Please add a new variable into VerbatimMatchForURL().
> 
> I made a change based on your suggestion but without adding a new variable.
What
> do you mean by `a new variable`?

No, we want the description to be the title. That's the whole point of the
changelist. :-P

But VerbatimMatchForURL() may be called for example like
VerbatimMatchForURL(... input{current_url="google.com", current_title="Google"},
destination_url="irs.gov")
and the code as of the patchset I'm commenting on will create a irs.gov match
with the title Google.  That's not right.

[gcomanici, 2017-04-12 20:47:48]

The CQ bit was checked by gcomanici@chromium.org to run a CQ dry run

[gcomanici, 2017-04-12 20:48:23]

Thanks Mark! PTAL.

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
File components/omnibox/browser/verbatim_match.cc (right):

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
components/omnibox/browser/verbatim_match.cc:43: if
(base::FeatureList::IsEnabled(omnibox::kDisplayTitleForCurrentUrl)) {
On 2017/04/12 19:38:01, Mark P wrote:
> On 2017/04/12 19:25:00, gcomanici wrote:
> > On 2017/04/12 17:58:46, Mark P wrote:
> > > nit: include-what-you-use (omnibox_field_trial.h defines this const)
> > 
> > I couldn't find it there. Could you be more specific? 
> 
> Please include components/omnibox/browser/omnibox_field_trial.h here.
> 
> kDisplayTitleForCurrentUrl is a constant that's defined there.  The only
reason
> this code currently compiles is because you're getting it via a transitive
> include somewhere.

Done.

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
components/omnibox/browser/verbatim_match.cc:44: match.description =
input.current_title();
On 2017/04/12 19:38:01, Mark P wrote:
> On 2017/04/12 19:25:00, gcomanici wrote:
> > On 2017/04/12 17:58:46, Mark P wrote:
> > > |destination_url| does not necessarily correspond to input.current_url or
> > > input.current_title.  Please add a new variable into
VerbatimMatchForURL().
> > 
> > I made a change based on your suggestion but without adding a new variable.
> What
> > do you mean by `a new variable`?
> 
> No, we want the description to be the title. That's the whole point of the
> changelist. :-P
> 
> But VerbatimMatchForURL() may be called for example like
> VerbatimMatchForURL(... input{current_url="google.com",
current_title="Google"},
> destination_url="irs.gov")
> and the code as of the patchset I'm commenting on will create a http://irs.gov
match
> with the title Google.  That's not right.

Got it. Added a check on destination_url.

[commit-bot: I haz the power, 2017-04-12 20:49:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mark P, 2017-04-12 21:16:57]

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
File components/omnibox/browser/verbatim_match.cc (right):

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
components/omnibox/browser/verbatim_match.cc:44: match.description =
input.current_title();
On 2017/04/12 20:48:22, gcomanici wrote:
> On 2017/04/12 19:38:01, Mark P wrote:
> > On 2017/04/12 19:25:00, gcomanici wrote:
> > > On 2017/04/12 17:58:46, Mark P wrote:
> > > > |destination_url| does not necessarily correspond to input.current_url
or
> > > > input.current_title.  Please add a new variable into
> VerbatimMatchForURL().
> > > 
> > > I made a change based on your suggestion but without adding a new
variable.
> > What
> > > do you mean by `a new variable`?
> > 
> > No, we want the description to be the title. That's the whole point of the
> > changelist. :-P
> > 
> > But VerbatimMatchForURL() may be called for example like
> > VerbatimMatchForURL(... input{current_url="google.com",
> current_title="Google"},
> > destination_url="irs.gov")
> > and the code as of the patchset I'm commenting on will create a
http://irs.gov
> match
> > with the title Google.  That's not right.
> 
> Got it. Added a check on destination_url.

I was more thinking that you have callers pass in a |title|, just like they do a
|destination_url|.  I think that makes more sense than having this weird side
effect of setting the title only if the destination URL happens to be the same
as the current page.  (VerbatimMatchForURL() isn't called in that many places,
so it's relatively easy to make this change than has better semantics.)

[commit-bot: I haz the power, 2017-04-12 21:55:19]

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

[commit-bot: I haz the power, 2017-04-12 21:55:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gcomanici, 2017-04-13 16:37:37]

The CQ bit was checked by gcomanici@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 16:38:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gcomanici, 2017-04-13 16:38:38]

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
File components/omnibox/browser/verbatim_match.cc (right):

https://codereview.chromium.org/2738003002/diff/200001/components/omnibox/bro...
components/omnibox/browser/verbatim_match.cc:44: match.description =
input.current_title();
Makes sense. Done.

[commit-bot: I haz the power, 2017-04-13 17:39:29]

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

[commit-bot: I haz the power, 2017-04-13 17:39:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mark P, 2017-04-13 21:12:29]

lgtm modulo comments
(a few minor bugs, some user-visible; feel free to submit after you fix them
without hearing back from me.  We can deal with follow-up in later changelists.)

By the way, if we decide to launch this behavior, we'd want to update some of
on-focus provider unit tests to actually test this code.  I'm okay waiting for
later to do that.  (Can you make a note on the bug about this request?)

cheers,
mark

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
File components/omnibox/browser/clipboard_url_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
components/omnibox/browser/clipboard_url_provider.cc:51:
VerbatimMatchForURL(client_, input, input.current_url(),
Ditto comment about using the real title (again under field trial control).

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
File components/omnibox/browser/physical_web_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider.cc:106:
VerbatimMatchForURL(client_, input, input.current_url(),
Please use the title of the current page here too (under field trial control),
otherwise the results may be inconsistent.  (If a physical web URL happens to
show up on focus, then the current page URL doesn't have a title, otherwise it
does.  That seems odd.)

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
File components/omnibox/browser/verbatim_match.cc (right):

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
components/omnibox/browser/verbatim_match.cc:12: #include
"components/omnibox/browser/omnibox_field_trial.h"
nit: unnecessary

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
components/omnibox/browser/verbatim_match.cc:44: match.description =
destination_description;
This block should go in the if statement, just after line 33.  It's not
appropriate to use the passed in description for the destionation_url if the
destionation_url wasn't valid (and thus we're not using that destination_url).

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
File components/omnibox/browser/verbatim_match.h (right):

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
components/omnibox/browser/verbatim_match.h:20: // implementation for details)
than the default code path.
Update this comment, perhaps replace "this may be more efficient" with the "use
|destination_description| as the description.  Providing a
|history_url_provider| also may be more efficient" ...

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
File components/omnibox/browser/zero_suggest_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
components/omnibox/browser/zero_suggest_provider.cc:489: ? tmp.current_title()
Is there a difference between tmp.current_title() here and simply
current_title_?  If not, I suggest you use the latter.

[gcomanici, 2017-04-18 18:52:51]

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

[gcomanici, 2017-04-18 18:52:53]

Thanks, Mark!

Peter, can you review chrome/browser/ui/* ? Thanks!

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
File components/omnibox/browser/clipboard_url_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
components/omnibox/browser/clipboard_url_provider.cc:51:
VerbatimMatchForURL(client_, input, input.current_url(),
On 2017/04/13 21:12:28, Mark P (away Mon Apr 17) wrote:
> Ditto comment about using the real title (again under field trial control).

Done.

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
File components/omnibox/browser/physical_web_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider.cc:106:
VerbatimMatchForURL(client_, input, input.current_url(),
On 2017/04/13 21:12:28, Mark P (away Mon Apr 17) wrote:
> Please use the title of the current page here too (under field trial control),
> otherwise the results may be inconsistent.  (If a physical web URL happens to
> show up on focus, then the current page URL doesn't have a title, otherwise it
> does.  That seems odd.)

Done.

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
File components/omnibox/browser/verbatim_match.cc (right):

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
components/omnibox/browser/verbatim_match.cc:12: #include
"components/omnibox/browser/omnibox_field_trial.h"
On 2017/04/13 21:12:28, Mark P (away Mon Apr 17) wrote:
> nit: unnecessary

Removed.

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
components/omnibox/browser/verbatim_match.cc:44: match.description =
destination_description;
On 2017/04/13 21:12:28, Mark P (away Mon Apr 17) wrote:
> This block should go in the if statement, just after line 33.  It's not
> appropriate to use the passed in description for the destionation_url if the
> destionation_url wasn't valid (and thus we're not using that destination_url).

Done.

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
File components/omnibox/browser/verbatim_match.h (right):

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
components/omnibox/browser/verbatim_match.h:20: // implementation for details)
than the default code path.
On 2017/04/13 21:12:28, Mark P (away Mon Apr 17) wrote:
> Update this comment, perhaps replace "this may be more efficient" with the
"use
> |destination_description| as the description.  Providing a
> |history_url_provider| also may be more efficient" ...

Done.

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
File components/omnibox/browser/zero_suggest_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/260001/components/omnibox/bro...
components/omnibox/browser/zero_suggest_provider.cc:489: ? tmp.current_title()
On 2017/04/13 21:12:29, Mark P (away Mon Apr 17) wrote:
> Is there a difference between tmp.current_title() here and simply
> current_title_?  If not, I suggest you use the latter.

No difference. I followed your suggestion.

[Peter Kasting, 2017-04-18 20:35:33]

c/b/ui/* LGTM; it's OK to TBR OWNERS for this sort of mechanical change.

[Mark P, 2017-04-18 21:05:10]

two trivial nits, otherwise still lgtm

--mark

https://codereview.chromium.org/2738003002/diff/280001/components/omnibox/bro...
File components/omnibox/browser/clipboard_url_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/280001/components/omnibox/bro...
components/omnibox/browser/clipboard_url_provider.cc:17: #include
"ui/base/l10n/l10n_util.h"
Please include omnibox_field_trial.

https://codereview.chromium.org/2738003002/diff/280001/components/omnibox/bro...
File components/omnibox/browser/physical_web_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/280001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider.cc:28: 
Please include omnibox_field_trial.

[gcomanici, 2017-04-19 02:53:06]

gcomanici@chromium.org changed reviewers:
+ jochen@chromium.org

[gcomanici, 2017-04-19 02:53:08]

Thank you, Peter! I don't really know what TBR OWNERS means and how it is done.

+jochen for chrome/browser/extensions/api/omnibox/* and
	chrome/browser/android/omnibox/*

https://codereview.chromium.org/2738003002/diff/280001/components/omnibox/bro...
File components/omnibox/browser/clipboard_url_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/280001/components/omnibox/bro...
components/omnibox/browser/clipboard_url_provider.cc:17: #include
"ui/base/l10n/l10n_util.h"
On 2017/04/18 21:05:10, Mark P (away Mon Apr 17) wrote:
> Please include omnibox_field_trial.

Done.

https://codereview.chromium.org/2738003002/diff/280001/components/omnibox/bro...
File components/omnibox/browser/physical_web_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/280001/components/omnibox/bro...
components/omnibox/browser/physical_web_provider.cc:28: 
On 2017/04/18 21:05:10, Mark P (away Mon Apr 17) wrote:
> Please include omnibox_field_trial.

There already.

[Mark P, 2017-04-19 03:50:06]

Description was changed from

==========
Add title to current page in zero suggest.

BUG=697624
==========

to

==========
Add title to current page in zero suggest.

TBR=jochen
for mechanical changes to chrome/browser/extensions/api/omnibox/* and
chrome/browser/android/omnibox/*

BUG=697624
==========

[Mark P, 2017-04-19 03:50:22]

The CQ bit was checked by mpearson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 03:50:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mark P, 2017-04-19 04:02:01]

On 2017/04/19 02:53:08, gcomanici wrote:
> Thank you, Peter! I don't really know what TBR OWNERS means and how it is
done.

https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md#...

I set the changelist up for it for you in this case.  You should be able to
simply check the CQ box if the "dry run" (test run) comes up clean.

--mark


> +jochen for chrome/browser/extensions/api/omnibox/* and
> 	chrome/browser/android/omnibox/*

[commit-bot: I haz the power, 2017-04-19 04:47:08]

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

[commit-bot: I haz the power, 2017-04-19 04:47:09]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jochen (gone - plz use gerrit), 2017-04-19 10:18:10]

for both directories, there are owners closer to the directory - also for
TBR'ing, please pick owners as close as possible to the files in the future

anyways, mechanical changes lgtm

[gcomanici, 2017-04-19 13:36:45]

The CQ bit was checked by gcomanici@chromium.org

[gcomanici, 2017-04-19 13:36:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2738003002/#ps300001
(title: "Fix small nit.")

[commit-bot: I haz the power, 2017-04-19 13:37:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 14:33:13]

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

[commit-bot: I haz the power, 2017-04-19 14:33:15]

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

[gcomanici, 2017-04-26 15:52:58]

Patchset #12 (id:320001) has been deleted

[gcomanici, 2017-04-26 20:27:46]

The CQ bit was checked by gcomanici@chromium.org to run a CQ dry run

[gcomanici, 2017-04-26 20:28:02]

The CQ bit was unchecked by gcomanici@chromium.org

[gcomanici, 2017-04-26 20:28:18]

Patchset #12 (id:340001) has been deleted

[gcomanici, 2017-04-26 20:38:21]

The CQ bit was checked by gcomanici@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 20:39:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 21:00:23]

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

[commit-bot: I haz the power, 2017-04-26 21:00:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gcomanici, 2017-04-26 21:32:19]

The CQ bit was checked by gcomanici@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 21:33:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gcomanici, 2017-04-26 21:34:37]

On 2017/04/19 14:33:15, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Hi Peter, 

Can you please take a look at c/b/u/o/chrome_omnibox_client.cc? There is a line
of code that needs to be checked for nullptr. It was causing a UI test on Mac to
crash.

Thanks!

[commit-bot: I haz the power, 2017-04-26 23:43:29]

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

[commit-bot: I haz the power, 2017-04-26 23:43:30]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-04-27 00:33:57]

LGTM with change

https://codereview.chromium.org/2738003002/diff/400001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right):

https://codereview.chromium.org/2738003002/diff/400001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/chrome_omnibox_client.cc:154: static const auto* const
kEmptyTitle = new base::string16();
Don't do this; return EmptyString16() below instead.

[gcomanici, 2017-04-27 02:08:02]

Patchset #13 (id:380001) has been deleted

[gcomanici, 2017-04-27 02:09:28]

The CQ bit was checked by gcomanici@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 02:10:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gcomanici, 2017-04-27 02:11:23]

https://codereview.chromium.org/2738003002/diff/400001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right):

https://codereview.chromium.org/2738003002/diff/400001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/chrome_omnibox_client.cc:154: static const auto* const
kEmptyTitle = new base::string16();
On 2017/04/27 00:33:57, Peter Kasting wrote:
> Don't do this; return EmptyString16() below instead.

Done.

[gcomanici, 2017-04-27 02:40:50]

The CQ bit was unchecked by gcomanici@chromium.org

[gcomanici, 2017-04-27 02:50:50]

Patchset #15 (id:440001) has been deleted

[gcomanici, 2017-04-27 02:51:36]

The CQ bit was checked by gcomanici@chromium.org to run a CQ dry run

[gcomanici, 2017-04-27 02:51:46]

The CQ bit was unchecked by gcomanici@chromium.org

[commit-bot: I haz the power, 2017-04-27 02:52:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mark P, 2017-04-27 05:44:09]

not lgtm, as I noticed some possible errors from debugging / rebasing that
should be resolved.

--mark

https://codereview.chromium.org/2738003002/diff/460001/chrome/browser/flag_de...
File chrome/browser/flag_descriptions.h (right):

https://codereview.chromium.org/2738003002/diff/460001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.h:3325: extern const char
kIncludeTitleToACMatchForCurrentUrlName[];
Is this a rebase error?  Should this block be removed?

https://codereview.chromium.org/2738003002/diff/460001/components/omnibox/bro...
File components/omnibox/browser/omnibox_field_trial.cc (right):

https://codereview.chromium.org/2738003002/diff/460001/components/omnibox/bro...
components/omnibox/browser/omnibox_field_trial.cc:199: return
base::TimeDelta::FromMilliseconds(5500);
Did you intend to change this value?  Please talk to me first if so.

https://codereview.chromium.org/2738003002/diff/460001/components/omnibox/bro...
File components/omnibox/browser/zero_suggest_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/460001/components/omnibox/bro...
components/omnibox/browser/zero_suggest_provider.cc:179: LOG(ERROR) <<
suggest_url.possibly_invalid_spec();
Did you intend to keep this here?  Please reconsider.

[gcomanici, 2017-04-27 15:38:41]

The CQ bit was checked by gcomanici@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 15:39:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 16:58:59]

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

[commit-bot: I haz the power, 2017-04-27 16:59:01]

Dry run: This issue passed the CQ dry run.

[gcomanici, 2017-04-27 17:27:58]

Patchset #12 (id:360001) has been deleted

[gcomanici, 2017-04-27 17:29:30]

Hi Mark,

Thanks for point out the errors related to rebasing/debugging. I did a pass over
the entire CL to address all of them. PTAL.

https://codereview.chromium.org/2738003002/diff/460001/chrome/browser/flag_de...
File chrome/browser/flag_descriptions.h (right):

https://codereview.chromium.org/2738003002/diff/460001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.h:3325: extern const char
kIncludeTitleToACMatchForCurrentUrlName[];
On 2017/04/27 05:44:08, Mark P wrote:
> Is this a rebase error?  Should this block be removed?

Removed.

https://codereview.chromium.org/2738003002/diff/460001/components/omnibox/bro...
File components/omnibox/browser/omnibox_field_trial.cc (right):

https://codereview.chromium.org/2738003002/diff/460001/components/omnibox/bro...
components/omnibox/browser/omnibox_field_trial.cc:199: return
base::TimeDelta::FromMilliseconds(5500);
On 2017/04/27 05:44:08, Mark P wrote:
> Did you intend to change this value?  Please talk to me first if so.

Fixed back. I changed it for debugging. Forgot to remove.

https://codereview.chromium.org/2738003002/diff/460001/components/omnibox/bro...
File components/omnibox/browser/zero_suggest_provider.cc (right):

https://codereview.chromium.org/2738003002/diff/460001/components/omnibox/bro...
components/omnibox/browser/zero_suggest_provider.cc:179: LOG(ERROR) <<
suggest_url.possibly_invalid_spec();
On 2017/04/27 05:44:08, Mark P wrote:
> Did you intend to keep this here?  Please reconsider.

Removed.

[Mark P, 2017-04-27 19:21:29]

lgtm again :-)

--mark

[gcomanici, 2017-04-27 19:45:57]

The CQ bit was checked by gcomanici@chromium.org

[gcomanici, 2017-04-27 19:45:57]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2738003002/#ps480001
(title: "Remove debugging info.")

[commit-bot: I haz the power, 2017-04-27 19:46:38]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 20:04:58]

CQ is committing da patch.

Bot data: {"patchset_id": 480001, "attempt_start_ts": 1493322357100810,
"parent_rev": "1ce49f64ab5be7af3fa0a06d9bd1f826a13a0962", "commit_rev":
"8cabc77f6204866cbcdc8d581e130f287acaeb06"}

[commit-bot: I haz the power, 2017-04-27 20:05:17]

Description was changed from

==========
Add title to current page in zero suggest.

TBR=jochen
for mechanical changes to chrome/browser/extensions/api/omnibox/* and
chrome/browser/android/omnibox/*

BUG=697624
==========

to

==========
Add title to current page in zero suggest.

TBR=jochen
for mechanical changes to chrome/browser/extensions/api/omnibox/* and
chrome/browser/android/omnibox/*

BUG=697624

Review-Url: https://codereview.chromium.org/2738003002
Cr-Commit-Position: refs/heads/master@{#467762}
Committed:
https://chromium.googlesource.com/chromium/src/+/8cabc77f6204866cbcdc8d581e13...
==========

[commit-bot: I haz the power, 2017-04-27 20:05:19]

Committed patchset #15 (id:480001) as
https://chromium.googlesource.com/chromium/src/+/8cabc77f6204866cbcdc8d581e13...