views: Move views specific portions out of omnibox_view_browsertest.cc

views: Move views specific portions out of omnibox_view_browsertest.cc

This moves the views parts out of cross-platform omnibox_view_browsertest.cc into a views specific file (c/b/ui/views/omnibox_view_views_browsertest.cc).

BUG=125846
TEST=interactive_ui_tests --gtest_filter=OmniboxViewViewsTest.SelectAllOnClick
R=pkasting@chromium.org

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

[Peter Kasting, 2013-09-03 21:10:26]

LGTM with question

https://codereview.chromium.org/23494016/diff/21001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/23494016/diff/21001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:43: //
content::NotificationObserver:
Why is this necessary?

[tfarina, 2013-09-03 22:16:21]

https://codereview.chromium.org/23494016/diff/21001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/23494016/diff/21001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:43: //
content::NotificationObserver:
On 2013/09/03 21:10:26, Peter Kasting wrote:
> Why is this necessary?
I was sure you were going to notice.

Yes, it was necessary.

When I removed it, the test crashed.

[Peter Kasting, 2013-09-03 22:18:19]

On 2013/09/03 22:16:21, tfarina wrote:
>
https://codereview.chromium.org/23494016/diff/21001/chrome/browser/ui/views/o...
> File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
(right):
> 
>
https://codereview.chromium.org/23494016/diff/21001/chrome/browser/ui/views/o...
> chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:43: //
> content::NotificationObserver:
> On 2013/09/03 21:10:26, Peter Kasting wrote:
> > Why is this necessary?
> I was sure you were going to notice.
> 
> Yes, it was necessary.
> 
> When I removed it, the test crashed.

That doesn't sound like a very good explanation.  Why did the test crash?  Why
does a quit task fix it?  Why are there no comments about this?  Who is even
adding us as an observer of anything, and what message are we observing, since
there is no NotificationRegistrar in this file?

[tfarina, 2013-09-03 23:51:03]

On 2013/09/03 22:18:19, Peter Kasting wrote:
> On 2013/09/03 22:16:21, tfarina wrote:
> >
>
https://codereview.chromium.org/23494016/diff/21001/chrome/browser/ui/views/o...
> > File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/23494016/diff/21001/chrome/browser/ui/views/o...
> > chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:43: //
> > content::NotificationObserver:
> > On 2013/09/03 21:10:26, Peter Kasting wrote:
> > > Why is this necessary?
> > I was sure you were going to notice.
> > 
> > Yes, it was necessary.
> > 
> > When I removed it, the test crashed.
> 
> That doesn't sound like a very good explanation.  Why did the test crash?  Why
> does a quit task fix it?  Why are there no comments about this?  Who is even
> adding us as an observer of anything, and what message are we observing, since
> there is no NotificationRegistrar in this file?

Although it is amazing how you never let anything escape. Actually, it was my
bad as I don't have the sandbox stuff installed/configured.

So running with --disable-setuid-sandbox worked.

$ interactive_ui_tests --disable-setuid-sandbox
--gtest_filter=OmniboxViewViewsTest.SelectAllOnClick

Since the Observe stuff was not necessary, I removed it. ptal.

[Peter Kasting, 2013-09-04 00:02:23]

LGTM, thanks

https://codereview.chromium.org/23494016/diff/31001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/23494016/diff/31001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:42: void
GetOmniboxView(OmniboxView** omnibox_view) {
Nit: Since this is only called once, I'd just inline it into the test below that
calls it.

https://codereview.chromium.org/23494016/diff/31001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:46: const
BrowserView* GetBrowserView() const {
Inlining this is also possible, but more arguable; up to you.

[tfarina, 2013-09-04 01:00:53]

https://codereview.chromium.org/23494016/diff/31001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/23494016/diff/31001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:42: void
GetOmniboxView(OmniboxView** omnibox_view) {
On 2013/09/04 00:02:23, Peter Kasting wrote:
> Nit: Since this is only called once, I'd just inline it into the test below
that
> calls it.

Done.

https://codereview.chromium.org/23494016/diff/31001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:46: const
BrowserView* GetBrowserView() const {
On 2013/09/04 00:02:23, Peter Kasting wrote:
> Inlining this is also possible, but more arguable; up to you.

Done.

[commit-bot: I haz the power, 2013-09-04 16:33:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/23494016/29001

[commit-bot: I haz the power, 2013-09-04 20:10:36]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-04 22:29:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/23494016/29001

[commit-bot: I haz the power, 2013-09-05 03:39:18]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-05 10:44:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/23494016/29001

[commit-bot: I haz the power, 2013-09-05 13:15:38]

Change committed as 221400