ServiceWorker: Add ServiceWorkerDispatcherTest

ServiceWorker: Add ServiceWorkerDispatcherTest

This is a retry patch to add ServiceWorkerDispatcherTest and not to modify
the test from the original patch[1]. Main difference from the original
patch is directly to make ThreadSafeSender mockable instead of introducing
an interlayer (ie. ServiceWorkerMessageSender in [1]).

[1] https://codereview.chromium.org/1011643002/

BUG=452403
TEST=content_unittests --gtest_filter=ServiceWorkerDispatcherTest.*

Committed: https://crrev.com/c384ab1582a3b0c029c973630b93b7937645ade3
Cr-Commit-Position: refs/heads/master@{#321252}

[nhiroki, 2015-03-18 04:59:29]

Patchset #3 (id:40001) has been deleted

[nhiroki, 2015-03-18 05:19:35]

Patchset #1 (id:1) has been deleted

[nhiroki, 2015-03-18 05:59:54]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2015-03-18 05:59:59]

Patchset #1 (id:60001) has been deleted

[nhiroki, 2015-03-18 06:14:35]

nhiroki@chromium.org changed reviewers:
+ michaeln@chromium.org

[nhiroki, 2015-03-18 06:14:36]

Hi Michael, can you review this? Thanks.

[michaeln, 2015-03-18 20:32:37]

lgtm and thnx!

[nhiroki, 2015-03-18 22:11:48]

nhiroki@chromium.org changed reviewers:
+ nasko@chromium.org

[nhiroki, 2015-03-18 22:12:50]

+nasko@, can you review content/child/thread_safe_sender.{cc,h}?

[nasko, 2015-03-18 22:21:44]

LGTM with some nits.

https://codereview.chromium.org/1017013002/diff/80001/content/child/service_w...
File content/child/service_worker/service_worker_dispatcher.h (right):

https://codereview.chromium.org/1017013002/diff/80001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher.h:44: class
CONTENT_EXPORT ServiceWorkerDispatcher
Is this needed for linking to the unit tests or something else?

https://codereview.chromium.org/1017013002/diff/80001/content/child/service_w...
File content/child/service_worker/service_worker_dispatcher_unittest.cc (right):

https://codereview.chromium.org/1017013002/diff/80001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher_unittest.cc:83: 
nit: no need for empty line.

https://codereview.chromium.org/1017013002/diff/80001/content/child/thread_sa...
File content/child/thread_safe_sender.h (right):

https://codereview.chromium.org/1017013002/diff/80001/content/child/thread_sa...
content/child/thread_safe_sender.h:32: ThreadSafeSender();  // for testing
nit: I'd put the comment above and expand it to explain that this is constructor
only used for testing and that is should *never* be used in actual shipping
code. Stronger language works better to discourage misuse : )

[nhiroki, 2015-03-18 23:10:19]

Patchset #2 (id:100001) has been deleted

[nhiroki, 2015-03-18 23:14:37]

Thank you! I slightly modified the ctor of ThreadSafeSender.

https://codereview.chromium.org/1017013002/diff/80001/content/child/service_w...
File content/child/service_worker/service_worker_dispatcher.h (right):

https://codereview.chromium.org/1017013002/diff/80001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher.h:44: class
CONTENT_EXPORT ServiceWorkerDispatcher
On 2015/03/18 22:21:44, nasko wrote:
> Is this needed for linking to the unit tests or something else?

Right. This is used for linking to the tests.

https://codereview.chromium.org/1017013002/diff/80001/content/child/service_w...
File content/child/service_worker/service_worker_dispatcher_unittest.cc (right):

https://codereview.chromium.org/1017013002/diff/80001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher_unittest.cc:83: 
On 2015/03/18 22:21:44, nasko wrote:
> nit: no need for empty line.

Done.

https://codereview.chromium.org/1017013002/diff/80001/content/child/thread_sa...
File content/child/thread_safe_sender.h (right):

https://codereview.chromium.org/1017013002/diff/80001/content/child/thread_sa...
content/child/thread_safe_sender.h:32: ThreadSafeSender();  // for testing
On 2015/03/18 22:21:44, nasko wrote:
> nit: I'd put the comment above and expand it to explain that this is
constructor
> only used for testing and that is should *never* be used in actual shipping
> code. Stronger language works better to discourage misuse : )

Agree. And it'd be far better that implementation disallows the usage. I moved
2-args private ctor to protected.

[nhiroki, 2015-03-19 00:26:36]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2015-03-19 00:26:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1017013002/#ps120001
(title: "address nasko@'s comments")

[commit-bot: I haz the power, 2015-03-19 00:28:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1017013002/120001

[commit-bot: I haz the power, 2015-03-19 00:33:22]

Committed patchset #2 (id:120001)

[commit-bot: I haz the power, 2015-03-19 00:34:22]

Patchset 2 (id:??) landed as
https://crrev.com/c384ab1582a3b0c029c973630b93b7937645ade3
Cr-Commit-Position: refs/heads/master@{#321252}