Instant: Use only one hidden WebContents per profile.

Instant: Use only one hidden WebContents per profile.

Other big changes:
+ Split up extended and non-extended implementations.
+ ...
TODO(sreeram): Catalog major changes here, in this CL description.

BUG=none
R=samarth@chromium.org,dhollowa@chromium.org,shishir@chromium.org,sky@chromium.org
TEST=none

[samarth, 2013-03-01 17:59:53]

Some initial comments from my first pass.  Overall, this is looking good!  In
terms of splitting this up, I see at least three-four parts:
1) InstantPreview -> InstantOverlay renaming (plus any other renaming, e.g.
InstantModel -> InstantOverlayModel).
2) Rearchitecting things so InstantService owns InstantLoader and the
accompanying changes
3) Splitting up InstantController and the general clean-up/rewrite of core
InstantController logic.
4) Rewriting and fixing up tests

Honestly, I think the best use of your time is on #3 since that will lead to
most bug-fixes + improving the long-term maintainability of the code, so perhaps
Jered,Michael or I could take what you've done in the other parts and work on it
in parallel. What do you think?

Samarth

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:17: instance = new
InstantControllerExtendedImpl(browser, profile);
Just return here (and below) instead of saving to a local variable first.

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:55: static
scoped_ptr<InstantController> GetInstance(
GetInstance makes me think of singletons.  How about just Create()?

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:55: static
scoped_ptr<InstantController> GetInstance(
Also comments here.

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:94: virtual bool
IsOverlayingSearchResults() const = 0;
Search-and-replace fail? :)

How about IsOverlayShowingSearchResults?

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_model.h (right):

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_model.h:20: class InstantModel {
Calling this InstantModel is very confusing since it's definitely not the
"model" for all of Instant.  How about InstantOverlayModel?

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_model.h:27: bool is_height_in_pixels() const {
return is_height_in_pixels_; }
Eh, not a fan of this change; I like the explicit units much better.  Even if
you add comments here, it's not clear at all at other places what this means if
it's false.

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_service.cc (right):

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_service.cc:85: if (observers_.size() == 1)
Huh, can you explain?

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_service.h (right):

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_service.h:92: void PrefChanged(const std::string&
pref_name);
Comments for what these do please.

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_service.h:100: std::list<std::pair<int64,
std::string> > debug_events_;
A typedef here (mapping InstantService::DebugMessage to std::pair<int64,
std::string>>) would reduce c++ type insanity but up to you.

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_service_observer.h (right):

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_service_observer.h:11: virtual void
InstantStatusChanged() = 0;
Please document each of these.

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_service_observer.h:15: // For use in tests only.
I've seen an #ifdef UNIT_TEST in other places.  Is there a #ifdef TEST or
something that works for browser tests too that we can use here to enforce this?

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_web_contents_container.h (right):

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_web_contents_container.h:21: //
InstantWebContentsContainer owns a WebContents that's shown as an Instant
I'm finding the relationship between InstantLoader, InstantWebContentsContainer
and InstantOverlay to be confusing.

How about something like this?
InstantLoader (or perhaps InstantContentsContainer) is responsible for both for
loading the instant contents and is also the web contents delegate.  When a
pre-loaded page becomes an overlay, InstantService just hands off ownership of
its instance of InstantLoader to InstantOverlay, and loads up a new one.

If we want somewhat different behavior for the page while it's a preloaded page
and while it's an overlay, we can codify that as
InstantContentsContainer::Delegate--initially it's InstantService, but when you
hand it off to InstantOverlay, InstantOverlay becomes the delegate.

[sreeram, 2013-03-07 18:18:45]

Ready for another look.

As before, things to note:

1. All the Instant Extended stuff is still not here. I do hope to get it all in
for review Real Soon Now. Meanwhile, the rest *is* ready for review, so PTAL.

2. Large chunks of this can be broken off into separate CLs, just like we did
with the 'preview' -> 'overlay' rename. For example, the changes to
instant_ui.*. I will do so. As and when the pieces land separately, I'll update
this CL and rebaseline. So, for the time being, please focus your energies on
the stuff in chrome/browser/instant/*.

3. Please mark places that need more comments and/or LogDebugEvents. I'll be
happy to add them. A simple "?" will suffice to let me know.

Thanks!

On 2013/03/01 17:59:53, samarth wrote:
> Some initial comments from my first pass.  Overall, this is looking good!  In
> terms of splitting this up, I see at least three-four parts:
> 1) InstantPreview -> InstantOverlay renaming (plus any other renaming, e.g.
> InstantModel -> InstantOverlayModel).
> 2) Rearchitecting things so InstantService owns InstantLoader and the
> accompanying changes
> 3) Splitting up InstantController and the general clean-up/rewrite of core
> InstantController logic.
> 4) Rewriting and fixing up tests
> 
> Honestly, I think the best use of your time is on #3 since that will lead to
> most bug-fixes + improving the long-term maintainability of the code, so
perhaps
> Jered,Michael or I could take what you've done in the other parts and work on
it
> in parallel. What do you think?

#1 is done (rename). #2 is this CL as it stands. #3 is mostly done, but I'm not
going to tackle too much of the cleanup of the logic. I.e., I'm fixing a few
bugs, but the rest will still exist. I think you, Jered and others are already
doing a great job whacking at the bugs, so we can continue that in parallel.
Once this CL is submitted, I'll join in as well. I'm doing as much of #4 as
reasonable in this CL, and will make it the focus of what I do after this CL
lands.

In short, let's proceed. The real work (fixing bugs, #3) will continue in
parallel with everyone's help.

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:17: instance = new
InstantControllerExtendedImpl(browser, profile);
On 2013/03/01 17:59:53, samarth wrote:
> Just return here (and below) instead of saving to a local variable first.

Done.

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:55: static
scoped_ptr<InstantController> GetInstance(
On 2013/03/01 17:59:53, samarth wrote:
> GetInstance makes me think of singletons.  How about just Create()?

Done.

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:55: static
scoped_ptr<InstantController> GetInstance(
On 2013/03/01 17:59:53, samarth wrote:
> Also comments here.

Done. Create() along with the scoped_ptr<> return value seems self-evident, so
I'm not sure my comment adds much value. Let me know if I misunderstood you.

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:94: virtual bool
IsOverlayingSearchResults() const = 0;
On 2013/03/01 17:59:53, samarth wrote:
> Search-and-replace fail? :)
> 
> How about IsOverlayShowingSearchResults?

Sorry, this ship has sailed. I don't think it's unreasonable to say something
like "is the controller overlaying search results?"

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_model.h (right):

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_model.h:20: class InstantModel {
On 2013/03/01 17:59:53, samarth wrote:
> Calling this InstantModel is very confusing since it's definitely not the
> "model" for all of Instant.  How about InstantOverlayModel?

Done.

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_model.h:27: bool is_height_in_pixels() const {
return is_height_in_pixels_; }
On 2013/03/01 17:59:53, samarth wrote:
> Eh, not a fan of this change; I like the explicit units much better.  Even if
> you add comments here, it's not clear at all at other places what this means
if
> it's false.

Done. Agreed. Sorry I got overzealous. I also reverted the change to
InstantSuggestionType. I think these enums are worth their weight for the
clarity they provide.

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_service.cc (right):

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_service.cc:85: if (observers_.size() == 1)
On 2013/03/01 17:59:53, samarth wrote:
> Huh, can you explain?

Done.

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_service.h (right):

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_service.h:92: void PrefChanged(const std::string&
pref_name);
On 2013/03/01 17:59:53, samarth wrote:
> Comments for what these do please.

Done.

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_service.h:100: std::list<std::pair<int64,
std::string> > debug_events_;
On 2013/03/01 17:59:53, samarth wrote:
> A typedef here (mapping InstantService::DebugMessage to std::pair<int64,
> std::string>>) would reduce c++ type insanity but up to you.

Done.

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_service_observer.h (right):

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_service_observer.h:11: virtual void
InstantStatusChanged() = 0;
On 2013/03/01 17:59:53, samarth wrote:
> Please document each of these.

Done.

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_service_observer.h:15: // For use in tests only.
On 2013/03/01 17:59:53, samarth wrote:
> I've seen an #ifdef UNIT_TEST in other places.  Is there a #ifdef TEST or
> something that works for browser tests too that we can use here to enforce
this?

Done. Took this out. It was a bad idea to force regular objects
(InstantControllerImpl and InstantPreloader) to implement this
meant-for-tests-only interface. Instead, we now use a notification.

Aside: #define UNIT_TEST works for browser tests too, despite its name. However,
I couldn't have used it here. The macro is set when compiling any test .cc file,
but since instant_service.cc would've referenced this method, it wouldn't have
worked. I.e., when instant_service.cc is compiled, the macro is false, so fine,
all references to InstantSupportDecided() vanish. But now, you compile a
*-test.cc file, and sure, it defines the macro, so you happily wait for
InstantSupportDecided() to be called. Now, instant_service.cc is NOT going to be
recompiled just to link your unit_test, so the call never arrives.

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_web_contents_container.h (right):

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_web_contents_container.h:21: //
InstantWebContentsContainer owns a WebContents that's shown as an Instant
On 2013/03/01 17:59:53, samarth wrote:
> I'm finding the relationship between InstantLoader,
InstantWebContentsContainer
> and InstantOverlay to be confusing.
> 
> How about something like this?
> InstantLoader (or perhaps InstantContentsContainer) is responsible for both
for
> loading the instant contents and is also the web contents delegate.  When a
> pre-loaded page becomes an overlay, InstantService just hands off ownership of
> its instance of InstantLoader to InstantOverlay, and loads up a new one.
> 
> If we want somewhat different behavior for the page while it's a preloaded
page
> and while it's an overlay, we can codify that as
> InstantContentsContainer::Delegate--initially it's InstantService, but when
you
> hand it off to InstantOverlay, InstantOverlay becomes the delegate.

As discussed offline, I've kept the current structure, but renamed InstantLoader
to InstantPreloader to make the distinction clearer (thanks Shishir for the
name!).

[sreeram, 2013-03-07 18:49:22]

https://codereview.chromium.org/12386019/diff/9001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/embedded_searchbox_api.js (right):

https://codereview.chromium.org/12386019/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/embedded_searchbox_api.js:1: // Copyright
2012 The Chromium Authors. All rights reserved.
This file is bogus. Ignore. The real embedded_searchbox_api.js is coming soon.

[samarth, 2013-03-11 19:03:23]

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:55: static
scoped_ptr<InstantController> GetInstance(
On 2013/03/07 18:18:46, sreeram wrote:
> On 2013/03/01 17:59:53, samarth wrote:
> > Also comments here.
> 
> Done. Create() along with the scoped_ptr<> return value seems self-evident, so
> I'm not sure my comment adds much value. Let me know if I misunderstood you.

Not sure what I had in mind when I wrote that :) Agreed that the current comment
isn't particularly useful so feel free to take it out.

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_web_contents_container.h (right):

https://codereview.chromium.org/12386019/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_web_contents_container.h:21: //
InstantWebContentsContainer owns a WebContents that's shown as an Instant
On 2013/03/07 18:18:46, sreeram wrote:
> On 2013/03/01 17:59:53, samarth wrote:
> > I'm finding the relationship between InstantLoader,
> InstantWebContentsContainer
> > and InstantOverlay to be confusing.
> > 
> > How about something like this?
> > InstantLoader (or perhaps InstantContentsContainer) is responsible for both
> for
> > loading the instant contents and is also the web contents delegate.  When a
> > pre-loaded page becomes an overlay, InstantService just hands off ownership
of
> > its instance of InstantLoader to InstantOverlay, and loads up a new one.
> > 
> > If we want somewhat different behavior for the page while it's a preloaded
> page
> > and while it's an overlay, we can codify that as
> > InstantContentsContainer::Delegate--initially it's InstantService, but when
> you
> > hand it off to InstantOverlay, InstantOverlay becomes the delegate.
> 
> As discussed offline, I've kept the current structure, but renamed
InstantLoader
> to InstantPreloader to make the distinction clearer (thanks Shishir for the
> name!).

Sorry, I'm looking at this again and this still doesn't feel right to me.  For
instance, we have this structure where InstantOverlay <-
InstantWebContentsContainer <- WebContentsDelegate (<- = inherits from) and
methods from WebContentsDelegate are being overridden in both places. This makes
it hard to see exactly what things get overriden.

In our offline discussion, you said the main reason for having
InstantWebContentsContainer is to prevent code duplication. AFAICT, the only
non-trivial pieces of InstantWebContentsContainer that are used by both
InstantPreloader and InstantOverlay are OpenURLFromTab and
SetUp/TearDownContents. We could easily move the logic from those into helpers
that InstantOverlay and InstantPreloader call. And then InstantOverlay and
InstantPreloader could each just separately inherit from
content::WebContentsDelegate. To go from the Preloader to Overlay, you would
just release from the former to give it to the latter.

I really think this will make things clearer and easier to understand, but let's
talk more offline (perhaps with David as well) when you're in.

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.h (left):

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:185: // These functions are called before
processing messages received from the
I can't see InstantExtendedController yet, but are you putting it in charge of
deciding which messages are honored from Overlay vs Tab?

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
File chrome/browser/instant/instant_service.cc (right):

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_service.cc:39: // Stub for unit tests.
Makes me slightly nervous. How about wrapping this in a #ifdef UNIT_TEST

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_service.cc:116: // If we just added our first
observer, we have our first browser window.
This seems too subtle and easy to go wrong. I'd prefer a more explicit signal
that we should start preloading.

Can we explicitly listen for a window being opened? (And we can skip preloading
if a InstantPreloader already exists.)

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_service.cc:125: // If there's only one observer
left, it must be the InstantPreloader. All
Again, I'd prefer a more explicit signal if possible.

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_service.cc:189: if (pref_name ==
prefs::kInstantEnabled)
I'm a little confused why you reset some of the prefs always and only reset this
one if the pref_name matches. Can you comment with what is going on here?

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_service.cc:226: if (alignment &
ThemeProperties::ALIGN_LEFT) {
nit: no braces?  (not sure if Chromium makes an exception for  if/else if/elses)

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_service.cc:249: theme_info_.image_tiling =
THEME_BKGRND_IMAGE_NO_REPEAT;
Indent looks wrong

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
File chrome/browser/instant/instant_service.h (right):

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_service.h:37: // Tracks Instant-related
per-profile things, such as render process host IDs,
nit: Tracks per-profile Instant-related things

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
File chrome/browser/instant/instant_service_observer.h (right):

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_service_observer.h:9: class
InstantServiceObserver {
Did you leave out the default no-op implementation on purpose?

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_service_observer.h:12: //
chrome/browser/ui/search/search.h) has changed in value.
nit: c/b/ui/search -> c/b/instant

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

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:1011:
instant_controller_->instant()->TabDeactivated(contents);
I always found the fact that BrowserInstantController is called
instant_controller_ confusing. Here's a good chance to fix it :)

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/location_bar.h (left):

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/location_bar.h:34: // Sets the suggested text to show
in the omnibox. This is shown in addition
What's going on with this change? Seems like a reasonable one to split off into
it's own change.

https://codereview.chromium.org/12386019/diff/16001/chrome/common/omnibox_typ...
File chrome/common/omnibox_types.h (right):

https://codereview.chromium.org/12386019/diff/16001/chrome/common/omnibox_typ...
chrome/common/omnibox_types.h:8: // Omnibox focus state.
via sky@ in https://chromiumcodereview.appspot.com/11413217:
"""
This is specific to the omnibox, it should be in that directory. In general
chrome/common isn't meant to be a random grab bag of stuff.

I would put the constants in either omnibox_edit_model.h or
c/b/ui/omnibox_types.h. I would ping Peter for his preference for these.
"""

Any reason you had to move this here?

https://codereview.chromium.org/12386019/diff/16001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.h (right):

https://codereview.chromium.org/12386019/diff/16001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.h:24: // Returns the v8::Extension
object handling searchbox bindings. Caller takes
Can you use scoped_ptr instead?

[samarth, 2013-03-12 21:31:20]

Couple more comments from our offline discussion.

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
File chrome/browser/instant/instant_service_observer.h (right):

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_service_observer.h:22: ~InstantServiceObserver()
{}
Don't inline and make it virtual.

https://codereview.chromium.org/12386019/diff/16001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.h (right):

https://codereview.chromium.org/12386019/diff/16001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.h:28: static bool
DetermineInstantSupport(content::RenderView* render_view);
Per offline discussion, we should consider just moving these into a namespace
instead of this class.

[Jered, 2013-03-26 16:36:41]

Just a few quick drive-by comments, mean to read in more detail...

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:60: InstantController();
Make this protected.

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:98: virtual const
InstantOverlayModel* model() const = 0;
model() -> GetModel()

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.h (right):

https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:64: virtual void SetFocusState(const
content::WebContents* contents,
Can we call this SetOmniboxFocusState?

https://codereview.chromium.org/12386019/diff/16001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.cc (left):

https://codereview.chromium.org/12386019/diff/16001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:70: static const char
kSearchBoxExtensionName[] = "v8/EmbeddedSearch";
Where did all this stuff go? :-)

https://codereview.chromium.org/12386019/diff/16001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/12386019/diff/16001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:23: "chrome &&"
Is it safe to omit window (could this have function scope with a local
this.chrome when invoked?)

[sreeram, 2013-06-03 18:30:07]

Closing this issue. This patch is severely obsolete. I'll leave it around as a
reference.