Fix focus for in-tab navigations.

Fix focus for in-tab navigations.

For in-tab navigations, if the contents is supposed to be focused, we should be
calling SetInitialFocus() instead of directly calling Focus() which blindly focuses
the contents view. This way, WebContents can send focus to the location bar on
load if it wants to.

TESTED=manual, per bugs
BUG=179499, 175147
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185989

[samarth, 2013-03-01 23:18:30]

Please take a look.

Thanks,
Samarth

[sky, 2013-03-01 23:50:48]

This makes me nervous. Have you verified this on all platforms?

[samarth, 2013-03-01 23:56:46]

No, only tested on chromeos so far.  Let me try it out on Windows and Mac as
well and I'll get back to you.

Just so I have a better sense of things to look out for, anything in particular
you think might break with this change?

Thanks for the quick review.
Samarth

[sky, 2013-03-02 00:09:11]

Focus is always subtle. AFAICT the current call has been there forever
and no one noticed.

  -Scott

On Fri, Mar 1, 2013 at 3:56 PM,  <samarth@chromium.org> wrote:
> No, only tested on chromeos so far.  Let me try it out on Windows and Mac as
> well and I'll get back to you.
>
> Just so I have a better sense of things to look out for, anything in
> particular
> you think might break with this change?
>
> Thanks for the quick review.
> Samarth
>
> https://codereview.chromium.org/12390042/

[samarth, 2013-03-02 00:32:34]

Yeah I tried to poke back in the revision history and didn't find anything
interesting related to this line.  I think it never was a problem before because
a) most pages don't / can't focus the omnibox by default
b) for the old NTP, an actual navigation happens which has a separate codepath
that sets the focus correctly (for instant extended, we swap in a preloaded
contents).

Would calling SetInitialFocus() directly be safer? That's a much simpler
function:
  if (web_contents_->FocusLocationBarByDefault())
    web_contents_->SetFocusToLocationBar(false);
  else
    Focus();
and I don't see a good reason why the page would already have a stored focus at
this point.

Thanks,
Samarth

[sky, 2013-03-04 02:36:57]

SGTM

On Fri, Mar 1, 2013 at 4:32 PM,  <samarth@chromium.org> wrote:
> Yeah I tried to poke back in the revision history and didn't find anything
> interesting related to this line.  I think it never was a problem before
> because
> a) most pages don't / can't focus the omnibox by default
> b) for the old NTP, an actual navigation happens which has a separate
> codepath
> that sets the focus correctly (for instant extended, we swap in a preloaded
> contents).
>
> Would calling SetInitialFocus() directly be safer? That's a much simpler
> function:
>   if (web_contents_->FocusLocationBarByDefault())
>     web_contents_->SetFocusToLocationBar(false);
>   else
>     Focus();
> and I don't see a good reason why the page would already have a stored focus
> at
> this point.
>
> Thanks,
> Samarth
>
> https://codereview.chromium.org/12390042/

[samarth, 2013-03-04 16:40:14]

Ok, changed to SetInitialFocus. Also tried this out on Windows and Mac and it
seems to work.

Thanks,
Samarth

[sky, 2013-03-04 18:10:07]

LGTM

[commit-bot: I haz the power, 2013-03-04 18:53:24]

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

[commit-bot: I haz the power, 2013-03-04 22:16:05]

Change committed as 185989