Add MediaControlsImplTestWithMockScheduler.

Add MediaControlsImplTestWithMockScheduler.

Currently, MediaControlsImplTest.ControlsRemainVisibleDuringKeyboardInteraction
does the following:

SetUp()
  InitializePage()
  Creates a DummyPageHolder, which indirectly creates a
  blink::PerformanceMonitor. blink::PerformanceMonitor registers
  ifself as a task time observer with the current
  TaskQueueManager.

Body
  Instantiate
  ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>.
  This overrides the current TaskQueueManager with a mock.

  InitializePage()
  This deletes the existing DummyPageHolder, which indirectly
  deletes the blink::PerformanceMonitor. Before it is deleted, 
  blink::PerformanceMonitor unregisters from the current
  TaskQueueManager, which is unfortunately *not the same* as the one
  to which it was registered. That means that the initial
  TaskQueueManager is left with a pointer to a deleted
  blink::PerformanceMonitor.

TearDownBlinkTestEnvironment()
  We would like to flush the ScopedTaskEnvironment from here but
  we can't because that causes the initial TaskQueueManager to
  access its invalid blink::PerformanceMonitor pointer.

This CL fixes this issue by creating a
MediaControlsImplTestWithMockScheduler which instantiates a
ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>
before SetUp() to make sure that blink::PerformanceMonitor
registers itself with the mock TaskQueueManager rather than
with the real one.

BUG=724077
Review-Url: https://codereview.chromium.org/2909093002
Cr-Commit-Position: refs/heads/master@{#475382}
Committed: https://chromium.googlesource.com/chromium/src/+/1f3a7e66d633e18507101436900fa98625d8e987

[fdoray, 2017-05-29 16:02:53]

fdoray@chromium.org changed reviewers:
+ avayvod@chromium.org

[fdoray, 2017-05-29 16:02:53]

PTAL

[whywhat, 2017-05-29 16:27:00]

avayvod@chromium.org changed reviewers:
+ mlamouri@chromium.org

[whywhat, 2017-05-29 16:27:00]

avayvod@chromium.org changed required reviewers:
+ mlamouri@chromium.org

[whywhat, 2017-05-29 16:27:01]

+Tommi, Mounir as I'm not very familiar with the test and not an owner anyway

On the first glance, the fix seems unclear (esp. when reading the code later
without digging up this CL). Copying the CL description to the call of
ResetPage() would be the least change that'd help IMO.

Perhaps InitializePage() could be split up into what is still okay to do in
SetUp() and the rest could live in InitializePage and be called when needed by
each test case? So the failing test would only call InitializePage once, after
platform is changed.

[fdoray, 2017-05-29 17:37:45]

Description was changed from

==========
Reset page at the beginning of
MediaControlsImplTest.ControlsRemainVisibleDuringKeyboardInteraction.

blink::PerformanceMonitor registers itself as a task time observer with
the current TaskQueueManager in its constructor and unregisters itself
with the current TaskQueueManager when it is shutdown. Therefore, it is
important to instantiate and shutdown a blink::PerformanceMonitor in the
scope of the same TaskQueueManager.

Before this CL,
MediaControlsImplTest.ControlsRemainVisibleDuringKeyboardInteraction
created a blink::PerformanceMonitor within the scope of a
TaskQueueManager in SetUp() and destroyed it within the scope of a
different TaskQueueManager in InitializePage(). This prevented
ScopedTaskEnvironment from flushing tasks in its destructor (the
destroyed blink::PerformanceMonitor remained an observer of the
first TaskQueueManager).

This CL fixes this issue by deleting the blink::PerformanceMonitor
before switching the TaskQueueManager (done within the constructor
of ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>.

BUG=724077
==========

to

==========
Add MediaControlsImplTestWithMockScheduler.

Currently, MediaControlsImplTest.ControlsRemainVisibleDuringKeyboardInteraction
does the following:

SetUp()
  InitializePage()
  Creates a DummyPageHolder, which indirectly creates a
  blink::PerformanceMonitor. blink::PerformanceMonitor registers
  ifself as a task time observer with the current
  TaskQueueManager.

Body
  Instantiate
  ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>.
  This overrides the current TaskQueueManager with a mock.

  InitializePage()
  This deletes the existing DummyPageHolder, which indirectly
  deletes the blink::PerformanceMonitor. Before it is deleted, 
  blink::PerformanceMonitor unregisters from the current
  TaskQueueManager, which is unfortunately *not the same* as the one
  to which it was registered. That means that the initial
  TaskQueueManager is left with a pointer to a deleted
  blink::PerformanceMonitor.

TearDownBlinkTestEnvironment()
  We would like to flush the ScopedTaskEnvironment from here but
  we can't because that causes the initial TaskQueueManager to
  access its invalid blink::PerformanceMonitor pointer.

This CL fixes this issue by creating a
MediaControlsImplTestWithMockScheduler which instantiates a
ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>
before SetUp() to make sure that blink::PerformanceMonitor
registers itself with the mock TaskQueueManager rather than
with the real one.

BUG=724077
==========

[fdoray, 2017-05-29 17:37:57]

On 2017/05/29 16:27:01, whywhat wrote:
> +Tommi, Mounir as I'm not very familiar with the test and not an owner anyway
> 
> On the first glance, the fix seems unclear (esp. when reading the code later
> without digging up this CL). Copying the CL description to the call of
> ResetPage() would be the least change that'd help IMO.
> 
> Perhaps InitializePage() could be split up into what is still okay to do in
> SetUp() and the rest could live in InitializePage and be called when needed by
> each test case? So the failing test would only call InitializePage once, after
> platform is changed.

Please take another look.

[whywhat, 2017-05-29 18:19:53]

I don't think anything has changed in the CL, hasn't it?

Thanks for the expanded CL description but it doesn't address my concern.
I clarified my previous reply in a code comment.

In any case I'd let Mounir take a look first before doing any substantial work
to fix the test.

https://codereview.chromium.org/2909093002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/media_controls/MediaControlsImplTest.cpp
(right):

https://codereview.chromium.org/2909093002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/media_controls/MediaControlsImplTest.cpp:797:
ResetPage();
This is the line that would likely look confusing to anyone reading this code in
a month from now. Hence I asked for a comment here in my previous reply.

Could we avoid the issue by making ScopedTestingPlatformSupport<> a member of
MediaControlsImplTest reset in SetUp() before InitializePage with a new value?

Then it could be exposed as Platform() to the test case below which won't have
to create a scoped platform or InitializePage() hence avoiding the issue with
PerformanceMonitor.

Something like:

...

class MediaControlsImplTest {
  void SetUp() override {
    testing_platform_ =
MakeUnique<ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>>();
    InitializePage();
  }

  ScopedTestingPlatformSupport* Platform() { return testing_platform_.get(); }

  ...

 private:
 
unique_ptr<ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>>
testing_platform_;
  ...
};

...

TEST_F(MediaControlsImplTest, ControlsRemainVisibleDuringKeyboardInteraction) {
  // DocumentParserTiming has DCHECKS to make sure time > 0.0.
  Platform()->AdvanceClockSeconds(1);
  EnsureSizing();

  Element* panel = MediaControls().PanelElement();

  ...
  Platform()->RunForPeriodSeconds(2);
  ...
}

[whywhat, 2017-05-29 18:21:39]

Actually from the bits of the new CL description it sounds like you already did
something like what I suggested but perhaps the upload failed?

[fdoray, 2017-05-29 18:28:37]

On 2017/05/29 18:21:39, whywhat wrote:
> Actually from the bits of the new CL description it sounds like you already
did
> something like what I suggested but perhaps the upload failed?

Yes, sorry. Patch set is now uploaded.

[whywhat, 2017-05-29 19:13:54]

non-owners lgtm

[whywhat, 2017-05-29 19:15:10]

avayvod@chromium.org changed reviewers:
- mlamouri@chromium.org

[whywhat, 2017-05-29 19:15:10]

avayvod@chromium.org changed required reviewers:
- mlamouri@chromium.org

[whywhat, 2017-05-29 19:15:11]

Actually, I seem to be the owner now so just lgtm :)

[fdoray, 2017-05-29 19:16:02]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2017-05-29 19:16:09]

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-05-29 21:16:58]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496085362375310,
"parent_rev": "01f99abb0f0a8a68fcf01527068d481c16bd70c0", "commit_rev":
"1f3a7e66d633e18507101436900fa98625d8e987"}

[commit-bot: I haz the power, 2017-05-29 21:17:04]

Description was changed from

==========
Add MediaControlsImplTestWithMockScheduler.

Currently, MediaControlsImplTest.ControlsRemainVisibleDuringKeyboardInteraction
does the following:

SetUp()
  InitializePage()
  Creates a DummyPageHolder, which indirectly creates a
  blink::PerformanceMonitor. blink::PerformanceMonitor registers
  ifself as a task time observer with the current
  TaskQueueManager.

Body
  Instantiate
  ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>.
  This overrides the current TaskQueueManager with a mock.

  InitializePage()
  This deletes the existing DummyPageHolder, which indirectly
  deletes the blink::PerformanceMonitor. Before it is deleted, 
  blink::PerformanceMonitor unregisters from the current
  TaskQueueManager, which is unfortunately *not the same* as the one
  to which it was registered. That means that the initial
  TaskQueueManager is left with a pointer to a deleted
  blink::PerformanceMonitor.

TearDownBlinkTestEnvironment()
  We would like to flush the ScopedTaskEnvironment from here but
  we can't because that causes the initial TaskQueueManager to
  access its invalid blink::PerformanceMonitor pointer.

This CL fixes this issue by creating a
MediaControlsImplTestWithMockScheduler which instantiates a
ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>
before SetUp() to make sure that blink::PerformanceMonitor
registers itself with the mock TaskQueueManager rather than
with the real one.

BUG=724077
==========

to

==========
Add MediaControlsImplTestWithMockScheduler.

Currently, MediaControlsImplTest.ControlsRemainVisibleDuringKeyboardInteraction
does the following:

SetUp()
  InitializePage()
  Creates a DummyPageHolder, which indirectly creates a
  blink::PerformanceMonitor. blink::PerformanceMonitor registers
  ifself as a task time observer with the current
  TaskQueueManager.

Body
  Instantiate
  ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>.
  This overrides the current TaskQueueManager with a mock.

  InitializePage()
  This deletes the existing DummyPageHolder, which indirectly
  deletes the blink::PerformanceMonitor. Before it is deleted, 
  blink::PerformanceMonitor unregisters from the current
  TaskQueueManager, which is unfortunately *not the same* as the one
  to which it was registered. That means that the initial
  TaskQueueManager is left with a pointer to a deleted
  blink::PerformanceMonitor.

TearDownBlinkTestEnvironment()
  We would like to flush the ScopedTaskEnvironment from here but
  we can't because that causes the initial TaskQueueManager to
  access its invalid blink::PerformanceMonitor pointer.

This CL fixes this issue by creating a
MediaControlsImplTestWithMockScheduler which instantiates a
ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>
before SetUp() to make sure that blink::PerformanceMonitor
registers itself with the mock TaskQueueManager rather than
with the real one.

BUG=724077

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

[commit-bot: I haz the power, 2017-05-29 21:17:05]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/1f3a7e66d633e18507101436900f...