Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2763063002:
[omnibox] Narrow condition for resetting selection (Closed)
|
Created:
3 years, 9 months ago by Kevin Bailey Modified:
3 years, 8 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, jdonnelly+watch_chromium.org, tfarina Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[omnibox] Narrow condition for resetting selection
Previously, we were resetting the selection if text could have been
changed. Now, only do so if we spot difference. Fixes cursor location
after an alt-tab focus change.
BUG=701519
Review-Url: https://codereview.chromium.org/2763063002
Cr-Commit-Position: refs/heads/master@{#467499}
Committed: https://chromium.googlesource.com/chromium/src/+/83a82e0c55fca9efc29c89dd5852fdbfe6118889
Patch Set 1 #Patch Set 2 : Restored include file #
Total comments: 4
Patch Set 3 : Updated, plus beginning of unit test #Patch Set 4 : A better result type? #Patch Set 5 : Added interactive test #
Total comments: 24
Patch Set 6 : Responses #Patch Set 7 : Cheered up comments and trimmed tests #
Total comments: 12
Patch Set 8 : Fill contents to eliminate async crash #
Total comments: 4
Patch Set 9 : Some attempts #
Total comments: 6
Patch Set 10 : An improvement #Patch Set 11 : Another variant, but crashes #Patch Set 12 : Trimmed a little more #Patch Set 13 : Massively trimmed, streamlined #
Total comments: 4
Patch Set 14 : Restored code to trigger error in previous code #
Total comments: 7
Patch Set 15 : Use key instead of mouse press #Patch Set 16 : Hitting 'end' twice makes it work #Patch Set 17 : Removed check, added todo #
Messages
Total messages: 47 (12 generated)
Description was changed from ========== [omnibox] Narrow condition for resetting selection Previously, we were resetting the selection if text could have been changed. Now, only do so if we spot difference. Fixes cursor location after an alt-tab focus change. BUG=701519 ========== to ========== [omnibox] Narrow condition for resetting selection Previously, we were resetting the selection if text could have been changed. Now, only do so if we spot difference. Fixes cursor location after an alt-tab focus change. BUG=701519 ==========
krb@chromium.org changed reviewers: + pkasting@chromium.org
On 2017/03/21 15:36:37, Kevin Bailey wrote: > mailto:krb@chromium.org changed reviewers: > + mailto:pkasting@chromium.org Can you take a look? thx
Can you find a way to test this? I know it's excessively difficult to mock out relevant portions of the system today, but I would hope we can manually twiddle enough bits in the model/view to change the permanent text while a (fake) popup is thought to be open, in order to test this. https://codereview.chromium.org/2763063002/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/2763063002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:343: controller_->GetToolbarModel()->GetFormattedURL(nullptr); Why would the |permanent_text_| be out of sync with the toolbar model? In OmniboxEditModel::UpdatePermanentText(), we unconditionally update |permanent_text_|, and then we return whether the view should actually change something visibly. So I'd expect these two values to be in sync unless we haven't been Update()d yet, in which case something else is funky. I'd think this bug would be solvable by comparing PermanentURL() to the visible text in the view, which can all be done on the view side.
https://codereview.chromium.org/2763063002/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/2763063002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:343: controller_->GetToolbarModel()->GetFormattedURL(nullptr); On 2017/03/21 22:47:18, Peter Kasting wrote: > Why would the |permanent_text_| be out of sync with the toolbar model? The comment in ::OnBlur() implies that some update gets delayed. I couldn't be certain which delay that it was referring to, but I assumed it was this delay we were trying to detect, and that permanent_url_ is downstream of what the user typed. > I'd think this bug would be solvable by comparing PermanentURL() to the visible > text in the view, which can all be done on the view side. ::PermanentURL() includes 'https://.../'. I'm hoping that there is an intermediate value that is more similar?
https://codereview.chromium.org/2763063002/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/2763063002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:343: controller_->GetToolbarModel()->GetFormattedURL(nullptr); On 2017/03/23 13:14:37, Kevin Bailey wrote: > On 2017/03/21 22:47:18, Peter Kasting wrote: > > Why would the |permanent_text_| be out of sync with the toolbar model? > > The comment in ::OnBlur() implies that some update gets delayed. I couldn't be > certain which delay that it was referring to, but I assumed it was this delay we > were trying to detect, and that permanent_url_ is downstream of what the user > typed. The paragraph I wrote describes the place this is referring to. Basically, the permanent_url_ should already be up-to-date, so this function you've added should always return false. (If that's not what you saw, then I'm curious about how that could be.) > > I'd think this bug would be solvable by comparing PermanentURL() to the > visible > > text in the view, which can all be done on the view side. > > ::PermanentURL() includes 'https://.../'. I'm hoping that there is an > intermediate value that is more similar? Did you mean http://? (i.e., do you mean this includes a scheme that is normally stripped when displaying the URL in the steady-state omnibox?) You probably want to compare against the raw permanent_text_ instead, but I don't think there's an accessor for it today. You could add one.
And added interactive test that fails with old code. https://codereview.chromium.org/2763063002/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.cc (right): https://codereview.chromium.org/2763063002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.cc:343: controller_->GetToolbarModel()->GetFormattedURL(nullptr); On 2017/03/23 21:52:12, Peter Kasting wrote: > > The paragraph I wrote describes the place this is referring to. Basically, the > permanent_url_ should already be up-to-date, so this function you've added > should always return false. (If that's not what you saw, then I'm curious about > how that could be.) I haven't seen anything different. The code was my best guess at the time. > Did you mean http://? (i.e., do you mean this includes a scheme that is > normally stripped when displaying the URL in the steady-state omnibox?) I meant that it wasn't anything like the visible text. I didn't realize that PermanentURL() mangled it so much.
https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:779: // it will be halted. Nit: Maybe something in this comment should talk about why we don't just call RevertAll() in all cases. https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:354: OmniboxView* omnibox_view = NULL; Nit: nullptr_t https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:368: match.relevance = 500; Do things like the type and relevance matter? https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:371: matches.push_back(match); Do we need two suggestions instead of just the one here? Why? https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:388: // Simulate a mouse click before dragging the mouse. (a) I don't understand the "why" of this (b) I don't see any drag https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:389: gfx::Point point(omnibox_view_views->x(), omnibox_view_views->y()); Nit: (omnibox_view_views->origin())? https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:410: EXPECT_NE(static_cast<size_t>(0), start); Nit: Just say 0U https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:411: // Blur or it dchecks when rendering. What dchecks? Why? https://codereview.chromium.org/2763063002/diff/80001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.h (right): https://codereview.chromium.org/2763063002/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.h:164: base::string16 PermanentText() { return permanent_text_; } Nit: const base::string16& permanent_text() const
https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:779: // it will be halted. On 2017/04/10 23:04:13, Peter Kasting wrote: > Nit: Maybe something in this comment should talk about why we don't just call > RevertAll() in all cases. It seems to me that number the cases of calling RevertAll() has reduced. What did you have in mind? https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:354: OmniboxView* omnibox_view = NULL; On 2017/04/10 23:04:13, Peter Kasting wrote: > Nit: nullptr_t Fixed above too. https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:368: match.relevance = 500; On 2017/04/10 23:04:13, Peter Kasting wrote: > Do things like the type and relevance matter? I copy-pasted the previous test, since it was mostly what I wanted, and I didn't know what the code cared about. Would you like me to factor the common code, or would you genuinely like to shrink it? https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:371: matches.push_back(match); On 2017/04/10 23:04:13, Peter Kasting wrote: > Do we need two suggestions instead of just the one here? Why? Same as above. https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:388: // Simulate a mouse click before dragging the mouse. On 2017/04/10 23:04:13, Peter Kasting wrote: > (a) I don't understand the "why" of this > (b) I don't see any drag Stale. All I want is a mouse click and release. Comment fixed. https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:389: gfx::Point point(omnibox_view_views->x(), omnibox_view_views->y()); On 2017/04/10 23:04:13, Peter Kasting wrote: > Nit: (omnibox_view_views->origin())? Fixed above too. https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:410: EXPECT_NE(static_cast<size_t>(0), start); On 2017/04/10 23:04:13, Peter Kasting wrote: > Nit: Just say 0U Done. https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:411: // Blur or it dchecks when rendering. On 2017/04/10 23:04:13, Peter Kasting wrote: > What dchecks? Why? There seem to be stale messages in the work queue which, when executed, refer to text fields (or RenderText's, not sure) which no longer exist. (There is garbage in various fields, that change from run to run.) I could try emptying the queue prior to shut down, but it lives until leaving this test routine, so it seems like a test artifact. https://codereview.chromium.org/2763063002/diff/80001/components/omnibox/brow... File components/omnibox/browser/omnibox_edit_model.h (right): https://codereview.chromium.org/2763063002/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_edit_model.h:164: base::string16 PermanentText() { return permanent_text_; } On 2017/04/10 23:04:13, Peter Kasting wrote: > Nit: const base::string16& permanent_text() const Done.
https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:779: // it will be halted. On 2017/04/11 14:12:41, Kevin Bailey wrote: > On 2017/04/10 23:04:13, Peter Kasting wrote: > > Nit: Maybe something in this comment should talk about why we don't just call > > RevertAll() in all cases. > > It seems to me that number the cases of calling RevertAll() has reduced. What > did you have in mind? Right. My point is that, to a reader, it's not immediately obvious why there's a conditional here. The comment currently says _what_ the conditional is checking and the effect of the revert is, but not why it would have been unsafe to simply write: RevertAll(); ...with no conditional check. So I'd describe the negative side effects that would occur if we were to call RevertAll() outside of the cases where we currently do. https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:368: match.relevance = 500; On 2017/04/11 14:12:42, Kevin Bailey wrote: > On 2017/04/10 23:04:13, Peter Kasting wrote: > > Do things like the type and relevance matter? > > I copy-pasted the previous test, since it was mostly what I wanted, and I didn't > know what the code cared about. Would you like me to factor the common code, or > would you genuinely like to shrink it? I would likely shrink rather than refactor. Maybe both places can be shrunk, and maybe at that point they're still similar, and the code would be better factoring these bits out, and so we can do both. I don't know. I mostly want to ensure that tests do as little as possible beyond what they need in order to test what they're trying to test. https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:411: // Blur or it dchecks when rendering. On 2017/04/11 14:12:42, Kevin Bailey wrote: > On 2017/04/10 23:04:13, Peter Kasting wrote: > > What dchecks? Why? > > There seem to be stale messages in the work queue which, when executed, refer to > text fields (or RenderText's, not sure) which no longer exist. (There is garbage > in various fields, that change from run to run.) I could try emptying the queue > prior to shut down, but it lives until leaving this test routine, so it seems > like a test artifact. OK. I'm not sure I fully understand the problem yet. At the least, I'd suggest having a much more detailed explanation here of exactly what the problem is and why this solution is both useful and correct. As it is, this comment basically says "don't remove this line", which is more valuable than having no comment, but leaves the reader unsure exactly what's going on.
https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:779: // it will be halted. On 2017/04/11 20:04:39, Peter Kasting wrote: > On 2017/04/11 14:12:41, Kevin Bailey wrote: > > On 2017/04/10 23:04:13, Peter Kasting wrote: > > > Nit: Maybe something in this comment should talk about why we don't just > call > > > RevertAll() in all cases. > > > > It seems to me that number the cases of calling RevertAll() has reduced. What > > did you have in mind? > > Right. My point is that, to a reader, it's not immediately obvious why there's > a conditional here. The comment currently says _what_ the conditional is > checking and the effect of the revert is, but not why it would have been unsafe > to simply write: > > RevertAll(); > > ...with no conditional check. So I'd describe the negative side effects that > would occur if we were to call RevertAll() outside of the cases where we > currently do. There are a dozen things that revert does additionally, so I hope I covered the one you were thinking of. https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:368: match.relevance = 500; On 2017/04/11 20:04:39, Peter Kasting wrote: > On 2017/04/11 14:12:42, Kevin Bailey wrote: > > On 2017/04/10 23:04:13, Peter Kasting wrote: > > > Do things like the type and relevance matter? > > > > I copy-pasted the previous test, since it was mostly what I wanted, and I > didn't > > know what the code cared about. Would you like me to factor the common code, > or > > would you genuinely like to shrink it? > > I would likely shrink rather than refactor. Maybe both places can be shrunk, > and maybe at that point they're still similar, and the code would be better > factoring these bits out, and so we can do both. I don't know. I mostly want > to ensure that tests do as little as possible beyond what they need in order to > test what they're trying to test. It needs the type and relevance, but only needed one result. I assume there's not especially much left to factor. https://codereview.chromium.org/2763063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:411: // Blur or it dchecks when rendering. On 2017/04/11 20:04:39, Peter Kasting wrote: > On 2017/04/11 14:12:42, Kevin Bailey wrote: > > On 2017/04/10 23:04:13, Peter Kasting wrote: > > > What dchecks? Why? > > > > There seem to be stale messages in the work queue which, when executed, refer > to > > text fields (or RenderText's, not sure) which no longer exist. (There is > garbage > > in various fields, that change from run to run.) I could try emptying the > queue > > prior to shut down, but it lives until leaving this test routine, so it seems > > like a test artifact. > > OK. I'm not sure I fully understand the problem yet. At the least, I'd suggest > having a much more detailed explanation here of exactly what the problem is and > why this solution is both useful and correct. As it is, this comment basically > says "don't remove this line", which is more valuable than having no comment, > but leaves the reader unsure exactly what's going on. Done.
https://codereview.chromium.org/2763063002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:362: AutocompleteMatch match; Nit: Could do AutocompleteMatch match(nullptr, 500, false, AutocompleteMatchType::HISTORY_TITLE); ...and ditch the explicit type/relevance setting below. https://codereview.chromium.org/2763063002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:396: // Redo the above Nit: Be clearer (since "the above" covers a lot); e.g. "Refocus the omnibox." https://codereview.chromium.org/2763063002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:405: // Make sure cursor isn't homed. Nit: This really goes above the declaration of start/end (since getting the selection bounds is part of checking the selection bounds) and should have a blank line above it (to distinguish between the block that's about refocusing and this one). https://codereview.chromium.org/2763063002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:406: EXPECT_NE(0U, start); Nit: It's not immediately obvious why this should be nonzero. I think what we really want is to ensure the cursor position/selection is preserved across blur/focus. Maybe we should explicitly set the cursor position to some value before blurring and then check that it's that value after refocusing. https://codereview.chromium.org/2763063002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:409: // them. OK... this doesn't really make things clearer than your explanation to me, which left me still not really understanding the problem :). Also this comment is still pretty ambiguous: "it dchecks" -> what dchecks? "the task" -> which task? "get handler after the test is finished" -> why does that cause a dcheck? "Blurring quiesces them" -> why and how? I would be picking on this less if I truly trusted that this is the right solution. But I don't; AFAIK no other tests blur on exit like this, so it seems extremely unusual. I'd imagine either we should spin the message loop here, or we should ensure the problematic messages don't get queued, or we should ensure running them as this test shuts down doesn't cause problems, or we should ensure they don't get run after the test finishes.
All comments sound good, but remark on last one. https://codereview.chromium.org/2763063002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:409: // them. On 2017/04/12 23:48:24, Peter Kasting wrote: > OK... this doesn't really make things clearer than your explanation to me, which > left me still not really understanding the problem :). Also this comment is > still pretty ambiguous: "it dchecks" -> what dchecks? "the task" -> which task? > "get handler after the test is finished" -> why does that cause a dcheck? > "Blurring quiesces them" -> why and how? > > I would be picking on this less if I truly trusted that this is the right > solution. But I don't; AFAIK no other tests blur on exit like this, so it seems > extremely unusual. I'd imagine either we should spin the message loop here, or > we should ensure the problematic messages don't get queued, or we should ensure > running them as this test shuts down doesn't cause problems, or we should ensure > they don't get run after the test finishes. The part about this that bothers me is that the new test isn't doing anything crazy (that I can see); just blur, re-focus. Regarding blurring, someone might just as well ask, "None of the other tests spin the message loop." It feels more like the test is forcing the Omnibox machinery through states that it doesn't expect and that a correct sequence wouldn't have this problem (but I'm at a bit of a loss as to what's wrong with it.)
https://codereview.chromium.org/2763063002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:409: // them. On 2017/04/13 00:07:12, Kevin Bailey wrote: > On 2017/04/12 23:48:24, Peter Kasting wrote: > > OK... this doesn't really make things clearer than your explanation to me, > which > > left me still not really understanding the problem :). Also this comment is > > still pretty ambiguous: "it dchecks" -> what dchecks? "the task" -> which > task? > > "get handler after the test is finished" -> why does that cause a dcheck? > > "Blurring quiesces them" -> why and how? > > > > I would be picking on this less if I truly trusted that this is the right > > solution. But I don't; AFAIK no other tests blur on exit like this, so it > seems > > extremely unusual. I'd imagine either we should spin the message loop here, > or > > we should ensure the problematic messages don't get queued, or we should > ensure > > running them as this test shuts down doesn't cause problems, or we should > ensure > > they don't get run after the test finishes. > > The part about this that bothers me is that the new test isn't doing anything > crazy (that I can see); just blur, re-focus. Regarding blurring, someone might > just as well ask, "None of the other tests spin the message loop." It feels more > like the test is forcing the Omnibox machinery through states that it doesn't > expect and that a correct sequence wouldn't have this problem (but I'm at a bit > of a loss as to what's wrong with it.) Yes, I agree with your instinct. Spinning the message loop still kinda makes me uncomfortable. Would focusing alone (no blur cycles) cause problems? If so don't other tests focus the omnibox, and what do they do to handle this? What are the precise DCHECKs and how are they triggering?
Tests now passing, even when the browser becomes visible, which would always hit the dcheck previously. https://codereview.chromium.org/2763063002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:409: // them. On 2017/04/13 00:12:15, Peter Kasting wrote: > On 2017/04/13 00:07:12, Kevin Bailey wrote: > > On 2017/04/12 23:48:24, Peter Kasting wrote: > > > OK... this doesn't really make things clearer than your explanation to me, > > which > > > left me still not really understanding the problem :). Also this comment is > > > still pretty ambiguous: "it dchecks" -> what dchecks? "the task" -> which > > task? > > > "get handler after the test is finished" -> why does that cause a dcheck? > > > "Blurring quiesces them" -> why and how? > > > > > > I would be picking on this less if I truly trusted that this is the right > > > solution. But I don't; AFAIK no other tests blur on exit like this, so it > > seems > > > extremely unusual. I'd imagine either we should spin the message loop here, > > or > > > we should ensure the problematic messages don't get queued, or we should > > ensure > > > running them as this test shuts down doesn't cause problems, or we should > > ensure > > > they don't get run after the test finishes. > > > > The part about this that bothers me is that the new test isn't doing anything > > crazy (that I can see); just blur, re-focus. Regarding blurring, someone might > > just as well ask, "None of the other tests spin the message loop." It feels > more > > like the test is forcing the Omnibox machinery through states that it doesn't > > expect and that a correct sequence wouldn't have this problem (but I'm at a > bit > > of a loss as to what's wrong with it.) > > Yes, I agree with your instinct. Spinning the message loop still kinda makes me > uncomfortable. Would focusing alone (no blur cycles) cause problems? If so > don't other tests focus the omnibox, and what do they do to handle this? > > What are the precise DCHECKs and how are they triggering? (For the reader, since this was discussed off-line) the 'contents' field of a match hadn't been filled in yet, which didn't matter in the other tests, but this one was sometimes getting to the point where it would attempt to render an empty (invalid) match. Fixed, albeit forced, by manually filling in the contents.
https://codereview.chromium.org/2763063002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:388: // Simulate a mouse click to trigger focus. Click well right of text. This seems a bit fiddly when all you really want (I think) is to get a focused location bar with not-select-all. There are various methods that will do that directly, e.g. call chrome::FocusLocationBar(browser()); and then omnibox_view->SetWindowTextAndCaretPos(). If you pass true to |update_popup|, you can probably trigger autocompletion as well, meaning you could skip basically everything above as this should substitute for it. Similarly, below when you want to refocus, you could probably just FocusLocationBar() and then call omnibox_view->UpdatePopup(). The only thing I'd worry about would be that you might need to "really blur" the popup in between these, as OnBlur() is just the blur handler, and won't result in the system actually blurring; and who knows whether that means FocusLocationBar() will early-exit without "properly" refocusing things. I'm sorry didn't think to suggest this route sooner, as it ought to be a lot simpler overall. https://codereview.chromium.org/2763063002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:414: size_t start = 0; Nit: No need to init this, but if you do, init the ones above too
https://codereview.chromium.org/2763063002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:388: // Simulate a mouse click to trigger focus. Click well right of text. > The only thing I'd worry about would be that you might need to "really blur" the > popup in between these, as OnBlur() is just the blur handler, and won't result > in the system actually blurring; and who knows whether that means > FocusLocationBar() will early-exit without "properly" refocusing things. Note that calling ClickBrowserWindowCenter() would probably accomplish a "true blur".
I'm sure all this is wrong because it either crashes or the test fails. https://codereview.chromium.org/2763063002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:399: omnibox_view_views->SetWindowTextAndCaretPos( If I replace 'false, false' with 'true, false', it dchecks because 'user_text_' isn't set. If I uncomment setting it, it dchecks in a stack trace that I can recognize very little of. https://codereview.chromium.org/2763063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:421: omnibox_view_views->UpdatePopup(); With this 'UpdatePopup()', the test fails because the cursor is at 0. Without it, the cursor is at 11, and the box no doubt contains 'about:blank'.
This is the only combination that I've found that works. Feel free to comment on the previous patch if you can see a way to improve it. I'm happy to try something, but it was my understanding that an interactive UI test should poke it like a user, and prefer not calling underlying routines directly. https://codereview.chromium.org/2763063002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:362: AutocompleteMatch match; On 2017/04/12 23:48:24, Peter Kasting wrote: > Nit: Could do > > AutocompleteMatch match(nullptr, 500, false, > AutocompleteMatchType::HISTORY_TITLE); > > ...and ditch the explicit type/relevance setting below. Done. https://codereview.chromium.org/2763063002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:396: // Redo the above On 2017/04/12 23:48:24, Peter Kasting wrote: > Nit: Be clearer (since "the above" covers a lot); e.g. "Refocus the omnibox." Done. https://codereview.chromium.org/2763063002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:405: // Make sure cursor isn't homed. On 2017/04/12 23:48:24, Peter Kasting wrote: > Nit: This really goes above the declaration of start/end (since getting the > selection bounds is part of checking the selection bounds) and should have a > blank line above it (to distinguish between the block that's about refocusing > and this one). Done. https://codereview.chromium.org/2763063002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:406: EXPECT_NE(0U, start); On 2017/04/12 23:48:25, Peter Kasting wrote: > Nit: It's not immediately obvious why this should be nonzero. I think what we > really want is to ensure the cursor position/selection is preserved across > blur/focus. Maybe we should explicitly set the cursor position to some value > before blurring and then check that it's that value after refocusing. Done. https://codereview.chromium.org/2763063002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:414: size_t start = 0; On 2017/04/17 17:46:39, Peter Kasting wrote: > Nit: No need to init this, but if you do, init the ones above too I like to be paranoid and initialize things such that, if the routine leaves them alone, the test fails, but maybe this is too paranoid. I took out the initialization.
https://codereview.chromium.org/2763063002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:399: omnibox_view_views->SetWindowTextAndCaretPos( On 2017/04/18 13:40:09, Kevin Bailey wrote: > If I replace 'false, false' with 'true, false', it dchecks because 'user_text_' > isn't set. If I uncomment setting it, it dchecks in a stack trace that I can > recognize very little of. Sorry, but there's too much functionality under these to know offhand what might be going wrong without you giving me actual stacks. Note that trying to do this stuff without removing all the code above at the beginning of the test (as I suggested) is highly likely to screw things up, since that code pokes at the underlying data structures in ways that won't leave the internal state consistent (it calls the notification methods for autocompletion runs without actually running autocompletion or setting the omnibox text). I'd move your #if 0 up to cover most of the preceding code and then see what your stacks look like. Separately, calling SetUserText() alone, without SetWindowTextAndCaretPos(), is probably even better at doing what you want here. https://codereview.chromium.org/2763063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:421: omnibox_view_views->UpdatePopup(); On 2017/04/18 13:40:09, Kevin Bailey wrote: > With this 'UpdatePopup()', the test fails because the cursor is at 0. Without > it, the cursor is at 11, and the box no doubt contains 'about:blank'. Part of the problem here is that FocusLocationBar() will also select all (which I didn't realize). However, you could call LocationBarView::FocusLocation() to avoid this. As for UpdatePopup(), do you even care about reopening the popup? You just care about preserving the cursor position on refocusing, right? In which case that call can just be dropped. In either case, you also need to remove all the mucking with |results| and calling of OnResultChanged() below, since it has the same problem as above (it goofs with the internal methods instead of just letting the normal control flow pathways handle this). Either you don't want this at all, or else it would be done for you by UpdatePopup().
Once I replace the code with 'SetUserText()', it crashes on shut-down, after passing all tests, with a stack trace that I recognize nothing of. None of the other changes seem to make a difference. https://codereview.chromium.org/2763063002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:399: omnibox_view_views->SetWindowTextAndCaretPos( On 2017/04/18 20:47:49, Peter Kasting wrote: > On 2017/04/18 13:40:09, Kevin Bailey wrote: > > If I replace 'false, false' with 'true, false', it dchecks because > 'user_text_' > > isn't set. If I uncomment setting it, it dchecks in a stack trace that I can > > recognize very little of. > > Sorry, but there's too much functionality under these to know offhand what might > be going wrong without you giving me actual stacks. > > Note that trying to do this stuff without removing all the code above at the > beginning of the test (as I suggested) is highly likely to screw things up, > since that code pokes at the underlying data structures in ways that won't leave > the internal state consistent (it calls the notification methods for > autocompletion runs without actually running autocompletion or setting the > omnibox text). > > I'd move your #if 0 up to cover most of the preceding code and then see what > your stacks look like. > > Separately, calling SetUserText() alone, without SetWindowTextAndCaretPos(), is > probably even better at doing what you want here. By "most", I assume "OnResultChanged()", since we need the results. https://codereview.chromium.org/2763063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:421: omnibox_view_views->UpdatePopup(); On 2017/04/18 20:47:49, Peter Kasting wrote: > On 2017/04/18 13:40:09, Kevin Bailey wrote: > > With this 'UpdatePopup()', the test fails because the cursor is at 0. Without > > it, the cursor is at 11, and the box no doubt contains 'about:blank'. > > Part of the problem here is that FocusLocationBar() will also select all (which > I didn't realize). > > However, you could call LocationBarView::FocusLocation() to avoid this. > > As for UpdatePopup(), do you even care about reopening the popup? You just care > about preserving the cursor position on refocusing, right? In which case that > call can just be dropped. > > In either case, you also need to remove all the mucking with |results| and > calling of OnResultChanged() below, since it has the same problem as above (it > goofs with the internal methods instead of just letting the normal control flow > pathways handle this). Either you don't want this at all, or else it would be > done for you by UpdatePopup(). This part is actually working fine, although I trimmed it as you requested.
For readers at home, filling results was completely unnecessary to open the pop-up. Simply filling the user text and letting autocomplete run is sufficient. Also, the dcheck in question had something to do with running the test with --single-process. The dcheck was a complaint that something was run on the wrong thread. This final version is a minimal, passing test.
https://codereview.chromium.org/2763063002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:361: EXPECT_TRUE(omnibox_view->model()->popup_model()->IsOpen()); I was thinking about why you even care whether the popup is open, and I realized I don't think this test regression-tests your change at all. It basically checks that CloseOmniboxPopup() doesn't nuke cursor position. But that was true before the change. To test this, you need to actually trigger zerosuggest. This may be hard to do in a test unless you can inject providers or args somehow -- I dunno how? Another way might be to have a unittest that actually creates an omnibox view, autocomplete controller, and fake autocomplete provider that always returns matches when run when the input is from focus. Then, basically, do the body of this test. I'm not sure anybody does something like this today for you to copy from. It may be that your original test, which mucked with the popup without actually triggering user input, got closer to testing this stuff, albeit in a rather artificial sort of way instead of by actually using zerosuggest. Which means I may have spent several days of your time leading you on a complete wild goose chase. Which would be... very embarrassing. :/ I would try and verify that your original test fails without the rest of your patch, and this one passes. That would be confirmation that we want to do something like what it does, and not this. Although really we might want to expand your original test to also check the case where there are no zerosuggest results (which should preserve selection both before and after your patch) and the case where the permanent text changes while the popup is open (which should reset selection before and after your patch).
https://codereview.chromium.org/2763063002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:361: EXPECT_TRUE(omnibox_view->model()->popup_model()->IsOpen()); On 2017/04/20 04:14:38, Peter Kasting (OOO until 4-24) wrote: > I was thinking about why you even care whether the popup is open, and I realized > I don't think this test regression-tests your change at all. It basically > checks that CloseOmniboxPopup() doesn't nuke cursor position. But that was true > before the change. > > To test this, you need to actually trigger zerosuggest. This may be hard to do > in a test unless you can inject providers or args somehow -- I dunno how? > Another way might be to have a unittest that actually creates an omnibox view, > autocomplete controller, and fake autocomplete provider that always returns > matches when run when the input is from focus. Then, basically, do the body of > this test. > > I'm not sure anybody does something like this today for you to copy from. > > It may be that your original test, which mucked with the popup without actually > triggering user input, got closer to testing this stuff, albeit in a rather > artificial sort of way instead of by actually using zerosuggest. Which means I > may have spent several days of your time leading you on a complete wild goose > chase. Which would be... very embarrassing. :/ > > I would try and verify that your original test fails without the rest of your > patch, and this one passes. That would be confirmation that we want to do > something like what it does, and not this. > > Although really we might want to expand your original test to also check the > case where there are no zerosuggest results (which should preserve selection > both before and after your patch) and the case where the permanent text changes > while the popup is open (which should reset selection before and after your > patch). The last one that differentiates the fix is patchset 10, which manufactures results and simulates a click (ie. before SetUserText) Should I cheer up the end of it - use FocusAppMenu() and lose the second set of results mucking - or do we want an omnibus test? I'm leaning 'not for this CL', but if the existing tests don't have enough coverage, we should work on improving them.
https://codereview.chromium.org/2763063002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:361: EXPECT_TRUE(omnibox_view->model()->popup_model()->IsOpen()); On 2017/04/20 15:13:04, Kevin Bailey wrote: > On 2017/04/20 04:14:38, Peter Kasting (OOO until 4-24) wrote: > > I was thinking about why you even care whether the popup is open, and I > realized > > I don't think this test regression-tests your change at all. It basically > > checks that CloseOmniboxPopup() doesn't nuke cursor position. But that was > true > > before the change. > > > > To test this, you need to actually trigger zerosuggest. This may be hard to > do > > in a test unless you can inject providers or args somehow -- I dunno how? > > Another way might be to have a unittest that actually creates an omnibox view, > > autocomplete controller, and fake autocomplete provider that always returns > > matches when run when the input is from focus. Then, basically, do the body > of > > this test. > > > > I'm not sure anybody does something like this today for you to copy from. > > > > It may be that your original test, which mucked with the popup without > actually > > triggering user input, got closer to testing this stuff, albeit in a rather > > artificial sort of way instead of by actually using zerosuggest. Which means > I > > may have spent several days of your time leading you on a complete wild goose > > chase. Which would be... very embarrassing. :/ > > > > I would try and verify that your original test fails without the rest of your > > patch, and this one passes. That would be confirmation that we want to do > > something like what it does, and not this. > > > > Although really we might want to expand your original test to also check the > > case where there are no zerosuggest results (which should preserve selection > > both before and after your patch) and the case where the permanent text > changes > > while the popup is open (which should reset selection before and after your > > patch). > > The last one that differentiates the fix is patchset 10, which manufactures > results and simulates a click (ie. before SetUserText) Should I cheer up the end > of it - use FocusAppMenu() and lose the second set of results mucking - or do we > want an omnibus test? I'm leaning 'not for this CL', but if the existing tests > don't have enough coverage, we should work on improving them. I think it's OK to clean up that patch and land it and then think about how to get better test coverage here. The omnibox _is_ historically poorly tested (blame me, I didn't really believe in testing ten years ago when I was writing it), and while there are scattered low-level and very-high-level tests, it's really difficult to test something "in the middle", which is what you want here, and what we'd want for a lot of different subtle OmniboxView polish fixes.
https://codereview.chromium.org/2763063002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:361: EXPECT_TRUE(omnibox_view->model()->popup_model()->IsOpen()); On 2017/04/20 17:16:07, Peter Kasting (OOO until 4-24) wrote: > On 2017/04/20 15:13:04, Kevin Bailey wrote: > > On 2017/04/20 04:14:38, Peter Kasting (OOO until 4-24) wrote: > > > I was thinking about why you even care whether the popup is open, and I > > realized > > > I don't think this test regression-tests your change at all. It basically > > > checks that CloseOmniboxPopup() doesn't nuke cursor position. But that was > > true > > > before the change. > > > > > > To test this, you need to actually trigger zerosuggest. This may be hard to > > do > > > in a test unless you can inject providers or args somehow -- I dunno how? > > > Another way might be to have a unittest that actually creates an omnibox > view, > > > autocomplete controller, and fake autocomplete provider that always returns > > > matches when run when the input is from focus. Then, basically, do the body > > of > > > this test. > > > > > > I'm not sure anybody does something like this today for you to copy from. > > > > > > It may be that your original test, which mucked with the popup without > > actually > > > triggering user input, got closer to testing this stuff, albeit in a rather > > > artificial sort of way instead of by actually using zerosuggest. Which > means > > I > > > may have spent several days of your time leading you on a complete wild > goose > > > chase. Which would be... very embarrassing. :/ > > > > > > I would try and verify that your original test fails without the rest of > your > > > patch, and this one passes. That would be confirmation that we want to do > > > something like what it does, and not this. > > > > > > Although really we might want to expand your original test to also check the > > > case where there are no zerosuggest results (which should preserve selection > > > both before and after your patch) and the case where the permanent text > > changes > > > while the popup is open (which should reset selection before and after your > > > patch). > > > > The last one that differentiates the fix is patchset 10, which manufactures > > results and simulates a click (ie. before SetUserText) Should I cheer up the > end > > of it - use FocusAppMenu() and lose the second set of results mucking - or do > we > > want an omnibus test? I'm leaning 'not for this CL', but if the existing tests > > don't have enough coverage, we should work on improving them. > > I think it's OK to clean up that patch and land it and then think about how to > get better test coverage here. The omnibox _is_ historically poorly tested > (blame me, I didn't really believe in testing ten years ago when I was writing > it), and while there are scattered low-level and very-high-level tests, it's > really difficult to test something "in the middle", which is what you want here, > and what we'd want for a lot of different subtle OmniboxView polish fixes. Ok, brought back as little (I hope) code as necessary to trigger old error.
https://codereview.chromium.org/2763063002/diff/250001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/250001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:386: EXPECT_TRUE(omnibox_view->IsSelectAll()); It doesn't seem like testing this is valuable. Also see next comment. https://codereview.chromium.org/2763063002/diff/250001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:388: // Simulate a mouse click to trigger focus. Click well right of text. (1) It seems like we should focus the location bar before we fake a zerosuggest run (by putting results in, as we do above). (2) I would think you could just FocusLocationBar() to focus the bar. So I'd try to structure like: // Focus the omnibox, which in normal operation might trigger ZeroSuggest. FocusLocationBar(); // Fake ZeroSuggest opening the popup. AutocompleteController* autocomplete_controller = omnibox_view->model()->popup_model()->autocomplete_controller(); ... EXPECT_TRUE(omnibox_view->model()->popup_model()->IsOpen()); // Place the cursor in mid-text. SendKeyPressSync(VKEY_END); SendKeyPressSync(VKEY_LEFT); // Or whatever key combo you want // Save cursor position to verify it's properly restored later. ...
No joy. https://codereview.chromium.org/2763063002/diff/250001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/250001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:386: EXPECT_TRUE(omnibox_view->IsSelectAll()); On 2017/04/24 21:55:06, Peter Kasting wrote: > It doesn't seem like testing this is valuable. Also see next comment. I'll leave it in until below is resolved. https://codereview.chromium.org/2763063002/diff/250001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:388: // Simulate a mouse click to trigger focus. Click well right of text. On 2017/04/24 21:55:06, Peter Kasting wrote: > (1) It seems like we should focus the location bar before we fake a zerosuggest > run (by putting results in, as we do above). > (2) I would think you could just FocusLocationBar() to focus the bar. > > So I'd try to structure like: > > // Focus the omnibox, which in normal operation might trigger ZeroSuggest. > FocusLocationBar(); > > // Fake ZeroSuggest opening the popup. > AutocompleteController* autocomplete_controller = > omnibox_view->model()->popup_model()->autocomplete_controller(); > ... > EXPECT_TRUE(omnibox_view->model()->popup_model()->IsOpen()); > > // Place the cursor in mid-text. > SendKeyPressSync(VKEY_END); > SendKeyPressSync(VKEY_LEFT); // Or whatever key combo you want > > // Save cursor position to verify it's properly restored later. > ... > I don't know if I caught your intent. I'm assuming e.g. the autocomplete_controller line corresponds to (left side)#360. Adding a call to FocusLocationBar() before that makes no difference, I assume because it's already focused (or is just focusing twice.) Using a key instead of a mouse press causes it to pass even with the old code. I believe part of the problem is that the assertion on (right side)#388 fails (the text is still selected).
https://codereview.chromium.org/2763063002/diff/250001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/250001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:388: // Simulate a mouse click to trigger focus. Click well right of text. On 2017/04/25 14:16:43, Kevin Bailey wrote: > On 2017/04/24 21:55:06, Peter Kasting wrote: > > (1) It seems like we should focus the location bar before we fake a > zerosuggest > > run (by putting results in, as we do above). > > (2) I would think you could just FocusLocationBar() to focus the bar. > > > > So I'd try to structure like: > > > > // Focus the omnibox, which in normal operation might trigger ZeroSuggest. > > FocusLocationBar(); > > > > // Fake ZeroSuggest opening the popup. > > AutocompleteController* autocomplete_controller = > > omnibox_view->model()->popup_model()->autocomplete_controller(); > > ... > > EXPECT_TRUE(omnibox_view->model()->popup_model()->IsOpen()); > > > > // Place the cursor in mid-text. > > SendKeyPressSync(VKEY_END); > > SendKeyPressSync(VKEY_LEFT); // Or whatever key combo you want > > > > // Save cursor position to verify it's properly restored later. > > ... > > > > I don't know if I caught your intent. I'm assuming e.g. the > autocomplete_controller line corresponds to (left side)#360. Yes. > Adding a call to FocusLocationBar() before that makes no difference, I assume > because it's already focused (or is just focusing twice.) OK. Interesting. > Using a key instead of a mouse press causes it to pass even with the old code. I > believe part of the problem is that the assertion on (right side)#388 fails (the > text is still selected). That seems strange. How can that be? If the omnibox is really focused, sending VKEY_END should place the caret at the end of the text. Is there any way to e.g. sleep for a second after each step to watch onscreen what the test is doing, so you can see what's up? Could calling OnResultChanged() somehow lose focus, so FocusLocationBar() after rather than before that works? This seems really odd. Does sending key presses to type characters cause the omnibox text to change?
Mixed news. https://codereview.chromium.org/2763063002/diff/250001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/250001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:388: // Simulate a mouse click to trigger focus. Click well right of text. On 2017/04/26 01:35:40, Peter Kasting wrote: > > That seems strange. How can that be? If the omnibox is really focused, sending > VKEY_END should place the caret at the end of the text. Is there any way to > e.g. sleep for a second after each step to watch onscreen what the test is > doing, so you can see what's up? Could calling OnResultChanged() somehow lose > focus, so FocusLocationBar() after rather than before that works? This seems > really odd. Does sending key presses to type characters cause the omnibox text > to change? Neither sleeping nor things like RunUntilIdle help. Strangely, what works consistently is sending 2 'end' key events. It's as if the keys are doing something like what clicking does: When you click the first time, it selects the word. When you click the second time, it unselects it and places a cursor. It's definitely focused. I had a test and print-out that verified it. However, I've no clue if OnResultsChanged() is doing something else more subtle.
https://codereview.chromium.org/2763063002/diff/250001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/2763063002/diff/250001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:388: // Simulate a mouse click to trigger focus. Click well right of text. On 2017/04/26 14:27:45, Kevin Bailey wrote: > On 2017/04/26 01:35:40, Peter Kasting wrote: > > > > That seems strange. How can that be? If the omnibox is really focused, > sending > > VKEY_END should place the caret at the end of the text. Is there any way to > > e.g. sleep for a second after each step to watch onscreen what the test is > > doing, so you can see what's up? Could calling OnResultChanged() somehow lose > > focus, so FocusLocationBar() after rather than before that works? This seems > > really odd. Does sending key presses to type characters cause the omnibox > text > > to change? > > Neither sleeping nor things like RunUntilIdle help. Strangely, what works > consistently is sending 2 'end' key events. It's as if the keys are doing > something like what clicking does: When you click the first time, it selects the > word. When you click the second time, it unselects it and places a cursor. Wat. What if you send VK_RIGHT instead of VK_END? Maybe print out the selection range after sending varieties of different cursor keys?
On 2017/04/26 17:28:34, Peter Kasting wrote: > https://codereview.chromium.org/2763063002/diff/250001/chrome/browser/ui/view... > File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): > > https://codereview.chromium.org/2763063002/diff/250001/chrome/browser/ui/view... > chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:388: // > Simulate a mouse click to trigger focus. Click well right of text. > On 2017/04/26 14:27:45, Kevin Bailey wrote: > > On 2017/04/26 01:35:40, Peter Kasting wrote: > > > > > > That seems strange. How can that be? If the omnibox is really focused, > > sending > > > VKEY_END should place the caret at the end of the text. Is there any way to > > > e.g. sleep for a second after each step to watch onscreen what the test is > > > doing, so you can see what's up? Could calling OnResultChanged() somehow > lose > > > focus, so FocusLocationBar() after rather than before that works? This > seems > > > really odd. Does sending key presses to type characters cause the omnibox > > text > > > to change? > > > > Neither sleeping nor things like RunUntilIdle help. Strangely, what works > > consistently is sending 2 'end' key events. It's as if the keys are doing > > something like what clicking does: When you click the first time, it selects > the > > word. When you click the second time, it unselects it and places a cursor. > > Wat. > > What if you send VK_RIGHT instead of VK_END? Maybe print out the selection > range after sending varieties of different cursor keys? Sorry for the drive-by.... this feels painful from here, seeing the 24h delays. Would you (Kevin, Peter), be OK with a) landing the fix itself, because... it fixes the issue :) b) Following up on the test and having the convo, including experiments, via IM, so we can have a bit more than an iteration a day? Thank you!
On 2017/04/26 18:18:42, groby wrote: > On 2017/04/26 17:28:34, Peter Kasting wrote: > > > https://codereview.chromium.org/2763063002/diff/250001/chrome/browser/ui/view... > > File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc > (right): > > > > > https://codereview.chromium.org/2763063002/diff/250001/chrome/browser/ui/view... > > chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:388: // > > Simulate a mouse click to trigger focus. Click well right of text. > > On 2017/04/26 14:27:45, Kevin Bailey wrote: > > > On 2017/04/26 01:35:40, Peter Kasting wrote: > > > > > > > > That seems strange. How can that be? If the omnibox is really focused, > > > sending > > > > VKEY_END should place the caret at the end of the text. Is there any way > to > > > > e.g. sleep for a second after each step to watch onscreen what the test is > > > > doing, so you can see what's up? Could calling OnResultChanged() somehow > > lose > > > > focus, so FocusLocationBar() after rather than before that works? This > > seems > > > > really odd. Does sending key presses to type characters cause the omnibox > > > text > > > > to change? > > > > > > Neither sleeping nor things like RunUntilIdle help. Strangely, what works > > > consistently is sending 2 'end' key events. It's as if the keys are doing > > > something like what clicking does: When you click the first time, it selects > > the > > > word. When you click the second time, it unselects it and places a cursor. > > > > Wat. > > > > What if you send VK_RIGHT instead of VK_END? Maybe print out the selection > > range after sending varieties of different cursor keys? > > Sorry for the drive-by.... this feels painful from here, seeing the 24h delays. It's felt really painful to me too. Thanks for stepping in and saying something to help us resolve this. > Would you (Kevin, Peter), be OK with > > a) landing the fix itself, because... it fixes the issue :) > b) Following up on the test and having the convo, including experiments, via IM, > so we can have a bit more than an iteration a day? I think this patch can land mostly as-is. I'd merely add a comment above the ENDs about why we're doing them at all ("Move the cursor so the text isn't all selected by default), with a TODO to investigate why two keypresses are needed and condense to one.
And LGTM so that can happen.
The CQ bit was checked by krb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by krb@chromium.org
The CQ bit was unchecked by krb@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by krb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2763063002/#ps310001 (title: "Removed check, added todo")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 310001, "attempt_start_ts": 1493241885299420,
"parent_rev": "c4abf7fbfae66c20c30fdce1c95827e687cd4976", "commit_rev":
"83a82e0c55fca9efc29c89dd5852fdbfe6118889"}
Message was sent while issue was closed.
Description was changed from ========== [omnibox] Narrow condition for resetting selection Previously, we were resetting the selection if text could have been changed. Now, only do so if we spot difference. Fixes cursor location after an alt-tab focus change. BUG=701519 ========== to ========== [omnibox] Narrow condition for resetting selection Previously, we were resetting the selection if text could have been changed. Now, only do so if we spot difference. Fixes cursor location after an alt-tab focus change. BUG=701519 Review-Url: https://codereview.chromium.org/2763063002 Cr-Commit-Position: refs/heads/master@{#467499} Committed: https://chromium.googlesource.com/chromium/src/+/83a82e0c55fca9efc29c89dd5852... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:310001) as https://chromium.googlesource.com/chromium/src/+/83a82e0c55fca9efc29c89dd5852... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
