Use SetUserText() in OmniboxViewViews emphasis tests to ensure system coherence.

Use SetUserText() in OmniboxViewViews emphasis tests to ensure system coherence.

The tests as first written used SetText(), which changes the text in the
textfield itself but not the text that the view/edit data structures believe is
present.  Similarly, the edit model never thinks input is in progress.  Since
some of the tests want to pretend input _is_ in progress, the ability to
override CurrentTextIsUrl() was introducd to hack around the issue.  This does
things more "the right way".

BUG=none
TEST=none

Review-Url: https://codereview.chromium.org/2735533002
Cr-Commit-Position: refs/heads/master@{#455440}
Committed: https://chromium.googlesource.com/chromium/src/+/1b06902a8e1ba9c3dabbd48f7fcb96d7d6ceedc3

[Peter Kasting, 2017-03-04 05:25:29]

pkasting@chromium.org changed reviewers:
+ elawrence@chromium.org

[Peter Kasting, 2017-03-04 05:27:47]

The CQ bit was checked by pkasting@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-04 05:27:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Kasting, 2017-03-04 05:29:13]

BTW, I hope the last sentences of the CL description don't sound like they're
demeaning your original solution -- it took me a couple evenings of work to
figure out how to do this without silly amounts of plumbing, and your proposal
was definitely the right thing to do for expediency's sake.

[commit-bot: I haz the power, 2017-03-04 06:28:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-04 06:28:27]

Dry run: This issue passed the CQ dry run.

[elawrence, 2017-03-04 18:41:48]

LGTM, thanks for the improvements! 

Would it make sense to have test variants (for the URL cases) where the input is
in progress (e.g. to verify that the scheme_text emphasis function /isn't/
called)?

https://codereview.chromium.org/2735533002/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right):

https://codereview.chromium.org/2735533002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.h:88: using
OmniboxView::SetUserText;
I must plead ignorance to what this using declaration does.

[Peter Kasting, 2017-03-06 21:18:03]

You need not re-review unless you want to.

On 2017/03/04 18:41:48, elawrence wrote:
> LGTM, thanks for the improvements! 
> 
> Would it make sense to have test variants (for the URL cases) where the input
is
> in progress (e.g. to verify that the scheme_text emphasis function /isn't/
> called)?

Good idea.  Done.

https://codereview.chromium.org/2735533002/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right):

https://codereview.chromium.org/2735533002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.h:88: using
OmniboxView::SetUserText;
On 2017/03/04 18:41:48, elawrence wrote:
> I must plead ignorance to what this using declaration does.

It exposes the base class methods that are not overridden in this class to name
lookup.  In this case, it lets me call the one-arg SetUserText() in the base
class without doing:

  static_cast<OmniboxView*>(ptr)->SetUserText(...);

Or:

  ptr->OmniboxView::SetUserText(...);

...both of which feel more ugly, and force each caller that wants this to do it.

https://codereview.chromium.org/2735533002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right):

https://codereview.chromium.org/2735533002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:281: constexpr
struct {
Note: This depends on https://codereview.chromium.org/2734663002/ .  Depending
on what order things get reviewed in I may move it there.

[elawrence, 2017-03-07 03:16:04]

Ah, thanks for the explanation regarding the "using"!

New tests LGTM, thanks for adding them!

https://codereview.chromium.org/2735533002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right):

https://codereview.chromium.org/2735533002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:311: if
(test_case.scheme_range.IsValid()) {
Maybe consider a comment here to clarify the less-obvious behavior ("The scheme
should only be emphasized after input is accepted")?

[Peter Kasting, 2017-03-08 11:25:28]

The CQ bit was checked by pkasting@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 11:25:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 11:30:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 11:30:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Peter Kasting, 2017-03-08 12:22:10]

The CQ bit was checked by pkasting@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 12:22:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 12:27:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 12:27:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Kasting, 2017-03-08 12:30:49]

The CQ bit was checked by pkasting@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 12:30:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 13:12:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 13:12:29]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-03-08 13:12:47]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2017-03-08 13:12:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
elawrence@chromium.org
Link to the patchset: https://codereview.chromium.org/2735533002/#ps100001
(title: "Hopefully compiles deux")

[commit-bot: I haz the power, 2017-03-08 13:12:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 13:16:31]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488978767837090,
"parent_rev": "d2895ee2c86e1695b46d13f97454625a5929f8f9", "commit_rev":
"1b06902a8e1ba9c3dabbd48f7fcb96d7d6ceedc3"}

[commit-bot: I haz the power, 2017-03-08 13:17:11]

Description was changed from

==========
Use SetUserText() in OmniboxViewViews emphasis tests to ensure system coherence.

The tests as first written used SetText(), which changes the text in the
textfield itself but not the text that the view/edit data structures believe is
present.  Similarly, the edit model never thinks input is in progress.  Since
some of the tests want to pretend input _is_ in progress, the ability to
override CurrentTextIsUrl() was introducd to hack around the issue.  This does
things more "the right way".

BUG=none
TEST=none
==========

to

==========
Use SetUserText() in OmniboxViewViews emphasis tests to ensure system coherence.

The tests as first written used SetText(), which changes the text in the
textfield itself but not the text that the view/edit data structures believe is
present.  Similarly, the edit model never thinks input is in progress.  Since
some of the tests want to pretend input _is_ in progress, the ability to
override CurrentTextIsUrl() was introducd to hack around the issue.  This does
things more "the right way".

BUG=none
TEST=none

Review-Url: https://codereview.chromium.org/2735533002
Cr-Commit-Position: refs/heads/master@{#455440}
Committed:
https://chromium.googlesource.com/chromium/src/+/1b06902a8e1ba9c3dabbd48f7fcb...
==========

[commit-bot: I haz the power, 2017-03-08 13:17:14]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/1b06902a8e1ba9c3dabbd48f7fcb...