ServiceWorker: keep dispatcher hosts for unittests in the test helper

ServiceWorker: keep dispatcher hosts for unittests in the test helper

Currently tests which have their mock dispatcher hosts remove the dispatcher
host only from SWContextCore but not from the test helper. This patch is to fix
that so that tests will have only one appropriate dispatcher host at the same
time.

This is split off from https://crrev.com/2779763004/ .

BUG=629701, 676983, 668633
Review-Url: https://codereview.chromium.org/2933793002
Cr-Commit-Position: refs/heads/master@{#478578}
Committed: https://chromium.googlesource.com/chromium/src/+/4f172ae8bad230163d337b8b07c9969232dc1d0d

[shimazu, 2017-06-12 05:48:43]

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

[commit-bot: I haz the power, 2017-06-12 05:48:50]

Dry run: CQ is trying da patch.

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

[shimazu, 2017-06-12 05:50:08]

shimazu@chromium.org changed reviewers:
+ falken@chromium.org

[shimazu, 2017-06-12 05:50:09]

ptal, thanks! :)

[falken, 2017-06-12 06:37:33]

lgtm

https://codereview.chromium.org/2933793002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2933793002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:126: void
RegisterMockDispatcherHost(
I think we don't need to say Mock since it takes a real
ServiceWorkerDispatcherHost.

Add a comment that if a nullptr is passed, it removes the existing dispatcher
host from the map.

[shimazu, 2017-06-12 06:51:08]

https://codereview.chromium.org/2933793002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2933793002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:126: void
RegisterMockDispatcherHost(
On 2017/06/12 06:37:32, falken wrote:
> I think we don't need to say Mock since it takes a real
> ServiceWorkerDispatcherHost.

I prefer saying Mock here because unittests should treat the dispatcher host
registered from here as a mock even though the type is SWDispatcherHost. I
thought having "Mock" in the method name clarifies how the dispatcher host
should be handled in the test. 
WDYT? 

> 
> Add a comment that if a nullptr is passed, it removes the existing dispatcher
> host from the map.

That seems great. Added.

[falken, 2017-06-12 07:08:21]

https://codereview.chromium.org/2933793002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2933793002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:126: void
RegisterMockDispatcherHost(
On 2017/06/12 06:51:08, shimazu wrote:
> On 2017/06/12 06:37:32, falken wrote:
> > I think we don't need to say Mock since it takes a real
> > ServiceWorkerDispatcherHost.
> 
> I prefer saying Mock here because unittests should treat the dispatcher host
> registered from here as a mock even though the type is SWDispatcherHost. I
> thought having "Mock" in the method name clarifies how the dispatcher host
> should be handled in the test. 
> WDYT? 

I think it's confusing because all the other occurrences of "Mock" in this file
are about a type with "Mock" in the name. If I saw MockDispatcherHost I would
first think I need to pass a MockServiceWorkerDispatcherHost to it. That seems
to be the convention in most of the codebase.

But maybe I don't understand what you mean by the unittests should treat the
dispatcher host as a mock.

[shimazu, 2017-06-12 08:04:07]

https://codereview.chromium.org/2933793002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2933793002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:126: void
RegisterMockDispatcherHost(
On 2017/06/12 07:08:20, falken wrote:
> On 2017/06/12 06:51:08, shimazu wrote:
> > On 2017/06/12 06:37:32, falken wrote:
> > > I think we don't need to say Mock since it takes a real
> > > ServiceWorkerDispatcherHost.
> > 
> > I prefer saying Mock here because unittests should treat the dispatcher host
> > registered from here as a mock even though the type is SWDispatcherHost. I
> > thought having "Mock" in the method name clarifies how the dispatcher host
> > should be handled in the test. 
> > WDYT? 
> 
> I think it's confusing because all the other occurrences of "Mock" in this
file
> are about a type with "Mock" in the name. If I saw MockDispatcherHost I would
> first think I need to pass a MockServiceWorkerDispatcherHost to it. That seems
> to be the convention in most of the codebase.

I see, I agree with that. 

> 
> But maybe I don't understand what you mean by the unittests should treat the
> dispatcher host as a mock.

I just thought "Mock" might be good to indicate it's different from the usual
path, but now I think it's clear because this is the method for testing helper.
Updated.

[shimazu, 2017-06-12 08:04:25]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2017-06-12 08:04:25]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2933793002/#ps60001
(title: "RegisterMockDispatcherHost -> RegisterDispatcherHost")

[commit-bot: I haz the power, 2017-06-12 08:04:32]

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-06-12 10:15:41]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497254665407280,
"parent_rev": "45a4254da274bb79c1e0ad1a27c1644fc23156c3", "commit_rev":
"4f172ae8bad230163d337b8b07c9969232dc1d0d"}

[commit-bot: I haz the power, 2017-06-12 10:15:53]

Description was changed from

==========
ServiceWorker: keep dispatcher hosts for unittests in the test helper

Currently tests which have their mock dispatcher hosts remove the dispatcher
host only from SWContextCore but not from the test helper. This patch is to fix
that so that tests will have only one appropriate dispatcher host at the same
time.

This is split off from https://crrev.com/2779763004/ .

BUG=629701, 676983, 668633
==========

to

==========
ServiceWorker: keep dispatcher hosts for unittests in the test helper

Currently tests which have their mock dispatcher hosts remove the dispatcher
host only from SWContextCore but not from the test helper. This patch is to fix
that so that tests will have only one appropriate dispatcher host at the same
time.

This is split off from https://crrev.com/2779763004/ .

BUG=629701, 676983, 668633

Review-Url: https://codereview.chromium.org/2933793002
Cr-Commit-Position: refs/heads/master@{#478578}
Committed:
https://chromium.googlesource.com/chromium/src/+/4f172ae8bad230163d337b8b07c9...
==========

[commit-bot: I haz the power, 2017-06-12 10:15:55]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4f172ae8bad230163d337b8b07c9...