Instant Extended: Add prominent search term support

Instant Extended: Add prominent search term support

Currently if there's a mixed content warning or if the search term looks like a URL we disable search term extraction.

With this CL adds a new 'URL_LIKE_SEARCH_TERMS' type that the UI can optionally allow.

BUG=233494
TEST=Verified that tests passed. No user visible change in this CL.

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

[sreeram, 2013-04-23 22:56:45]

https://codereview.chromium.org/14259008/diff/4001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/14259008/diff/4001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:397: }
Nit: no curlies. I believe the style is to put curlies only when the body of the
statement spans more than one line, as opposed to the conditional spanning
multiple lines.

https://codereview.chromium.org/14259008/diff/4001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:428:
ToolbarModel::SEARCH_TERM_NONE) {
Nit: No curlies. Maybe also indent line 428 four more spaces?

https://codereview.chromium.org/14259008/diff/4001/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/14259008/diff/4001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:147: if
(AreProminentSearchTerms(search_terms))
This now runs the classifier for the second time (GetSearchTerms() already ran
it once). Can we avoid this somehow? Perhaps by changing GetSearchTerms to this:
    ToolbarModel::SearchTermType GetSearchTerms(string16* search_terms);

https://codereview.chromium.org/14259008/diff/4001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:200: security_level !=
SECURITY_WARNING)) {
Do we need the security_level check? AreProminentSearchTerms() already does the
check, so what does it buy us to do it here again?

https://codereview.chromium.org/14259008/diff/4001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:279: return true;
Shouldn't this be "security_level != EV_SECURE && ... != SECURE"? I'm wondering
whether security_level can be NONE or SECURITY_ERROR and you'd still return
false.

[sail, 2013-04-24 00:22:23]

https://codereview.chromium.org/14259008/diff/4001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/14259008/diff/4001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:397: }
On 2013/04/23 22:56:45, sreeram wrote:
> Nit: no curlies. I believe the style is to put curlies only when the body of
the
> statement spans more than one line, as opposed to the conditional spanning
> multiple lines.

Done.

https://codereview.chromium.org/14259008/diff/4001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:428:
ToolbarModel::SEARCH_TERM_NONE) {
On 2013/04/23 22:56:45, sreeram wrote:
> Nit: No curlies. Maybe also indent line 428 four more spaces?

Done.
(Kept the indentation so that I don't have to fight clang-format :-P)

https://codereview.chromium.org/14259008/diff/4001/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/14259008/diff/4001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:147: if
(AreProminentSearchTerms(search_terms))
On 2013/04/23 22:56:45, sreeram wrote:
> This now runs the classifier for the second time (GetSearchTerms() already ran
> it once). Can we avoid this somehow? Perhaps by changing GetSearchTerms to
this:
>     ToolbarModel::SearchTermType GetSearchTerms(string16* search_terms);

Changing the API won't reduce the number of times the classifier is run.
Normally callers that care about search terms don't care about search type and
vice versa.

It seems like being able to cache search terms in general would be useful. On my
machine it takes about 100 microseconds and is called hundreds of times when the
window is resized.

https://codereview.chromium.org/14259008/diff/4001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:200: security_level !=
SECURITY_WARNING)) {
On 2013/04/23 22:56:45, sreeram wrote:
> Do we need the security_level check? AreProminentSearchTerms() already does
the
> check, so what does it buy us to do it here again?

This check is needed when the search term is URL shaped.

https://codereview.chromium.org/14259008/diff/4001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:279: return true;
On 2013/04/23 22:56:45, sreeram wrote:
> Shouldn't this be "security_level != EV_SECURE && ... != SECURE"? I'm
wondering
> whether security_level can be NONE or SECURITY_ERROR and you'd still return
> false.

Done.

[sail, 2013-04-25 02:08:16]

Updated, please take another look.

https://codereview.chromium.org/14259008/diff/4001/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/14259008/diff/4001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:147: if
(AreProminentSearchTerms(search_terms))
On 2013/04/23 22:56:45, sreeram wrote:
> This now runs the classifier for the second time (GetSearchTerms() already ran
> it once). Can we avoid this somehow? Perhaps by changing GetSearchTerms to
this:
>     ToolbarModel::SearchTermType GetSearchTerms(string16* search_terms);

Done.
Gah, I obviously didn't understand this at all. Fixed, exactly as you
recommended.

[sail, 2013-04-25 02:09:57]

(Also, I still haven't been able to test mixed content warnings. It might be
that chrome::GetSearchTerms() return an empty string in that case. I'm still
investigating.)

[sreeram, 2013-04-25 16:37:10]

lgtm

https://codereview.chromium.org/14259008/diff/16001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/14259008/diff/16001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:282: search_terms = string16();
search_terms.clear();

https://codereview.chromium.org/14259008/diff/16001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model_impl.h (right):

https://codereview.chromium.org/14259008/diff/16001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_impl.h:67: // prominent search term UI
is not supported. Also returns the search term
"... not supported, in which case this returns an empty string."

[sail, 2013-04-25 16:41:22]

https://codereview.chromium.org/14259008/diff/16001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/14259008/diff/16001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:282: search_terms = string16();
On 2013/04/25 16:37:10, sreeram wrote:
> search_terms.clear();

Done.

https://codereview.chromium.org/14259008/diff/16001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model_impl.h (right):

https://codereview.chromium.org/14259008/diff/16001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_impl.h:67: // prominent search term UI
is not supported. Also returns the search term
On 2013/04/25 16:37:10, sreeram wrote:
> "... not supported, in which case this returns an empty string."

Done.

[commit-bot: I haz the power, 2013-04-25 16:41:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/14259008/23001

[commit-bot: I haz the power, 2013-04-25 16:41:59]

Presubmit check for 14259008-23001 failed and returned exit status 1.

INFO:root:Found 14 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/views/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc
    chrome/browser/ui/omnibox/omnibox_edit_model.cc
    chrome/browser/ui/omnibox/omnibox_edit_unittest.cc
    chrome/browser/ui/toolbar/test_toolbar_model.cc
    chrome/browser/ui/toolbar/test_toolbar_model.h
    chrome/browser/ui/toolbar/toolbar_model.h
    chrome/browser/ui/toolbar/toolbar_model_impl.cc
    chrome/browser/ui/toolbar/toolbar_model_impl.h
    chrome/browser/ui/toolbar/toolbar_model_unittest.cc
    chrome/browser/ui/views/location_bar/location_bar_view.cc
    chrome/browser/ui/views/omnibox/omnibox_view_views.cc

Presubmit checks took 1.5s to calculate.

[sail, 2013-04-25 16:43:39]

+pkasting for ui/omnibox OWNERS review

[Peter Kasting, 2013-04-25 19:42:54]

https://chromiumcodereview.appspot.com/14259008/diff/23001/chrome/browser/ui/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://chromiumcodereview.appspot.com/14259008/diff/23001/chrome/browser/ui/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:426: if (sel_min != 0 ||
view_->toolbar_model()->GetSearchTermType() !=
Nit: Odd indenting; how about:

  if ((sel_min != 0) ||
      (view_->toolbar_model()->GetSearchTermType() !=
          ToolbarModel::SEARCH_TERM_NONE))

https://chromiumcodereview.appspot.com/14259008/diff/23001/chrome/browser/ui/...
File chrome/browser/ui/omnibox/omnibox_edit_unittest.cc (right):

https://chromiumcodereview.appspot.com/14259008/diff/23001/chrome/browser/ui/...
chrome/browser/ui/omnibox/omnibox_edit_unittest.cc:191: if
(input[i].extracted_search_terms)
Nit: Simpler:

    toolbar_model()->set_search_term_type(input[i].extracted_search_terms ?
        ToolbarModel::SEARCH_TERM_NORMAL : ToolbarModel::SEARCH_TERM_NONE);

https://chromiumcodereview.appspot.com/14259008/diff/23001/chrome/browser/ui/...
File chrome/browser/ui/toolbar/toolbar_model.h (right):

https://chromiumcodereview.appspot.com/14259008/diff/23001/chrome/browser/ui/...
chrome/browser/ui/toolbar/toolbar_model.h:32: enum SearchTermType {
Nit: Add comment above enum:

If search URLs may be displayed as search terms, these are the types of search
terms that may be extracted from the URL.  See comments on GetText() and
GetSearchTermsType().

https://chromiumcodereview.appspot.com/14259008/diff/23001/chrome/browser/ui/...
chrome/browser/ui/toolbar/toolbar_model.h:40: SEARCH_TERM_PROMINENT,
I think these would make more sense named as:

NO_SEARCH_TERMS,
NORMAL_SEARCH_TERMS,
URL_LIKE_SEARCH_TERMS

This removes the idea of "what is the UI to show these" from the ToolbarModel,
which really doesn't care about such a concept, and instead just phrases things
in terms of different kinds of search terms.  It also avoids using "PROMINENT"
alone which is a bit confusing (as compared to something like
PROMINENT_UI_NEEDED).

I realize that URL_LIKE_... may be a misnomer for normal-looking search terms
with a mixed-content warning present; since that's intended to be an
error-handling case (Google should normally never have mixed-content problems),
I think it's OK to just add a comment about why we're doing this to the
appropriate spot in the implementation.

https://chromiumcodereview.appspot.com/14259008/diff/23001/chrome/browser/ui/...
chrome/browser/ui/toolbar/toolbar_model.h:66: virtual SearchTermType
GetSearchTermType() const = 0;
Tiny nit: GetSearchTermsType() seems more correct

https://chromiumcodereview.appspot.com/14259008/diff/23001/chrome/browser/ui/...
chrome/browser/ui/toolbar/toolbar_model.h:70: virtual void
SetIsProminentSearchTermUISupported(bool value) = 0;
With the enum naming change above, this would become

  SetSupportsExtractionOfURLLikeSearchTerms()

https://chromiumcodereview.appspot.com/14259008/diff/23001/chrome/browser/ui/...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://chromiumcodereview.appspot.com/14259008/diff/23001/chrome/browser/ui/...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:196: (search_term_type ==
SEARCH_TERM_PROMINENT && is_secure))
Presumably these conditions are so we display a lock-with-warning-overlay in the
mixed-content case?  Is that the desired UX?  I didn't see anything about it on
the bug.

https://chromiumcodereview.appspot.com/14259008/diff/23001/chrome/browser/ui/...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:260: string16 search_terms =
Shorter, simpler implementation.

(1) Replace GetSearchTerms() with a private one-arg function GetSearchTermType()
(in addition to the public, no-arg version).  Implementation follows:

ToolbarModel::SearchTermType ToolbarModelImpl::GetSearchTermType(
    const string16& search_terms) const {
  if (search_terms.empty())
    return SEARCH_TERM_NONE;
  AutocompleteMatch match;
  AutocompleteClassifierFactory::GetForProfile(Profile::FromBrowserContext(
      delegate_->GetActiveWebContents()->GetBrowserContext()))->Classify(
          search_terms, false, false, &match, NULL);
  ToolbarModel::SecurityLevel security_level = GetSecurityLevel();
  if (AutocompleteMatch::IsSearchType(match.type) &&
      ((security_level == SECURE) || (security_level == EV_SECURE)))
    return SEARCH_TERM_NORMAL;
  return is_prominent_search_term_ui_supported_ ?
      SEARCH_TERM_PROMINENT : SEARCH_TERM_NONE;
}

(2) Change GetText() to use this:

string16 ToolbarModelImpl::GetText(
    bool display_search_urls_as_search_terms) const {
  if (display_search_urls_as_search_terms) {
    string16 search_terms(
        chrome::GetSearchTerms(delegate_->GetActiveWebContents()));
    if (GetSearchTermType(search_terms) != SEARCH_TERM_NONE)
      return search_terms;
  }
  ...

(3) Change the public GetSearchTermType() to use this:

ToolbarModel::SearchTermType ToolbarModelImpl::GetSearchTermType() const {
  return GetSearchTermType(
      chrome::GetSearchTerms(delegate_->GetActiveWebContents()));
}

https://chromiumcodereview.appspot.com/14259008/diff/23001/chrome/browser/ui/...
File chrome/browser/ui/toolbar/toolbar_model_impl.h (right):

https://chromiumcodereview.appspot.com/14259008/diff/23001/chrome/browser/ui/...
chrome/browser/ui/toolbar/toolbar_model_impl.h:68: // empty string.. Also
returns the search term type.
Nit: Two periods

https://chromiumcodereview.appspot.com/14259008/diff/23001/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/14259008/diff/23001/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:430:
model_->GetSearchTermType() != ToolbarModel::SEARCH_TERM_NONE) {
Nit: File style: parens around binary subexpressions

https://chromiumcodereview.appspot.com/14259008/diff/23001/chrome/browser/ui/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://chromiumcodereview.appspot.com/14259008/diff/23001/chrome/browser/ui/...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:772: return
toolbar_model()->GetSearchTermType() !=
Nit: Odd indenting; how about:

    return !model()->user_input_in_progress() &&
        (toolbar_model()->GetSearchTermType() !=
            ToolbarModel::SEARCH_TERM_NONE);

[sail, 2013-04-25 22:18:46]

https://codereview.chromium.org/14259008/diff/23001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/14259008/diff/23001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:426: if (sel_min != 0 ||
view_->toolbar_model()->GetSearchTermType() !=
On 2013/04/25 19:42:54, Peter Kasting wrote:
> Nit: Odd indenting; how about:
> 
>   if ((sel_min != 0) ||
>       (view_->toolbar_model()->GetSearchTermType() !=
>           ToolbarModel::SEARCH_TERM_NONE))

Done.

https://codereview.chromium.org/14259008/diff/23001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_unittest.cc (right):

https://codereview.chromium.org/14259008/diff/23001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_unittest.cc:191: if
(input[i].extracted_search_terms)
On 2013/04/25 19:42:54, Peter Kasting wrote:
> Nit: Simpler:
> 
>     toolbar_model()->set_search_term_type(input[i].extracted_search_terms ?
>         ToolbarModel::SEARCH_TERM_NORMAL : ToolbarModel::SEARCH_TERM_NONE);

Done.

https://codereview.chromium.org/14259008/diff/23001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model.h (right):

https://codereview.chromium.org/14259008/diff/23001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model.h:32: enum SearchTermType {
On 2013/04/25 19:42:54, Peter Kasting wrote:
> Nit: Add comment above enum:
> 
> If search URLs may be displayed as search terms, these are the types of search
> terms that may be extracted from the URL.  See comments on GetText() and
> GetSearchTermsType().

Done.

https://codereview.chromium.org/14259008/diff/23001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model.h:40: SEARCH_TERM_PROMINENT,
On 2013/04/25 19:42:54, Peter Kasting wrote:
> I think these would make more sense named as:
> 
> NO_SEARCH_TERMS,
> NORMAL_SEARCH_TERMS,
> URL_LIKE_SEARCH_TERMS
> 
> This removes the idea of "what is the UI to show these" from the ToolbarModel,
> which really doesn't care about such a concept, and instead just phrases
things
> in terms of different kinds of search terms.  It also avoids using "PROMINENT"
> alone which is a bit confusing (as compared to something like
> PROMINENT_UI_NEEDED).
> 
> I realize that URL_LIKE_... may be a misnomer for normal-looking search terms
> with a mixed-content warning present; since that's intended to be an
> error-handling case (Google should normally never have mixed-content
problems),
> I think it's OK to just add a comment about why we're doing this to the
> appropriate spot in the implementation.

Done.

https://codereview.chromium.org/14259008/diff/23001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model.h:66: virtual SearchTermType
GetSearchTermType() const = 0;
On 2013/04/25 19:42:54, Peter Kasting wrote:
> Tiny nit: GetSearchTermsType() seems more correct

Done.

https://codereview.chromium.org/14259008/diff/23001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model.h:70: virtual void
SetIsProminentSearchTermUISupported(bool value) = 0;
On 2013/04/25 19:42:54, Peter Kasting wrote:
> With the enum naming change above, this would become
> 
>   SetSupportsExtractionOfURLLikeSearchTerms()

Done.

https://codereview.chromium.org/14259008/diff/23001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/14259008/diff/23001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:196: (search_term_type ==
SEARCH_TERM_PROMINENT && is_secure))
On 2013/04/25 19:42:54, Peter Kasting wrote:
> Presumably these conditions are so we display a lock-with-warning-overlay in
the
> mixed-content case?  Is that the desired UX?  I didn't see anything about it
on
> the bug.

It's linked in the bug. Here's a direct link:
https://docs.google.com/a/google.com/folder/d/0B7Vi6tBJGkUeVW9Sa1lra3dZU0U/ed...

https://codereview.chromium.org/14259008/diff/23001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:260: string16 search_terms =
On 2013/04/25 19:42:54, Peter Kasting wrote:
> Shorter, simpler implementation.
> 
> (1) Replace GetSearchTerms() with a private one-arg function
GetSearchTermType()
> (in addition to the public, no-arg version).  Implementation follows:
> 
> ToolbarModel::SearchTermType ToolbarModelImpl::GetSearchTermType(
>     const string16& search_terms) const {
>   if (search_terms.empty())
>     return SEARCH_TERM_NONE;
>   AutocompleteMatch match;
>   AutocompleteClassifierFactory::GetForProfile(Profile::FromBrowserContext(
>       delegate_->GetActiveWebContents()->GetBrowserContext()))->Classify(
>           search_terms, false, false, &match, NULL);
>   ToolbarModel::SecurityLevel security_level = GetSecurityLevel();
>   if (AutocompleteMatch::IsSearchType(match.type) &&
>       ((security_level == SECURE) || (security_level == EV_SECURE)))
>     return SEARCH_TERM_NORMAL;
>   return is_prominent_search_term_ui_supported_ ?
>       SEARCH_TERM_PROMINENT : SEARCH_TERM_NONE;
> }
> 
> (2) Change GetText() to use this:
> 
> string16 ToolbarModelImpl::GetText(
>     bool display_search_urls_as_search_terms) const {
>   if (display_search_urls_as_search_terms) {
>     string16 search_terms(
>         chrome::GetSearchTerms(delegate_->GetActiveWebContents()));
>     if (GetSearchTermType(search_terms) != SEARCH_TERM_NONE)
>       return search_terms;
>   }
>   ...
> 
> (3) Change the public GetSearchTermType() to use this:
> 
> ToolbarModel::SearchTermType ToolbarModelImpl::GetSearchTermType() const {
>   return GetSearchTermType(
>       chrome::GetSearchTerms(delegate_->GetActiveWebContents()));
> }

Done.
Cool, that's even better.

https://codereview.chromium.org/14259008/diff/23001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model_impl.h (right):

https://codereview.chromium.org/14259008/diff/23001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_impl.h:68: // empty string.. Also
returns the search term type.
On 2013/04/25 19:42:54, Peter Kasting wrote:
> Nit: Two periods

Done.

https://codereview.chromium.org/14259008/diff/23001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/14259008/diff/23001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:430:
model_->GetSearchTermType() != ToolbarModel::SEARCH_TERM_NONE) {
On 2013/04/25 19:42:54, Peter Kasting wrote:
> Nit: File style: parens around binary subexpressions

Done.

https://codereview.chromium.org/14259008/diff/23001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/14259008/diff/23001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:772: return
toolbar_model()->GetSearchTermType() !=
On 2013/04/25 19:42:54, Peter Kasting wrote:
> Nit: Odd indenting; how about:
> 
>     return !model()->user_input_in_progress() &&
>         (toolbar_model()->GetSearchTermType() !=
>             ToolbarModel::SEARCH_TERM_NONE);

Done.

[Peter Kasting, 2013-04-25 22:48:39]

LGTM

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/gtk/omn...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right):

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/gtk/omn...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1268:
!model()->user_input_in_progress());
Tiny nit: Might be fractionally clearer as:

    gtk_widget_set_sensitive(copy_url_menuitem,
        !model()->user_input_in_progress() &&
            (toolbar_model()->GetSearchTermsType() !=
                ToolbarModel::NO_SEARCH_TERMS));

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model.h (right):

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model.h:43: URL_LIKE_SEARCH_TERMS
Tiny nit: I like leaving a trailing comma here for convenient later maintenance

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model.h:72: // that require it.
Nit: Update comment to not talk about UI

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:269: // For now treat mixed
content similar to url shaped queries.
I don't think this TODO is correct, because I think the issue is not the current
presence of mixed content but a backstop against possible future exploits that
inject mixed content into a Google Search page.

If I'm right, a better comment might be:

If the current page is not being displayed securely (e.g. due to mixed content
errors), force any extracted search terms to be considered as "URL-like".  This
will cause callers to display a more obvious indicator that the terms are a
search query.  This aims to mitigate an attack scenario where attackers inject
content into a search result page to make it look like e.g. Paypal and also
navigate the URL to that of a search for "Paypal" -- hopefully, showing an
obvious indicator that this is a search will decrease the chances of users
falling for the phishing attempt.

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model_impl.h (right):

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_impl.h:75: // shaped like URLs.
Nit: Also fix this comment to not talk about UI (probably can just be deleted
entirely)

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:773:
(toolbar_model()->GetSearchTermsType() !=
Nit: Indent 4 from "return" rather than even with "!model"

[sail, 2013-04-25 23:39:16]

Addressed review comments and fixed tests.

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/gtk/omn...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right):

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/gtk/omn...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1268:
!model()->user_input_in_progress());
On 2013/04/25 22:48:39, Peter Kasting wrote:
> Tiny nit: Might be fractionally clearer as:
> 
>     gtk_widget_set_sensitive(copy_url_menuitem,
>         !model()->user_input_in_progress() &&
>             (toolbar_model()->GetSearchTermsType() !=
>                 ToolbarModel::NO_SEARCH_TERMS));

Done.

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model.h (right):

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model.h:43: URL_LIKE_SEARCH_TERMS
On 2013/04/25 22:48:39, Peter Kasting wrote:
> Tiny nit: I like leaving a trailing comma here for convenient later
maintenance

Done.

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model.h:72: // that require it.
On 2013/04/25 22:48:39, Peter Kasting wrote:
> Nit: Update comment to not talk about UI

Done.

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:269: // For now treat mixed
content similar to url shaped queries.
On 2013/04/25 22:48:39, Peter Kasting wrote:
> I don't think this TODO is correct, because I think the issue is not the
current
> presence of mixed content but a backstop against possible future exploits that
> inject mixed content into a Google Search page.
> 
> If I'm right, a better comment might be:
> 
> If the current page is not being displayed securely (e.g. due to mixed content
> errors), force any extracted search terms to be considered as "URL-like". 
This
> will cause callers to display a more obvious indicator that the terms are a
> search query.  This aims to mitigate an attack scenario where attackers inject
> content into a search result page to make it look like e.g. Paypal and also
> navigate the URL to that of a search for "Paypal" -- hopefully, showing an
> obvious indicator that this is a search will decrease the chances of users
> falling for the phishing attempt.

Done.

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model_impl.h (right):

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_impl.h:75: // shaped like URLs.
On 2013/04/25 22:48:39, Peter Kasting wrote:
> Nit: Also fix this comment to not talk about UI (probably can just be deleted
> entirely)

Done.

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/14259008/diff/35001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:773:
(toolbar_model()->GetSearchTermsType() !=
On 2013/04/25 22:48:39, Peter Kasting wrote:
> Nit: Indent 4 from "return" rather than even with "!model"

Done.

[commit-bot: I haz the power, 2013-04-26 01:01:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/14259008/41002

[commit-bot: I haz the power, 2013-04-26 01:01:37]

Presubmit check for 14259008-41002 failed and returned exit status 1.

INFO:root:Found 16 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/policy/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/views/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/policy/policy_browsertest.cc

Presubmit checks took 1.7s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[sail, 2013-04-26 01:11:49]

+joaodasilva@chromium.org for policy/* OWNERS review

[Andrew T Wilson (Slow), 2013-04-26 17:33:18]

On 2013/04/26 01:11:49, sail wrote:
> mailto:+joaodasilva@chromium.org for policy/* OWNERS review

lgtm

[Joao da Silva, 2013-04-26 17:36:53]

Opened this tab in the morning and lost it during the day... thanks for noticing
Drew.

Late policy/ lgtm.

[commit-bot: I haz the power, 2013-04-26 17:43:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/14259008/41002

[commit-bot: I haz the power, 2013-04-26 17:58:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/14259008/57001

[commit-bot: I haz the power, 2013-04-26 18:01:01]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-04-26 18:14:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/14259008/57001

[commit-bot: I haz the power, 2013-04-26 21:40:18]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-04-27 16:01:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/14259008/57001

[commit-bot: I haz the power, 2013-04-29 13:59:00]

Change committed as 197028