Add onfocuschange to the Extended Search API, with associated isFocused attribute.

Add onfocuschange to the Extended Search API, with associated isFocused attribute.

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

[Donn Denman, 2013-05-08 23:43:02]

Michael, here's what I have so far.  Still TODO includes adding a unit test. 
Let me know if you think I'm on the right track.

[melevin, 2013-05-09 16:41:11]

Looks good to me. I don't know what could be causing the multiple focus calls
issue... it may not be related to this CL, but only surfaced by it.

https://codereview.chromium.org/14646034/diff/1/chrome/browser/ui/search/inst...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14646034/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_controller.cc:961: }
Instead of using the "focus_changed" variable you could also early return here
in an "else" block.

https://codereview.chromium.org/14646034/diff/1/chrome/renderer/searchbox/sea...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/14646034/diff/1/chrome/renderer/searchbox/sea...
chrome/renderer/searchbox/searchbox.h:146: 
No newline here.

[samarth, 2013-05-10 00:55:31]

https://codereview.chromium.org/14646034/diff/1/chrome/browser/ui/search/inst...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14646034/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_controller.cc:964: if (focus_changed && state
!= OMNIBOX_FOCUS_INVISIBLE) {
We should do the "did focus actually change" check in the renderer
(searchbox.cc) rather than here.   There is a SearchBox instance per page and so
by doing the checks there rather than here (which is per-browser), we can ensure
the page always gets an update when necessary.

Also, instead of having separate KeyCaptureChanged and FocusChanged IPCs, lets
just use a single IPC that sends the focus state enum value (and change reason),
and move the logic for whether we should send a keycapture into the SearchBox
code.

[Donn Denman, 2013-05-13 18:00:39]

Samarth, Can you take another look?  I'm unsure about a couple of things with
the focus change reason: 1) if it really makes sense to introduce the "INIT"
reason.  2) it seems like the reason is never really used, maybe more for
debugging?

https://codereview.chromium.org/14646034/diff/1/chrome/browser/ui/search/inst...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14646034/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_controller.cc:961: }
On 2013/05/09 16:41:11, melevin wrote:
> Instead of using the "focus_changed" variable you could also early return here
> in an "else" block.
Given Samarth's comments, I think this will no longer be an issue.  For the
record, I prefer to avoid early returns, especially in the middle of functions,
because they are easy to miss.

https://codereview.chromium.org/14646034/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_controller.cc:964: if (focus_changed && state
!= OMNIBOX_FOCUS_INVISIBLE) {
On 2013/05/10 00:55:32, samarth wrote:
> We should do the "did focus actually change" check in the renderer
> (searchbox.cc) rather than here.   There is a SearchBox instance per page and
so
> by doing the checks there rather than here (which is per-browser), we can
ensure
> the page always gets an update when necessary.
> 
> Also, instead of having separate KeyCaptureChanged and FocusChanged IPCs, lets
> just use a single IPC that sends the focus state enum value (and change
reason),
> and move the logic for whether we should send a keycapture into the SearchBox
> code.

Done.

[samarth, 2013-05-13 20:56:52]

https://codereview.chromium.org/14646034/diff/7001/chrome/browser/resources/l...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/14646034/diff/7001/chrome/browser/resources/l...
chrome/browser/resources/local_ntp/local_ntp.js:1675: // TODO(donnd): consider
adding an onfocuschange handler.
Not need to add a TODO here.  I don't see a good reason for adding it to the
local page, and if it comes up, we can just add it.

https://codereview.chromium.org/14646034/diff/7001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14646034/diff/7001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:937: if (extended_enabled_ &&
GetOverlayContents() &&
Just need to check extended_enabled_ here. The GetOverlayContents() check is
leftover from before and doesn't really apply anymore. The reason check should
be moved to the searchbox.cc code (see comments there).

https://codereview.chromium.org/14646034/diff/7001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:1163: OMNIBOX_FOCUS_CHANGE_INIT);
Don't need a new INIT value.  OMNIBOX_FOCUS_CHANGE_EXPLICIT is good enough.

https://codereview.chromium.org/14646034/diff/7001/chrome/common/omnibox_focu...
File chrome/common/omnibox_focus_state.h (right):

https://codereview.chromium.org/14646034/diff/7001/chrome/common/omnibox_focu...
chrome/common/omnibox_focus_state.h:37: // Focus being established during
initialization.
This shouldn't be necessary.

https://codereview.chromium.org/14646034/diff/7001/chrome/common/render_messa...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/14646034/diff/7001/chrome/common/render_messa...
chrome/common/render_messages.h:350:
IPC_MESSAGE_ROUTED2(ChromeViewMsg_SearchBoxKeyCaptureChanged,
I'd rename this to SearchBoxFocusChanged

https://codereview.chromium.org/14646034/diff/7001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/14646034/diff/7001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/searchbox_api.js:369: this.onfocuschange =
null;
I think this better belongs in the searchbox part of the api. So add it to
this.searchBox rather than this.newTabPage (see where onkeycapturechange is
defined).

https://codereview.chromium.org/14646034/diff/7001/chrome/renderer/searchbox/...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/14646034/diff/7001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.cc:366: if (new_focus_state !=
omnibox_focus_state_) {
Sorry, this is all a little confusing so let me give a bit more background. The
invisible focus craziness is a way to have a searchbox on the NTP such that if
you click on it and start typing, your keystrokes appear in the omnibox. We
solve this by creating a dummy searchbox that's not really an input field and
when the user clicks on it, the omnibox is actually focused but without a
blinking cursor.  (This is only way we've been able to implement it that won't
break IMEs.)

However, we don't want to expose the precise implementation of this to the API,
which is why it's referred to as "capturing key strokes" rather than talking
about invisible focus. So, in the chrome implementation today, we have 3 states
(not focused, visible focus, invisible focus). But from the point of view of the
API, there are 2 bools (iskeycaptureenabled and isfocused).

So, with that in mind, I think it makes the most sense to maintain 2bools in
this class corresponding to the 2 api bools. Then in this function, for each of
those, we can convert the OmniboxFocusState to the correct new value, compare to
the old value and send an update to the page if the value changed.

(The only other wrinkle with all of this if state goes from INVISIBLE -> VISIBLE
for reason TYPING, we don't consider iscapturingkeystrokes to have changed. See
comments in InstantController::OmniboxFocusChanged for why that is.)

[samarth, 2013-05-13 21:01:09]

https://codereview.chromium.org/14646034/diff/7001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14646034/diff/7001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:1163: OMNIBOX_FOCUS_CHANGE_INIT);
On 2013/05/13 20:56:53, samarth wrote:
> Don't need a new INIT value.  OMNIBOX_FOCUS_CHANGE_EXPLICIT is good enough.

Actually, we should be saving the reason inside InstantController like we do
omnibox_focus_state_ so we can send the correct value to the page.

https://codereview.chromium.org/14646034/diff/7001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:1543: OMNIBOX_FOCUS_CHANGE_INIT);
 // TODO(donnd): right reason?
Likewise, here.

[Donn Denman, 2013-05-13 23:44:50]

Thanks Samarth, hopefully we're getting close.

https://codereview.chromium.org/14646034/diff/7001/chrome/browser/resources/l...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/14646034/diff/7001/chrome/browser/resources/l...
chrome/browser/resources/local_ntp/local_ntp.js:1675: // TODO(donnd): consider
adding an onfocuschange handler.
On 2013/05/13 20:56:53, samarth wrote:
> Not need to add a TODO here.  I don't see a good reason for adding it to the
> local page, and if it comes up, we can just add it.

Done.

https://codereview.chromium.org/14646034/diff/7001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14646034/diff/7001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:937: if (extended_enabled_ &&
GetOverlayContents() &&
On 2013/05/13 20:56:53, samarth wrote:
> Just need to check extended_enabled_ here. The GetOverlayContents() check is
> leftover from before and doesn't really apply anymore. The reason check should
> be moved to the searchbox.cc code (see comments there).

Done.

https://codereview.chromium.org/14646034/diff/7001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:1163: OMNIBOX_FOCUS_CHANGE_INIT);
On 2013/05/13 20:56:53, samarth wrote:
> Don't need a new INIT value.  OMNIBOX_FOCUS_CHANGE_EXPLICIT is good enough.

Done.

https://codereview.chromium.org/14646034/diff/7001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:1163: OMNIBOX_FOCUS_CHANGE_INIT);
On 2013/05/13 21:01:10, samarth wrote:
> On 2013/05/13 20:56:53, samarth wrote:
> > Don't need a new INIT value.  OMNIBOX_FOCUS_CHANGE_EXPLICIT is good enough.
> 
> Actually, we should be saving the reason inside InstantController like we do
> omnibox_focus_state_ so we can send the correct value to the page.

Done.

https://codereview.chromium.org/14646034/diff/7001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:1543: OMNIBOX_FOCUS_CHANGE_INIT);
 // TODO(donnd): right reason?
On 2013/05/13 21:01:10, samarth wrote:
> Likewise, here.

Done.

https://codereview.chromium.org/14646034/diff/7001/chrome/common/omnibox_focu...
File chrome/common/omnibox_focus_state.h (right):

https://codereview.chromium.org/14646034/diff/7001/chrome/common/omnibox_focu...
chrome/common/omnibox_focus_state.h:37: // Focus being established during
initialization.
On 2013/05/13 20:56:53, samarth wrote:
> This shouldn't be necessary.

Done.

https://codereview.chromium.org/14646034/diff/7001/chrome/common/render_messa...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/14646034/diff/7001/chrome/common/render_messa...
chrome/common/render_messages.h:350:
IPC_MESSAGE_ROUTED2(ChromeViewMsg_SearchBoxKeyCaptureChanged,
On 2013/05/13 20:56:53, samarth wrote:
> I'd rename this to SearchBoxFocusChanged

Done.

https://codereview.chromium.org/14646034/diff/7001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/14646034/diff/7001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/searchbox_api.js:369: this.onfocuschange =
null;
On 2013/05/13 20:56:53, samarth wrote:
> I think this better belongs in the searchbox part of the api. So add it to
> this.searchBox rather than this.newTabPage (see where onkeycapturechange is
> defined).

Done.

https://codereview.chromium.org/14646034/diff/7001/chrome/renderer/searchbox/...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/14646034/diff/7001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.cc:366: if (new_focus_state !=
omnibox_focus_state_) {
On 2013/05/13 20:56:53, samarth wrote:
> Sorry, this is all a little confusing so let me give a bit more background.
The
> invisible focus craziness is a way to have a searchbox on the NTP such that if
> you click on it and start typing, your keystrokes appear in the omnibox. We
> solve this by creating a dummy searchbox that's not really an input field and
> when the user clicks on it, the omnibox is actually focused but without a
> blinking cursor.  (This is only way we've been able to implement it that won't
> break IMEs.)
> 
> However, we don't want to expose the precise implementation of this to the
API,
> which is why it's referred to as "capturing key strokes" rather than talking
> about invisible focus. So, in the chrome implementation today, we have 3
states
> (not focused, visible focus, invisible focus). But from the point of view of
the
> API, there are 2 bools (iskeycaptureenabled and isfocused).
> 
> So, with that in mind, I think it makes the most sense to maintain 2bools in
> this class corresponding to the 2 api bools. Then in this function, for each
of
> those, we can convert the OmniboxFocusState to the correct new value, compare
to
> the old value and send an update to the page if the value changed.
> 
> (The only other wrinkle with all of this if state goes from INVISIBLE ->
VISIBLE
> for reason TYPING, we don't consider iscapturingkeystrokes to have changed.
See
> comments in InstantController::OmniboxFocusChanged for why that is.)

Thanks for explaining this.  Done.

[melevin, 2013-05-14 22:06:22]

Still seeing the double "onfocuschange" calls when clicking on the fakebox.

https://codereview.chromium.org/14646034/diff/18001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.h (right):

https://codereview.chromium.org/14646034/diff/18001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.h:480: // The reason for the most
recent omni-box focus change.
omnibox

https://codereview.chromium.org/14646034/diff/18001/chrome/common/render_mess...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/14646034/diff/18001/chrome/common/render_mess...
chrome/common/render_messages.h:351: OmniboxFocusState /* new_focus_state */,
Align with 'ChromeViewMsg' above.

[Donn Denman, 2013-05-14 22:39:26]

I think we decided it's an independent bug/feature that clicking in the fakebox
triggers multiple onfocuschange calls, since that won't really be a problem and
we can file a bug to address it later.

https://codereview.chromium.org/14646034/diff/18001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.h (right):

https://codereview.chromium.org/14646034/diff/18001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.h:480: // The reason for the most
recent omni-box focus change.
On 2013/05/14 22:06:23, melevin wrote:
> omnibox

Done.

https://codereview.chromium.org/14646034/diff/18001/chrome/common/render_mess...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/14646034/diff/18001/chrome/common/render_mess...
chrome/common/render_messages.h:351: OmniboxFocusState /* new_focus_state */,
On 2013/05/14 22:06:23, melevin wrote:
> Align with 'ChromeViewMsg' above.

Done.

[samarth, 2013-05-15 19:05:53]

+jschuh for IPC review

Almost ready to go. Mostly just nits below.  And also, please add a test.

Thanks!
Samarth

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (left):

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:37: // Reasons why the Omnibox
focus state could change.
This shouldn't need to change anymore, right?

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:253:
omnibox_focus_state_(OMNIBOX_FOCUS_NONE),
initialize reason_ here (to EXPLICIT)

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1155:
OMNIBOX_FOCUS_CHANGE_EXPLICIT);
Use the saved reason_ here.

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1535:
OMNIBOX_FOCUS_CHANGE_EXPLICIT);
Here as well.

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.h (right):

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.h:481: OmniboxFocusChangeReason
reason_;
s/reason_/omnibox_focus_change_reason_/

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page.h (right):

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.h:187: // Tells the page that the input
focus has changed.  This may cause the
nit: just "Tells the page that the omnibox focus has changed."

https://codereview.chromium.org/14646034/diff/23001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/14646034/diff/23001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:206: OnFocusChange)
nit: OnFocusChanged to be consistent with the rest of the functions here.

https://codereview.chromium.org/14646034/diff/23001/chrome/test/data/instant_...
File chrome/test/data/instant_extended.html (right):

https://codereview.chromium.org/14646034/diff/23001/chrome/test/data/instant_...
chrome/test/data/instant_extended.html:105: function handleFocusChange() {
A test for this would be good.  I assume you added these here for that purpose?

The test should go in instant_extended_interactive_ui_test.cc.

[Donn Denman, 2013-05-20 23:48:14]

Samarth, some great bugs caught by your recent review!  I'll work on adding a
test tomorrow and let you know when it's all ready to review.  Thanks.

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (left):

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:37: // Reasons why the Omnibox
focus state could change.
On 2013/05/15 19:05:53, samarth wrote:
> This shouldn't need to change anymore, right?

True.  However, I'd argue that it's better this way.  There may be future
changes to the omnibox focus code that would benefit from the
OmniboxFocusChangeReason being in the same file as OmniboxFocusState because
they are so closely related.

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:253:
omnibox_focus_state_(OMNIBOX_FOCUS_NONE),
On 2013/05/15 19:05:53, samarth wrote:
> initialize reason_ here (to EXPLICIT)

Done.

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1155:
OMNIBOX_FOCUS_CHANGE_EXPLICIT);
On 2013/05/15 19:05:53, samarth wrote:
> Use the saved reason_ here.

Done.

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1535:
OMNIBOX_FOCUS_CHANGE_EXPLICIT);
On 2013/05/15 19:05:53, samarth wrote:
> Here as well.

Done.

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.h (right):

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.h:481: OmniboxFocusChangeReason
reason_;
On 2013/05/15 19:05:53, samarth wrote:
> s/reason_/omnibox_focus_change_reason_/

Done.

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page.h (right):

https://codereview.chromium.org/14646034/diff/23001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.h:187: // Tells the page that the input
focus has changed.  This may cause the
On 2013/05/15 19:05:53, samarth wrote:
> nit: just "Tells the page that the omnibox focus has changed."

Done.

https://codereview.chromium.org/14646034/diff/23001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/14646034/diff/23001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:206: OnFocusChange)
On 2013/05/15 19:05:53, samarth wrote:
> nit: OnFocusChanged to be consistent with the rest of the functions here.

Done.

https://codereview.chromium.org/14646034/diff/23001/chrome/test/data/instant_...
File chrome/test/data/instant_extended.html (right):

https://codereview.chromium.org/14646034/diff/23001/chrome/test/data/instant_...
chrome/test/data/instant_extended.html:105: function handleFocusChange() {
On 2013/05/15 19:05:53, samarth wrote:
> A test for this would be good.  I assume you added these here for that
purpose?
> 
> The test should go in instant_extended_interactive_ui_test.cc.
OK, coming soon!

[Donn Denman, 2013-05-22 02:41:52]

Samarth, I think this is ready now.  

Should I add Jered for review?

[samarth, 2013-05-22 04:40:56]

+sky for c/b/ui/omnibox and c/common

LGTM with a few last comments.  Thanks for adding a test.

Samarth

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1534:
instant_tab_->FocusChanged(omnibox_focus_state_,
nit: fits on one line?

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right):

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2042: // Focus
the Omnibox.
super nit: lower-case "omnibox" (and below)

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2044:
EXPECT_TRUE(UpdateSearchState(instant()->GetOverlayContents()));
nit: EXPECT -> ASSERT here and for other calls to UpdateSearchState.

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2056:
FocusOmniboxAndWaitForInstantOverlaySupport();
Hmm, I'm surprised that this works.  In particular, waiting for
InstantOverlaySupport shouldn't be necessary and in fact should cause this test
to hang (because instant support for the overlay has been established).  You can
just call FocusOmnibox() here.

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page.h (right):

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.h:187: // Tells the page that the input
focus has changed.
nit: s/input/omnibox/

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.h:188: void FocusChanged(OmniboxFocusState
state,
nit: fits on one line?

https://codereview.chromium.org/14646034/diff/36001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/14646034/diff/36001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:205:
IPC_MESSAGE_HANDLER(ChromeViewMsg_SearchBoxFocusChanged,
nit: fits on one line?

https://codereview.chromium.org/14646034/diff/36001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:361: is_key_capture_enabled_ =
key_capture_enabled;
Put this inside the if below, or otherwise the state visible to the page will
change without a corresponding onkeycapturechange event.

[sky, 2013-05-22 15:47:38]

LGTM

[Donn Denman, 2013-05-22 17:37:36]

Thanks Samarth and sky.

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1534:
instant_tab_->FocusChanged(omnibox_focus_state_,
On 2013/05/22 04:40:56, samarth wrote:
> nit: fits on one line?
Not quite.

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right):

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2042: // Focus
the Omnibox.
On 2013/05/22 04:40:56, samarth wrote:
> super nit: lower-case "omnibox" (and below)

Done.

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2044:
EXPECT_TRUE(UpdateSearchState(instant()->GetOverlayContents()));
On 2013/05/22 04:40:56, samarth wrote:
> nit: EXPECT -> ASSERT here and for other calls to UpdateSearchState.

Ah, that makes the code much more readable!  Done.

Q: all of the other tests in this file use EXPECT for UpdateSearchState either. 
Should I go ahead and change them as well?

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2056:
FocusOmniboxAndWaitForInstantOverlaySupport();
On 2013/05/22 04:40:56, samarth wrote:
> Hmm, I'm surprised that this works.  In particular, waiting for
> InstantOverlaySupport shouldn't be necessary and in fact should cause this
test
> to hang (because instant support for the overlay has been established).  You
can
> just call FocusOmnibox() here.

Done.

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page.h (right):

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.h:187: // Tells the page that the input
focus has changed.
On 2013/05/22 04:40:56, samarth wrote:
> nit: s/input/omnibox/

Done.

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.h:188: void FocusChanged(OmniboxFocusState
state,
On 2013/05/22 04:40:56, samarth wrote:
> nit: fits on one line?

Done.

https://codereview.chromium.org/14646034/diff/36001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/14646034/diff/36001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:205:
IPC_MESSAGE_HANDLER(ChromeViewMsg_SearchBoxFocusChanged,
On 2013/05/22 04:40:56, samarth wrote:
> nit: fits on one line?

Done.

https://codereview.chromium.org/14646034/diff/36001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:361: is_key_capture_enabled_ =
key_capture_enabled;
On 2013/05/22 04:40:56, samarth wrote:
> Put this inside the if below, or otherwise the state visible to the page will
> change without a corresponding onkeycapturechange event.

Done.

https://codereview.chromium.org/14646034/diff/36001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:361: is_key_capture_enabled_ =
key_capture_enabled;
On 2013/05/22 04:40:56, samarth wrote:
> Put this inside the if below, or otherwise the state visible to the page will
> change without a corresponding onkeycapturechange event.
I was wondering about this (and should have put in a TODO to make sure you
caught it)!  This means that when typing implicitly changes the focus, the
capture change won't be visible to a script polling the isKeyCaptureEnabled
property, which didn't seem right to me.  But if that's the way it's supposed to
work then that's certainly correct.  I tested this as best I could with the
change and it all seems fine, so in practice I guess the problem I was worried
about never happens.  Great catch!

Done.

[jschuh, 2013-05-23 01:15:02]

looks like innocuous enum values. ipc security lgtm.

[samarth, 2013-05-23 21:06:33]

SLGTM

Thanks,
Samarth

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right):

https://codereview.chromium.org/14646034/diff/36001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2044:
EXPECT_TRUE(UpdateSearchState(instant()->GetOverlayContents()));
On 2013/05/22 17:37:36, Donn Denman wrote:
> On 2013/05/22 04:40:56, samarth wrote:
> > nit: EXPECT -> ASSERT here and for other calls to UpdateSearchState.
> 
> Ah, that makes the code much more readable!  Done.
> 
> Q: all of the other tests in this file use EXPECT for UpdateSearchState
either. 
> Should I go ahead and change them as well?

Yeah, in general, any failed check that would make the rest of the test
pointless to run should be an ASSERT, so yeah, I think all the others qualify.
I'd just update them in a follow-up CL, in case I'm wrong :)

[Donn Denman, 2013-05-23 21:28:16]

Filed issue https://code.google.com/p/chromium/issues/detail?id=243480 to
fix the other instances.

Should I go ahead and change them as well?
>>
>
> Yeah, in general, any failed check that would make the rest of the test
> pointless to run should be an ASSERT, so yeah, I think all the others
> qualify. I'd just update them in a follow-up CL, in case I'm wrong :)
>

[commit-bot: I haz the power, 2013-05-23 22:24:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/donnd@chromium.org/14646034/56001

[commit-bot: I haz the power, 2013-05-23 22:24:21]

Failed to apply patch for chrome/browser/ui/search/instant_controller.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/search/instant_controller.cc
  Hunk #1 succeeded at 276 (offset 25 lines).
  Hunk #2 FAILED at 927.
  Hunk #3 succeeded at 1194 (offset 29 lines).
  Hunk #4 succeeded at 1610 (offset 38 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/ui/search/instant_controller.cc.rej

Patch:       chrome/browser/ui/search/instant_controller.cc
Index: chrome/browser/ui/search/instant_controller.cc
diff --git a/chrome/browser/ui/search/instant_controller.cc
b/chrome/browser/ui/search/instant_controller.cc
index
2c2fbd7d05e34b36e5556793830fd67ce6a3b291..8b991f7e58903b4c50bf7016cc9ee26d22b7e60f
100644
--- a/chrome/browser/ui/search/instant_controller.cc
+++ b/chrome/browser/ui/search/instant_controller.cc
@@ -251,6 +251,7 @@
InstantController::InstantController(BrowserInstantController* browser,
       last_transition_type_(content::PAGE_TRANSITION_LINK),
       last_match_was_search_(false),
       omnibox_focus_state_(OMNIBOX_FOCUS_NONE),
+      omnibox_focus_change_reason_(OMNIBOX_FOCUS_CHANGE_EXPLICIT),
       omnibox_bounds_(-1, -1, 0, 0),
       allow_overlay_to_show_search_suggestions_(false),
       weak_ptr_factory_(this) {
@@ -926,25 +927,15 @@ void InstantController::OmniboxFocusChanged(
 
   OmniboxFocusState old_focus_state = omnibox_focus_state_;
   omnibox_focus_state_ = state;
+  omnibox_focus_change_reason_ = reason;
   if (!extended_enabled_ && !instant_enabled_)
     return;
 
-  // Tell the page if the key capture mode changed unless the focus state
-  // changed because of TYPING. This is because in that case, the browser
hasn't
-  // really stopped capturing key strokes.
-  //
-  // (More practically, if we don't do this check, the page would receive
-  // onkeycapturechange before the corresponding onchange, and the page would
-  // have no way of telling whether the keycapturechange happened because of
-  // some actual user action or just because they started typing.)
-  if (extended_enabled_ && GetOverlayContents() &&
-      reason != OMNIBOX_FOCUS_CHANGE_TYPING) {
-    const bool is_key_capture_enabled =
-        omnibox_focus_state_ == OMNIBOX_FOCUS_INVISIBLE;
+  if (extended_enabled_) {
     if (overlay_)
-      overlay_->KeyCaptureChanged(is_key_capture_enabled);
+      overlay_->FocusChanged(omnibox_focus_state_, reason);
     if (instant_tab_)
-      instant_tab_->KeyCaptureChanged(is_key_capture_enabled);
+      instant_tab_->FocusChanged(omnibox_focus_state_, reason);
   }
 
   if (state == OMNIBOX_FOCUS_VISIBLE && old_focus_state == OMNIBOX_FOCUS_NONE)
{
@@ -1164,8 +1155,7 @@ void InstantController::InstantPageRenderViewCreated(
   // Ensure the searchbox API has the correct initial state.
   if (IsContentsFrom(overlay(), contents)) {
     overlay_->SetDisplayInstantResults(instant_enabled_);
-    overlay_->KeyCaptureChanged(
-        omnibox_focus_state_ == OMNIBOX_FOCUS_INVISIBLE);
+    overlay_->FocusChanged(omnibox_focus_state_, omnibox_focus_change_reason_);
     overlay_->SetOmniboxBounds(omnibox_bounds_);
     overlay_->InitializeFonts();
   } else if (IsContentsFrom(ntp(), contents)) {
@@ -1572,8 +1562,8 @@ void InstantController::UpdateInfoForInstantTab() {
     instant_tab_->SetOmniboxBounds(omnibox_bounds_);
     instant_tab_->InitializeFonts();
     StartListeningToMostVisitedChanges();
-    instant_tab_->KeyCaptureChanged(
-        omnibox_focus_state_ == OMNIBOX_FOCUS_INVISIBLE);
+    instant_tab_->FocusChanged(omnibox_focus_state_,
+        omnibox_focus_change_reason_);
   }
 }

[Donn Denman, 2013-05-24 16:38:02]

These new tests are failing on mac_rel, but work on other platforms.  It looks
like the problem involves unfocusing of the omnibox.  The tests use:
  ui_test_utils::ClickOnView(browser(), VIEW_ID_TAB_CONTAINER);
but it looks like that does not trigger the onfocuschange handler on Mac.

I'll investigate more this afternoon, but if anyone has some quick guesses as to
what might be going wrong, let us know!

[commit-bot: I haz the power, 2013-05-29 16:24:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/donnd@chromium.org/14646034/71001

[Donn Denman, 2013-05-29 16:26:37]

On 2013/05/24 16:38:02, Donn Denman wrote:
> These new tests are failing on mac_rel, but work on other platforms.  It looks
> like the problem involves unfocusing of the omnibox.  The tests use:
>   ui_test_utils::ClickOnView(browser(), VIEW_ID_TAB_CONTAINER);
> but it looks like that does not trigger the onfocuschange handler on Mac.
> 
> I'll investigate more this afternoon, but if anyone has some quick guesses as
to
> what might be going wrong, let us know!

Committing now.  

I added a single line to my Mac tests that seems to fix the issues I was seeing
on Mac:
ui_test_utils::BringBrowserWindowToFront(browser())

[commit-bot: I haz the power, 2013-05-29 19:39:54]

Change committed as 202937