Add unit tests for DesktopMediaPickerViews.

Add unit tests for DesktopMediaPickerViews.

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

[jiayl, 2014-07-15 00:20:05]

PTAL

[Sergey Ulanov, 2014-07-15 00:28:16]

LGTM with one nit

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc (right):

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:82:
GetPickerDialogView()->IsDialogButtonEnabled(ui::DIALOG_BUTTON_OK));
I think it's also good idea to verify that OK button becomes disabled if there
was a window selected and it's unselected somehow, e.g. because it was closed.

[msw, 2014-07-15 03:29:56]

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
File chrome/browser/ui/views/desktop_media_picker_views.h (right):

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views.h:21: // View that shows list
of desktop media sources available from
nit: "a list"

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc (right):

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:19: static const
char* kFakeAppName = "foo";
nit: inline this in SetUp: base::string16 app_name = base::ASCIIToUTF16("foo");

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:25: new
content::TestBrowserThread(content::BrowserThread::UI,
Supposedly this is "DEPRECATED: use TestBrowserThreadBundle instead."
(content/public/test/test_browser_thread.h)

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:45:
parent_widget_->GetNativeWindow(),
Would DesktopMediaPickerViews work okay if you just passed in NULL here?

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:57:
MOCK_METHOD1(OnPickerDone, void(content::DesktopMediaID));
Hmm, I've been discouraged from using MOCK_METHOD* before by ben@. Can you
explicitly override this method to record the pertinent information on class
members, and then check that in the tests?

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:66:
TEST_F(DesktopMediaPickerViewsTest, DoneCallbackCalledWhenWindowDeleted) {
This seems odd, why do we need a test for this? Perhaps just test that the
correct thing happens on GetPickerDialogView()->GetWidget()->Close[Now]()?

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:169:
source_view_0->OnFocus();
Any tests involving focus or activation should be interactive_ui_tests,
otherwise they're bound to be flaky when run concurrently with other tests.

[jiayl, 2014-07-15 16:59:43]

PTAL

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc (right):

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:19: static const
char* kFakeAppName = "foo";
On 2014/07/15 03:29:56, msw wrote:
> nit: inline this in SetUp: base::string16 app_name =
base::ASCIIToUTF16("foo");

Done.

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:25: new
content::TestBrowserThread(content::BrowserThread::UI,
On 2014/07/15 03:29:56, msw wrote:
> Supposedly this is "DEPRECATED: use TestBrowserThreadBundle instead."
> (content/public/test/test_browser_thread.h)

TestBrowserThreadBundle does not work here, because the thread already has a
message loop from ViewsTestBase while TestBrowserThreadBundle always tries to
create a new message loop.

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:45:
parent_widget_->GetNativeWindow(),
On 2014/07/15 03:29:56, msw wrote:
> Would DesktopMediaPickerViews work okay if you just passed in NULL here?

No, it will fire an assert in creating the widget.

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:57:
MOCK_METHOD1(OnPickerDone, void(content::DesktopMediaID));
On 2014/07/15 03:29:56, msw wrote:
> Hmm, I've been discouraged from using MOCK_METHOD* before by ben@. Can you
> explicitly override this method to record the pertinent information on class
> members, and then check that in the tests?

Why? MOCK_METHOD seems a cleaner solution.

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:66:
TEST_F(DesktopMediaPickerViewsTest, DoneCallbackCalledWhenWindowDeleted) {
On 2014/07/15 03:29:56, msw wrote:
> This seems odd, why do we need a test for this? Perhaps just test that the
> correct thing happens on GetPickerDialogView()->GetWidget()->Close[Now]()?

Done.

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:82:
GetPickerDialogView()->IsDialogButtonEnabled(ui::DIALOG_BUTTON_OK));
On 2014/07/15 00:28:16, Sergey Ulanov wrote:
> I think it's also good idea to verify that OK button becomes disabled if there
> was a window selected and it's unselected somehow, e.g. because it was closed.

Done.

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:169:
source_view_0->OnFocus();
On 2014/07/15 03:29:56, msw wrote:
> Any tests involving focus or activation should be interactive_ui_tests,
> otherwise they're bound to be flaky when run concurrently with other tests.

It does not depends on the actual UI focus. We only call onFocus to emulate a
focus action.

[msw, 2014-07-15 18:59:58]

LGTM, but please ping a TestBrowserThread/TestBrowserThreadBundle reviewer (+CC
earthdok@).

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc (right):

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:25: new
content::TestBrowserThread(content::BrowserThread::UI,
On 2014/07/15 16:59:43, jiayl wrote:
> On 2014/07/15 03:29:56, msw wrote:
> > Supposedly this is "DEPRECATED: use TestBrowserThreadBundle instead."
> > (content/public/test/test_browser_thread.h)
> 
> TestBrowserThreadBundle does not work here, because the thread already has a
> message loop from ViewsTestBase while TestBrowserThreadBundle always tries to
> create a new message loop.

Please ping a reviewer knowledgeable about that deprecated class or its
replacement before introducing another use of a deprecated class.

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:45:
parent_widget_->GetNativeWindow(),
On 2014/07/15 16:59:43, jiayl wrote:
> On 2014/07/15 03:29:56, msw wrote:
> > Would DesktopMediaPickerViews work okay if you just passed in NULL here?
> 
> No, it will fire an assert in creating the widget.

Acknowledged.

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:57:
MOCK_METHOD1(OnPickerDone, void(content::DesktopMediaID));
On 2014/07/15 16:59:43, jiayl wrote:
> On 2014/07/15 03:29:56, msw wrote:
> > Hmm, I've been discouraged from using MOCK_METHOD* before by ben@. Can you
> > explicitly override this method to record the pertinent information on class
> > members, and then check that in the tests?
> 
> Why? MOCK_METHOD seems a cleaner solution.

I don't recall the concerns, so I won't advocate for the change any further.

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:169:
source_view_0->OnFocus();
On 2014/07/15 16:59:42, jiayl wrote:
> On 2014/07/15 03:29:56, msw wrote:
> > Any tests involving focus or activation should be interactive_ui_tests,
> > otherwise they're bound to be flaky when run concurrently with other tests.
> 
> It does not depends on the actual UI focus. We only call onFocus to emulate a
> focus action.

Acknowledged.

[jiayl, 2014-07-15 19:22:40]

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc (right):

https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:25: new
content::TestBrowserThread(content::BrowserThread::UI,
On 2014/07/15 18:59:58, msw wrote:
> On 2014/07/15 16:59:43, jiayl wrote:
> > On 2014/07/15 03:29:56, msw wrote:
> > > Supposedly this is "DEPRECATED: use TestBrowserThreadBundle instead."
> > > (content/public/test/test_browser_thread.h)
> > 
> > TestBrowserThreadBundle does not work here, because the thread already has a
> > message loop from ViewsTestBase while TestBrowserThreadBundle always tries
to
> > create a new message loop.
> 
> Please ping a reviewer knowledgeable about that deprecated class or its
> replacement before introducing another use of a deprecated class.

I removed the use of TestBrowserThread by removing the dependency on
ViewTestBase.

[earthdok, 2014-07-15 19:28:45]

TestBrowserThread is deprecated because it tends to introduce racy behavior in
test teardown (most frequently manifesting as flaky memory leaks, which make
reliable leak detection troublesome). Stopping the threads in a clean way is
somewhat tricky, and ~TestBrowserThreadBundle() should be the only place that
has to care about that.

Thanks for not using TestBrowserThread.

On 2014/07/15 18:59:58, msw wrote:
> LGTM, but please ping a TestBrowserThread/TestBrowserThreadBundle reviewer
(+CC
> earthdok@).
> 
>
https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
> File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc (right):
> 
>
https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
> chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:25: new
> content::TestBrowserThread(content::BrowserThread::UI,
> On 2014/07/15 16:59:43, jiayl wrote:
> > On 2014/07/15 03:29:56, msw wrote:
> > > Supposedly this is "DEPRECATED: use TestBrowserThreadBundle instead."
> > > (content/public/test/test_browser_thread.h)
> > 
> > TestBrowserThreadBundle does not work here, because the thread already has a
> > message loop from ViewsTestBase while TestBrowserThreadBundle always tries
to
> > create a new message loop.
> 
> Please ping a reviewer knowledgeable about that deprecated class or its
> replacement before introducing another use of a deprecated class.
> 
>
https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
> chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:45:
> parent_widget_->GetNativeWindow(),
> On 2014/07/15 16:59:43, jiayl wrote:
> > On 2014/07/15 03:29:56, msw wrote:
> > > Would DesktopMediaPickerViews work okay if you just passed in NULL here?
> > 
> > No, it will fire an assert in creating the widget.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
> chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:57:
> MOCK_METHOD1(OnPickerDone, void(content::DesktopMediaID));
> On 2014/07/15 16:59:43, jiayl wrote:
> > On 2014/07/15 03:29:56, msw wrote:
> > > Hmm, I've been discouraged from using MOCK_METHOD* before by ben@. Can you
> > > explicitly override this method to record the pertinent information on
class
> > > members, and then check that in the tests?
> > 
> > Why? MOCK_METHOD seems a cleaner solution.
> 
> I don't recall the concerns, so I won't advocate for the change any further.
> 
>
https://codereview.chromium.org/393653004/diff/1/chrome/browser/ui/views/desk...
> chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:169:
> source_view_0->OnFocus();
> On 2014/07/15 16:59:42, jiayl wrote:
> > On 2014/07/15 03:29:56, msw wrote:
> > > Any tests involving focus or activation should be interactive_ui_tests,
> > > otherwise they're bound to be flaky when run concurrently with other
tests.
> > 
> > It does not depends on the actual UI focus. We only call onFocus to emulate
a
> > focus action.
> 
> Acknowledged.

[jiayl, 2014-07-15 20:22:22]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-07-15 20:23:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiayl@chromium.org/393653004/40001

[jiayl, 2014-07-15 22:02:31]

The CQ bit was unchecked by jiayl@chromium.org

[jiayl, 2014-07-15 22:08:15]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-07-15 22:11:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiayl@chromium.org/393653004/60001

[jiayl, 2014-07-16 00:15:50]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-07-16 00:17:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiayl@chromium.org/393653004/80001

[commit-bot: I haz the power, 2014-07-16 08:25:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-16 08:57:10]

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

[commit-bot: I haz the power, 2014-07-16 08:57:11]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[jiayl, 2014-07-16 16:42:00]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-07-16 16:43:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiayl@chromium.org/393653004/100001

[commit-bot: I haz the power, 2014-07-17 08:43:13]

Change committed as 283717