InstantExtended: Committed NTP

InstantExtended: Committed NTP

Use separate dedicated WebContents for the NTP and Instant overlay (both residing in the Instant process). Intercept any navigations to the New Tab Page and swap in the preloaded Instant NTP contents when available.

BUG=168828, 174498, 174498
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181397

[samarth, 2013-01-10 07:25:40]

Ready for a first look.
dhollowa: please look at chrome/browser/ui/... and content/...
sreeram, jered: please look at the instant bits
(Although I'd appreciate a full look if you have the time)

Some known issues:
- NTP shows google favicon
- NTP title is "Google"
- Omnibox loses focus if you navigate to NTP by typing in "chrome://newtab"
... probably more

We should be able to clean up the Instant API, server and client code some more
after this goes in (e.g. don't need to worry about context anymore).

Thanks!
Samarth

[Jered, 2013-01-10 15:53:54]

I did a quick first pass, just some nits. The overlay_ and ntp_ split in
InstantController feels a bit repetitive. Will read more closely this afternoon.

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
chrome/browser/instant/instant_controller.h:44: // the Embedded Search API (cf.
http://dev.chromium.org/embeddedsearch). It
cf -> see

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
File chrome/browser/instant/instant_overlay.cc (right):

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
chrome/browser/instant/instant_overlay.cc:209: const content::WebContents*
active_tab) {
reindent

https://codereview.chromium.org/11824050/diff/25/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11824050/diff/25/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:1957: return (entry && (
next line

https://codereview.chromium.org/11824050/diff/25/content/public/common/url_co...
File content/public/common/url_constants.cc (right):

https://codereview.chromium.org/11824050/diff/25/content/public/common/url_co...
content/public/common/url_constants.cc:46: const char kChromeUINewTabURL[] =
"chrome://newtab";
this is in src/chrome/common/url_constants.cc already

[dhollowa, 2013-01-10 18:50:29]

You'll need to add sky@ for chrome/browser/ui stuff.

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
chrome/browser/instant/instant_controller.h:156: // Invoked by an Instant page
to request the browser to start capturing user
The term "an Instant page" is not so clear to the casual reader.  Possibly "an
Instant page (i.e. an InstantClient::Delegate)"?

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
chrome/browser/instant/instant_controller.h:271: // A committed WebContents that
supports Instant. If non-NULL, the |overlay_|
The term "that supports Instant" is a bit vague.  How about "that supports the
SearchBoxAPI via the InstantClient::Delegate interface."?

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
File chrome/browser/instant/instant_ntp.cc (right):

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
chrome/browser/instant/instant_ntp.cc:112:
ALLOW_THIS_IN_INITIALIZER_LIST(this))),
nit: indent seems off here.

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
File chrome/browser/instant/instant_ntp.h (right):

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
chrome/browser/instant/instant_ntp.h:86: InstantClient client_;
I notice that all three InstantOverlay, InstantNTP, and InstantTab have the same
state: client_, controller_, and supports_instant_.

This makes me think there is informal commonality that probably should be
formalized.  Either by keeping this state in InstantClient::Delegate directly,
or better yet, introduce a common concrete base-class, InstantPage say.  It
would look something like this:

class InstantPage : public InstantClient::Delegate {
  ...
 private:
  InstantClient client_;
  InstantController* controller_;
  bool supports_instant_;
}

class InstantOverlay : public InstantPage {...}
class InstantNTP : public InstantNTP { ... }
class InstantTab : public InstantTab { ... }

Thoughts?

https://codereview.chromium.org/11824050/diff/25/chrome/browser/ui/search/sea...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/11824050/diff/25/chrome/browser/ui/search/sea...
chrome/browser/ui/search/search_tab_helper.cc:102: // TODO(samarth): check with
Kuan that this is safe to remove.
I expect this is ok, but you'll need to change line 188 to remove the
conditional otherwise navigating to NTP won't get noticed at all.

[tfarina, 2013-01-10 20:24:41]

On Thu, Jan 10, 2013 at 4:50 PM,  <dhollowa@chromium.org> wrote:
>
https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
> File chrome/browser/instant/instant_ntp.h (right):
>
>
https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
> chrome/browser/instant/instant_ntp.h:86: InstantClient client_;
> I notice that all three InstantOverlay, InstantNTP, and InstantTab have
> the same state: client_, controller_, and supports_instant_.
>
> This makes me think there is informal commonality that probably should
> be formalized.  Either by keeping this state in InstantClient::Delegate
> directly, or better yet, introduce a common concrete base-class,
> InstantPage say.  It would look something like this:
>
> class InstantPage : public InstantClient::Delegate {
>   ...
>  private:
>   InstantClient client_;
>   InstantController* controller_;
>   bool supports_instant_;
> }
>
> class InstantOverlay : public InstantPage {...}
> class InstantNTP : public InstantNTP { ... }

here you meant public InstantOverlay, right?

> https://codereview.chromium.org/11824050/
>

-- 
Thiago

[dhollowa, 2013-01-10 21:08:03]

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
File chrome/browser/instant/instant_ntp.h (right):

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
chrome/browser/instant/instant_ntp.h:86: InstantClient client_;
On 2013/01/10 18:50:29, dhollowa wrote:
> I notice that all three InstantOverlay, InstantNTP, and InstantTab have the
same
> state: client_, controller_, and supports_instant_.
> 
> This makes me think there is informal commonality that probably should be
> formalized.  Either by keeping this state in InstantClient::Delegate directly,
> or better yet, introduce a common concrete base-class, InstantPage say.  It
> would look something like this:
> 
> class InstantPage : public InstantClient::Delegate {
>   ...
>  private:
>   InstantClient client_;
>   InstantController* controller_;
>   bool supports_instant_;
> }
> 
> class InstantOverlay : public InstantPage {...}
> class InstantNTP : public InstantNTP { ... }
> class InstantTab : public InstantTab { ... }
> 
> Thoughts?

Err... correction:
class InstantNTP : public InstantPage { ... }
class InstantTab : public InstantPage { ... }

(Thanks Thiago).

[samarth, 2013-01-11 19:43:05]

Thanks for the comments!  I'll send this to sky (and other OWNERS) once you guys
are mostly happy with this.

Thanks,
Samarth

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
chrome/browser/instant/instant_controller.h:44: // the Embedded Search API (cf.
http://dev.chromium.org/embeddedsearch). It
On 2013/01/10 15:53:54, Jered wrote:
> cf -> see

Done.

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
chrome/browser/instant/instant_controller.h:156: // Invoked by an Instant page
to request the browser to start capturing user
On 2013/01/10 18:50:29, dhollowa wrote:
> The term "an Instant page" is not so clear to the casual reader.  Possibly "an
> Instant page (i.e. an InstantClient::Delegate)"?

Done.

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
chrome/browser/instant/instant_controller.h:271: // A committed WebContents that
supports Instant. If non-NULL, the |overlay_|
On 2013/01/10 18:50:29, dhollowa wrote:
> The term "that supports Instant" is a bit vague.  How about "that supports the
> SearchBoxAPI via the InstantClient::Delegate interface."?

Done.

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
File chrome/browser/instant/instant_ntp.cc (right):

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
chrome/browser/instant/instant_ntp.cc:112:
ALLOW_THIS_IN_INITIALIZER_LIST(this))),
On 2013/01/10 18:50:29, dhollowa wrote:
> nit: indent seems off here.

Done.

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
File chrome/browser/instant/instant_ntp.h (right):

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
chrome/browser/instant/instant_ntp.h:86: InstantClient client_;
On 2013/01/10 21:08:03, dhollowa wrote:
> On 2013/01/10 18:50:29, dhollowa wrote:
> > I notice that all three InstantOverlay, InstantNTP, and InstantTab have the
> same
> > state: client_, controller_, and supports_instant_.
> > 
> > This makes me think there is informal commonality that probably should be
> > formalized.  Either by keeping this state in InstantClient::Delegate
directly,
> > or better yet, introduce a common concrete base-class, InstantPage say.  It
> > would look something like this:
> > 
> > class InstantPage : public InstantClient::Delegate {
> >   ...
> >  private:
> >   InstantClient client_;
> >   InstantController* controller_;
> >   bool supports_instant_;
> > }
> > 
> > class InstantOverlay : public InstantPage {...}
> > class InstantNTP : public InstantNTP { ... }
> > class InstantTab : public InstantTab { ... }
> > 
> > Thoughts?
> 
> Err... correction:
> class InstantNTP : public InstantPage { ... }
> class InstantTab : public InstantPage { ... }
> 
> (Thanks Thiago).

Channeling sky (who was channeling Ben):
"""
Ben was griping today about a review he was looking at (not this one) and
happened to say, 'god calls a kitten every time you subclass a concrete type.'
It made me think of this patch.
"""
Plus I'm hesistant to add yet another class here .. the object hierarchy here is
already a little tricky to follow.

That said, I agree that something about the repetitiveness between
Instant{NTP,Overlay,Tab} smells wrong.  Likewise for some of the code
duplication in InstantController that Jered pointed out.

Let me mull this over some more; feels like some more refactoring is in order
here.

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
File chrome/browser/instant/instant_overlay.cc (right):

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
chrome/browser/instant/instant_overlay.cc:209: const content::WebContents*
active_tab) {
On 2013/01/10 15:53:54, Jered wrote:
> reindent

Done.

https://codereview.chromium.org/11824050/diff/25/chrome/browser/ui/search/sea...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/11824050/diff/25/chrome/browser/ui/search/sea...
chrome/browser/ui/search/search_tab_helper.cc:102: // TODO(samarth): check with
Kuan that this is safe to remove.
On 2013/01/10 18:50:29, dhollowa wrote:
> I expect this is ok, but you'll need to change line 188 to remove the
> conditional otherwise navigating to NTP won't get noticed at all.

Done (I assume you meant line 118?).  I've sent an email to kuan@ but went ahead
and just deleted this stuff.  I can add it back in if necessary.

https://codereview.chromium.org/11824050/diff/25/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11824050/diff/25/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:1957: return (entry && (
On 2013/01/10 15:53:54, Jered wrote:
> next line

Done.

https://codereview.chromium.org/11824050/diff/25/content/public/common/url_co...
File content/public/common/url_constants.cc (right):

https://codereview.chromium.org/11824050/diff/25/content/public/common/url_co...
content/public/common/url_constants.cc:46: const char kChromeUINewTabURL[] =
"chrome://newtab";
On 2013/01/10 15:53:54, Jered wrote:
> this is in src/chrome/common/url_constants.cc already

Based on my understanding, these need to be redefined because content/ can't
depend on chrome/

[dhollowa, 2013-01-11 19:55:01]

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
File chrome/browser/instant/instant_ntp.h (right):

https://codereview.chromium.org/11824050/diff/25/chrome/browser/instant/insta...
chrome/browser/instant/instant_ntp.h:86: InstantClient client_;
On 2013/01/11 19:43:05, samarth wrote:
> On 2013/01/10 21:08:03, dhollowa wrote:
> > On 2013/01/10 18:50:29, dhollowa wrote:
> > > I notice that all three InstantOverlay, InstantNTP, and InstantTab have
the
> > same
> > > state: client_, controller_, and supports_instant_.
> > > 
> > > This makes me think there is informal commonality that probably should be
> > > formalized.  Either by keeping this state in InstantClient::Delegate
> directly,
> > > or better yet, introduce a common concrete base-class, InstantPage say. 
It
> > > would look something like this:
> > > 
> > > class InstantPage : public InstantClient::Delegate {
> > >   ...
> > >  private:
> > >   InstantClient client_;
> > >   InstantController* controller_;
> > >   bool supports_instant_;
> > > }
> > > 
> > > class InstantOverlay : public InstantPage {...}
> > > class InstantNTP : public InstantNTP { ... }
> > > class InstantTab : public InstantTab { ... }
> > > 
> > > Thoughts?
> > 
> > Err... correction:
> > class InstantNTP : public InstantPage { ... }
> > class InstantTab : public InstantPage { ... }
> > 
> > (Thanks Thiago).
> 
> Channeling sky (who was channeling Ben):
> """
> Ben was griping today about a review he was looking at (not this one) and
> happened to say, 'god calls a kitten every time you subclass a concrete type.'
> It made me think of this patch.
> """
> Plus I'm hesistant to add yet another class here .. the object hierarchy here
is
> already a little tricky to follow.
> 
> That said, I agree that something about the repetitiveness between
> Instant{NTP,Overlay,Tab} smells wrong.  Likewise for some of the code
> duplication in InstantController that Jered pointed out.
> 
> Let me mull this over some more; feels like some more refactoring is in order
> here.

God also calls a kitten every time the DRY (don't repeat yourself) principle is
violated.  A possibly more acceptable alternate to a the concrete subclass is to
define a common data struct that any delegate needs to provide to *some*
concrete common methods declared and defined in the InstantClient::Delegate
class.  So then common code lives in the InstantClient::Delegate, but data it
acts upon lives in the subclasses of the delegate.

This way no kittens need to be harmed.

[samarth, 2013-01-14 16:55:32]

Ok, how about this? I ended up going with David's initial suggestion re
InstantPage but also ended up refactoring things some more to address the
repetitiveness.  Let me know if this makes thing clearer or just more
confusing...

Thanks,
Samarth

[Jered, 2013-01-14 18:22:33]

This seems a bit clearer though InstantPage isn't really doing much, and I think
it may be simpler to omit it in favor of default implementations or moving some
calls around.

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
File chrome/browser/instant/instant_api_wrapper.cc (right):

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_api_wrapper.cc:156: if (result) {
result -> supports_instant

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
File chrome/browser/instant/instant_api_wrapper.h (right):

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_api_wrapper.h:40: class Controller {
Calling this a controller feels a bit off. Some of the methods mutate state, but
most seem to be part of a Policy about whether to route messages. Perhaps this
can be made a Policy object?

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_api_wrapper.h:46: virtual bool supports_instant()
const = 0;
Consider just implementing these in the Wrapper, since I expect all implementors
will need it.

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_api_wrapper.h:50: virtual void OnUpdate() = 0;
Can whoever is calling Update() do this?

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:794: } else if
(ContentsFrom(ntp_.get(), contents)) {
The body of this else if block and the following else if block seem to be doing
similar things. Does it make sense to factor this out somehow?

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

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:152: // Called when the |ntp_| might
be stale. If it's actually state, the |ntp_|
state -> stale

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:271: // InstantClient::Delegate
interface. If non-NULL, the |overlay_| is
InstantClient -> InstantAPIWrapper

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:521: GURL url = NormalizeURL(params);
I think the inline block was clearer here, or NormalizeURL doesn't tell me
enough about what this is doing.

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:524: swapped_in_instant = instant &&
instant->MaybeSwapInInstantContents(
if (instant)
  swapped_in_instant = ...

https://codereview.chromium.org/11824050/diff/24001/content/public/common/url...
File content/public/common/url_constants.cc (right):

https://codereview.chromium.org/11824050/diff/24001/content/public/common/url...
content/public/common/url_constants.cc:46: const char kChromeUINewTabURL[] =
"chrome://newtab";
Isn't this already defined somewhere else?

[dhollowa, 2013-01-14 19:59:38]

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
File chrome/browser/instant/instant_api_wrapper.cc (right):

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_api_wrapper.cc:105:
IPC_MESSAGE_HANDLER(ChromeViewHostMsg_SetSuggestions, SetSuggestions)
We should change these handler names to OnSetSuggestions,
OnInstantSupportDetermined, etc.

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
File chrome/browser/instant/instant_api_wrapper.h (right):

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_api_wrapper.h:36: class InstantAPIWrapper :
public content::WebContentsObserver {
Not a fan of this name.  How about just InstantPage?  I agree with Jered, the
separate class InstantPage doesn't really do enough to warrant its existence. 
Best collapse {InstantAPIWrapper + InstantPage} => {InstantPage} pulling the
state/data-members in too.

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_api_wrapper.h:40: class Controller {
On 2013/01/14 18:22:34, Jered wrote:
> Calling this a controller feels a bit off. Some of the methods mutate state,
but
> most seem to be part of a Policy about whether to route messages. Perhaps this
> can be made a Policy object?

+1.  And this should not be pure-virtual, there should be a default
implementation for each policy.

The methods in the Policy should not have prefix "On" as this is typically used
for IPC handlers.  A "Should" prefix would be better.

I also favor keeping these "policy" methods as part of the InstantAPIWrapper
class itself (as protected methods) and not as an interface with pointers
floating around.

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_api_wrapper.h:70: class Receiver {
I preferred the original name "Delegate".

[samarth, 2013-01-15 00:01:40]

Hey Sreeram,

I realize that going through all the details of this change will take a while,
but can you take a look at the overall structure to see if it makes sense to
you?  I'd like to address David and Jered's comments but I'd like to first make
sure you won't make me rewrite all of this :)

Thanks,
Samarth

[sreeram, 2013-01-18 19:48:48]

On 2013/01/15 00:01:40, samarth wrote:
> I realize that going through all the details of this change will take a while,
> but can you take a look at the overall structure to see if it makes sense to
> you?  I'd like to address David and Jered's comments but I'd like to first
make
> sure you won't make me rewrite all of this :)

Sorry for the delay. The high level class structure is fine with me (in fact,
great job!), so please proceed addressing the David's/Jered's comments.

I'll review the CL in more detail over the long weekend and send my comments
too.

[samarth, 2013-01-22 15:59:06]

Collapsed InstantAPIWrapper and InstantPage into InstantPage as suggested. PTAL.

I'll update existing tests and add some new ones next.

Thanks,
Samarth

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
File chrome/browser/instant/instant_api_wrapper.cc (right):

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_api_wrapper.cc:105:
IPC_MESSAGE_HANDLER(ChromeViewHostMsg_SetSuggestions, SetSuggestions)
On 2013/01/14 19:59:39, dhollowa wrote:
> We should change these handler names to OnSetSuggestions,
> OnInstantSupportDetermined, etc.

Done.

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_api_wrapper.cc:156: if (result) {
On 2013/01/14 18:22:34, Jered wrote:
> result -> supports_instant

Done.

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
File chrome/browser/instant/instant_api_wrapper.h (right):

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_api_wrapper.h:36: class InstantAPIWrapper :
public content::WebContentsObserver {
On 2013/01/14 19:59:39, dhollowa wrote:
> Not a fan of this name.  How about just InstantPage?  I agree with Jered, the
> separate class InstantPage doesn't really do enough to warrant its existence. 
> Best collapse {InstantAPIWrapper + InstantPage} => {InstantPage} pulling the
> state/data-members in too.

Done.

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_api_wrapper.h:40: class Controller {
On 2013/01/14 19:59:39, dhollowa wrote:
> On 2013/01/14 18:22:34, Jered wrote:
> > Calling this a controller feels a bit off. Some of the methods mutate state,
> but
> > most seem to be part of a Policy about whether to route messages. Perhaps
this
> > can be made a Policy object?
> 
> +1.  And this should not be pure-virtual, there should be a default
> implementation for each policy.
> 
> The methods in the Policy should not have prefix "On" as this is typically
used
> for IPC handlers.  A "Should" prefix would be better.
> 
> I also favor keeping these "policy" methods as part of the InstantAPIWrapper
> class itself (as protected methods) and not as an interface with pointers
> floating around.

Done.

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_api_wrapper.h:46: virtual bool supports_instant()
const = 0;
On 2013/01/14 18:22:34, Jered wrote:
> Consider just implementing these in the Wrapper, since I expect all
implementors
> will need it.

Done.

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_api_wrapper.h:50: virtual void OnUpdate() = 0;
On 2013/01/14 18:22:34, Jered wrote:
> Can whoever is calling Update() do this?

Just overriding Update() now in InstantOverlay.  (I don't want to have
InstantController to have to remember to clear the history every time it wants
to call Update on an overlay.)

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_api_wrapper.h:70: class Receiver {
On 2013/01/14 19:59:39, dhollowa wrote:
> I preferred the original name "Delegate".

Done.

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:794: } else if
(ContentsFrom(ntp_.get(), contents)) {
On 2013/01/14 18:22:34, Jered wrote:
> The body of this else if block and the following else if block seem to be
doing
> similar things. Does it make sense to factor this out somehow?

They're different enough that I don't think it would help much.  Were you
thinking of something specific?

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

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:152: // Called when the |ntp_| might
be stale. If it's actually state, the |ntp_|
On 2013/01/14 18:22:34, Jered wrote:
> state -> stale

Done.

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:271: // InstantClient::Delegate
interface. If non-NULL, the |overlay_| is
On 2013/01/14 18:22:34, Jered wrote:
> InstantClient -> InstantAPIWrapper

Done.

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:521: GURL url = NormalizeURL(params);
On 2013/01/14 18:22:34, Jered wrote:
> I think the inline block was clearer here, or NormalizeURL doesn't tell me
> enough about what this is doing.

Yeah agreed.  I'd factored it out because I needed to call it twice the way I
had this originally, but doesn't make sense now.

https://codereview.chromium.org/11824050/diff/24001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:524: swapped_in_instant = instant &&
instant->MaybeSwapInInstantContents(
On 2013/01/14 18:22:34, Jered wrote:
> if (instant)
>   swapped_in_instant = ...

Done.

https://codereview.chromium.org/11824050/diff/24001/content/public/common/url...
File content/public/common/url_constants.cc (right):

https://codereview.chromium.org/11824050/diff/24001/content/public/common/url...
content/public/common/url_constants.cc:46: const char kChromeUINewTabURL[] =
"chrome://newtab";
On 2013/01/14 18:22:34, Jered wrote:
> Isn't this already defined somewhere else?

Only in chrome/..., and I believe content/ shouldn't depend on chrome/ (that's
why all these other constants are redefined here).

[sreeram, 2013-01-22 19:26:59]

I'm sending my comments piecemeal. Don't bother uploading new patch sets until
I'm done reviewing.

Here are comments on InstantPage:

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.cc (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.cc:123: if (is_main_frame)
if (is_main_frame && !supports_instant_)

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

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:29: 
Add these includes:
content/public/common/page_transition_types.h

and these declarations:
class GURL;

I can explain the principle behind it, if you wanna know.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:42: // the page loading or because we
recevied some other message.
"received"

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:55: // Called when the page has
suggestions. Usually in response to Change(),
Change -> Update

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:61: // Called when the page wants to be
shown. Usually in response to Change(),
Change -> Update

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:75: virtual void
StopCapturingKeyStrokes(content::WebContents* contents) = 0;
Make the comments on these two methods clearer. Talk about what it means to
"capture user key strokes". I.e., ostensibly the focus is in the |contents|, but
we'd like the omnibox to actually grab the keystrokes.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:83: virtual void OnStalePage(const
content::WebContents* contents) = 0;
This is too hacky for my taste. Figure out a cleaner way for InstantController
to receive this call (i.e., don't shim InstantLoader calls through
InstantPage::Delegate just out of convenience). Let InstantController implement
an additional interface if necessary.

There are bunch of other methods like this (InstantPageFocused,
InstantPageClicked, etc.) that also should be moved out.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:99: const content::WebContents* contents)
= 0;
Could you reorder these so that the three "open URL" methods are placed
together, and better explain the differences amongst them? E.g., NavigateToURL()
is an explicit request from the page to the browser to open a URL, sent through
the searchbox API, because the user chose a URL suggestion (since pages can't
navigate to some URLs, such as chrome:// on their own). I don't yet appreciate
why we need both InstantPageAboutToOpenURL() as well as
InstantPageAboutToNavigateMainFrame(), but I understand the former is needed to
process redirects properly.

@shishir: Can we get rid of  InstantPageAboutToNavigateMainFrame() and do
everything we need using just InstantPageAboutToOpenURL()?

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:120: virtual void Update(const string16&
text,
No need for this to be virtual, I think.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:138: void SetMarginSize(const int start,
const int end);
Remove the const. This falls into const-craziness territory.
void SetMarginSize(int start, int end);

Also, the comment for this method is very weak. It should explain what "margin"
means, and why it's different from the other methods or how it's used. I.e.,
more like the comment for SetPopupBounds() above.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:187: virtual bool
ShouldProcessNavigateToURL() const;
I'd remove "const" from these methods. In general, unless you have a very good
reason, don't force those overriding you to be const. They may want to update
some state.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:219: bool supports_instant_;
DISALLOW_COPY_AND_ASSIGN()?

[dhollowa, 2013-01-22 22:34:58]

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:398: if (!extended_enabled_ ||
!ntp_)
Should this be a DCHECK?  Should the caller be asking for ownership if no
contents exist?

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:763: delete
ntp_->ReleaseContents();
This deletion logic looks similar to :773, :789, :794, :401.  Opportunity for
smaller/common method maybe?

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

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:49: class InstantController : public
InstantPage::Delegate {
I'm a bit confused here...  The controller is a page-delegate but it sets itself
as the page-delegate for all three of instant-overlay, instant-ntp, and
instant-tab.  How does it know (or does it need to know) which of the three it
is responding too.  Is only on of the three "in effect" at a time?

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:81: content::WebContents*
ReleaseNTPContents();
This should use scoped_ptr<contents::WebContents> as the return type.  This
gives compiler support to ownership transfer.  See
https://mail.google.com/mail/u/0/?ui=2&shva=1#search/label%3A*cr-dev+scoped_p...
for details.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:152: // Overridden from
InstantPage::Delegate
nit: Should end with ":".

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:176: virtual void OnStalePage(const
content::WebContents* contents) OVERRIDE;
nit: Shouldn't begin with "On".

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:192: void OnStaleOverlay();
nit: Shouldn't begin with "On".

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:229: // with the given |height|.
nit: "...with the given |height| and |units|."

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:265: scoped_ptr<InstantOverlay>
overlay_;
It would be helpful to articulate the invariants between the |overlay_| the
|ntp_| and the |instant_tab_|.  I.e. What is expected to be NULL and when, any
mutual exclusivity?  And with these invariants are there opportunities to add
DCHECKs in the code?

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.cc (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:32: Delegate* delegate)
nit: looks like this will fit on previous line, no?

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:108: InstantLoader* loader_;
const ptr?

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:109: Delegate* delegate_;
const ptr?

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.h (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:33: virtual content::WebContents*
ReplaceAndReleaseContents(
Return type should be scoped_ptr<content::WebContents> to indicate transfer of
ownernship.  And I'm assuming |new_contents| is now owned by the delegate, yes? 
If so the it should be of type scoped_ptr<content::WebContents> too.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:46: virtual void OnPointerDown() = 0;
How about the terms "mouse down" / "mouse up"?  Or (less good) "pointer pressed"
/ "pointer released".

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:77: void
SetContents(content::WebContents* contents);
arg: scoped_ptr<WebContents> contents

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:93: scoped_ptr<WebContentsDelegateImpl>
contents_delegate_;
Can this be a direct data member instead of a scoped_ptr?

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
File chrome/browser/instant/instant_ntp.cc (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_ntp.cc:1: // Copyright 2012 The Chromium Authors.
All rights reserved.
nit: 2013

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_ntp.cc:13: const int kStalePageTimeoutMS = 3 *
3600 * 1000;  // 3 hours
DRY violation.  Move to common location in instant_page.h?

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
File chrome/browser/instant/instant_ntp.h (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_ntp.h:1: // Copyright 2012 The Chromium Authors.
All rights reserved.
nit: 2013

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_ntp.h:35: content::WebContents* ReleaseContents()
WARN_UNUSED_RESULT;
Return type should be scoped_ptr<content::WebContents>.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_ntp.h:62: scoped_ptr<InstantLoader> loader_;
Can this be a direct data member?

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
File chrome/browser/instant/instant_overlay.cc (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.cc:1: // Copyright 2012 The Chromium
Authors. All rights reserved.
nit: 2013

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.cc:13: const int kStalePageTimeoutMS = 3
* 3600 * 1000;  // 3 hours
DRY violation.  Move to instant_page.h?

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
File chrome/browser/instant/instant_overlay.h (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.h:1: // Copyright 2012 The Chromium
Authors. All rights reserved.
nit: 2013

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.h:86: scoped_ptr<InstantLoader> loader_;
Can this be a direct data member?

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.h (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.h:130: void
ReplaceWebContentsAt(int index, content::WebContents* new_contents);
scoped_ptr<WebContents> new_contents.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:233: if (params->url.is_empty()) {
nit: Slight preference for: if (!params...) \n return GURL();

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_delegate.cc (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/ui/search/...
chrome/browser/ui/search/search_delegate.cc:34: // TODO: ok that this gets
called with mode DEFAULT when new tab is created?
Can we answer this now?

https://codereview.chromium.org/11824050/diff/35002/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11824050/diff/35002/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1973: entry->GetVirtualURL()
== GURL(chrome::kChromeUINewTabURL)));
Should this not test whether the InstantExteded feature is enabled?

[Shishir, 2013-01-25 20:41:13]

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

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:99: const content::WebContents* contents)
= 0;
On 2013/01/22 19:26:59, sreeram wrote:
> Could you reorder these so that the three "open URL" methods are placed
> together, and better explain the differences amongst them? E.g.,
NavigateToURL()
> is an explicit request from the page to the browser to open a URL, sent
through
> the searchbox API, because the user chose a URL suggestion (since pages can't
> navigate to some URLs, such as chrome:// on their own). I don't yet appreciate
> why we need both InstantPageAboutToOpenURL() as well as
> InstantPageAboutToNavigateMainFrame(), but I understand the former is needed
to
> process redirects properly.
> 
> @shishir: Can we get rid of  InstantPageAboutToNavigateMainFrame() and do
> everything we need using just InstantPageAboutToOpenURL()?

In the present code there are two places where the InstantLoader detects that it
is trying to navigate. 

For same process navigations, the webContentsObserver's method
DidCommitProvisionalLoadForFrame is called which should call
InstantPageAboutToNavigateMainFrame.

The other code path is through the WebContentDelegate call to OpenURLFromTab
(for cross process navigations) which does not strictly belong here since
InstantPage is not a webContentsDelegate. So for eg. InstantTab will never get
that call. 

The calls are slightly different in the sense that in the first case, whether IC
commits the page or not, the navigation will happen. But in the second case, it
is upto the WebContentsDelegate to make the navigation happen.

I am not sure how the InstantNTP is supposed to behave for these cases.

[sreeram, 2013-01-25 20:47:07]

On 2013/01/25 20:41:13, Shishir wrote:
> The calls are slightly different in the sense that in the first case, whether
IC
> commits the page or not, the navigation will happen. But in the second case,
it
> is upto the WebContentsDelegate to make the navigation happen.
> 
> I am not sure how the InstantNTP is supposed to behave for these cases.

Thanks for the explanation!

[samarth, 2013-01-25 21:08:40]

David and Sreeram: addressed all your comments.  PTAL.

Thanks,
Samarth

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:398: if (!extended_enabled_ ||
!ntp_)
On 2013/01/22 22:34:58, dhollowa wrote:
> Should this be a DCHECK?  Should the caller be asking for ownership if no
> contents exist?

There are legitimate cases where this can happen (e.g. if GetInstantURL had
return false because of the blacklist).  I'm open to some other name besides
ReleaseNTPContents if you think it'll make this possibility more obvious (the
docs in the header do say the result might be NULL).

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:763: delete
ntp_->ReleaseContents();
On 2013/01/22 22:34:58, dhollowa wrote:
> This deletion logic looks similar to :773, :789, :794, :401.  Opportunity for
> smaller/common method maybe?

Done.

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

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:49: class InstantController : public
InstantPage::Delegate {
On 2013/01/22 22:34:58, dhollowa wrote:
> I'm a bit confused here...  The controller is a page-delegate but it sets
itself
> as the page-delegate for all three of instant-overlay, instant-ntp, and
> instant-tab.  How does it know (or does it need to know) which of the three it
> is responding too.  Is only on of the three "in effect" at a time?

It's responding (potentially) to all three.  That's the reason we send up the
contents in each response, so InstantController can tell who the message came
from.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:81: content::WebContents*
ReleaseNTPContents();
On 2013/01/22 22:34:58, dhollowa wrote:
> This should use scoped_ptr<contents::WebContents> as the return type.  This
> gives compiler support to ownership transfer.  See
>
https://mail.google.com/mail/u/0/?ui=2&shva=1#search/label%253A*cr-dev+scoped...
> for details.

Ah, didn't know about this.  This is really nice, much better than all the
comment sprinkled about!

Changed to use scoped_ptrs wherever it made sense in this CL and added some
comments for future cleanup in tangentially related code.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:152: // Overridden from
InstantPage::Delegate
On 2013/01/22 22:34:58, dhollowa wrote:
> nit: Should end with ":".

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:176: virtual void OnStalePage(const
content::WebContents* contents) OVERRIDE;
On 2013/01/22 22:34:58, dhollowa wrote:
> nit: Shouldn't begin with "On".

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:192: void OnStaleOverlay();
On 2013/01/22 22:34:58, dhollowa wrote:
> nit: Shouldn't begin with "On".

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:229: // with the given |height|.
On 2013/01/22 22:34:58, dhollowa wrote:
> nit: "...with the given |height| and |units|."

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:265: scoped_ptr<InstantOverlay>
overlay_;
On 2013/01/22 22:34:58, dhollowa wrote:
> It would be helpful to articulate the invariants between the |overlay_| the
> |ntp_| and the |instant_tab_|.  I.e. What is expected to be NULL and when, any
> mutual exclusivity?  And with these invariants are there opportunities to add
> DCHECKs in the code?

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.cc (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:32: Delegate* delegate)
On 2013/01/22 22:34:58, dhollowa wrote:
> nit: looks like this will fit on previous line, no?

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:108: InstantLoader* loader_;
On 2013/01/22 22:34:58, dhollowa wrote:
> const ptr?

Not relevant anymore.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:109: Delegate* delegate_;
On 2013/01/22 22:34:58, dhollowa wrote:
> const ptr?

Not relevant anymore.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.h (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:33: virtual content::WebContents*
ReplaceAndReleaseContents(
On 2013/01/22 22:34:58, dhollowa wrote:
> Return type should be scoped_ptr<content::WebContents> to indicate transfer of
> ownernship.  And I'm assuming |new_contents| is now owned by the delegate,
yes? 
> If so the it should be of type scoped_ptr<content::WebContents> too.
> 

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:46: virtual void OnPointerDown() = 0;
On 2013/01/22 22:34:58, dhollowa wrote:
> How about the terms "mouse down" / "mouse up"?  Or (less good) "pointer
pressed"
> / "pointer released".

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:77: void
SetContents(content::WebContents* contents);
On 2013/01/22 22:34:58, dhollowa wrote:
> arg: scoped_ptr<WebContents> contents

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:93: scoped_ptr<WebContentsDelegateImpl>
contents_delegate_;
On 2013/01/22 22:34:58, dhollowa wrote:
> Can this be a direct data member instead of a scoped_ptr?

Got rid of this entirely and just folded it into InstantLoader.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
File chrome/browser/instant/instant_ntp.cc (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_ntp.cc:1: // Copyright 2012 The Chromium Authors.
All rights reserved.
On 2013/01/22 22:34:58, dhollowa wrote:
> nit: 2013

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_ntp.cc:13: const int kStalePageTimeoutMS = 3 *
3600 * 1000;  // 3 hours
On 2013/01/22 22:34:58, dhollowa wrote:
> DRY violation.  Move to common location in instant_page.h?

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
File chrome/browser/instant/instant_ntp.h (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_ntp.h:1: // Copyright 2012 The Chromium Authors.
All rights reserved.
On 2013/01/22 22:34:58, dhollowa wrote:
> nit: 2013

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_ntp.h:35: content::WebContents* ReleaseContents()
WARN_UNUSED_RESULT;
On 2013/01/22 22:34:58, dhollowa wrote:
> Return type should be scoped_ptr<content::WebContents>.

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_ntp.h:62: scoped_ptr<InstantLoader> loader_;
On 2013/01/22 22:34:58, dhollowa wrote:
> Can this be a direct data member?

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
File chrome/browser/instant/instant_overlay.cc (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.cc:13: const int kStalePageTimeoutMS = 3
* 3600 * 1000;  // 3 hours
On 2013/01/22 22:34:58, dhollowa wrote:
> DRY violation.  Move to instant_page.h?

I was leaving open the possibility that ntp and overlay could have different
timeouts.  But we can do this if/when that ever happens.  Moved to
InstantLoader.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
File chrome/browser/instant/instant_overlay.h (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.h:1: // Copyright 2012 The Chromium
Authors. All rights reserved.
On 2013/01/22 22:34:58, dhollowa wrote:
> nit: 2013

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.h:86: scoped_ptr<InstantLoader> loader_;
On 2013/01/22 22:34:58, dhollowa wrote:
> Can this be a direct data member?

It couldn't before because of the forward-declared WebContentsDelegateImpl in
InstantLoader.  I've now folded that into InstantLoader itself, so this can be a
direct member now.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.cc (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.cc:123: if (is_main_frame)
On 2013/01/22 19:26:59, sreeram wrote:
> if (is_main_frame && !supports_instant_)

Done.

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

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:29: 
On 2013/01/22 19:26:59, sreeram wrote:
> Add these includes:
> content/public/common/page_transition_types.h
> 
> and these declarations:
> class GURL;
> 
> I can explain the principle behind it, if you wanna know.

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:42: // the page loading or because we
recevied some other message.
On 2013/01/22 19:26:59, sreeram wrote:
> "received"

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:55: // Called when the page has
suggestions. Usually in response to Change(),
On 2013/01/22 19:26:59, sreeram wrote:
> Change -> Update

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:61: // Called when the page wants to be
shown. Usually in response to Change(),
On 2013/01/22 19:26:59, sreeram wrote:
> Change -> Update

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:75: virtual void
StopCapturingKeyStrokes(content::WebContents* contents) = 0;
On 2013/01/22 19:26:59, sreeram wrote:
> Make the comments on these two methods clearer. Talk about what it means to
> "capture user key strokes". I.e., ostensibly the focus is in the |contents|,
but
> we'd like the omnibox to actually grab the keystrokes.

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:83: virtual void OnStalePage(const
content::WebContents* contents) = 0;
On 2013/01/22 19:26:59, sreeram wrote:
> This is too hacky for my taste. Figure out a cleaner way for InstantController
> to receive this call (i.e., don't shim InstantLoader calls through
> InstantPage::Delegate just out of convenience). Let InstantController
implement
> an additional interface if necessary.
> 
> There are bunch of other methods like this (InstantPageFocused,
> InstantPageClicked, etc.) that also should be moved out.

You're right.  Got rid of the InstantLoader related callbacks from here.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:99: const content::WebContents* contents)
= 0;
On 2013/01/22 19:26:59, sreeram wrote:
> Could you reorder these so that the three "open URL" methods are placed
> together, and better explain the differences amongst them? E.g.,
NavigateToURL()
> is an explicit request from the page to the browser to open a URL, sent
through
> the searchbox API, because the user chose a URL suggestion (since pages can't
> navigate to some URLs, such as chrome:// on their own). I don't yet appreciate
> why we need both InstantPageAboutToOpenURL() as well as
> InstantPageAboutToNavigateMainFrame(), but I understand the former is needed
to
> process redirects properly.

Added more comments for NavigateToURL.  Got rid of the third.
 
> @shishir: Can we get rid of  InstantPageAboutToNavigateMainFrame() and do
> everything we need using just InstantPageAboutToOpenURL()?

Emailed shishir about this.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:120: virtual void Update(const string16&
text,
On 2013/01/22 19:26:59, sreeram wrote:
> No need for this to be virtual, I think.

This is virtual to allow InstantOverlay to override it.  It needs to also update
last_navigation_ each time update is called.  Two other alternatives:
(1) Force InstantController to do this manually.  Seems like it could break
easily.
(2) Add a OnBeforeUpdate() or similar callback.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:138: void SetMarginSize(const int start,
const int end);
On 2013/01/22 19:26:59, sreeram wrote:
> Remove the const. This falls into const-craziness territory.
> void SetMarginSize(int start, int end);

Agree that these consts are useless.  Removed.

 
> Also, the comment for this method is very weak. It should explain what
"margin"
> means, and why it's different from the other methods or how it's used. I.e.,
> more like the comment for SetPopupBounds() above.

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:187: virtual bool
ShouldProcessNavigateToURL() const;
On 2013/01/22 19:26:59, sreeram wrote:
> I'd remove "const" from these methods. In general, unless you have a very good
> reason, don't force those overriding you to be const. They may want to update
> some state.

I wouldn't expect a "bool ShouldProcess..." method to alter any state.  I argue
that if we do need to do so in the future, we should rename the method (and then
make it non-const).

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:219: bool supports_instant_;
On 2013/01/22 19:26:59, sreeram wrote:
> DISALLOW_COPY_AND_ASSIGN()?

Is it necessary here?  I'm adding them to the subclasses and you can't
instantiate this class directly.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.h (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.h:130: void
ReplaceWebContentsAt(int index, content::WebContents* new_contents);
On 2013/01/22 22:34:58, dhollowa wrote:
> scoped_ptr<WebContents> new_contents.

Done.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:233: if (params->url.is_empty()) {
On 2013/01/22 22:34:58, dhollowa wrote:
> nit: Slight preference for: if (!params...) \n return GURL();

Not relevant anymore.

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_delegate.cc (right):

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/ui/search/...
chrome/browser/ui/search/search_delegate.cc:34: // TODO: ok that this gets
called with mode DEFAULT when new tab is created?
On 2013/01/22 22:34:58, dhollowa wrote:
> Can we answer this now?

Yeah, this was stray comment from my early debugging.

https://codereview.chromium.org/11824050/diff/35002/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11824050/diff/35002/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1973: entry->GetVirtualURL()
== GURL(chrome::kChromeUINewTabURL)));
On 2013/01/22 22:34:58, dhollowa wrote:
> Should this not test whether the InstantExteded feature is enabled?

Wouldn't that break the content/chrome separation? This is a no-op for 1993:
currently, checking the URL is not necessary because the NTP uses WebUI which
can set the focus bit on itself.

[melevin, 2013-01-25 21:44:03]

Can you sync? Your patch doesn't apply at head.

[samarth, 2013-01-25 22:53:10]

On 2013/01/25 21:44:03, melevin wrote:
> Can you sync? Your patch doesn't apply at head.

OK, just uploaded a rebased patch.

[samarth, 2013-01-28 01:09:59]

FYI, just fixed existing tests and added some new ones for the committed NTP.

Thanks,
Samarth

[dhollowa, 2013-01-29 02:37:53]

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

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:219: bool supports_instant_;
On 2013/01/25 21:08:40, samarth wrote:
> On 2013/01/22 19:26:59, sreeram wrote:
> > DISALLOW_COPY_AND_ASSIGN()?
> 
> Is it necessary here?  I'm adding them to the subclasses and you can't
> instantiate this class directly.

The style guide is pretty clear on this point.  Always required unless your
class is meant to be copy/assignable.

https://codereview.chromium.org/11824050/diff/35002/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11824050/diff/35002/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1973: entry->GetVirtualURL()
== GURL(chrome::kChromeUINewTabURL)));
On 2013/01/25 21:08:40, samarth wrote:
> On 2013/01/22 22:34:58, dhollowa wrote:
> > Should this not test whether the InstantExteded feature is enabled?
> 
> Wouldn't that break the content/chrome separation? This is a no-op for 1993:
> currently, checking the URL is not necessary because the NTP uses WebUI which
> can set the focus bit on itself.

Ya, ok.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:160: bool ContentsFrom(const
InstantPage* page,
nit: IsContentsFrom

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:405: if (!extended_enabled_ ||
!ntp_)
Seems like the caller is in error if they try to release the ntp_ with
old-instant.  Shouldn't this be a DCHECK?

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:829:
content::NotificationService::current()->Notify(
This doesn't indicate *which* page's Instant support was determined.  So a
listener to this may get confused.  And listeners should be changed to make sure
explicitly that they're hearing about the one they care about.  I haven't looked
to see who/where all the listeners are but I'm guessing this is potential
trouble.

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

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:52: // maintains a WebContents used
to show search suggestions and results in an
The wording makes it sound like there are two web-contents of interest where
there is really three.  Could this be reworded so that the core job of
coordinating the three "pages" is described?  And maybe how each page is backed
by a web-contents (owned by Instant or not).

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:144: void SwappedOverlayContents();
To keep terminology straight, how about renaming
InstantOverlay::ReplaceAndReleaseContents to InstantOverlay::SwapContents?

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:152: void HandleStaleOverlay();
This is not always about being stale (in the temporal sense) it may be due to
focus or visibility changes.  So how about we rename this OverlayMayBeChanged()?
 (Better names welcome...).

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:220: // Recreates the |overlay_|
with |instant_url|. The caller should ensure that
nit: This comment is overly specific - it can just say that |overlay_| is
deleted.  There are many things the caller should not do with deleted objects. 
;-)

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:257: bool GetInstantURL(Profile*
profile,
nit: This will fit on one line now.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:97:
EXPECT_TRUE(instant()->ntp_.get());
ASSERT_NE(static_cast<InstantNTP>(NULL), instant()->ntp_.get())
since the next line will crash if NULL.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:108:
EXPECT_TRUE(instant()->ntp_.get());
ASSERT_NE(static_cast<InstantNTP>(NULL), ...

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:129:
EXPECT_TRUE(instant()->ntp_.get());
ASSERT_NE(static_cast<InstantNTP>(NULL), ...

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:187:
EXPECT_TRUE(preview_tab);
ASSERT_NE(static_cast<InstantNTP>(NULL), ...?  Will next line crash/assert if
NULL?

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:213:
FocusOmniboxAndWaitForInstantSupport();
As we discussed, this is most likely racey because the
INSTANT_SUPPORT_DETERMINED stuff fires for the preview contents but the ntp
contents may not yet be ready.  Which causes flake below at :223.

This may be true of other tests above as well.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.cc (right):

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:52: create_params.site_instance =
content::SiteInstance::CreateForURL(
I believe this is ok, but please double-check with creis@ to confirm.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:90:
CoreTabHelper::FromWebContents(contents())->
nit: This should fit on one line.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:114: &contents_->GetController()));
nit: Indent

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:164:
ignore_result(released_contents.release());
I'm puzzled... is released_contents == old_contents == contents() ?

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:185: bool
InstantLoader::CanDownload(content::RenderViewHost* /* render_view_host */,
nit: No need for the commented params.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.h (right):

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:32: // contents currently held is
released and returned.
The phrase "...is released and returned." is a bit ambiguous.  How about "...is
returned with ownership transferring to the caller."?

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:55: // Does not take ownership of
|delegate|.
It is rare for a class to own its delegate.  So this may be redundant.  Your
choice.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
File chrome/browser/instant/instant_tab.h (right):

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_tab.h:25: // Overridden from InstantPage.
nit: ":"

[samarth, 2013-01-29 05:42:01]

creis@: Can you take a look at InstantLoader::Load to make sure I'm getting a
SiteInstance for instant correctly?

sky@: please review
chrome/browser/ui
chrome/browser/history

jam@: please review
content/...

Thanks,
Samarth

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

https://codereview.chromium.org/11824050/diff/35002/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:219: bool supports_instant_;
On 2013/01/29 02:37:53, dhollowa wrote:
> On 2013/01/25 21:08:40, samarth wrote:
> > On 2013/01/22 19:26:59, sreeram wrote:
> > > DISALLOW_COPY_AND_ASSIGN()?
> > 
> > Is it necessary here?  I'm adding them to the subclasses and you can't
> > instantiate this class directly.
> 
> The style guide is pretty clear on this point.  Always required unless your
> class is meant to be copy/assignable.

Done.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:160: bool ContentsFrom(const
InstantPage* page,
On 2013/01/29 02:37:53, dhollowa wrote:
> nit: IsContentsFrom

Done.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:405: if (!extended_enabled_ ||
!ntp_)
On 2013/01/29 02:37:53, dhollowa wrote:
> Seems like the caller is in error if they try to release the ntp_ with
> old-instant.  Shouldn't this be a DCHECK?

Done.  Note that as I mentioned earlier, ntp_ may be NULL for good reasons (e.g.
if the instant url is blacklisted).

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:829:
content::NotificationService::current()->Notify(
On 2013/01/29 02:37:53, dhollowa wrote:
> This doesn't indicate *which* page's Instant support was determined.  So a
> listener to this may get confused.  And listeners should be changed to make
sure
> explicitly that they're hearing about the one they care about.  I haven't
looked
> to see who/where all the listeners are but I'm guessing this is potential
> trouble.

Luckily it turns out the only user of this is the test.  Changed this so we send
a different notification in the two cases.  (Alternatively, I can add details in
the notification but that seems a bit more convoluted.)

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

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:52: // maintains a WebContents used
to show search suggestions and results in an
On 2013/01/29 02:37:53, dhollowa wrote:
> The wording makes it sound like there are two web-contents of interest where
> there is really three.  Could this be reworded so that the core job of
> coordinating the three "pages" is described?  And maybe how each page is
backed
> by a web-contents (owned by Instant or not).

Moved some of the comments from below into here.  Better?

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:144: void SwappedOverlayContents();
On 2013/01/29 02:37:53, dhollowa wrote:
> To keep terminology straight, how about renaming
> InstantOverlay::ReplaceAndReleaseContents to InstantOverlay::SwapContents?

Done.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:152: void HandleStaleOverlay();
On 2013/01/29 02:37:53, dhollowa wrote:
> This is not always about being stale (in the temporal sense) it may be due to
> focus or visibility changes.  So how about we rename this
OverlayMayBeChanged()?
>  (Better names welcome...).

How about ReloadOverlayIfStale?

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:220: // Recreates the |overlay_|
with |instant_url|. The caller should ensure that
On 2013/01/29 02:37:53, dhollowa wrote:
> nit: This comment is overly specific - it can just say that |overlay_| is
> deleted.  There are many things the caller should not do with deleted objects.

> ;-)

Done.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:257: bool GetInstantURL(Profile*
profile,
On 2013/01/29 02:37:53, dhollowa wrote:
> nit: This will fit on one line now.

Done.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:97:
EXPECT_TRUE(instant()->ntp_.get());
On 2013/01/29 02:37:53, dhollowa wrote:
> ASSERT_NE(static_cast<InstantNTP>(NULL), instant()->ntp_.get())
> since the next line will crash if NULL.

Done.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:108:
EXPECT_TRUE(instant()->ntp_.get());
On 2013/01/29 02:37:53, dhollowa wrote:
> ASSERT_NE(static_cast<InstantNTP>(NULL), ...

Done.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:129:
EXPECT_TRUE(instant()->ntp_.get());
On 2013/01/29 02:37:53, dhollowa wrote:
> ASSERT_NE(static_cast<InstantNTP>(NULL), ...

Done.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:187:
EXPECT_TRUE(preview_tab);
On 2013/01/29 02:37:53, dhollowa wrote:
> ASSERT_NE(static_cast<InstantNTP>(NULL), ...?  Will next line crash/assert if
> NULL?

No, it won't.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:213:
FocusOmniboxAndWaitForInstantSupport();
On 2013/01/29 02:37:53, dhollowa wrote:
> As we discussed, this is most likely racey because the
> INSTANT_SUPPORT_DETERMINED stuff fires for the preview contents but the ntp
> contents may not yet be ready.  Which causes flake below at :223.
> 
> This may be true of other tests above as well.

Done.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.cc (right):

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:52: create_params.site_instance =
content::SiteInstance::CreateForURL(
On 2013/01/29 02:37:53, dhollowa wrote:
> I believe this is ok, but please double-check with creis@ to confirm.

Ok, will do.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:90:
CoreTabHelper::FromWebContents(contents())->
On 2013/01/29 02:37:53, dhollowa wrote:
> nit: This should fit on one line.

Done.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:114: &contents_->GetController()));
On 2013/01/29 02:37:53, dhollowa wrote:
> nit: Indent

Done.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:164:
ignore_result(released_contents.release());
On 2013/01/29 02:37:53, dhollowa wrote:
> I'm puzzled... is released_contents == old_contents == contents() ?

Yes.  I could add another DCHECK if it would make it clearer.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:185: bool
InstantLoader::CanDownload(content::RenderViewHost* /* render_view_host */,
On 2013/01/29 02:37:53, dhollowa wrote:
> nit: No need for the commented params.

Done.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.h (right):

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:32: // contents currently held is
released and returned.
On 2013/01/29 02:37:53, dhollowa wrote:
> The phrase "...is released and returned." is a bit ambiguous.  How about
"...is
> returned with ownership transferring to the caller."?

Done.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:55: // Does not take ownership of
|delegate|.
On 2013/01/29 02:37:53, dhollowa wrote:
> It is rare for a class to own its delegate.  So this may be redundant.  Your
> choice.

Oh yeah, meant to remove all of these after switching over to scoped_ptrs.
Missed this one.  Done.

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
File chrome/browser/instant/instant_tab.h (right):

https://codereview.chromium.org/11824050/diff/55001/chrome/browser/instant/in...
chrome/browser/instant/instant_tab.h:25: // Overridden from InstantPage.
On 2013/01/29 02:37:53, dhollowa wrote:
> nit: ":"

Done.

[Charlie Reis, 2013-01-29 07:30:55]

At first glance, the creation of SiteInstances in InstantLoader::Load looks
right.  I'd like to take a closer look over the whole CL if possible (given the
description), though I may not be able to get to it until later in the week. 
Just let me know if I'm holding things up.

[sreeram, 2013-01-29 13:35:42]

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.cc (right):

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:61: DVLOG(1) << "LoadURL: " <<
instant_url.spec();
No need for the ".spec()". GURL provides an overload for operator<< that calls
possibly_invalid_spec().

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:120: // Undo tab helper work done in
SetupPreviewContents().
SetupPreviewContents() -> SetContents().

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:143: const
content::NotificationDetails& details) {
Comment out source and details.

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:165: // Overridden from
content::WebContentsDelegate:
No need for this comment.

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:167: // Any message shown during
Instant cancels Instant, so we suppress them.
Any message -> Messages
(to match the "them" and ...Dialogs())

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:179: void
InstantLoader::WebContentsFocused(content::WebContents* contents) {
/* contents */

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:185: const std::string&) {
Provide the names of the parameters, but comment them out.

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:213: bool
InstantLoader::OnGoToEntryOffset(int offset) {
/* offset */

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:221: return
contents_->GetDelegate()->OpenURLFromTab(source, params);
Isn't this an infinite recursion? contents_->GetDelegate() == this, no?

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.h (right):

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:18: class Profile;
Add forward declarations for content::WebContents and base::Closure.

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:34: virtual
scoped_ptr<content::WebContents> SwapContents(
This is confusing (comment as well as the logic of having the Delegate return
the old_contents to us, which we already have). I think the following is
simpler:

instant_loader.h:
// Called when someone else (such as prerendering) swaps a different contents
into us.
virtual void SwappedContents() = 0;

(Note the name. The delegate doesn't have to perform the swap. I'm okay with
OnSwappedContents() as well, to match the style of the other methods.)

instant_loader.cc:
void InstantLoader::SwapTabContents(old_contents, new_contents) {
  DCHECK_EQ(old_contents, contents());
  ignore_result(ReleaseContents());  // Caller is responsible for deleting.
  SetContents(new_contents);
  delegate_->SwappedContents();
}

instant_overlay.cc:
void InstantOverlay::SwappedContents() {
  SetContents(loader_.contents());
  contents()->SetUserData(&kUserDataKey, new InstantOverlayUserData(this));
  instant_controller()->SwappedOverlayContents();
}

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:44: virtual bool OnOpenURL() = 0;
Given the comment, this is better named "ShouldOpenURL".

I'd also make a slight change in the comment wording: "If the Delegate returns
true, we try to ..."

Also, how is this expected to work? Isn't the InstantLoader itself the delegate
of |contents_|?

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.cc (right):

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_page.cc:146: void
InstantPage::RenderViewGone(base::TerminationStatus status) {
/* status */

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_page.cc:156: content::RenderViewHost*
render_view_host) {
Comment out names that are unused.

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_page.cc:189: size_t omnibox_font_size =
omnibox_font.GetFontSize();
Note: Shishir is fixing this in https://codereview.chromium.org/11833043/. So,
if you submit after him, remember to merge his fix.

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

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:31: class GURL;
Chrome style is to put unnamespaced declarations first, so this should go on
line 16. (I can't seem to find the style guide reference for this, though if you
look at existing .h files, you'll see this style is prevalent.)

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:45: // the page loading or because we
receivied some other message.
receivied -> received

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:177: virtual bool
ShouldProcessNavigateToURL() const;
On 2013/01/25 21:08:40, samarth wrote:
> On 2013/01/22 19:26:59, sreeram wrote:
> > I'd remove "const" from these methods. In general, unless you have a very
good
> > reason, don't force those overriding you to be const. They may want to
update
> > some state.
> 
> I wouldn't expect a "bool ShouldProcess..." method to alter any state.  I
argue
> that if we do need to do so in the future, we should rename the method (and
then
> make it non-const).

No, there's nothing inherently const about "ShouldProcess...". The subclass may
need to alter some state in response. For example, before @jam changed the API
in http://crrev.com/158725, instant_loader.cc overrode
WebContentsDelegate::ShouldAddNavigationToHistory() and did exactly that. It
stored the argument in local state and returned false. We wouldn't have been
able to do that (or would've been forced to use "mutable") if the
WebContentsDelegate authors had forced constness on us.

[sreeram, 2013-01-29 17:33:24]

Reference for the "comment out unused variable names":
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

[dhollowa, 2013-01-29 17:45:03]

On 2013/01/29 17:33:24, sreeram wrote:
> Reference for the "comment out unused variable names":
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

FTR... grep "int \/\*" tells a different story...  Almost no use of this
particular style rule in the Chromium sources except for with IPC message decls.
 So "local style" may trump google-style in this case.

[jam, 2013-01-29 17:51:54]

https://codereview.chromium.org/11824050/diff/62001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11824050/diff/62001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1990: entry->GetVirtualURL()
== GURL(chrome::kChromeUINewTabURL)));
i dont understand why this change is even needed?

kchromeuinewtaburl should mean that this webcontents is displaying the new
tabui, and so

https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/webui/n...

should ensure that ShouldFocusLocationBarByDefault returns true?

https://codereview.chromium.org/11824050/diff/62001/content/public/common/url...
File content/public/common/url_constants.h (right):

https://codereview.chromium.org/11824050/diff/62001/content/public/common/url...
content/public/common/url_constants.h:59: CONTENT_EXPORT extern const char
kChromeUINewTabURL[];
content layer should not know about chrome layer's features, i.e. new tab page

[samarth, 2013-01-31 16:24:57]

Thanks for the reviews.  PTAL.

David/Sreeram: I went with the commented-out params.  That's what the style
guide says and it's how things were already in instant/ (so if anything that
local style trumps chrome's local style :)).

Charlie: please feel free to take a look at the overall CL when you get a
chance.  I'm still waiting for Scott to review so you're not holding me up right
now, but I'd appreciate a look soon since this change is blocking a lot of
fixes.

I'll also sync and upload a rebased patch.

Thanks,
Samarth

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.cc (right):

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:61: DVLOG(1) << "LoadURL: " <<
instant_url.spec();
On 2013/01/29 13:35:42, sreeram wrote:
> No need for the ".spec()". GURL provides an overload for operator<< that calls
> possibly_invalid_spec().

Done.

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:120: // Undo tab helper work done in
SetupPreviewContents().
On 2013/01/29 13:35:42, sreeram wrote:
> SetupPreviewContents() -> SetContents().

Done.

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:143: const
content::NotificationDetails& details) {
On 2013/01/29 13:35:42, sreeram wrote:
> Comment out source and details.

Done.

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:165: // Overridden from
content::WebContentsDelegate:
On 2013/01/29 13:35:42, sreeram wrote:
> No need for this comment.

Done.

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:167: // Any message shown during
Instant cancels Instant, so we suppress them.
On 2013/01/29 13:35:42, sreeram wrote:
> Any message -> Messages
> (to match the "them" and ...Dialogs())

Done.

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:179: void
InstantLoader::WebContentsFocused(content::WebContents* contents) {
On 2013/01/29 13:35:42, sreeram wrote:
> /* contents */

Done.

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:185: const std::string&) {
On 2013/01/29 13:35:42, sreeram wrote:
> Provide the names of the parameters, but comment them out.

Done.

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:213: bool
InstantLoader::OnGoToEntryOffset(int offset) {
On 2013/01/29 13:35:42, sreeram wrote:
> /* offset */

Done.

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:221: return
contents_->GetDelegate()->OpenURLFromTab(source, params);
On 2013/01/29 13:35:42, sreeram wrote:
> Isn't this an infinite recursion? contents_->GetDelegate() == this, no?

See my comments in the header about this.  (There was a bug here though:
contents_ would no longer be non-NULL since OnOpenURL could commit the page.)

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.h (right):

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:18: class Profile;
On 2013/01/29 13:35:42, sreeram wrote:
> Add forward declarations for content::WebContents and base::Closure.

Done.

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:34: virtual
scoped_ptr<content::WebContents> SwapContents(
On 2013/01/29 13:35:42, sreeram wrote:
> This is confusing (comment as well as the logic of having the Delegate return
> the old_contents to us, which we already have). I think the following is
> simpler:
> 
> instant_loader.h:
> // Called when someone else (such as prerendering) swaps a different contents
> into us.
> virtual void SwappedContents() = 0;
> 
> (Note the name. The delegate doesn't have to perform the swap. I'm okay with
> OnSwappedContents() as well, to match the style of the other methods.)
> 
> instant_loader.cc:
> void InstantLoader::SwapTabContents(old_contents, new_contents) {
>   DCHECK_EQ(old_contents, contents());
>   ignore_result(ReleaseContents());  // Caller is responsible for deleting.
>   SetContents(new_contents);
>   delegate_->SwappedContents();
> }
> 
> instant_overlay.cc:
> void InstantOverlay::SwappedContents() {
>   SetContents(loader_.contents());
>   contents()->SetUserData(&kUserDataKey, new InstantOverlayUserData(this));
>   instant_controller()->SwappedOverlayContents();
> }

Done.

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:44: virtual bool OnOpenURL() = 0;
On 2013/01/29 13:35:42, sreeram wrote:
> Given the comment, this is better named "ShouldOpenURL".
> 
> I'd also make a slight change in the comment wording: "If the Delegate returns
> true, we try to ..."
> 
> Also, how is this expected to work? Isn't the InstantLoader itself the
delegate
> of |contents_|?

It works because we only allow the navigation if the page is committed (meaning
the contents delegate is now the tab strip).  I tried to make the code clearer
by moving that logic in one place (to InstantOverlay) and changed this to be
OpenURLFromTab().

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.cc (right):

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_page.cc:146: void
InstantPage::RenderViewGone(base::TerminationStatus status) {
On 2013/01/29 13:35:42, sreeram wrote:
> /* status */

Done.

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_page.cc:156: content::RenderViewHost*
render_view_host) {
On 2013/01/29 13:35:42, sreeram wrote:
> Comment out names that are unused.

Done.

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_page.cc:189: size_t omnibox_font_size =
omnibox_font.GetFontSize();
On 2013/01/29 13:35:42, sreeram wrote:
> Note: Shishir is fixing this in https://codereview.chromium.org/11833043/. So,
> if you submit after him, remember to merge his fix.

Noted.

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

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:31: class GURL;
On 2013/01/29 13:35:42, sreeram wrote:
> Chrome style is to put unnamespaced declarations first, so this should go on
> line 16. (I can't seem to find the style guide reference for this, though if
you
> look at existing .h files, you'll see this style is prevalent.)

Done.

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:45: // the page loading or because we
receivied some other message.
On 2013/01/29 13:35:42, sreeram wrote:
> receivied -> received

Done.

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:177: virtual bool
ShouldProcessNavigateToURL() const;
On 2013/01/29 13:35:42, sreeram wrote:
> On 2013/01/25 21:08:40, samarth wrote:
> > On 2013/01/22 19:26:59, sreeram wrote:
> > > I'd remove "const" from these methods. In general, unless you have a very
> good
> > > reason, don't force those overriding you to be const. They may want to
> update
> > > some state.
> > 
> > I wouldn't expect a "bool ShouldProcess..." method to alter any state.  I
> argue
> > that if we do need to do so in the future, we should rename the method (and
> then
> > make it non-const).
> 
> No, there's nothing inherently const about "ShouldProcess...". The subclass
may
> need to alter some state in response. For example, before @jam changed the API
> in http://crrev.com/158725, instant_loader.cc overrode
> WebContentsDelegate::ShouldAddNavigationToHistory() and did exactly that. It
> stored the argument in local state and returned false. We wouldn't have been
> able to do that (or would've been forced to use "mutable") if the
> WebContentsDelegate authors had forced constness on us.

Ok, done.

https://codereview.chromium.org/11824050/diff/62001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11824050/diff/62001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1990: entry->GetVirtualURL()
== GURL(chrome::kChromeUINewTabURL)));
On 2013/01/29 17:51:55, jam wrote:
> i dont understand why this change is even needed?
> 
> kchromeuinewtaburl should mean that this webcontents is displaying the new
> tabui, and so
> 
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/webui/n...
> 
> should ensure that ShouldFocusLocationBarByDefault returns true?

It's because with Instant Extended, the new tab page is no longer necessarily a
WebUI.  Instead, with this change, we're swapping in the instant web contents
when you navigate to the new tab page.

https://codereview.chromium.org/11824050/diff/62001/content/public/common/url...
File content/public/common/url_constants.h (right):

https://codereview.chromium.org/11824050/diff/62001/content/public/common/url...
content/public/common/url_constants.h:59: CONTENT_EXPORT extern const char
kChromeUINewTabURL[];
On 2013/01/29 17:51:55, jam wrote:
> content layer should not know about chrome layer's features, i.e. new tab page

Do you have other suggestions for how best to communicate to the content layer
that a particular contents should have FocusLocationBarByDefault() set to true?
Some options:

(1) Add some explicit API that chrome/ code can call on a web contents to set
FocusLocationBarByDefault(true)
(2) Add another channel for communicating this bit?  (Similar to how it's done
right now with webui but a new one since we don't have a webui in this case
anymore.)
(3) Don't do it content/ at all and just try to cover all the cases on the
chrome side.  It may be possible but it would definitely be more bug-prone.
(4) Keep it like this. It's simplest and it's how we're currently handling
focus-by-default for about://blank.  Also it looks like there's already a bunch
of chrome-specific URLs in this list (e.g. ChromeUIAppCacheInternals,
ChromeUIDevTools, etc.)

[jam, 2013-02-01 17:44:33]

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.cc (right):

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:179: void
InstantLoader::WebContentsFocused(content::WebContents* contents) {
On 2013/01/31 16:24:57, samarth wrote:
> On 2013/01/29 13:35:42, sreeram wrote:
> > /* contents */
> 
> Done.

btw i just saw this. this isn't chrome style. please let's not spread
conventions from other projects into chrome.

https://codereview.chromium.org/11824050/diff/62001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11824050/diff/62001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1990: entry->GetVirtualURL()
== GURL(chrome::kChromeUINewTabURL)));
On 2013/01/31 16:24:57, samarth wrote:
> On 2013/01/29 17:51:55, jam wrote:
> > i dont understand why this change is even needed?
> > 
> > kchromeuinewtaburl should mean that this webcontents is displaying the new
> > tabui, and so
> > 
> >
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/webui/n...
> > 
> > should ensure that ShouldFocusLocationBarByDefault returns true?
> 
> It's because with Instant Extended, the new tab page is no longer necessarily
a
> WebUI.  Instead, with this change, we're swapping in the instant web contents
> when you navigate to the new tab page.

i see. then you'll need to find another way of doing this, as chrome://newtab
does not belong in the content layer

[sreeram, 2013-02-01 17:50:02]

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.cc (right):

https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:179: void
InstantLoader::WebContentsFocused(content::WebContents* contents) {
On 2013/02/01 17:44:34, jam wrote:
> On 2013/01/31 16:24:57, samarth wrote:
> > On 2013/01/29 13:35:42, sreeram wrote:
> > > /* contents */
> > 
> > Done.
> 
> btw i just saw this. this isn't chrome style. please let's not spread
> conventions from other projects into chrome.

This style is in fact explicitly mentioned in the Google C++ style guide (and
the Chromium style guide makes no mention of it). See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

I agree that the style is not prevalent in Chrome, but it's not zero. A grep for
something like "/\*.*\*/ *[{,]$" will show you quite a lot of matches (including
false positives).

[jam, 2013-02-01 17:56:38]

On 2013/02/01 17:50:02, sreeram wrote:
>
https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
> File chrome/browser/instant/instant_loader.cc (right):
> 
>
https://codereview.chromium.org/11824050/diff/62001/chrome/browser/instant/in...
> chrome/browser/instant/instant_loader.cc:179: void
> InstantLoader::WebContentsFocused(content::WebContents* contents) {
> On 2013/02/01 17:44:34, jam wrote:
> > On 2013/01/31 16:24:57, samarth wrote:
> > > On 2013/01/29 13:35:42, sreeram wrote:
> > > > /* contents */
> > > 
> > > Done.
> > 
> > btw i just saw this. this isn't chrome style. please let's not spread
> > conventions from other projects into chrome.
> 
> This style is in fact explicitly mentioned in the Google C++ style guide (and
> the Chromium style guide makes no mention of it). See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> 
> I agree that the style is not prevalent in Chrome, but it's not zero. A grep
for
> something like "/\*.*\*/ *[{,]$" will show you quite a lot of matches
(including
> false positives).

yes, I understand it's in the google style guide. but, in the chrome code, this
was never used.

a search of the code for any particular style that's not "chrome style", or even
style violations, will always find matches. so just because something that's not
chrome style leaked in, doesn't mean it set precedent.

what i really value is consistency in style across the chrome code. having
subsets of it use different styles is annoying.

[sreeram, 2013-02-01 18:16:36]

On 2013/02/01 17:56:38, jam wrote:
> what i really value is consistency in style across the chrome code. having
> subsets of it use different styles is annoying.

I'm not trying to be pedantic nor trying to annoy you, but that's exactly why we
have a style guide, to have consistency in style. If we don't want this
particular style in Chromium, we should just say so, in the Chromium Style
Guide.

In the absence of which, it seems likely that this style will continue to "leak
in". Particularly because the Google guide demands it (i.e., not a "you may" or
"consider" or "try to"; it's a direct order -- "Comment out"). You're going to
have an uphill battle trying to stop it without the aid of a style guide rule.

But also because reviewers seem to disagree. For example, you say this style is
not preferred. However, I believe at least at one time, @sky preferred it. See
https://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/in...
(which is when I became aware of this style and started using it, albeit
inconsistently, in the Instant code).

[jam, 2013-02-01 18:22:08]

On 2013/02/01 18:16:36, sreeram wrote:
> On 2013/02/01 17:56:38, jam wrote:
> > what i really value is consistency in style across the chrome code. having
> > subsets of it use different styles is annoying.
> 
> I'm not trying to be pedantic nor trying to annoy you, but that's exactly why
we
> have a style guide, to have consistency in style. If we don't want this
> particular style in Chromium, we should just say so, in the Chromium Style
> Guide.
> 
> In the absence of which, it seems likely that this style will continue to
"leak
> in". Particularly because the Google guide demands it (i.e., not a "you may"
or
> "consider" or "try to"; it's a direct order -- "Comment out"). You're going to
> have an uphill battle trying to stop it without the aid of a style guide rule.
> 
> But also because reviewers seem to disagree. For example, you say this style
is
> not preferred. However, I believe at least at one time, @sky preferred it. See
>
https://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/in...
> (which is when I became aware of this style and started using it, albeit
> inconsistently, in the Instant code).

ok, i've chatted with a few people about this, and the consensus was the same as
the arguments you give, so i relent.

i do always like to follow the style guide. i also hate adding exceptions in the
chrome style guide. i guess the hard part for me to deal with is in situations
like this, where parts of the google style guide that we never used in chrome
code start being used.

sorry for the noise.

[Charlie Reis, 2013-02-01 19:58:56]

I'll give my specific LGTM for the SiteInstance creation in InstantLoader and
defer on the rest.

https://codereview.chromium.org/11824050/diff/62001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11824050/diff/62001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1990: entry->GetVirtualURL()
== GURL(chrome::kChromeUINewTabURL)));
On 2013/02/01 17:44:34, jam wrote:
> On 2013/01/31 16:24:57, samarth wrote:
> > On 2013/01/29 17:51:55, jam wrote:
> > > i dont understand why this change is even needed?
> > > 
> > > kchromeuinewtaburl should mean that this webcontents is displaying the new
> > > tabui, and so
> > > 
> > >
> >
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/webui/n...
> > > 
> > > should ensure that ShouldFocusLocationBarByDefault returns true?
> > 
> > It's because with Instant Extended, the new tab page is no longer
necessarily
> a
> > WebUI.  Instead, with this change, we're swapping in the instant web
contents
> > when you navigate to the new tab page.
> 
> i see. then you'll need to find another way of doing this, as chrome://newtab
> does not belong in the content layer

Perhaps FocusLocationBarByDefault could call out to the ContentBrowserClient?

[sreeram, 2013-02-01 20:36:53]

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.cc (right):

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:60: const char kInstantHeader[] =
"X-Purpose: Instant";
Move this also to the anonymous namespace at the top.

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:166: // Messages shown during Instant
cancels Instant, so we suppress them.
cancels -> cancel

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.h (right):

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:21: namespace content {
Add a forward declaration for content::OpenURLParams.

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:27: // preloading an instant page.
preloading -> preload
instant -> Instant

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:66: // called after the designated
amount of time has elapsed.
The "designated amount of time" is mildly confusing since there's no timeout
specified anywhere nearby. Perhaps say "... after kStalePageTimeoutMS have
elapsed without any activity".

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
File chrome/browser/instant/instant_overlay.cc (right):

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.cc:28: };
How about we get rid of this class, and just make InstantOverlay directly
implement base::SupportsUserData::Data? Then, we instead of sticking a pointer
to InstantOverlayUserData in a web contents, we can just stick the
InstantOverlay pointer directly. So, FromWebContents would look like this:

// static
InstantOverlay* InstantOverlay::FromWebContents(...) {
  return static_cast<InstantOverlay*>(web_contents->GetUserData(&kUserDataKey));
}

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.cc:56: base::Unretained(this)));
You need to split this into two methods:
  loader_.InitContents(...);
  SetContents(loader_.contents());
  contents()->SetUserData(&kUserDataKey, this);
  loader_.LoadURL(...);

The reason is that as it stands, it won't play nicely with Shishir's upcoming
change to do SearchBox initialization in RenderViewCreated. RenderViewCreated()
is called on the WebContentsObserver, and it happens *during* the call to
LoadURL(). But since you SetContents() only later below, that's too late; we'll
miss the RenderViewCreated call.

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.cc:93: bool tmp =
is_pointer_down_from_activate_;
Use base::AutoReset<> here.

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.cc:113: content::WebContents* overlay =
contents();
Add a comment on top of this line to say why we have to cache the overlay
WebContents* (because CommitIfPossible() will ReleaseContents(), so after that,
contents() will be NULL).

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.cc:117: // be able to handle the
navigation.
To be safe:
DCHECK_NE(this, overlay->GetDelegate());

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
File chrome/browser/instant/instant_overlay.h (right):

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.h:10: #include "base/basictypes.h"
#include "base/gtest_prod_util.h"
(for the FRIEND macro)

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.h:13: #include "base/string16.h"
No need for this include (the only reference to string16 is in an overridden
method; which means the original .h file which declared the method we are
overriding must necessarily have declared string16).

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.h:15: #include
"chrome/browser/instant/instant_controller.h"
You don't need this include. You just need a forward declaration for
InstantController.

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.h:19: class Profile;
Add a forward declaration for content::WebContents.

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.cc (right):

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_page.cc:174: // Nothing to do if the page already
supports instant.
instant -> Instant

[samarth, 2013-02-04 20:43:32]

jam@: removed the content/ stuff for now (see comment below).

Thanks!
Samarth

https://codereview.chromium.org/11824050/diff/62001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11824050/diff/62001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1990: entry->GetVirtualURL()
== GURL(chrome::kChromeUINewTabURL)));
On 2013/02/01 17:44:34, jam wrote:
> On 2013/01/31 16:24:57, samarth wrote:
> > On 2013/01/29 17:51:55, jam wrote:
> > > i dont understand why this change is even needed?
> > > 
> > > kchromeuinewtaburl should mean that this webcontents is displaying the new
> > > tabui, and so
> > > 
> > >
> >
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/webui/n...
> > > 
> > > should ensure that ShouldFocusLocationBarByDefault returns true?
> > 
> > It's because with Instant Extended, the new tab page is no longer
necessarily
> a
> > WebUI.  Instead, with this change, we're swapping in the instant web
contents
> > when you navigate to the new tab page.
> 
> i see. then you'll need to find another way of doing this, as chrome://newtab
> does not belong in the content layer

OK, I backed out all the content/ changes for this change. Focusing in the che
common case (opening new tab with ctrl+t or new tab button) can be handled
easily enough in chrome/. I'll file a separate bug to fix the uncommon cases
(navigating to the NTP using back/forward). We may be able to use
ContentBrowserClient as Charlie suggested or do something else.

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.cc (right):

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:60: const char kInstantHeader[] =
"X-Purpose: Instant";
On 2013/02/01 20:36:53, sreeram wrote:
> Move this also to the anonymous namespace at the top.

Done.

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:166: // Messages shown during Instant
cancels Instant, so we suppress them.
On 2013/02/01 20:36:53, sreeram wrote:
> cancels -> cancel

Done.

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.h (right):

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:21: namespace content {
On 2013/02/01 20:36:53, sreeram wrote:
> Add a forward declaration for content::OpenURLParams.

Done.

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:27: // preloading an instant page.
On 2013/02/01 20:36:53, sreeram wrote:
> preloading -> preload
> instant -> Instant

Done.

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.h:66: // called after the designated
amount of time has elapsed.
On 2013/02/01 20:36:53, sreeram wrote:
> The "designated amount of time" is mildly confusing since there's no timeout
> specified anywhere nearby. Perhaps say "... after kStalePageTimeoutMS have
> elapsed without any activity".

Done.  (Left out "without any activity" since that's not quite accurate.  We
reload after 3 hours no matter how much activity is going on.)

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
File chrome/browser/instant/instant_overlay.cc (right):

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.cc:28: };
On 2013/02/01 20:36:53, sreeram wrote:
> How about we get rid of this class, and just make InstantOverlay directly
> implement base::SupportsUserData::Data? Then, we instead of sticking a pointer
> to InstantOverlayUserData in a web contents, we can just stick the
> InstantOverlay pointer directly. So, FromWebContents would look like this:
> 
> // static
> InstantOverlay* InstantOverlay::FromWebContents(...) {
>   return
static_cast<InstantOverlay*>(web_contents->GetUserData(&kUserDataKey));
> }

As we discussed, this doesn't work because SetUserData takes ownership of the
passed in the pointer.

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.cc:56: base::Unretained(this)));
On 2013/02/01 20:36:53, sreeram wrote:
> You need to split this into two methods:
>   loader_.InitContents(...);
>   SetContents(loader_.contents());
>   contents()->SetUserData(&kUserDataKey, this);
>   loader_.LoadURL(...);
> 
> The reason is that as it stands, it won't play nicely with Shishir's upcoming
> change to do SearchBox initialization in RenderViewCreated.
RenderViewCreated()
> is called on the WebContentsObserver, and it happens *during* the call to
> LoadURL(). But since you SetContents() only later below, that's too late;
we'll
> miss the RenderViewCreated call.

Done.

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.cc:93: bool tmp =
is_pointer_down_from_activate_;
On 2013/02/01 20:36:53, sreeram wrote:
> Use base::AutoReset<> here.

Done.

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.cc:113: content::WebContents* overlay =
contents();
On 2013/02/01 20:36:53, sreeram wrote:
> Add a comment on top of this line to say why we have to cache the overlay
> WebContents* (because CommitIfPossible() will ReleaseContents(), so after
that,
> contents() will be NULL).

Done.

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.cc:117: // be able to handle the
navigation.
On 2013/02/01 20:36:53, sreeram wrote:
> To be safe:
> DCHECK_NE(this, overlay->GetDelegate());

Done.  (Actually compared it to loader_ which is the old delegate.)

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
File chrome/browser/instant/instant_overlay.h (right):

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.h:10: #include "base/basictypes.h"
On 2013/02/01 20:36:53, sreeram wrote:
> #include "base/gtest_prod_util.h"
> (for the FRIEND macro)

Done.

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.h:13: #include "base/string16.h"
On 2013/02/01 20:36:53, sreeram wrote:
> No need for this include (the only reference to string16 is in an overridden
> method; which means the original .h file which declared the method we are
> overriding must necessarily have declared string16).

Done.

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.h:15: #include
"chrome/browser/instant/instant_controller.h"
On 2013/02/01 20:36:53, sreeram wrote:
> You don't need this include. You just need a forward declaration for
> InstantController.

I need it for the instant_controller() accessor below (otherwise, the cast won't
compile).

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.h:19: class Profile;
On 2013/02/01 20:36:53, sreeram wrote:
> Add a forward declaration for content::WebContents.

Done.

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.cc (right):

https://codereview.chromium.org/11824050/diff/54006/chrome/browser/instant/in...
chrome/browser/instant/instant_page.cc:174: // Nothing to do if the page already
supports instant.
On 2013/02/01 20:36:53, sreeram wrote:
> instant -> Instant

Done.

[samarth, 2013-02-05 14:46:56]

FYI, synced past Shishir's latest changes.  PTAL.

[samarth, 2013-02-05 19:29:20]

ping sky@? Can you take a review for
chrome/browser/ui
chrome/browser/history
?

Thanks!
Samarth

[sky, 2013-02-05 21:40:13]

https://codereview.chromium.org/11824050/diff/71001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11824050/diff/71001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:132:
base::Bind(&BrowserInstantController::FocusOmniboxForNTP,
Why the delay here? What if this is destroyed before the task completes. Or what
if something else runs before the task that wants to change focus?

https://codereview.chromium.org/11824050/diff/71001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.h (right):

https://codereview.chromium.org/11824050/diff/71001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.h:13: #include
"chrome/browser/ui/browser.h"
Can you not inline profile() so that you don't need this?

https://codereview.chromium.org/11824050/diff/71001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/11824050/diff/71001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:233: GURL
NormalizeURL(chrome::NavigateParams* params) {
Add description.

https://codereview.chromium.org/11824050/diff/71001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:323: content::WebContents*
CreateTargetContents(chrome::NavigateParams* params,
const NavigateParams&

[samarth, 2013-02-05 22:41:57]

Thanks Scott! PTAL.

Samarth

https://codereview.chromium.org/11824050/diff/71001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11824050/diff/71001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:132:
base::Bind(&BrowserInstantController::FocusOmniboxForNTP,
On 2013/02/05 21:40:13, sky wrote:
> Why the delay here? What if this is destroyed before the task completes. Or
what
> if something else runs before the task that wants to change focus?

Yeah, you'right: this will probably break.

I needed to delay because later in the same event loop, the active WebContents
will take focus away from the omnibox. But the real solution is to fix
WebContents::ShouldFocusLocationBarByDefault. I'll follow up with jam@ about
that.

Removed this code.

https://codereview.chromium.org/11824050/diff/71001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.h (right):

https://codereview.chromium.org/11824050/diff/71001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.h:13: #include
"chrome/browser/ui/browser.h"
On 2013/02/05 21:40:13, sky wrote:
> Can you not inline profile() so that you don't need this?

Done.

https://codereview.chromium.org/11824050/diff/71001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/11824050/diff/71001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:233: GURL
NormalizeURL(chrome::NavigateParams* params) {
On 2013/02/05 21:40:13, sky wrote:
> Add description.

Oops left over from a previous iteration. Removed this function.

https://codereview.chromium.org/11824050/diff/71001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:323: content::WebContents*
CreateTargetContents(chrome::NavigateParams* params,
On 2013/02/05 21:40:13, sky wrote:
> const NavigateParams&

Done.

[sky, 2013-02-06 00:33:28]

LGTM

[samarth, 2013-02-06 18:19:44]

+jam for content/ changes

[jam, 2013-02-06 22:22:51]

content lgtm

https://codereview.chromium.org/11824050/diff/80004/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11824050/diff/80004/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1994: return false;
nit: return delegate_ && delegate_->Foo();
that matches the style that was there before

[dhollowa, 2013-02-07 02:30:55]

LGTM.

[sreeram, 2013-02-07 18:22:45]

LGTM

I didn't look at the tests at all, nor did I look too carefully at
instant_controller.cc. The main reason is that I'm going to be making changes in
these areas very soon, so I'll get to discover issues and fix them, so I didn't
want to hold up your CL (any more than I already have) pending such an audit.

Notes to self: For example, deleting with the caller on the stack
(ReloadOverlayIfState) and making sure that the search_mode vs what we allow is
valid for overlay vs tab (ntp).

Thanks for your hard work and patience! Enjoy your vacation!

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:651:
content::Source<content::WebContents>(preview.get());
It's uncommon to bind a reference to a temporary. (I do understand that the
compiler guarantees that the temporary will live on as long as the reference
does.) Recommend the simpler:
content::Source<content::WebContents> notification_source(preview.get());

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:846: ntp_->InitializeFonts();
Don't you need to send the theme info to the NTP? Here and in
ResetInstantTab()/ResetNTP() below.

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
File chrome/browser/instant/instant_overlay.cc (right):

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.cc:23: ~InstantOverlayUserData() {}
Add "virtual" here (cf: rsleevi's on going effort to fix style checker plugin
warnings).

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
File chrome/browser/instant/instant_overlay.h (right):

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.h:96: 
Seems to me that the InstantOverlay should return false for
ShouldProcessStartCapturingKeyStrokes() and
ShouldProcessStopCapturingKeyStrokes(), no?

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

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:185: virtual bool
ShouldProcessNavigateToURL();
It might be nice to flip the default from true to false. While reviewing this
CL, I often found myself doing a diff from the list here to the list that's
overridden by each subclass to see what's actually being handled, because that's
what matters (more than what they don't handle).

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
File chrome/browser/instant/instant_tab.h (right):

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
chrome/browser/instant/instant_tab.h:16: // Doesn't take ownership of either
|delegate|.
No need for this comment. As @dhollowa mentioned, the common practice is that
objects *don't* own their delegates, so a comment is warranted only if the
opposite is true.

[sreeram, 2013-02-07 21:17:49]

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:43:
chrome::search::IsInstantExtendedAPIEnabled(browser->profile())),
Replace browser_->profile() with just profile(), throughout this file.

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:157: const GURL& url =
preview->GetURL();
GURL url = preview->GetURL();

Because, given preview.Pass() on the next line, it isn't obvious that
preview->GetURL() will continue to be valid. We know it's valid for now, but
better to be safe about these things (maybe the tab strip model will do
something to invalidate |preview| in the future). For example, see
https://code.google.com/p/chromium/issues/detail?id=158056#c4

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:158: ReplaceWebContentsAt(index,
preview.Pass());
Nit: ReplaceWebContentsAt(browser_->tab_strip_model()->active_index(),
preview.Pass())

To match the style of line 118.

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:163: }
Move these lines (159-163) to outside the "if" statement, since it feels like it
(recording metrics about whether an Instant-committed URL is an app) should be
done for both in_new_tab vs not.

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.h (right):

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.h:77: // in screen coordinated.
coordinated -> coordinates

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:359: bool
SwapInInstantNTP(chrome::NavigateParams* params,
I guess you can't do "const chrome::NavigateParams&" because you need
"&params->target_contents" below to give you a non-const
"content::WebContents**"?

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:363: return false;
We don't need this check (and thus the include of search.h). Instant won't swap
in the NTP if it's not extended mode.

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:27: url.host() ==
chrome::kChromeUINewTabHost;
Why not just:
  return url == GURL(chrome::kChromeUINewTabURL);

(Which is what you do elsewhere in this CL.)

https://codereview.chromium.org/11824050/diff/72064/chrome/common/chrome_noti...
File chrome/common/chrome_notification_types.h (right):

https://codereview.chromium.org/11824050/diff/72064/chrome/common/chrome_noti...
chrome/common/chrome_notification_types.h:1108: // Sent when the Instant
Controller determines whether the ntp supports the
ntp -> NTP

https://codereview.chromium.org/11824050/diff/72064/content/public/browser/we...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/11824050/diff/72064/content/public/browser/we...
content/public/browser/web_contents_delegate.cc:66: bool
WebContentsDelegate::TakeFocus(WebContents* soruce, bool reverse) {
soruce -> source

[sreeram, 2013-02-07 22:09:53]

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.cc (right):

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/instant/in...
chrome/browser/instant/instant_page.cc:183: if (contents_ &&
contents_->IsActiveEntry(page_id)) {
No need for "if (contents_)" clause. There's no way this should be called if
contents_ is null, so if you really want, you can DCHECK(contents_) instead.
This applies to all the other methods below too.

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/instant/in...
chrome/browser/instant/instant_page.cc:202: Observe(NULL);
This code above works as is now, but I'd like to make it more defensive. Just in
case the delegate_ doesn't take care to delay-delete us (in response to the page
not supporting instant), we should try not to access anything after the delegate
call. So, something like this:

supports_instant_ = supports_instant;
content::WebContents* contents = web_contents();
if (!supports_instant)
  Observe(NULL);
delegate_->InstantSupportDetermined(contents, supports_instant);

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

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:218: content::WebContents* contents_;
Why do we need to store |contents_|? Since we always Observe(contents_) whenever
it changes, we can surely just get it from WebContentsObserver::web_contents().

[samarth, 2013-02-07 22:11:54]

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:651:
content::Source<content::WebContents>(preview.get());
On 2013/02/07 18:22:46, sreeram wrote:
> It's uncommon to bind a reference to a temporary. (I do understand that the
> compiler guarantees that the temporary will live on as long as the reference
> does.) Recommend the simpler:
> content::Source<content::WebContents> notification_source(preview.get());

Done.

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:846: ntp_->InitializeFonts();
On 2013/02/07 18:22:46, sreeram wrote:
> Don't you need to send the theme info to the NTP? Here and in
> ResetInstantTab()/ResetNTP() below.

As we discussed, this is already somewhat broken. Moved the call to
UpdateThemeInfo into here and also added it to ResetInstantTab.

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
File chrome/browser/instant/instant_overlay.cc (right):

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.cc:23: ~InstantOverlayUserData() {}
On 2013/02/07 18:22:46, sreeram wrote:
> Add "virtual" here (cf: rsleevi's on going effort to fix style checker plugin
> warnings).

Done.

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
File chrome/browser/instant/instant_overlay.h (right):

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
chrome/browser/instant/instant_overlay.h:96: 
On 2013/02/07 18:22:46, sreeram wrote:
> Seems to me that the InstantOverlay should return false for
> ShouldProcessStartCapturingKeyStrokes() and
> ShouldProcessStopCapturingKeyStrokes(), no?

Done.

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

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:185: virtual bool
ShouldProcessNavigateToURL();
On 2013/02/07 18:22:46, sreeram wrote:
> It might be nice to flip the default from true to false. While reviewing this
> CL, I often found myself doing a diff from the list here to the list that's
> overridden by each subclass to see what's actually being handled, because
that's
> what matters (more than what they don't handle).

Yeah, that's much nicer. Done.

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
File chrome/browser/instant/instant_tab.h (right):

https://codereview.chromium.org/11824050/diff/84005/chrome/browser/instant/in...
chrome/browser/instant/instant_tab.h:16: // Doesn't take ownership of either
|delegate|.
On 2013/02/07 18:22:46, sreeram wrote:
> No need for this comment. As @dhollowa mentioned, the common practice is that
> objects *don't* own their delegates, so a comment is warranted only if the
> opposite is true.

Yep, yep.  I keep missing some of these.

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:43:
chrome::search::IsInstantExtendedAPIEnabled(browser->profile())),
On 2013/02/07 21:17:50, sreeram wrote:
> Replace browser_->profile() with just profile(), throughout this file.

Done.

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:157: const GURL& url =
preview->GetURL();
On 2013/02/07 21:17:50, sreeram wrote:
> GURL url = preview->GetURL();
> 
> Because, given preview.Pass() on the next line, it isn't obvious that
> preview->GetURL() will continue to be valid. We know it's valid for now, but
> better to be safe about these things (maybe the tab strip model will do
> something to invalidate |preview| in the future). For example, see
> https://code.google.com/p/chromium/issues/detail?id=158056#c4

Done.

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:158: ReplaceWebContentsAt(index,
preview.Pass());
On 2013/02/07 21:17:50, sreeram wrote:
> Nit: ReplaceWebContentsAt(browser_->tab_strip_model()->active_index(),
> preview.Pass())
> 
> To match the style of line 118.

Done.

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:163: }
On 2013/02/07 21:17:50, sreeram wrote:
> Move these lines (159-163) to outside the "if" statement, since it feels like
it
> (recording metrics about whether an Instant-committed URL is an app) should be
> done for both in_new_tab vs not.

Moved it to the top (I assume that is OK?). That way, I don't have to worry
about saving GetURL to a temporary in the first place.

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.h (right):

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.h:77: // in screen coordinated.
On 2013/02/07 21:17:50, sreeram wrote:
> coordinated -> coordinates

Done.

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:359: bool
SwapInInstantNTP(chrome::NavigateParams* params,
On 2013/02/07 21:17:50, sreeram wrote:
> I guess you can't do "const chrome::NavigateParams&" because you need
> "&params->target_contents" below to give you a non-const
> "content::WebContents**"?

Exactly.

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:363: return false;
On 2013/02/07 21:17:50, sreeram wrote:
> We don't need this check (and thus the include of search.h). Instant won't
swap
> in the NTP if it's not extended mode.

Done.

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:27: url.host() ==
chrome::kChromeUINewTabHost;
On 2013/02/07 21:17:50, sreeram wrote:
> Why not just:
>   return url == GURL(chrome::kChromeUINewTabURL);
> 
> (Which is what you do elsewhere in this CL.)

No idea (to avoid creating another GURL?). It probably doesn't matter, but
unless you feel strongly, I'll leave this as-is. (Gotta limit the scope of this
CL somewhere :))

https://codereview.chromium.org/11824050/diff/72064/chrome/common/chrome_noti...
File chrome/common/chrome_notification_types.h (right):

https://codereview.chromium.org/11824050/diff/72064/chrome/common/chrome_noti...
chrome/common/chrome_notification_types.h:1108: // Sent when the Instant
Controller determines whether the ntp supports the
On 2013/02/07 21:17:50, sreeram wrote:
> ntp -> NTP

Done.

https://codereview.chromium.org/11824050/diff/72064/content/public/browser/we...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/11824050/diff/72064/content/public/browser/we...
content/public/browser/web_contents_delegate.cc:66: bool
WebContentsDelegate::TakeFocus(WebContents* soruce, bool reverse) {
On 2013/02/07 21:17:50, sreeram wrote:
> soruce -> source

Done.

[sreeram, 2013-02-07 22:18:15]

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:163: }
On 2013/02/07 22:11:55, samarth wrote:
> On 2013/02/07 21:17:50, sreeram wrote:
> > Move these lines (159-163) to outside the "if" statement, since it feels
like
> it
> > (recording metrics about whether an Instant-committed URL is an app) should
be
> > done for both in_new_tab vs not.
> 
> Moved it to the top (I assume that is OK?). That way, I don't have to worry
> about saving GetURL to a temporary in the first place.

Yup, moving to the top is totally fine.

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:27: url.host() ==
chrome::kChromeUINewTabHost;
On 2013/02/07 22:11:55, samarth wrote:
> On 2013/02/07 21:17:50, sreeram wrote:
> > Why not just:
> >   return url == GURL(chrome::kChromeUINewTabURL);
> > 
> > (Which is what you do elsewhere in this CL.)
> 
> No idea (to avoid creating another GURL?). It probably doesn't matter, but
> unless you feel strongly, I'll leave this as-is. (Gotta limit the scope of
this
> CL somewhere :))

I don't feel that strongly. I'm continuing to send you comments. Since you
already have my LGTM, please feel free to submit anytime. Whatever comments are
not addressed by the time you submit, I'll address in a followup CL.

[samarth, 2013-02-07 22:45:41]

Thanks for the reviews everyone! Sending to the CQ now...

Samarth

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.cc (right):

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/instant/in...
chrome/browser/instant/instant_page.cc:183: if (contents_ &&
contents_->IsActiveEntry(page_id)) {
On 2013/02/07 22:09:54, sreeram wrote:
> No need for "if (contents_)" clause. There's no way this should be called if
> contents_ is null, so if you really want, you can DCHECK(contents_) instead.
> This applies to all the other methods below too.

Done.

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/instant/in...
chrome/browser/instant/instant_page.cc:202: Observe(NULL);
On 2013/02/07 22:09:54, sreeram wrote:
> This code above works as is now, but I'd like to make it more defensive. Just
in
> case the delegate_ doesn't take care to delay-delete us (in response to the
page
> not supporting instant), we should try not to access anything after the
delegate
> call. So, something like this:
> 
> supports_instant_ = supports_instant;
> content::WebContents* contents = web_contents();
> if (!supports_instant)
>   Observe(NULL);
> delegate_->InstantSupportDetermined(contents, supports_instant);

Done.

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

https://codereview.chromium.org/11824050/diff/72064/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:218: content::WebContents* contents_;
On 2013/02/07 22:09:54, sreeram wrote:
> Why do we need to store |contents_|? Since we always Observe(contents_)
whenever
> it changes, we can surely just get it from
WebContentsObserver::web_contents().

Done.

[commit-bot: I haz the power, 2013-02-07 22:46:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/11824050/83049

[commit-bot: I haz the power, 2013-02-07 23:21:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/11824050/77103

[commit-bot: I haz the power, 2013-02-08 01:13:35]

Change committed as 181397