Implement {Start,Stop}CapturingKeyStrokes for Instant.

Implement {Start,Stop}CapturingKeyStrokes for Instant.

Implement {Start,Stop}CapturingKeyStrokes for Instant by transferring 
focus invisibly to the omnibox. This patch implements invisible focus for
Aura. Other platforms will follow.

BUG=

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

[samarth, 2012-11-08 18:36:22]

Please review
sreeram -> chrome/renderer/searchbox/..., chrome/browser/instant/...
sky -> chrome/common/..., chrome/browser/ui/...
jered -> sanity check

As I mentioned in the TODO, we'll probably need to fiddle with the api as we
figure out the interactions between the omnibox and static searchbox.  But I'd
like to get this in so we can start using it.

Thanks,
Samarth

[sreeram, 2012-11-12 14:48:44]

Just a quick status update: Samarth & others are playing with some fakebox
options. The final solution may need more IPCs than just this one. So, I'm
guessing there's no need to review this until he pings again (when the dust has
settled on those experiments).

[samarth, 2012-11-15 17:53:37]

Ok, I redid this change significantly: the API now just communicates the web
searchbox focus state to the browser, which uses SearchModel to keep track of
it.

Please look at the following parts
sreeram: searchbox, instant
dhollowa: chrome/browser/ui/search
sky: chrome/browser/ui/, chrome/common/

Thanks,
Samarth

[Mathieu, 2012-11-15 20:37:20]

http://codereview.chromium.org/11369137/diff/5012/chrome/browser/ui/search/se...
File chrome/browser/ui/search/search_tab_helper.cc (right):

http://codereview.chromium.org/11369137/diff/5012/chrome/browser/ui/search/se...
chrome/browser/ui/search/search_tab_helper.cc:89:
model_.SetMode(Mode(Mode::MODE_NTP_WEB_SEARCHBOX_HAS_FOCUS, false));
I think this is missing the origin parameter (2nd param). Same below.

[dhollowa, 2012-11-15 21:31:00]

http://codereview.chromium.org/11369137/diff/5012/chrome/browser/instant/inst...
File chrome/browser/instant/instant_controller.cc (right):

http://codereview.chromium.org/11369137/diff/5012/chrome/browser/instant/inst...
chrome/browser/instant/instant_controller.cc:633:
browser_->OnWebSearchBoxBlur();
BrowserInstantController is not needed to set this.  You should be able to call
|chrome::search::SearchTabHelper::FromWebContents| on the current tab and set
the focus state directly.

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

http://codereview.chromium.org/11369137/diff/5012/chrome/browser/ui/search/se...
chrome/browser/ui/search/search_types.h:53: MODE_NTP_WEB_SEARCHBOX_HAS_FOCUS,
I'm not so crazy about mixing this in with the Mode.  Seems like to model should
have a boolean for focus state.

[Mathieu, 2012-11-20 23:26:01]

Comments for patch set 6.

https://chromiumcodereview.appspot.com/11369137/diff/12002/chrome/browser/ins...
File chrome/browser/instant/instant_controller.h (right):

https://chromiumcodereview.appspot.com/11369137/diff/12002/chrome/browser/ins...
chrome/browser/instant/instant_controller.h:127: void
OnWebSearchBoxFocusChange(InstantLoader* loader, bool is_foxued);
*is_focused
*OnWebSearchBoxFocusChanged to be consistent with function right above
(InstantSupportDetermined)

https://chromiumcodereview.appspot.com/11369137/diff/12002/chrome/browser/ui/...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):

https://chromiumcodereview.appspot.com/11369137/diff/12002/chrome/browser/ui/...
chrome/browser/ui/omnibox/omnibox_edit_model.h:219: void
OnFocusVisibilityChange(bool visible_focus);
Again, *Changed may be better.

https://chromiumcodereview.appspot.com/11369137/diff/12002/chrome/browser/ui/...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

https://chromiumcodereview.appspot.com/11369137/diff/12002/chrome/browser/ui/...
chrome/browser/ui/omnibox/omnibox_view.h:155: // Sets the focus to the
autocomplete view but doesn't visible show focus to
*visibly

[samarth, 2012-11-20 23:31:02]

dhollowa: What do you think of this approach? I still need to make the
platform-specific changes for other platforms.

sky: let me take this off your plate while I iterate on this with the other
reviewers. I'll either add you back (or perhaps pkasting) once it settles down.

Thanks,
Samarth

[samarth, 2012-11-20 23:38:56]

FYI, just to keep you apprised for what the next steps here are:
1) Actually implement focus visibility (mathp@ is working on this)
2) Add back the omnibox focus/blur state into the searchbox API along with the
focus visibility bit. This way the instant overlay can properly react to the
omnibox going from an invisible focus state to a visible focus state.

Thanks,
Samarth

[dhollowa, 2012-11-20 23:50:17]

Yes, this seems like a better direction to me...  It keeps the focus visibility
state closer to the focus state - a more logical grouping IMO.

https://codereview.chromium.org/11369137/diff/9002/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11369137/diff/9002/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:625: browser_->SetInvisibleFocus();
I assume defocusing the fakebox by clicking elsewhere on the page, say, will
*actually* defocus the Omnibox, yes?

https://codereview.chromium.org/11369137/diff/9002/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11369137/diff/9002/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:127: void
OnWebSearchBoxFocusChange(InstantLoader* loader, bool is_foxued);
nit: |is_foxued|?  I hope so! ;-)

https://codereview.chromium.org/11369137/diff/9002/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11369137/diff/9002/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:116: if
(browser_->window()->GetLocationBar())
nit: need {} on if-body.

https://codereview.chromium.org/11369137/diff/9002/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11369137/diff/9002/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:729: is_focus_visible_ =
is_focus_visible;
Question: is there an invariant here that is_focus_visible_ == true implies
has_focus_ == true?  I.e. should we force it?  Or will other code be responsible
for setting has_focus_, therefore a DCHECK here?

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

https://codereview.chromium.org/11369137/diff/9002/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:174: static
v8::Handle<v8::Value> WebSearchBoxFocusChange(const v8::Arguments& args);
nit: 80col

[samarth, 2012-11-21 00:40:15]

Thanks for the quick reviews.  Addressed all your comment.  Working on other
platforms now.

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

https://codereview.chromium.org/11369137/diff/12002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:127: void
OnWebSearchBoxFocusChange(InstantLoader* loader, bool is_foxued);
On 2012/11/20 23:26:01, Mathieu Perreault wrote:
> *is_focused
> *OnWebSearchBoxFocusChanged to be consistent with function right above
> (InstantSupportDetermined)

Done.  Also renamed all the other methods to be consistent.

https://codereview.chromium.org/11369137/diff/12002/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):

https://codereview.chromium.org/11369137/diff/12002/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:219: void
OnFocusVisibilityChange(bool visible_focus);
On 2012/11/20 23:26:01, Mathieu Perreault wrote:
> Again, *Changed may be better.

Done.

https://codereview.chromium.org/11369137/diff/12002/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

https://codereview.chromium.org/11369137/diff/12002/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_view.h:155: // Sets the focus to the
autocomplete view but doesn't visible show focus to
On 2012/11/20 23:26:01, Mathieu Perreault wrote:
> *visibly

Done.

https://codereview.chromium.org/11369137/diff/9002/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11369137/diff/9002/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:625: browser_->SetInvisibleFocus();
On 2012/11/20 23:50:17, dhollowa wrote:
> I assume defocusing the fakebox by clicking elsewhere on the page, say, will
> *actually* defocus the Omnibox, yes?

Yes, that's handled as it is normally.

https://codereview.chromium.org/11369137/diff/9002/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11369137/diff/9002/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:127: void
OnWebSearchBoxFocusChange(InstantLoader* loader, bool is_foxued);
On 2012/11/20 23:50:17, dhollowa wrote:
> nit: |is_foxued|?  I hope so! ;-)

fox·ued
n.  1. Pursed by a fox

https://codereview.chromium.org/11369137/diff/9002/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11369137/diff/9002/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:116: if
(browser_->window()->GetLocationBar())
On 2012/11/20 23:50:17, dhollowa wrote:
> nit: need {} on if-body.

Done.

https://codereview.chromium.org/11369137/diff/9002/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11369137/diff/9002/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:729: is_focus_visible_ =
is_focus_visible;
On 2012/11/20 23:50:17, dhollowa wrote:
> Question: is there an invariant here that is_focus_visible_ == true implies
> has_focus_ == true?  I.e. should we force it?  Or will other code be
responsible
> for setting has_focus_, therefore a DCHECK here?

My preference was to completely decouple focus "visibility" from whether the box
actually has focus.  Or in other words, it shouldn't matter which order you set
these in--the end result would be the same.  Tying the two together can be
tricky because plenty of things rely on focus and plenty of low-level components
can give or take focus away.

I tried to clarify this in the comments for is_focus_visible_ in
omnibox_edit_model.h.  Let me know if it's still unclear of if you prefer
different semantics.

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

https://codereview.chromium.org/11369137/diff/9002/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:174: static
v8::Handle<v8::Value> WebSearchBoxFocusChange(const v8::Arguments& args);
On 2012/11/20 23:50:17, dhollowa wrote:
> nit: 80col

Done.

[dhollowa, 2012-11-21 00:55:18]

Will wait for the platform bits for final review.  Thanks.

https://codereview.chromium.org/11369137/diff/9002/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11369137/diff/9002/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:729: is_focus_visible_ =
is_focus_visible;
On 2012/11/21 00:40:15, samarth wrote:
> On 2012/11/20 23:50:17, dhollowa wrote:
> > Question: is there an invariant here that is_focus_visible_ == true implies
> > has_focus_ == true?  I.e. should we force it?  Or will other code be
> responsible
> > for setting has_focus_, therefore a DCHECK here?
> 
> My preference was to completely decouple focus "visibility" from whether the
box
> actually has focus.  Or in other words, it shouldn't matter which order you
set
> these in--the end result would be the same.  Tying the two together can be
> tricky because plenty of things rely on focus and plenty of low-level
components
> can give or take focus away.
> 
> I tried to clarify this in the comments for is_focus_visible_ in
> omnibox_edit_model.h.  Let me know if it's still unclear of if you prefer
> different semantics.

That's fine if we can get away with it.  Thanks for the clarification.

[samarth, 2012-11-21 01:04:50]

Ok, added Windows implementation (minus the actual visibility part which Mathieu
will handle).  Left Mac and GTK NOTIMPLEMENTED for now.

Kicking off tryjobs for these, since I don't have machines for these to test on
(or even compile).

Thanks,
Samarth

[samarth, 2012-11-26 22:56:10]

Peter: please review for chrome/browser/ui/...

As I say in the description, this just sets up the stub for invisible focus.
Mathieu already has a version for Aura and Windows is in the works, and he will
follow up with patches after my change lands.

Sreeram: please review the instant pieces.
David: you can continue reviewing but there's no longer any bits in
chrome/browser/ui/search/...

Thanks,
Samarth

[dhollowa, 2012-11-27 22:00:53]

Removing myself as reviewer.  This current direction doesn't touch search::
pieces.

[Mathieu, 2012-11-28 17:56:10]

LGTM with regards to the interface with OmniboxView that I will be using to
implement the invisible focus.

nit: Update the description to remove the current state of the work.

https://codereview.chromium.org/11369137/diff/19014/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

https://codereview.chromium.org/11369137/diff/19014/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_view.h:156: // the user (i.e. no blinking
caret or border highlight).
nit: On Aura, Windows and Gtk, I have not seen border highlight. I would leave
this out.

[samarth, 2012-11-28 19:42:43]

Thanks.  Fix the change description too.

Samarth

https://codereview.chromium.org/11369137/diff/19014/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

https://codereview.chromium.org/11369137/diff/19014/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_view.h:156: // the user (i.e. no blinking
caret or border highlight).
On 2012/11/28 17:56:10, Mathieu Perreault wrote:
> nit: On Aura, Windows and Gtk, I have not seen border highlight. I would leave
> this out.

The highlight only exists on Mac afaik. I'll take this out.

[samarth, 2012-11-29 00:05:33]

Peter, Sreeram: friendly ping?

[sreeram, 2012-11-29 14:25:24]

The CL itself is innocuous, so lgtm, but I have questions about the future
plans: What are the conditions under which the omnibox will put back the visible
focus on its own, without needing a StopCapturingKeyStrokes() call? I assume
that at the very least it's when the user presses a key (but what if the user
was pressing Ctrl plus char, such as Ctrl-T)?

[Mathieu, 2012-11-29 14:47:43]

On 2012/11/29 14:25:24, sreeram wrote:
> The CL itself is innocuous, so lgtm, but I have questions about the future
> plans: What are the conditions under which the omnibox will put back the
visible
> focus on its own, without needing a StopCapturingKeyStrokes() call? I assume
> that at the very least it's when the user presses a key (but what if the user
> was pressing Ctrl plus char, such as Ctrl-T)?

It's good to mention that I have another change that will go with this one:
https://codereview.chromium.org/11418144/.

A new tab will restore the visibility of the caret in the omnibox, as well as a
the first key press (done in OmniboxViewViews::OnBeforePossibleChange).

[samarth, 2012-11-29 16:57:52]

palmer: please review the IPC change in chrome/common

Thanks for the review Sreeram. As Mathieu mentioned, with his patch, each tab
keeps track of focus visibility. So there are a few ways you can lose invisible
focus:
1) Omnibox loses focus altogether (e.g. tabbing out, clicking somewhere else)
2) Text makes it into the omnibox -- we switch to visible focus before this
happens
3) Switch to a tab that doesn't have invisible focus

Is there anything else we're missing?

Thanks,
Samarth

[samarth, 2012-11-29 16:58:54]

(Actually added palmer this time.)

palmer: please review the IPC change in chrome/common

Thanks!
Samarth

[palmer, 2012-11-29 19:46:09]

LGTM from an IPC security point of view.

[Peter Kasting, 2012-12-01 03:49:57]

For chrome/browser/ui, I'm not really a fan of checking in a bunch of stubs here
and then implementing them in your other CL.  For example, in the other CL I
noted that you should perhaps move the API function external consumers call to
be on the model instead of the view, and change the conditions under which it's
called.  Working these sorts of issues out properly really requires that your
callers and implementations all be in one changelist.  So I think you should
combine them.

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

https://codereview.chromium.org/11369137/diff/20001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:134: if
(browser_->window()->GetLocationBar()) {
When will the window not have a location bar?  We should never NULL-check things
that can't be NULL.

https://codereview.chromium.org/11369137/diff/20001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/11369137/diff/20001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:448: // OmniboxViewViews,
AutocompleteEditView implementation:
Actually this class is called "OmniboxView" now anyway.

[samarth, 2012-12-03 06:46:15]

Thanks for the review Peter.  Per your suggestion, I folded in Mathieu's Aura
implementation into this change.  I'll leave the windows version out since it's
a bit more complicated (and I also don't have a windows set up to test on).

Let me know what you think.

Thanks!
Samarth

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

https://codereview.chromium.org/11369137/diff/20001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:134: if
(browser_->window()->GetLocationBar()) {
On 2012/12/01 03:49:58, Peter Kasting wrote:
> When will the window not have a location bar?  We should never NULL-check
things
> that can't be NULL.

I was just following the pattern in SetSuggestion before.  But if this can never
be NULL, I'll just take out the check here and above.

https://codereview.chromium.org/11369137/diff/20001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/11369137/diff/20001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:448: // OmniboxViewViews,
AutocompleteEditView implementation:
On 2012/12/01 03:49:58, Peter Kasting wrote:
> Actually this class is called "OmniboxView" now anyway.

Done.

[Mathieu, 2012-12-03 20:42:08]

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

https://codereview.chromium.org/11369137/diff/19028/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:135:
omnibox_view->model()->SetFocusVisibility(false);
Please switch this line and the line below.

[samarth, 2012-12-03 21:22:21]

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

https://codereview.chromium.org/11369137/diff/19028/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:135:
omnibox_view->model()->SetFocusVisibility(false);
On 2012/12/03 20:42:09, Mathieu Perreault wrote:
> Please switch this line and the line below.

Done.

[Peter Kasting, 2012-12-04 02:00:51]

This seems basically OK, but I have a couple questions.

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

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.h:67: void SetInvisibleFocus();
Nit: Maybe this should be:

  // Invoked by |instant_| to give the omnibox focus invisibly.
  void FocusOmniboxInvisibly();

...since we're not really setting anything on the controller per se.

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:760: is_focus_visible_ = true;
I'm a little confused about the distinction between setting focus to visible
here in OmniboxEditModel::OnKillFocus() versus setting it in
OmniboxViewViews::SetFocus().  Does OmniboxViewViews::SetFocus() call through to
OmniboxEditModel::OnSetFocus() eventually?  If so, should the change to focus
visibility be moved from the view to that function in the edit?  

Basically, the core question here is about why we don't have parallel structure.

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:195: 
Nit: Blank line unnecessary

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:221: // currently has focus or
not.
Nit: Add note like "This setting will be reset to "visible" if the omnibox loses
focus (except via a tab switch, which preserves the visibility state), or if
..." (and then fill in the other conditions under which it auto-changes)

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_view.h:155: // Styles the omnibox according to
the visibility of the focus stored in the
Nit: Awkward... how about "Shows or hides the caret based on whether the model's
is_focus_visible() is true."

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_view.h:157: virtual void
ApplyFocusVisibility() = 0;
Nit: Should we maybe call all these functions (in view and model?) "set caret
visibility" (etc.) instead of "set focus visibility"?  After all, we're really
just changing whether the caret is visible.

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:584: if
(textfield_->cursor_color() != textfield_->background_color()) {
Nit: No {}

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:620: // the user directly
clicks on the omnibox as well as when a character is
Really, this is called when the user clicks on the omnibox?  Through what
callstack?  That doesn't sound correct to me.  Anyway, doesn't the added line in
SetFocus() handle that case?

[samarth, 2012-12-04 04:55:34]

Thanks for the comments.  Addressed them all below.

Also, the way we've structured this now, SetCaretVisiblity only has meaning if
you call it when omnibox already has focus, so I made that dependency more
explicit.

Samarth

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

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.h:67: void SetInvisibleFocus();
On 2012/12/04 02:00:52, Peter Kasting wrote:
> Nit: Maybe this should be:
> 
>   // Invoked by |instant_| to give the omnibox focus invisibly.
>   void FocusOmniboxInvisibly();
> 
> ...since we're not really setting anything on the controller per se.

Done.

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:760: is_focus_visible_ = true;
On 2012/12/04 02:00:52, Peter Kasting wrote:
> I'm a little confused about the distinction between setting focus to visible
> here in OmniboxEditModel::OnKillFocus() versus setting it in
> OmniboxViewViews::SetFocus().  Does OmniboxViewViews::SetFocus() call through
to
> OmniboxEditModel::OnSetFocus() eventually?  If so, should the change to focus
> visibility be moved from the view to that function in the edit?  
> 
> Basically, the core question here is about why we don't have parallel
structure.

So the call in SetFocus can't be moved to OnSetFocus.  The latter is only
triggered by focus change from the underlying textfield.  So for example, if you
have a tab open with invisible focus and then open a second tab, this will
trigger calls to SetFocus but not to OnSetFocus.

But you're right. we should make this parallel and so I moved the reseting of
is_caret_visible to HandleFocusOut in the view.

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:195: 
On 2012/12/04 02:00:52, Peter Kasting wrote:
> Nit: Blank line unnecessary

Done.

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:221: // currently has focus or
not.
On 2012/12/04 02:00:52, Peter Kasting wrote:
> Nit: Add note like "This setting will be reset to "visible" if the omnibox
loses
> focus (except via a tab switch, which preserves the visibility state), or if
> ..." (and then fill in the other conditions under which it auto-changes)

Done.

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_view.h:155: // Styles the omnibox according to
the visibility of the focus stored in the
On 2012/12/04 02:00:52, Peter Kasting wrote:
> Nit: Awkward... how about "Shows or hides the caret based on whether the
model's
> is_focus_visible() is true."

Done.

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_view.h:157: virtual void
ApplyFocusVisibility() = 0;
On 2012/12/04 02:00:52, Peter Kasting wrote:
> Nit: Should we maybe call all these functions (in view and model?) "set caret
> visibility" (etc.) instead of "set focus visibility"?  After all, we're really
> just changing whether the caret is visible.

Done.

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:584: if
(textfield_->cursor_color() != textfield_->background_color()) {
On 2012/12/04 02:00:52, Peter Kasting wrote:
> Nit: No {}

Done.

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:620: // the user directly
clicks on the omnibox as well as when a character is
On 2012/12/04 02:00:52, Peter Kasting wrote:
> Really, this is called when the user clicks on the omnibox?  Through what
> callstack? 

It comes through OnBeforeUserAction() which is triggered by the click. 
Semantically, I find it strange too.

> That doesn't sound correct to me.  Anyway, doesn't the added line in
> SetFocus() handle that case?

SetFocus() doesn't get triggered when you click.  We could handle clicks
separately in HandleMouseReleaseEvent I think.  Is there something equivalent
for typing, i.e. a function that only gets called when the omnibox gets text
(but not when it is clicked)?  It would be cleaner to separate the two cases if
possible.

[sreeram, 2012-12-04 21:54:57]

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

https://codereview.chromium.org/11369137/diff/18028/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:748: if (extended_enabled_ &&
!instant_tab_ && loader_->contents() == contents &&
You can drop the "loader_->contents() == contents" clause.

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

https://codereview.chromium.org/11369137/diff/18028/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:135: void
StartCapturingKeyStrokes(const content::WebContents* contents);
You don't need to pass in |contents| if only InstantLoader is expected to call
this. (cf: ShowInstantPreview).

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

https://codereview.chromium.org/11369137/diff/18028/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:286: // NOTE(samarth): we don't do
anything else yet in this case.
Should this be a "TODO(samarth): Do something" instead?

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

https://codereview.chromium.org/11369137/diff/18028/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:756: if (render_view)
Perhaps follow the existing style (here and in the other method below):

if (!render_view) return v8::Undefined();

DVLOG(1) << render_view << " StartCapturingKeyStrokes";
SearchBox::Get(...)->...;
return v8::Undefined();

[samarth, 2012-12-04 22:13:43]

Addressed all your comments Sreeram.

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

https://codereview.chromium.org/11369137/diff/18028/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:748: if (extended_enabled_ &&
!instant_tab_ && loader_->contents() == contents &&
On 2012/12/04 21:54:58, sreeram wrote:
> You can drop the "loader_->contents() == contents" clause.

Done.

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

https://codereview.chromium.org/11369137/diff/18028/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:135: void
StartCapturingKeyStrokes(const content::WebContents* contents);
On 2012/12/04 21:54:58, sreeram wrote:
> You don't need to pass in |contents| if only InstantLoader is expected to call
> this. (cf: ShowInstantPreview).

Done.

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

https://codereview.chromium.org/11369137/diff/18028/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:286: // NOTE(samarth): we don't do
anything else yet in this case.
On 2012/12/04 21:54:58, sreeram wrote:
> Should this be a "TODO(samarth): Do something" instead?

Expanded the comment.

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

https://codereview.chromium.org/11369137/diff/18028/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:756: if (render_view)
On 2012/12/04 21:54:58, sreeram wrote:
> Perhaps follow the existing style (here and in the other method below):
> 
> if (!render_view) return v8::Undefined();
> 
> DVLOG(1) << render_view << " StartCapturingKeyStrokes";
> SearchBox::Get(...)->...;
> return v8::Undefined();

Done.

[sreeram, 2012-12-04 22:16:14]

SLGTM

[Peter Kasting, 2012-12-04 22:23:31]

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/11369137/diff/16008/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:620: // the user directly
clicks on the omnibox as well as when a character is
On 2012/12/04 04:55:35, samarth wrote:
> On 2012/12/04 02:00:52, Peter Kasting wrote:
> > Really, this is called when the user clicks on the omnibox?  Through what
> > callstack? 
> 
> It comes through OnBeforeUserAction() which is triggered by the click. 
> Semantically, I find it strange too.

That's odd.

> > That doesn't sound correct to me.  Anyway, doesn't the added line in
> > SetFocus() handle that case?
> 
> SetFocus() doesn't get triggered when you click.  We could handle clicks
> separately in HandleMouseReleaseEvent I think.

You really want just the case where the click gives us focus.  What about when
we gain focus due to e.g. taborder traversal in the UI?

I'm just wondering if some particular
SetFocus()/HandleFocusIn()/OnSetFocus()/something function is the right one to
hook here.

> Is there something equivalent
> for typing, i.e. a function that only gets called when the omnibox gets text
> (but not when it is clicked)?  It would be cleaner to separate the two cases
if
> possible.

You could try inserting something near the beginning of
OmniboxEditModel::OnAfterPossibleChange() that sets visibility to true if
(text_differs || selection_differs).  Make sure to test that with IMEs.

[samarth, 2012-12-04 22:47:12]

How about this?

As I say in the comments. HandleFocusIn() isn't sufficient for the case where
the omnibox has focus with a hidden caret and the user clicks again the omnibox.
But this can be handled by hooking on the click listener.

I'll need to flash a device to test with IMEs. But I'm pretty confident that if
this doesn't work, it should be fixable (we confirmed earlier that
OnBeforePossibleChange is definitely called when using IMEs).

Thanks,
Samarth

[Peter Kasting, 2012-12-04 22:55:48]

https://codereview.chromium.org/11369137/diff/17049/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/11369137/diff/17049/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:394:
model()->SetCaretVisibility(true);
Now that we've moved this to a function that unconditionally calls the model's
OnSetFocus() function, I think this can in fact be moved to that function, which
is a cross-platform place to do it.

https://codereview.chromium.org/11369137/diff/17049/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:416:
model()->SetCaretVisibility(true);
Why is this necessary if we set this on HandleFocusIn() (or OnSetFocus())?

https://codereview.chromium.org/11369137/diff/17049/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:652: // in the omnibox.
I was hoping to put this in the model's OnAfterPossibleChange function, since
that's a cross-platform place to do it.

[samarth, 2012-12-04 22:58:06]

Yep, those all make a lot of sense. Let me test it out and I'll upload another
patchset.

(Thanks for the quick turnaround on these reviews!)

Samarth

[samarth, 2012-12-04 23:13:32]

Alright, I think I have all the cases covered now:
- If text is entered (in the model)
- If omnibox receives focus (OnSetFocus in model)
- If you click on it while it has invisible focus (view)
- If we call SetFocus() while it has invisible focus (view)
  - this wont trigger any of the 3 above.

I also updated the comment for SetCaretVisibility with all of these cases.

Thanks,
Samarth

https://codereview.chromium.org/11369137/diff/17049/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/11369137/diff/17049/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:394:
model()->SetCaretVisibility(true);
On 2012/12/04 22:55:49, Peter Kasting wrote:
> Now that we've moved this to a function that unconditionally calls the model's
> OnSetFocus() function, I think this can in fact be moved to that function,
which
> is a cross-platform place to do it.

Done.

https://codereview.chromium.org/11369137/diff/17049/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:416:
model()->SetCaretVisibility(true);
On 2012/12/04 22:55:49, Peter Kasting wrote:
> Why is this necessary if we set this on HandleFocusIn() (or OnSetFocus())?

We'd added it out of paranoia before, but not necessary anymore.

https://codereview.chromium.org/11369137/diff/17049/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:652: // in the omnibox.
On 2012/12/04 22:55:49, Peter Kasting wrote:
> I was hoping to put this in the model's OnAfterPossibleChange function, since
> that's a cross-platform place to do it.

Done.

[Peter Kasting, 2012-12-05 00:52:53]

LGTM with question

https://codereview.chromium.org/11369137/diff/17077/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/11369137/diff/17077/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:582: // Restore caret
visibility if focused explicitly.
Nit: Add: "We need to do this here because if we already have invisible focus,
the RequestFocus() call below will short-circuit, preventing us from reaching
OmniboxEditModel::OnSetFocus(), which handles restoring visibility when we
didn't previously have focus."

...assuming I got the rationale correct.

Although... is this SetFocus() call called in a superset of the cases where
OnSetFocus() is reached?  Because if so, putting the call in there doesn't
actually buy us anything, so we could take it back out (and then update the
comment above to explain this).

[samarth, 2012-12-05 01:15:50]

Thanks all for the reviews!  Testing one final time and will then commit.

Samarth

https://codereview.chromium.org/11369137/diff/17077/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/11369137/diff/17077/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:582: // Restore caret
visibility if focused explicitly.
On 2012/12/05 00:52:53, Peter Kasting wrote:
> Nit: Add: "We need to do this here because if we already have invisible focus,
> the RequestFocus() call below will short-circuit, preventing us from reaching
> OmniboxEditModel::OnSetFocus(), which handles restoring visibility when we
> didn't previously have focus."
> 
> ...assuming I got the rationale correct.

Yes, exactly; updated comment.

> 
> Although... is this SetFocus() call called in a superset of the cases where
> OnSetFocus() is reached?  Because if so, putting the call in there doesn't
> actually buy us anything, so we could take it back out (and then update the
> comment above to explain this).

Unfortunately no--either can happen independently.  For example, if you tab into
the omnibox, OnSetFocus will be called but not SetFocus.  If the omnibox has
invisible focus and you open a second tab, SetFocus will be called but not
OnSetFocus.

[commit-bot: I haz the power, 2012-12-05 01:19:47]

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

[Peter Kasting, 2012-12-05 01:20:58]

On 2012/12/05 01:15:50, samarth wrote:
> Unfortunately no--either can happen independently.  For example, if you tab
into
> the omnibox, OnSetFocus will be called but not SetFocus.  If the omnibox has
> invisible focus and you open a second tab, SetFocus will be called but not
> OnSetFocus.

Make sure this kind of information is captured in comments somewhere.  Future
spelunkers will doubtless wonder the same thing.

[Peter Kasting, 2012-12-05 02:10:07]

Hey, I just thought of something else.

When the user clicks in the omnibox when it has invisible focus, that click
should act as if the omnibox didn't have focus at all.  Besides restoring focus
visibility, that means clicks (that aren't drags) should select all.

Now, maybe you won't actually hit this because you never set invisible focus
unless the box is empty.  But the code doesn't actually enforce that.  So
probably you should modify HandleMousePressEvent() to check
(!textfield_->HasFocus() || !model()->is_caret_visible()).  I think that's
probably enough to make this safe.

[samarth, 2012-12-05 02:23:04]

On 2012/12/05 02:10:07, Peter Kasting wrote:
> Hey, I just thought of something else.
> 
> When the user clicks in the omnibox when it has invisible focus, that click
> should act as if the omnibox didn't have focus at all.  Besides restoring
focus
> visibility, that means clicks (that aren't drags) should select all.
> 
> Now, maybe you won't actually hit this because you never set invisible focus
> unless the box is empty.  But the code doesn't actually enforce that.  So
> probably you should modify HandleMousePressEvent() to check
> (!textfield_->HasFocus() || !model()->is_caret_visible()).  I think that's
> probably enough to make this safe.

Yep, right--right now, this only ever happens with no text, but we should handle
it correctly.  Let me send you a patch with this fix and the updated comment you
requested.

By the way, I was thinking a bit more and it seems a little messy that sometimes
the view restores visibility and sometimes the model does.  What do you think
about adding OnMousePressed() and OnFocusRequested() to the model that all the
views just call into.  That way, restoring the caret is internal to the model. 
We could also simplify and limit the API to just SetInvisibleCaret (since
SetCaretVisibility(true) is no longer necessary).

What do you think?

Thanks,
Samarth

[Peter Kasting, 2012-12-05 02:41:54]

On 2012/12/05 02:23:04, samarth wrote:
> By the way, I was thinking a bit more and it seems a little messy that
sometimes
> the view restores visibility and sometimes the model does.  What do you think
> about adding OnMousePressed() and OnFocusRequested() to the model that all the
> views just call into.  That way, restoring the caret is internal to the model.

> We could also simplify and limit the API to just SetInvisibleCaret (since
> SetCaretVisibility(true) is no longer necessary).

Be careful adding something called "OnMousePressed()".  See my comments in
mathp's Win-specific change on how to implement the right focus/selection
behavior; following that implementation would leave him in a scenario where he
wouldn't be calling into the model every time a mouse press event happens.

In fact, I don't think you should either -- I think the code you're adding in
HandleMousePressEvent() should only trigger when |select_all_on_mouse_release_|
has just been set to true.  Notably, this prevents something like re-showing the
caret on a middle-click, which we don't want to do, because we want to ignore
middle-clicks entirely.

Once you've made this sort of change, you'll have a better perspective on any
model API you want.  I'm not convinced the change you propose is actually an
improvement -- there's nothing inherently wrong with the view wanting to make
the caret invisible, and by abstracting it in the way you describe, you add to
the model API surface and possibly obscure the intent of the calls on the view
side.  I'm not 100% convinced it's a bad idea, I'm just not sure.

[Peter Kasting, 2012-12-05 04:32:43]

Just for clarity, here, since I already LGTMed this, you don't need another
sign-off if you implement my feedback above.  In case you (or someone else)
wants to land this soonest.

[gideonwald, 2012-12-05 04:33:27]

On 2012/12/05 04:32:43, Peter Kasting wrote:
> Just for clarity, here, since I already LGTMed this, you don't need another
> sign-off if you implement my feedback above.  In case you (or someone else)
> wants to land this soonest.

> (or someone else)

:)

[commit-bot: I haz the power, 2012-12-05 04:42:14]

Change committed as 171160