Instant API: tell page whether the browser is capturing key strokes.

Instant API: tell page whether the browser is capturing key strokes.


TESTED=manual, per bug
BUG=164782
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172524

[samarth, 2012-11-28 07:11:46]

PTAL

I'm really not a fan of onkeystrokecapturingmodechange so if you have ideas for
anything pithier, I'm all ears.

Thanks!
Samarth

[Jered, 2012-11-28 17:50:15]

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

https://codereview.chromium.org/11413217/diff/3001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:494:
loader_->OnStoppedCapturingKeyStrokes();
OnStopKeyCapture

https://codereview.chromium.org/11413217/diff/3001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:517:
loader_->OnStartedCapturingKeyStrokes();
OnStartKeyCapture

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

https://codereview.chromium.org/11413217/diff/3001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:228: bool
is_omnibox_invisibly_focused_;
Would it make sense to have a single enum here, like OmniboxFocusState { NONE,
INVISIBLE, FOCUS }?

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

https://codereview.chromium.org/11413217/diff/3001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/searchbox_api.js:222:
this.onkeystrokecapturingmodechange = null;
Sounds German. How about "keystrokecapturing" -> "keycap" or "keycapture". This
would give us ontogglekeycapture, onsetkeycap, onkeycapchange, or any number of
exciting names.

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

https://codereview.chromium.org/11413217/diff/3001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:220: static
v8::Handle<v8::Value> GetIsCapturingKeyStrokes(
GetIsCapturingKeyStrokes -> IsKeyCaptureEnabled.

[samarth, 2012-11-28 19:33:09]

Improved and shortened the naming throughout.

Thanks,
Samarth

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

https://codereview.chromium.org/11413217/diff/3001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:494:
loader_->OnStoppedCapturingKeyStrokes();
On 2012/11/28 17:50:16, Jered wrote:
> OnStopKeyCapture

Now just OnKeyCaptureChange.

https://codereview.chromium.org/11413217/diff/3001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:517:
loader_->OnStartedCapturingKeyStrokes();
On 2012/11/28 17:50:16, Jered wrote:
> OnStartKeyCapture

Now just OnKeyCaptureChange.

https://codereview.chromium.org/11413217/diff/3001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:517:
loader_->OnStartedCapturingKeyStrokes();
On 2012/11/28 17:50:16, Jered wrote:
> OnStartKeyCapture

Now just OnKeyCaptureChange.

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

https://codereview.chromium.org/11413217/diff/3001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:228: bool
is_omnibox_invisibly_focused_;
On 2012/11/28 17:50:16, Jered wrote:
> Would it make sense to have a single enum here, like OmniboxFocusState { NONE,
> INVISIBLE, FOCUS }?

We could do that.  The downside is that just checking if the omnibox has focus
becomes complicated (need to check two values).  But that's fine by me if you
prefer that.

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

https://codereview.chromium.org/11413217/diff/3001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/searchbox_api.js:222:
this.onkeystrokecapturingmodechange = null;
On 2012/11/28 17:50:16, Jered wrote:
> Sounds German. How about "keystrokecapturing" -> "keycap" or "keycapture".
This
> would give us ontogglekeycapture, onsetkeycap, onkeycapchange, or any number
of
> exciting names.

Went with onKeyCaptureChange throughout.

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

https://codereview.chromium.org/11413217/diff/3001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:220: static
v8::Handle<v8::Value> GetIsCapturingKeyStrokes(
On 2012/11/28 17:50:16, Jered wrote:
> GetIsCapturingKeyStrokes -> IsKeyCaptureEnabled.

Done.

[Jered, 2012-11-28 19:56:56]

lgtm with nit

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

https://codereview.chromium.org/11413217/diff/3001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:228: bool
is_omnibox_invisibly_focused_;
On 2012/11/28 19:33:09, samarth wrote:
> On 2012/11/28 17:50:16, Jered wrote:
> > Would it make sense to have a single enum here, like OmniboxFocusState {
NONE,
> > INVISIBLE, FOCUS }?
> 
> We could do that.  The downside is that just checking if the omnibox has focus
> becomes complicated (need to check two values).  But that's fine by me if you
> prefer that.

I think we could just say: omnibox_focus_type_ != NONE, right? This is mostly
aesthetic, I mostly prefer it rather than introducing an "impossible" value.

[samarth, 2012-11-28 22:08:10]

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

https://codereview.chromium.org/11413217/diff/3001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:228: bool
is_omnibox_invisibly_focused_;
On 2012/11/28 19:56:57, Jered wrote:
> On 2012/11/28 19:33:09, samarth wrote:
> > On 2012/11/28 17:50:16, Jered wrote:
> > > Would it make sense to have a single enum here, like OmniboxFocusState {
> NONE,
> > > INVISIBLE, FOCUS }?
> > 
> > We could do that.  The downside is that just checking if the omnibox has
focus
> > becomes complicated (need to check two values).  But that's fine by me if
you
> > prefer that.
> 
> I think we could just say: omnibox_focus_type_ != NONE, right? This is mostly
> aesthetic, I mostly prefer it rather than introducing an "impossible" value.

Duh, you're right.  Done.

[sreeram, 2012-11-29 13:22:31]

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

https://codereview.chromium.org/11413217/diff/4013/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:492: SendKeyCaptureModeToPage();
IsCurrent() returns false when on the NTP. Don't you want to send the keycapture
mode when on the NTP? If so, you should put this above the IsCurrent() check.

https://codereview.chromium.org/11413217/diff/4013/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:858:
loader_->OnKeyCaptureChange(omnibox_focus_state_ == INVISIBLE);
You should guard this on these two clauses (either here, or at the call sites):
if (extended_enabled_ && GetPreviewContents())
  loader_->OnKeyCaptureChange(...);

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

https://codereview.chromium.org/11413217/diff/4013/chrome/common/render_messa...
chrome/common/render_messages.h:332:
IPC_MESSAGE_ROUTED0(ChromeViewMsg_SearchBoxStoppedCapturingKeyStrokes)
This message doesn't seem to be needed anymore. Remove?

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

https://codereview.chromium.org/11413217/diff/4013/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.cc:19: last_results_base_(0),
Initialize is_key_capture_enabled_ here.

[samarth, 2012-11-29 17:28:20]

jschuh -> Please review the IPC change in chrome/common/

Thanks for the review, Sreeram!

Samarth

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

https://codereview.chromium.org/11413217/diff/4013/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:492: SendKeyCaptureModeToPage();
On 2012/11/29 13:22:31, sreeram wrote:
> IsCurrent() returns false when on the NTP. Don't you want to send the
keycapture
> mode when on the NTP? If so, you should put this above the IsCurrent() check.

Good catch.  Fixed it so that the Send..() call is always right after the
omnibox focus changes.

https://codereview.chromium.org/11413217/diff/4013/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:858:
loader_->OnKeyCaptureChange(omnibox_focus_state_ == INVISIBLE);
On 2012/11/29 13:22:31, sreeram wrote:
> You should guard this on these two clauses (either here, or at the call
sites):
> if (extended_enabled_ && GetPreviewContents())
>   loader_->OnKeyCaptureChange(...);

Added the checks here to be safe.

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

https://codereview.chromium.org/11413217/diff/4013/chrome/common/render_messa...
chrome/common/render_messages.h:332:
IPC_MESSAGE_ROUTED0(ChromeViewMsg_SearchBoxStoppedCapturingKeyStrokes)
On 2012/11/29 13:22:31, sreeram wrote:
> This message doesn't seem to be needed anymore. Remove?

Done.

[Jered, 2012-11-29 17:32:47]

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

https://codereview.chromium.org/11413217/diff/7002/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.cc:12:
SearchBox::SearchBox(content::RenderView* render_view)
Initialize is_key_capture_enabled_ in the constructor.

[samarth, 2012-11-29 17:38:44]

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

https://codereview.chromium.org/11413217/diff/7002/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.cc:12:
SearchBox::SearchBox(content::RenderView* render_view)
On 2012/11/29 17:32:48, Jered wrote:
> Initialize is_key_capture_enabled_ in the constructor.

Done.

[sreeram, 2012-11-29 17:43:44]

lgtm

[jschuh, 2012-11-29 17:58:27]

lgtm for ipc.

[samarth, 2012-12-05 19:03:59]

Peter: this is a follow on to my previous change. We tell the instant overlay if
we're actually capturing key strokes so we can handle cases like:
1) user clicks on the searchbox on the overlay
2) user clicks on the omnibox again
The overlay needs to know that visible focus has been restored so it can handle
this correctly.

I also folded in your final comments from the last patch.

Sreeram: do you have time for another quick pass over the instant code? I had to
rebase this past your big refactoring.

Thanks,
Samarth

[sreeram, 2012-12-05 19:48:23]

Only nits. I don't have time today to think through what's happening in
InstantController::OmniboxFocusChanged() (sorry!), but I guess you'll figure it
out if something breaks.

LGTM

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

https://codereview.chromium.org/11413217/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:550: // (More practically, if we
don't do this check, the page would receieve
Nit: receieve -> receive

https://codereview.chromium.org/11413217/diff/16001/chrome/common/instant_typ...
File chrome/common/instant_types.h (right):

https://codereview.chromium.org/11413217/diff/16001/chrome/common/instant_typ...
chrome/common/instant_types.h:157: FOCUS_CHANGE_EXPLICIT,
Add line breaks for clarity (a la InstantShownReason).

https://codereview.chromium.org/11413217/diff/16001/chrome/common/instant_typ...
chrome/common/instant_types.h:158: // Focus changed to restore state from a tab
the user swtched to.
Nit: swtched -> switched

[Peter Kasting, 2012-12-06 01:34:03]

On 2012/12/05 19:03:59, samarth wrote:
> Peter: this is a follow on to my previous change.

Can you be clearer about what precisely I should review?

[samarth, 2012-12-06 01:36:02]

Oh sorry, please review the stuff in chrome/browser/ui.

Thanks,
Samarth

[Peter Kasting, 2012-12-06 06:02:30]

I just thought of something else I didn't think of last night... trying to set
the focus to invisible when we have a selection should probably do to the
selection whatever killing focus does to it.

I know, I know, you guys don't want to worry about the case where there's text
in the omnibox because you don't trigger it, but I'm really uncomfortable
building APIs with such assumptions subtly written into them :(

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

https://codereview.chromium.org/11413217/diff/16001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:226: // - SetFocus() is
explicitly called again
Nit: SetFocus() -> OmniboxView::SetFocus() ?

https://codereview.chromium.org/11413217/diff/16001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:231: // called but not
OnSetFocus.)
Rather than write so much here, perhaps we need more detail in the comments on
SetFocus() and OnSetFocus() that we can simply refer to from here, which will
make it clear precisely what triggers each one.  Because just from reading your
comments, I'm not sure how to generalize as a reader and guess when one will be
called versus the other.

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

https://codereview.chromium.org/11413217/diff/16001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:378:
(!textfield_->HasFocus() || !model()->is_caret_visible()))
This isn't right... select_all_on_mouse_release_ should contain this condition,
and then we should SetCaretVisibility() iff that member is true.  The way you've
written this, clicking when we have invisible focus doesn't select all, and it
should.

[Mathieu, 2012-12-06 13:17:26]

On 2012/12/06 06:02:30, Peter Kasting wrote:
> I just thought of something else I didn't think of last night... trying to set
> the focus to invisible when we have a selection should probably do to the
> selection whatever killing focus does to it.
> 
> I know, I know, you guys don't want to worry about the case where there's text
> in the omnibox because you don't trigger it, but I'm really uncomfortable
> building APIs with such assumptions subtly written into them :(
> 
>
https://codereview.chromium.org/11413217/diff/16001/chrome/browser/ui/omnibox...
> File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):
> 
>
https://codereview.chromium.org/11413217/diff/16001/chrome/browser/ui/omnibox...
> chrome/browser/ui/omnibox/omnibox_edit_model.h:226: // - SetFocus() is
> explicitly called again
> Nit: SetFocus() -> OmniboxView::SetFocus() ?
> 
>
https://codereview.chromium.org/11413217/diff/16001/chrome/browser/ui/omnibox...
> chrome/browser/ui/omnibox/omnibox_edit_model.h:231: // called but not
> OnSetFocus.)
> Rather than write so much here, perhaps we need more detail in the comments on
> SetFocus() and OnSetFocus() that we can simply refer to from here, which will
> make it clear precisely what triggers each one.  Because just from reading
your
> comments, I'm not sure how to generalize as a reader and guess when one will
be
> called versus the other.
> 
>
https://codereview.chromium.org/11413217/diff/16001/chrome/browser/ui/views/o...
> File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):
> 
>
https://codereview.chromium.org/11413217/diff/16001/chrome/browser/ui/views/o...
> chrome/browser/ui/views/omnibox/omnibox_view_views.cc:378:
> (!textfield_->HasFocus() || !model()->is_caret_visible()))
> This isn't right... select_all_on_mouse_release_ should contain this
condition,
> and then we should SetCaretVisibility() iff that member is true.  The way
you've
> written this, clicking when we have invisible focus doesn't select all, and it
> should.

Hi Peter,
I agree that we have not fully tested the invisible focus API with text and it
needs to be done. I've already started setting up the invisible focus on a timer
to be able to test this. I'll report/fix anything I find.
Sent from mobile

[samarth, 2012-12-06 16:15:50]

Ok, I tried to make the comments clearer; please let me know if this is better.

Re text the omnibox, I completely agree.  One other possibility though is to
restrict SetCaret(In)Visible to when the omnibox has no text in it (both in
documentation and implementation).  Perhaps we could think of some situation
where it might be useful, but it seems bad that the user will definitely not
know where within the text their keystrokes will appear.
Another nice side-effect is that the fact that typing restores visibility falls
out naturally as a result of this restriction.

But if we don't do that, I agree we should make sure this works in the cases
where we have text in the omnibox.

Thanks,
Samarth

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

https://codereview.chromium.org/11413217/diff/16001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:226: // - SetFocus() is
explicitly called again
On 2012/12/06 06:02:30, Peter Kasting wrote:
> Nit: SetFocus() -> OmniboxView::SetFocus() ?

Done.

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

https://codereview.chromium.org/11413217/diff/16001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:378:
(!textfield_->HasFocus() || !model()->is_caret_visible()))
On 2012/12/06 06:02:30, Peter Kasting wrote:
> This isn't right... select_all_on_mouse_release_ should contain this
condition,
> and then we should SetCaretVisibility() iff that member is true.  The way
you've
> written this, clicking when we have invisible focus doesn't select all, and it
> should.

Ah ok, makes sense. Done.

[Peter Kasting, 2012-12-07 23:22:02]

chrome/browser/ui LGTM with comments

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

https://codereview.chromium.org/11413217/diff/25001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:727: }
This block seems like it duplicates SetCaretVisibilityInternal(), I think you
should remove it?

https://codereview.chromium.org/11413217/diff/25001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:747: void
OmniboxEditModel::SetCaretVisibilityInternal(
Function definition order should match .h declaration order.

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

https://codereview.chromium.org/11413217/diff/25001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:413: // passed down to
InstantController when we notify it of focus state changes.
Nit: How about:

Actually changes the current caret visibility.  When the new visibility doesn't
match the old, we notify the InstantController about the change and pass
|reason| to it.

[samarth, 2012-12-08 00:55:47]

+sky: please review for chrome/common/instant_types.h

Thanks!
Samarth

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

https://codereview.chromium.org/11413217/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:550: // (More practically, if we
don't do this check, the page would receieve
On 2012/12/05 19:48:24, sreeram wrote:
> Nit: receieve -> receive

Done.

https://codereview.chromium.org/11413217/diff/16001/chrome/common/instant_typ...
File chrome/common/instant_types.h (right):

https://codereview.chromium.org/11413217/diff/16001/chrome/common/instant_typ...
chrome/common/instant_types.h:157: FOCUS_CHANGE_EXPLICIT,
On 2012/12/05 19:48:24, sreeram wrote:
> Add line breaks for clarity (a la InstantShownReason).

Done.

https://codereview.chromium.org/11413217/diff/16001/chrome/common/instant_typ...
chrome/common/instant_types.h:158: // Focus changed to restore state from a tab
the user swtched to.
On 2012/12/05 19:48:24, sreeram wrote:
> Nit: swtched -> switched

Done.

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

https://codereview.chromium.org/11413217/diff/25001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:727: }
On 2012/12/07 23:22:03, Peter Kasting wrote:
> This block seems like it duplicates SetCaretVisibilityInternal(), I think you
> should remove it?

It's sometimes necessary. Added comment.

https://codereview.chromium.org/11413217/diff/25001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:747: void
OmniboxEditModel::SetCaretVisibilityInternal(
On 2012/12/07 23:22:03, Peter Kasting wrote:
> Function definition order should match .h declaration order.

Done.

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

https://codereview.chromium.org/11413217/diff/25001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:413: // passed down to
InstantController when we notify it of focus state changes.
On 2012/12/07 23:22:03, Peter Kasting wrote:
> Nit: How about:
> 
> Actually changes the current caret visibility.  When the new visibility
doesn't
> match the old, we notify the InstantController about the change and pass
> |reason| to it.

Done.

[Peter Kasting, 2012-12-08 01:12:19]

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

https://codereview.chromium.org/11413217/diff/25001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:724: if (InstantController*
instant = controller_->GetInstant()) {
Oh, sorry, didn't notice this before.

Please do not use assignment-inside-conditional.  Do the assignments outside the
conditionals.  Please fix all the spots in this file like this.

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

https://codereview.chromium.org/11413217/diff/32001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:726: // send an update if the
caret was already visible.
But in that case why do we need to send an update?  The InstantController
shouldn't care, unless it doesn't realize the caret is already visible, in which
case you have a bug somewhere else.  No?

[samarth, 2012-12-08 01:51:34]

Thanks for the review Peter!

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

https://codereview.chromium.org/11413217/diff/25001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:724: if (InstantController*
instant = controller_->GetInstant()) {
On 2012/12/08 01:12:20, Peter Kasting wrote:
> Oh, sorry, didn't notice this before.
> 
> Please do not use assignment-inside-conditional.  Do the assignments outside
the
> conditionals.  Please fix all the spots in this file like this.

Happy to, I really don't like them either.

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

https://codereview.chromium.org/11413217/diff/32001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:726: // send an update if the
caret was already visible.
On 2012/12/08 01:12:21, Peter Kasting wrote:
> But in that case why do we need to send an update?  The InstantController
> shouldn't care, unless it doesn't realize the caret is already visible, in
which
> case you have a bug somewhere else.  No?

It's because InstantController also needs to know whether the omnibox has focus
or not (for other reasons), as in it cares it cares when the state changes from
FOCUS_NONE to FOCUS_VISIBLE.

[Peter Kasting, 2012-12-08 02:07:51]

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

https://codereview.chromium.org/11413217/diff/32001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:726: // send an update if the
caret was already visible.
On 2012/12/08 01:51:34, samarth wrote:
> On 2012/12/08 01:12:21, Peter Kasting wrote:
> > But in that case why do we need to send an update?  The InstantController
> > shouldn't care, unless it doesn't realize the caret is already visible, in
> which
> > case you have a bug somewhere else.  No?
> 
> It's because InstantController also needs to know whether the omnibox has
focus
> or not (for other reasons), as in it cares it cares when the state changes
from
> FOCUS_NONE to FOCUS_VISIBLE.

Perhaps we should move this to the top of the function and call it only if
!has_focus_.

It would also be good to change the comment to something like:

"SetCaretVisibility() will notify the InstantController if caret visibility
changes while we have focus, but it also needs to know when we actually gain
focus to begin with, so we have to manually notify here."

This also makes me wonder about a larger change: combine the caret visibility
and focus tracking bools in the edit to a single caret visibility member that
uses the FOCUS_VISIBLE, FOCUS_INVISIBLE, FOCUS_NONE values.  Do this right and I
suspect we eliminate or at least clarify a lot of separate calls to the instant
controller.

[sky, 2012-12-10 15:15:39]

https://codereview.chromium.org/11413217/diff/35001/chrome/common/instant_typ...
File chrome/common/instant_types.h (right):

https://codereview.chromium.org/11413217/diff/35001/chrome/common/instant_typ...
chrome/common/instant_types.h:148: FOCUS_NONE,  // Not focused.
Name this enums more specifically, eg OMNIBOX_FOCUS_STATE_NONE and
OMNIBOX_FOCUS_CHANGE_EXPLICIT. Also, why are these enums here and not in the
omnibox code?

[samarth, 2012-12-10 23:44:59]

Ok, how about this? I agree: this makes things much easier to understand.

Thanks,
Samarth

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

https://codereview.chromium.org/11413217/diff/32001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:726: // send an update if the
caret was already visible.
On 2012/12/08 02:07:52, Peter Kasting wrote:
> On 2012/12/08 01:51:34, samarth wrote:
> > On 2012/12/08 01:12:21, Peter Kasting wrote:
> > > But in that case why do we need to send an update?  The InstantController
> > > shouldn't care, unless it doesn't realize the caret is already visible, in
> > which
> > > case you have a bug somewhere else.  No?
> > 
> > It's because InstantController also needs to know whether the omnibox has
> focus
> > or not (for other reasons), as in it cares it cares when the state changes
> from
> > FOCUS_NONE to FOCUS_VISIBLE.
> 
> Perhaps we should move this to the top of the function and call it only if
> !has_focus_.
> 
> It would also be good to change the comment to something like:
> 
> "SetCaretVisibility() will notify the InstantController if caret visibility
> changes while we have focus, but it also needs to know when we actually gain
> focus to begin with, so we have to manually notify here."
> 
> This also makes me wonder about a larger change: combine the caret visibility
> and focus tracking bools in the edit to a single caret visibility member that
> uses the FOCUS_VISIBLE, FOCUS_INVISIBLE, FOCUS_NONE values.  Do this right and
I
> suspect we eliminate or at least clarify a lot of separate calls to the
instant
> controller.

Done.

https://codereview.chromium.org/11413217/diff/35001/chrome/common/instant_typ...
File chrome/common/instant_types.h (right):

https://codereview.chromium.org/11413217/diff/35001/chrome/common/instant_typ...
chrome/common/instant_types.h:148: FOCUS_NONE,  // Not focused.
On 2012/12/10 15:15:39, sky wrote:
> Name this enums more specifically, eg OMNIBOX_FOCUS_STATE_NONE and
> OMNIBOX_FOCUS_CHANGE_EXPLICIT.

Done.

> Also, why are these enums here and not in the
> omnibox code?

This definitely doesn't belong in instant_types anymore since as Peter
suggested, I'm using these types directly in OmniboxEditModel. I could add them
as public enums in OmniboxEditModel but then InstantController needs to pull in
all of omnibox_edit_model.h.  I assume that's bad (let me know if it isn't), so
I pulled this out into chrome/common/omnibox_types.h.

[sky, 2012-12-11 00:12:45]

https://codereview.chromium.org/11413217/diff/35001/chrome/common/instant_typ...
File chrome/common/instant_types.h (right):

https://codereview.chromium.org/11413217/diff/35001/chrome/common/instant_typ...
chrome/common/instant_types.h:148: FOCUS_NONE,  // Not focused.
On 2012/12/10 23:45:00, samarth wrote:
> On 2012/12/10 15:15:39, sky wrote:
> > Name this enums more specifically, eg OMNIBOX_FOCUS_STATE_NONE and
> > OMNIBOX_FOCUS_CHANGE_EXPLICIT.
> 
> Done.
> 
> > Also, why are these enums here and not in the
> > omnibox code?
> 
> This definitely doesn't belong in instant_types anymore since as Peter
> suggested, I'm using these types directly in OmniboxEditModel. I could add
them
> as public enums in OmniboxEditModel but then InstantController needs to pull
in
> all of omnibox_edit_model.h.  I assume that's bad (let me know if it isn't),
so
> I pulled this out into chrome/common/omnibox_types.h.

This is specific to the omnibox, it should be in that directory. In general
chrome/common isn't meant to be a random grab bag of stuff.

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

[samarth, 2012-12-11 00:25:16]

Ok, moved it to browser/ui/omnibox. Peter, please let me know if you'd rather I
just put it inside OmniboxEditModel and pull in the full header in
InstantController.

Thanks,
Samarth

[sky, 2012-12-11 00:55:10]

LGTM

[samarth, 2012-12-11 18:06:43]

Thanks sky!  Peter: did you have any more comments here?

Thanks,
Samarth

[Peter Kasting, 2012-12-11 19:20:50]

LGTM

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

https://codereview.chromium.org/11413217/diff/50001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:739: SetFocusState(visible ?
OMNIBOX_FOCUS_VISIBLE : OMNIBOX_FOCUS_INVISIBLE,
I wonder if maybe we should just nuke SetCaretVisibility() and make
SetFocusState() public?  Not sure.

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

https://codereview.chromium.org/11413217/diff/50001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:199: }
Normally I would not provide the has_focus() and is_caret_visible() accessors
given the focus_state() accessor, since callers can cheaply implement these
themselves.

I might be tempted to reverse this, though, if there were lots of callers or
adding them ended up saving a number of lines of code.  Do you know what the
impact would be of removing these?

https://codereview.chromium.org/11413217/diff/50001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_types.h (right):

https://codereview.chromium.org/11413217/diff/50001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_types.h:6: #define
CHROME_BROWSER_UI_OMNIBOX_OMNIBOX_TYPES_H_
I would probably put the contents of this file into the OmniboxEditModel header,
because they're basically core concepts to the edit model.

https://codereview.chromium.org/11413217/diff/50001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_types.h:13: // Visibily focused.
Nit: Visibly

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

https://codereview.chromium.org/11413217/diff/50001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:366:
model()->focus_state() == OMNIBOX_FOCUS_INVISIBLE);
Nit: Really minor, but I prefer to put parens around binary subexpressions.

https://codereview.chromium.org/11413217/diff/50001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

https://codereview.chromium.org/11413217/diff/50001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1778: model()->focus_state()
== OMNIBOX_FOCUS_INVISIBLE) &&
Nit: Same comment.

[samarth, 2012-12-12 01:11:25]

Thanks for the review!

Samarth

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

https://codereview.chromium.org/11413217/diff/50001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:739: SetFocusState(visible ?
OMNIBOX_FOCUS_VISIBLE : OMNIBOX_FOCUS_INVISIBLE,
On 2012/12/11 19:20:50, Peter Kasting wrote:
> I wonder if maybe we should just nuke SetCaretVisibility() and make
> SetFocusState() public?  Not sure.

That seems like overpromising.  We'd have to put a bunch of checks and most of
the state changes would not get honored (e.g. trying to go from NONE to
FOCUS_VISIBLE from some random code).

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

https://codereview.chromium.org/11413217/diff/50001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:199: }
On 2012/12/11 19:20:50, Peter Kasting wrote:
> Normally I would not provide the has_focus() and is_caret_visible() accessors
> given the focus_state() accessor, since callers can cheaply implement these
> themselves.
> 
> I might be tempted to reverse this, though, if there were lots of callers or
> adding them ended up saving a number of lines of code.  Do you know what the
> impact would be of removing these?

I left these in because I thought it made the call-sites much clearer
(is_focus() rather than focus_state() != OMNIBOX_FOCUS_NONE).

But how about I send you a follow up CL that kills these two and replaces them
with the equivalent, and we can decide if that is better or worse? I think it'll
be easier to decide in context of an actual change.

https://codereview.chromium.org/11413217/diff/50001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_types.h (right):

https://codereview.chromium.org/11413217/diff/50001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_types.h:6: #define
CHROME_BROWSER_UI_OMNIBOX_OMNIBOX_TYPES_H_
On 2012/12/11 19:20:50, Peter Kasting wrote:
> I would probably put the contents of this file into the OmniboxEditModel
header,
> because they're basically core concepts to the edit model.

Done.

https://codereview.chromium.org/11413217/diff/50001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_types.h:13: // Visibily focused.
On 2012/12/11 19:20:50, Peter Kasting wrote:
> Nit: Visibly

Done.

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

https://codereview.chromium.org/11413217/diff/50001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:366:
model()->focus_state() == OMNIBOX_FOCUS_INVISIBLE);
On 2012/12/11 19:20:50, Peter Kasting wrote:
> Nit: Really minor, but I prefer to put parens around binary subexpressions.

Done.

https://codereview.chromium.org/11413217/diff/50001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

https://codereview.chromium.org/11413217/diff/50001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1778: model()->focus_state()
== OMNIBOX_FOCUS_INVISIBLE) &&
On 2012/12/11 19:20:50, Peter Kasting wrote:
> Nit: Same comment.

Done.

[commit-bot: I haz the power, 2012-12-12 01:11:46]

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

[Peter Kasting, 2012-12-12 01:21:42]

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

https://codereview.chromium.org/11413217/diff/50001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:199: }
On 2012/12/12 01:11:26, samarth wrote:
> On 2012/12/11 19:20:50, Peter Kasting wrote:
> > Normally I would not provide the has_focus() and is_caret_visible()
accessors
> > given the focus_state() accessor, since callers can cheaply implement these
> > themselves.
> > 
> > I might be tempted to reverse this, though, if there were lots of callers or
> > adding them ended up saving a number of lines of code.  Do you know what the
> > impact would be of removing these?
> 
> I left these in because I thought it made the call-sites much clearer
> (is_focus() rather than focus_state() != OMNIBOX_FOCUS_NONE).
> 
> But how about I send you a follow up CL that kills these two and replaces them
> with the equivalent, and we can decide if that is better or worse? I think
it'll
> be easier to decide in context of an actual change.

OK.

[commit-bot: I haz the power, 2012-12-12 02:25:07]

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

[commit-bot: I haz the power, 2012-12-12 04:18:21]

Change committed as 172524