Add core plumbing for Instant Extended work

Add core plumbing for Instant Extended work

Adds UI model, types, and controller logic.  The controller logic is
encoded in search_tab_helper.cc/h.  The model observer changes are
multiplexed from the active tab through the browser's model via
search_delegate.h/cc.

The primary state in the model is the "mode" which changes depending on
interactions with the Omnibox and active URL of the page.  This state is
communicated from the tab to various "Top Chrome" components such as the
toolbar, the tab strip, etc.

BUG=133506
TEST=GoogleUtilTest.IsEmbeddedGoogleSearchUrl, SearchDelegateTest.*
R=sky@chromium.org, ben@chromium.org

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

[dhollowa, 2012-06-21 17:25:32]

ben, sky -> chrome/browser/ui changes
isherman -> google addition

Thanks!

[Ilya Sherman, 2012-06-21 17:46:58]

+pkasting

google/ LGTM, but Peter might want to take a look as well.

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/goog...
File chrome/browser/google/google_util.cc (right):

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/goog...
chrome/browser/google/google_util.cc:221: return value.length() > 0 && value !=
"0";
Optional nit: "value.length() > 0" -> "!value.empty()"

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/goog...
chrome/browser/google/google_util.cc:223: }
You might want to use GURL's implementation [1] for extracting query components,
rather than rolling your own... though this code is not very complex, so I'm not
sure which is actually simpler.
https://code.google.com/searchframe#OAMlx_jo-ck/src/googleurl/src/url_parse.h...

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/goog...
File chrome/browser/google/google_util.h (right):

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/goog...
chrome/browser/google/google_util.h:71: bool IsEmbeddedGoogleSearchUrl(const
std::string& url);
nit: The notion of "embedded search" is not very clear to me -- can you expand
this comment a bit to explain what the distinction is, or perhaps just mention
the expected use-case?

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/goog...
File chrome/browser/google/google_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/goog...
chrome/browser/google/google_util_unittest.cc:256:
"http://www.google.com/search?q=something&espv="));
nit: Please also add a test for espv=0 on google.com

[Peter Kasting, 2012-06-21 18:04:17]

Ilya's comments are all on target, but additionally, do not put your new
functionality in google_util.h; put it in search_tab_helper.cc as an
anonymous-namespaced function.  We should avoid using util headers unless we've
got something that's truly going to be used by multiple disparate consumers.

[sky, 2012-06-21 18:26:49]

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/ui/b...
File chrome/browser/ui/browser.cc (right):

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/ui/b...
chrome/browser/ui/browser.cc:2841: search_delegate_->OnTabClosing(contents);
I think you want TabDetachedAt, not this.

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/ui/b...
File chrome/browser/ui/browser.h (right):

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/ui/b...
chrome/browser/ui/browser.h:1350: // The model for the "active" search state.
Wrap these to 80 chars please.

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/ui/s...
File chrome/browser/ui/search/search_delegate.cc (right):

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/ui/s...
chrome/browser/ui/search/search_delegate.cc:31:
DCHECK(tab_model_->tab_contents() == contents);
DCHECK_EQ

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/ui/s...
File chrome/browser/ui/search/search_delegate_unittest.cc (right):

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/ui/s...
chrome/browser/ui/search/search_delegate_unittest.cc:20:
TEST_F(SearchDelegateTest, SearchModel) {
Can this be a unit test? Specifically a BrowserWithTestWindowTest?

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/ui/s...
File chrome/browser/ui/search/search_model.h (right):

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/ui/s...
chrome/browser/ui/search/search_model.h:25: extern const SkColor
kNTPBackgroundColor;
These don't really feel like model state. Should we move them to a separate
file? search_ui ? Same comment for GetNTPOmniboxBounds.

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/ui/s...
File chrome/browser/ui/search/search_types.h (right):

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/ui/s...
chrome/browser/ui/search/search_types.h:12: struct Mode {
Add a description.

https://chromiumcodereview.appspot.com/10644002/diff/6001/chrome/browser/ui/s...
chrome/browser/ui/search/search_types.h:54: bool animate;
Add a description for animate.

[dhollowa, 2012-06-21 22:10:52]

On 2012/06/21 18:04:17, Peter Kasting wrote:
> Ilya's comments are all on target, but additionally, do not put your new
> functionality in google_util.h; put it in search_tab_helper.cc as an
> anonymous-namespaced function.  We should avoid using util headers unless
we've
> got something that's truly going to be used by multiple disparate consumers.

Agreed.  The principle of narrowest scope  is valuable.  However, this is the
first patch of a series of patches, and patches to follow will use this utility
in some non-zero subset of:

  chrome/browser/instant/instant_controller.cc
  chrome/browser/ui/cocoa/tabs/tab_controller.mm
  chrome/browser/ui/search/search_tab_helper.cc
  chrome/browser/ui/search/search_tab_helper.cc
  chrome/browser/ui/toolbar/toolbar_model.cc
  chrome/browser/ui/views/tabs/tab.cc

So I'm inclined to leave this in google_util.h.

[dhollowa, 2012-06-21 22:16:43]

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/google/googl...
File chrome/browser/google/google_util.cc (right):

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/google/googl...
chrome/browser/google/google_util.cc:221: return value.length() > 0 && value !=
"0";
On 2012/06/21 17:46:58, Ilya Sherman wrote:
> Optional nit: "value.length() > 0" -> "!value.empty()"

Done.  Rephrased with |Component| terminology.

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/google/googl...
chrome/browser/google/google_util.cc:223: }
Much better.  Thanks for the tip!

On 2012/06/21 17:46:58, Ilya Sherman wrote:
> You might want to use GURL's implementation [1] for extracting query
components,
> rather than rolling your own... though this code is not very complex, so I'm
not
> sure which is actually simpler.
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/googleurl/src/url_parse.h...

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/google/googl...
File chrome/browser/google/google_util.h (right):

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/google/googl...
chrome/browser/google/google_util.h:71: bool IsEmbeddedGoogleSearchUrl(const
std::string& url);
On 2012/06/21 17:46:58, Ilya Sherman wrote:
> nit: The notion of "embedded search" is not very clear to me -- can you expand
> this comment a bit to explain what the distinction is, or perhaps just mention
> the expected use-case?

Agreed.  I've renamed to use the "Instant Extended API" terminology used
elsewhere.  It is a url alternate for use by the new Instant work.

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/google/googl...
File chrome/browser/google/google_util_unittest.cc (right):

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/google/googl...
chrome/browser/google/google_util_unittest.cc:256:
"http://www.google.com/search?q=something&espv="));
On 2012/06/21 17:46:58, Ilya Sherman wrote:
> nit: Please also add a test for espv=0 on http://google.com

Done.

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:2841: search_delegate_->OnTabClosing(contents);
TabDetachedAt is handled below.  So I've removed this.

On 2012/06/21 18:26:49, sky wrote:
> I think you want TabDetachedAt, not this.

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:1350: // The model for the "active" search state.
On 2012/06/21 18:26:49, sky wrote:
> Wrap these to 80 chars please.

Done.

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/ui/search/se...
File chrome/browser/ui/search/search_delegate.cc (right):

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/ui/search/se...
chrome/browser/ui/search/search_delegate.cc:31:
DCHECK(tab_model_->tab_contents() == contents);
On 2012/06/21 18:26:49, sky wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/ui/search/se...
File chrome/browser/ui/search/search_delegate_unittest.cc (right):

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/ui/search/se...
chrome/browser/ui/search/search_delegate_unittest.cc:20:
TEST_F(SearchDelegateTest, SearchModel) {
It is, see typedef above.

On 2012/06/21 18:26:49, sky wrote:
> Can this be a unit test? Specifically a BrowserWithTestWindowTest?

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/ui/search/se...
File chrome/browser/ui/search/search_model.h (right):

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/ui/search/se...
chrome/browser/ui/search/search_model.h:25: extern const SkColor
kNTPBackgroundColor;
On 2012/06/21 18:26:49, sky wrote:
> These don't really feel like model state. Should we move them to a separate
> file? search_ui ? Same comment for GetNTPOmniboxBounds.

Agreed.  I've split them off into search_ui.h/cc.

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/ui/search/se...
File chrome/browser/ui/search/search_types.h (right):

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/ui/search/se...
chrome/browser/ui/search/search_types.h:12: struct Mode {
On 2012/06/21 18:26:49, sky wrote:
> Add a description.

Done.

http://codereview.chromium.org/10644002/diff/6001/chrome/browser/ui/search/se...
chrome/browser/ui/search/search_types.h:54: bool animate;
On 2012/06/21 18:26:49, sky wrote:
> Add a description for animate.

Done.

[Peter Kasting, 2012-06-21 22:28:05]

Leaving in google_util is fine if you plan to call this all over.

http://codereview.chromium.org/10644002/diff/16001/chrome/browser/google/goog...
File chrome/browser/google/google_util.cc (right):

http://codereview.chromium.org/10644002/diff/16001/chrome/browser/google/goog...
chrome/browser/google/google_util.cc:217: url.c_str(), &parsed_url.query, &key,
&value)) {
Nit: Unusual indenting... just 4 from beginning of line would be more typical

http://codereview.chromium.org/10644002/diff/16001/chrome/browser/google/goog...
chrome/browser/google/google_util.cc:220: if (url.substr(key.begin, key.len) ==
embedded_search_key) {
Nit: Cheaper:

  if (!url.compare(key.begin, key.len, embedded_search_key))

http://codereview.chromium.org/10644002/diff/16001/chrome/browser/google/goog...
chrome/browser/google/google_util.cc:221: return value.is_nonempty() &&
url.substr(value.begin, value.len) != "0";
Nit: Seems like using base::StringToInt() might be safer?  Or do you want to
consider "espv=00" an instant extended URL?  (In that case, consider using
url.compare() again.)

http://codereview.chromium.org/10644002/diff/16001/chrome/browser/google/goog...
File chrome/browser/google/google_util.h (right):

http://codereview.chromium.org/10644002/diff/16001/chrome/browser/google/goog...
chrome/browser/google/google_util.h:69: // True if |url| represents a valid
Google search URL used by the the Instant
Nit: the the

[tfarina, 2012-06-21 22:28:54]

http://codereview.chromium.org/10644002/diff/24001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_model_observer.h (right):

http://codereview.chromium.org/10644002/diff/24001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_model_observer.h:12: struct Location;
nit: remove this, unused in this header file.

[tfarina, 2012-06-21 22:49:55]

http://codereview.chromium.org/10644002/diff/24001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_model.cc (right):

http://codereview.chromium.org/10644002/diff/24001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_model.cc:7: #include
"chrome/browser/google/google_util.h"
do you need this, url_constants.h and web_contents.h in this file?

http://codereview.chromium.org/10644002/diff/24001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_model.cc:25: if (contents_ == NULL)
nit: if (!contents_)
       return;

[sky, 2012-06-21 22:52:49]

LGTM

http://codereview.chromium.org/10644002/diff/24001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10644002/diff/24001/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:4807: search_delegate_->OnTabDettached(contents);
OnTabDetached

http://codereview.chromium.org/10644002/diff/24001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_ui.h (right):

http://codereview.chromium.org/10644002/diff/24001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_ui.h:10: #include "ui/gfx/rect.h"
forward declare gfx::Rect and gfx::Size

[dhollowa, 2012-06-21 23:53:42]

http://codereview.chromium.org/10644002/diff/16001/chrome/browser/google/goog...
File chrome/browser/google/google_util.cc (right):

http://codereview.chromium.org/10644002/diff/16001/chrome/browser/google/goog...
chrome/browser/google/google_util.cc:217: url.c_str(), &parsed_url.query, &key,
&value)) {
On 2012/06/21 22:28:05, Peter Kasting wrote:
> Nit: Unusual indenting... just 4 from beginning of line would be more typical

Done.  google-c-style.el did this for me...

http://codereview.chromium.org/10644002/diff/16001/chrome/browser/google/goog...
chrome/browser/google/google_util.cc:220: if (url.substr(key.begin, key.len) ==
embedded_search_key) {
On 2012/06/21 22:28:05, Peter Kasting wrote:
> Nit: Cheaper:
> 
>   if (!url.compare(key.begin, key.len, embedded_search_key))

Done.

http://codereview.chromium.org/10644002/diff/16001/chrome/browser/google/goog...
chrome/browser/google/google_util.cc:221: return value.is_nonempty() &&
url.substr(value.begin, value.len) != "0";
On 2012/06/21 22:28:05, Peter Kasting wrote:
> Nit: Seems like using base::StringToInt() might be safer?  Or do you want to
> consider "espv=00" an instant extended URL?  (In that case, consider using
> url.compare() again.)

Done.  And added additional test for this case.

http://codereview.chromium.org/10644002/diff/16001/chrome/browser/google/goog...
File chrome/browser/google/google_util.h (right):

http://codereview.chromium.org/10644002/diff/16001/chrome/browser/google/goog...
chrome/browser/google/google_util.h:69: // True if |url| represents a valid
Google search URL used by the the Instant
On 2012/06/21 22:28:05, Peter Kasting wrote:
> Nit: the the

Done.

http://codereview.chromium.org/10644002/diff/24001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10644002/diff/24001/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:4807: search_delegate_->OnTabDettached(contents);
On 2012/06/21 22:52:49, sky wrote:
> OnTabDetached

Done.

http://codereview.chromium.org/10644002/diff/24001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_model.cc (right):

http://codereview.chromium.org/10644002/diff/24001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_model.cc:7: #include
"chrome/browser/google/google_util.h"
On 2012/06/21 22:49:55, tfarina wrote:
> do you need this, url_constants.h and web_contents.h in this file?

Done.

http://codereview.chromium.org/10644002/diff/24001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_model.cc:25: if (contents_ == NULL)
On 2012/06/21 22:49:55, tfarina wrote:
> nit: if (!contents_)
>        return;

Done.

http://codereview.chromium.org/10644002/diff/24001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_model_observer.h (right):

http://codereview.chromium.org/10644002/diff/24001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_model_observer.h:12: struct Location;
On 2012/06/21 22:28:55, tfarina wrote:
> nit: remove this, unused in this header file.

Done.

http://codereview.chromium.org/10644002/diff/24001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_ui.h (right):

http://codereview.chromium.org/10644002/diff/24001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_ui.h:10: #include "ui/gfx/rect.h"
On 2012/06/21 22:52:49, sky wrote:
> forward declare gfx::Rect and gfx::Size

Done.

[dhollowa, 2012-06-22 00:22:05]

Ben, you good with this?

[Ben Goodger (Google), 2012-06-22 16:07:10]

lgtm

[Peter Kasting, 2012-06-22 17:31:29]

chrome/browser/google LGTM