Instant extended: Remove suggest commit on lost focus

Instant extended: Remove suggest commit on lost focus

With this CL we no longer commit instant suggest text when the
omnibox loses focus.

Also see the following discussion:
https://code.google.com/p/chromium/issues/detail?id=179981#c15

BUG=179981, 12664004
TEST=On Mac, ChromeOS, and Windows tested the following:
  - Typed "hello w"
  - clicked on the page and verified that suggest text remained
    uncommited
  - clicked back in the omnibox and pressed right arrow, verified
    that the suggest text was commited
Did the same test except instead of clicking on the page I clicked
into the find bar. Verified that this also worked.

Also tested switching tabs and verified that suggest text is not lost.

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

[sail, 2013-03-15 00:54:41]

sreeram, samarth: Let me know if this looks ok and I'll fix up the tests too.

[samarth, 2013-03-15 01:02:34]

I'll take a look at this more closely tomorrow.  But here's an important case to
try out:
1) Go to cnn.com
2) Type something in the omnibox that gives you gray text
3) Click on the page

Step 3 will commit the page but we should ideally still keep the gray text in
the omnibox.

Thanks,
Samarth

[Peter Kasting, 2013-03-15 01:29:58]

Rubber-stamp LGTM for omnibox OWNERS on this

[sail, 2013-03-15 17:31:25]

On 2013/03/15 01:02:34, samarth wrote:
> I'll take a look at this more closely tomorrow.  But here's an important case
to
> try out:
> 1) Go to http://cnn.com
> 2) Type something in the omnibox that gives you gray text
> 3) Click on the page
> 
> Step 3 will commit the page but we should ideally still keep the gray text in
> the omnibox.

So after step 2, if I wait for a while the page will commit (the tab title will
change and get a google favicon). Then clicking on the page will keep gray text.

If after step 2 I click on the page right away the gray text gets committed.

Should I fix this?

[sreeram, 2013-03-15 18:22:54]

On 2013/03/15 17:31:25, sail wrote:
> On 2013/03/15 01:02:34, samarth wrote:
> > I'll take a look at this more closely tomorrow.  But here's an important
case
> to
> > try out:
> > 1) Go to http://cnn.com
> > 2) Type something in the omnibox that gives you gray text
> > 3) Click on the page
> > 
> > Step 3 will commit the page but we should ideally still keep the gray text
in
> > the omnibox.
> 
> So after step 2, if I wait for a while the page will commit (the tab title
will
> change and get a google favicon). Then clicking on the page will keep gray
text.
> 
> If after step 2 I click on the page right away the gray text gets committed.
> 
> Should I fix this?

As discussed offline, yes, please fix cases where we commit an overlay and cause
the gray text to also be committed. Unlike what we discussed though, I think you
may need to move the gray text to the omnibox model so that it can be properly
restored when switching tabs and back. I'll send you a doc that Jered wrote
documenting the desired behaviour.

[sail, 2013-03-22 20:48:08]

sreeram, samarth: Please take another look. The new code saves suggest text when
switching tabs. I've also tested this on Mac, ChromeOS and Windows.

If you're ok with this I'll fix and update tests.

Thanks

[sreeram, 2013-03-28 15:08:54]

Wow. This is a complex change. Thanks for doing this! Awesome work!

It looks alright at first glance, but I really need to think through corner
cases. One of my concerns is that, it appears the suggest text doesn't
automatically get erased for most SetText() or edit type situations, so you're
relying on something else to erase the suggest text (i.e., DoInstant() returning
false or Instant calling SetInstantSuggestion()). This might be an issue,
particularly for browser tests.

So, please ping pkasting to also start reviewing this right away.

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

https://codereview.chromium.org/12792013/diff/17001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:198: (suggest_text.empty() ||
new_permanent_text != user_text_ + suggest_text);
Hmm... Try this:

0. Start from the NTP.
1. Type "f" into the omnibox, so that "acebook" appears as gray text.
2. Click on the top suggestion, which is "facebook".
-> I'd expect the omnibox to show "facebook" in all black.
--> My reading of the above clause suggests that the "acebook" will remain gray.
Right?

[sail, 2013-03-28 21:17:51]

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

https://codereview.chromium.org/12792013/diff/17001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:198: (suggest_text.empty() ||
new_permanent_text != user_text_ + suggest_text);
On 2013/03/28 15:08:54, sreeram wrote:
> Hmm... Try this:
> 
> 0. Start from the NTP.
> 1. Type "f" into the omnibox, so that "acebook" appears as gray text.
> 2. Click on the top suggestion, which is "facebook".
> -> I'd expect the omnibox to show "facebook" in all black.
> --> My reading of the above clause suggests that the "acebook" will remain
gray.
> Right?

I was nervous about this change as well but it seems to work ok.
In your scenario the suggestion is correctly cleared by this code:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[sail, 2013-03-28 21:18:37]

pkasting: Could you take another look. With the latest patch we now restore
instant suggest text on tab switch.

[Peter Kasting, 2013-03-30 18:48:21]

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

https://codereview.chromium.org/12792013/diff/24001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:198: (suggest_text.empty() ||
new_permanent_text != user_text_ + suggest_text);
Why do we have to add this final clause?  At the least, we need to expand the
comment above to discuss this.

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

https://codereview.chromium.org/12792013/diff/24001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:75: const string16 suggest_text;
Nit: Maybe name this "instant_suggestion" since that's what the API calls it.

[sreeram, 2013-04-01 20:17:37]

LGTM

I spent some time trying to reason about corner cases, but couldn't come up with
any issues. It would be great if you could run through a few scenarios on all
platforms (switching tabs, clicking on suggestions, unfocusing+refocusing,
unfocusing+navigating, etc) and make sure it all works (i.e., sorry I don't have
time to patch this in and test it myself). Thanks!

[sail, 2013-04-01 20:20:16]

On 2013/04/01 20:17:37, sreeram wrote:
> LGTM
> 
> I spent some time trying to reason about corner cases, but couldn't come up
with
> any issues. It would be great if you could run through a few scenarios on all
> platforms (switching tabs, clicking on suggestions, unfocusing+refocusing,
> unfocusing+navigating, etc) and make sure it all works (i.e., sorry I don't
have
> time to patch this in and test it myself). Thanks!

Thanks for taking a look. I'll test it tonight. I'll ping pkasting again.

[sail, 2013-04-02 21:34:49]

On 2013/04/01 20:20:16, sail wrote:
> On 2013/04/01 20:17:37, sreeram wrote:
> > LGTM
> > 
> > I spent some time trying to reason about corner cases, but couldn't come up
> with
> > any issues. It would be great if you could run through a few scenarios on
all
> > platforms (switching tabs, clicking on suggestions, unfocusing+refocusing,
> > unfocusing+navigating, etc) and make sure it all works (i.e., sorry I don't
> have
> > time to patch this in and test it myself). Thanks!
> 
> Thanks for taking a look. I'll test it tonight. I'll ping pkasting again.

I tested Windows, ChromeOS and Mac with the above scenarios and it looks good.

[sail, 2013-04-02 21:35:21]

Hey Scott, it looks like pkasting is out. Could you do a OWNERS review? Thanks.

[Peter Kasting, 2013-04-02 21:53:36]

On 2013/04/02 21:35:21, sail wrote:
> Hey Scott, it looks like pkasting is out. Could you do a OWNERS review?
Thanks.

I'm not out; you said you'd ping me again.  I was waiting until you made it
clear you wanted me to look.

[sail, 2013-04-02 22:02:50]

On 2013/04/02 21:53:36, Peter Kasting wrote:
> On 2013/04/02 21:35:21, sail wrote:
> > Hey Scott, it looks like pkasting is out. Could you do a OWNERS review?
> Thanks.
> 
> I'm not out; you said you'd ping me again.  I was waiting until you made it
> clear you wanted me to look.

Oh, sorry Peter! Yea, if you could take a look that'd be great.

[sail, 2013-04-02 22:03:07]

-sky since pkasting is taking a look.

[sail, 2013-04-02 22:04:02]

On 2013/04/02 21:53:36, Peter Kasting wrote:
> On 2013/04/02 21:35:21, sail wrote:
> > Hey Scott, it looks like pkasting is out. Could you do a OWNERS review?
> Thanks.
> 
> I'm not out; you said you'd ping me again.  I was waiting until you made it
> clear you wanted me to look.

Oh, I'm must be brain dead. You did comment on this CL 3 days ago. K, addressing
your comments now. Thanks.

[sail, 2013-04-02 22:32:06]

pkasting: addressed review comments. Please take a look.

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

https://codereview.chromium.org/12792013/diff/24001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:198: (suggest_text.empty() ||
new_permanent_text != user_text_ + suggest_text);
On 2013/03/30 18:48:21, Peter Kasting wrote:
> Why do we have to add this final clause?  At the least, we need to expand the
> comment above to discuss this.

This prevents the omnibox text from changing when a search result is commited.
For example:
  1. type "hello w"
  2. get "hello w" + suggest text "orld"
  3. wait a few seconds and page is commited and this code gets hit
After #3 this code gets run.
Updated comments.

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

https://codereview.chromium.org/12792013/diff/24001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:75: const string16 suggest_text;
On 2013/03/30 18:48:21, Peter Kasting wrote:
> Nit: Maybe name this "instant_suggestion" since that's what the API calls it.

Done.

[Peter Kasting, 2013-04-03 00:35:35]

LGTM

https://codereview.chromium.org/12792013/diff/34008/chrome/browser/ui/cocoa/o...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/12792013/diff/34008/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:502: void
OmniboxViewMac::OnStartingIME() {
Can this function be eliminated entirely?

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

https://codereview.chromium.org/12792013/diff/34008/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:195: // for the full text.
This added comment describes what, not why.  You need to say why.

Instead of adding this sentence, start a new paragraph and say this:

If the page is auto-committing an instant suggestion, however, we generally
don't want to make any change to the edit.  While auto-commits modify the
underlying permanent URL, they're intended to have no effect on the user's
editing process -- before and after the auto-commit, the omnibox should show the
same user text and the same instant suggestion, even if the auto-commit happens
while the edit doesn't have focus.

https://codereview.chromium.org/12792013/diff/34008/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:196: string16 suggest_text =
view_->GetInstantSuggestion();
Nit: Name this |instant_suggestion|

[sail, 2013-04-03 19:34:11]

https://codereview.chromium.org/12792013/diff/34008/chrome/browser/ui/cocoa/o...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/12792013/diff/34008/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:502: void
OmniboxViewMac::OnStartingIME() {
On 2013/04/03 00:35:35, Peter Kasting wrote:
> Can this function be eliminated entirely?

Done.
Good idea.

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

https://codereview.chromium.org/12792013/diff/34008/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:195: // for the full text.
On 2013/04/03 00:35:35, Peter Kasting wrote:
> This added comment describes what, not why.  You need to say why.
> 
> Instead of adding this sentence, start a new paragraph and say this:
> 
> If the page is auto-committing an instant suggestion, however, we generally
> don't want to make any change to the edit.  While auto-commits modify the
> underlying permanent URL, they're intended to have no effect on the user's
> editing process -- before and after the auto-commit, the omnibox should show
the
> same user text and the same instant suggestion, even if the auto-commit
happens
> while the edit doesn't have focus.

Done.

https://codereview.chromium.org/12792013/diff/34008/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:196: string16 suggest_text =
view_->GetInstantSuggestion();
On 2013/04/03 00:35:35, Peter Kasting wrote:
> Nit: Name this |instant_suggestion|

Done.

[commit-bot: I haz the power, 2013-04-05 02:10:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/12792013/48001

[commit-bot: I haz the power, 2013-04-05 02:10:06]

Failed to apply patch for
chrome/browser/ui/search/instant_extended_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/search/instant_extended_browsertest.cc
  Hunk #1 FAILED at 334.
  Hunk #2 FAILED at 377.
  2 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/ui/search/instant_extended_browsertest.cc.rej

Patch:       chrome/browser/ui/search/instant_extended_browsertest.cc
Index: chrome/browser/ui/search/instant_extended_browsertest.cc
diff --git a/chrome/browser/ui/search/instant_extended_browsertest.cc
b/chrome/browser/ui/search/instant_extended_browsertest.cc
index
3bfeb77f672bc69d825a0f9a724e7d26d86aef8e..ace1eebdcf36b75892c523fed907ca26346c6b79
100644
--- a/chrome/browser/ui/search/instant_extended_browsertest.cc
+++ b/chrome/browser/ui/search/instant_extended_browsertest.cc
@@ -334,24 +334,14 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
OmniboxTextUponFocusLostCommit) {
   // Commit the overlay by lost focus (e.g. clicking on the page).
   instant()->CommitIfPossible(INSTANT_COMMIT_FOCUS_LOST);
 
-  // Search term extraction should kick in with the autocompleted text.
-  EXPECT_EQ(ASCIIToUTF16("johnny depp"), omnibox()->GetText());
-
-  // Suggestion should be cleared at this point.
-  EXPECT_EQ(ASCIIToUTF16(""), omnibox()->GetInstantSuggestion());
+  // Omnibox text and suggestion should not be changed.
+  EXPECT_EQ(ASCIIToUTF16("johnny"), omnibox()->GetText());
+  EXPECT_EQ(ASCIIToUTF16(" depp"), omnibox()->GetInstantSuggestion());
 }
 
 // Test that omnibox text is correctly set when clicking on committed SERP.
-// Disabled on Mac because omnibox focus loss is not working correctly.
-#if defined(OS_MACOSX)
-#define MAYBE_OmniboxTextUponFocusedCommittedSERP \
-    DISABLED_OmniboxTextUponFocusedCommittedSERP
-#else
-#define MAYBE_OmniboxTextUponFocusedCommittedSERP \
-    OmniboxTextUponFocusedCommittedSERP
-#endif
 IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
-                       MAYBE_OmniboxTextUponFocusedCommittedSERP) {
+                       OmniboxTextUponFocusedCommittedSERP) {
   // Setup Instant.
   ASSERT_NO_FATAL_FAILURE(SetupInstant(browser()));
   FocusOmniboxAndWaitForInstantExtendedSupport();
@@ -377,25 +367,15 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
                                      "handleOnChange();"));
   instant()->instant_tab()->contents()->GetView()->Focus();
 
-  // Search term extraction should kick in with the autocompleted text.
-  EXPECT_EQ(ASCIIToUTF16("hello kitty"), omnibox()->GetText());
-
-  // Suggestion should be cleared at this point.
-  EXPECT_EQ(ASCIIToUTF16(""), omnibox()->GetInstantSuggestion());
+  // Omnibox text and suggestion should not be changed.
+  EXPECT_EQ(ASCIIToUTF16("hello"), omnibox()->GetText());
+  EXPECT_EQ(ASCIIToUTF16(" kitty"), omnibox()->GetInstantSuggestion());
 }
 
 // Checks that a previous Navigation suggestion is not re-used when a search
-// suggestion comes in. Disabled on Mac because omnibox focus loss is not
-// working correctly. http://crbug.com/178520
-#if defined(OS_MACOSX)
-#define MAYBE_NavigationSuggestionIsDiscardedUponSearchSuggestion \
-        DISABLED_NavigationSuggestionIsDiscardedUponSearchSuggestion
-#else
-#define MAYBE_NavigationSuggestionIsDiscardedUponSearchSuggestion \
-        NavigationSuggestionIsDiscardedUponSearchSuggestion
-#endif
+// suggestion comes in.
 IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
-    MAYBE_NavigationSuggestionIsDiscardedUponSearchSuggestion) {
+                       NavigationSuggestionIsDiscardedUponSearchSuggestion) {
   // Setup Instant.
   ASSERT_NO_FATAL_FAILURE(SetupInstant(browser()));
   FocusOmniboxAndWaitForInstantExtendedSupport();
@@ -413,7 +393,8 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
                             "behavior = 2;"
                             "handleOnChange();"));
   instant()->overlay()->contents()->GetView()->Focus();
-  EXPECT_EQ(ASCIIToUTF16("exams are great"), omnibox()->GetText());
+  EXPECT_EQ(ASCIIToUTF16("exam"), omnibox()->GetText());
+  EXPECT_EQ(ASCIIToUTF16("s are great"), omnibox()->GetInstantSuggestion());
 
   // TODO(jered): Remove this after fixing OnBlur().
   omnibox()->RevertAll();

[commit-bot: I haz the power, 2013-04-05 15:58:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/12792013/53001

[commit-bot: I haz the power, 2013-04-05 17:07:04]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-04-05 19:56:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/12792013/54016

[commit-bot: I haz the power, 2013-04-05 23:27:43]

Change committed as 192645