ServiceWorker: Introduce SWMessageSender in order to mock IPC messaging

ServiceWorker: Introduce SWMessageSender in order to mock IPC messaging

This CL introduces ServiceWorkerMessageSender that is a thin wrapper
of ThreadSafeSender so that unittests can inject a mock sender, and
adds some unittests related to reference counting using the mock.

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

Committed: https://crrev.com/4d44fa9f7fb3d97b916781a6051ed4dcc69c3668
Cr-Commit-Position: refs/heads/master@{#320885}

[nhiroki, 2015-03-17 01:10:39]

Patchset #1 (id:1) has been deleted

[nhiroki, 2015-03-17 01:10:47]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2015-03-17 01:10:52]

Patchset #1 (id:40001) has been deleted

[nhiroki, 2015-03-17 01:26:25]

Patchset #1 (id:60001) has been deleted

[nhiroki, 2015-03-17 01:58:18]

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

[nhiroki, 2015-03-17 01:58:18]

Hi ksakamoto@, can you review this?

In the curren implementation, ObjectInfo and VersionAttributes implicitly retain
references of object handles. This seems to make implementers confused and to be
a potential cause of reference leaks. I have some cleanup ideas and am going to
work on them after clarifying the current behavior with this CL.

[Kunihiko Sakamoto, 2015-03-17 03:40:38]

lgtm with minor comments

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

https://codereview.chromium.org/1011643002/diff/80001/content/child/service_w...
content/child/service_worker/service_worker_message_sender.h:19: class
CONTENT_EXPORT ServiceWorkerMessageSender
Can you add a short class comment that says this provides a mockable interface?

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

https://codereview.chromium.org/1011643002/diff/80001/content/child/service_w...
content/child/service_worker/service_worker_provider_context.cc:30:
ServiceWorkerDispatcher::GetOrCreateThreadSpecificInstance(
Why not make this take a ServiceWorkerMessageSender?

[nhiroki, 2015-03-17 07:36:54]

Thank you for reviewing! The latest patchset changes codebase more widely than
the previous one, so can you take another look?

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

https://codereview.chromium.org/1011643002/diff/80001/content/child/service_w...
content/child/service_worker/service_worker_message_sender.h:19: class
CONTENT_EXPORT ServiceWorkerMessageSender
On 2015/03/17 03:40:38, Kunihiko Sakamoto wrote:
> Can you add a short class comment that says this provides a mockable
interface?

Added.

In addition, I made this class ThreadSafe because ThreadSafeSender was obviously
a thread-safe object and some owners could release this on a different thread
(eg. SWProviderContext::DestructOnMainThread could delete it on the worker
thread).

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

https://codereview.chromium.org/1011643002/diff/80001/content/child/service_w...
content/child/service_worker/service_worker_provider_context.cc:30:
ServiceWorkerDispatcher::GetOrCreateThreadSpecificInstance(
On 2015/03/17 03:40:38, Kunihiko Sakamoto wrote:
> Why not make this take a ServiceWorkerMessageSender?

Done.

[Kunihiko Sakamoto, 2015-03-17 08:10:19]

LGTM

[nhiroki, 2015-03-17 08:50:09]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2015-03-17 08:50:19]

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

[commit-bot: I haz the power, 2015-03-17 09:48:01]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2015-03-17 09:48:32]

Patchset 3 (id:??) landed as
https://crrev.com/4d44fa9f7fb3d97b916781a6051ed4dcc69c3668
Cr-Commit-Position: refs/heads/master@{#320885}

[michaeln, 2015-03-18 00:18:03]

drive by: I don't understand the needs for the new ServiceWorkerMessageSender
class? The new class is threadsafe refcounted and has a virtual Send(msg)
method. It wraps a threadsafe refcounted class with a virtual Send(msg) method.

From the callers point of view, ServiceWorkerMessageSender looks just like a
ThreadSafeSender, so why the new class?

The CL description sayz this is for unittests to inject a mock sender impl.
Could the unittest inject a mock sender that dervives from ThreadSafeSender
instead of introducing a new class? I think it's be preferrable to add a
protected empty ctor to the ThreadSafeSender class to allow tests to mock it up.

The awkward ServiceWorkerMessageSender::thread_safe_sender() method seems like a
symptom of something being off about this composition.

[nhiroki, 2015-03-18 01:51:58]

On 2015/03/18 00:18:03, michaeln wrote:
> drive by: I don't understand the needs for the new ServiceWorkerMessageSender
> class? The new class is threadsafe refcounted and has a virtual Send(msg)
> method. It wraps a threadsafe refcounted class with a virtual Send(msg)
method.
> 
> From the callers point of view, ServiceWorkerMessageSender looks just like a
> ThreadSafeSender, so why the new class?
> 
> The CL description sayz this is for unittests to inject a mock sender impl.
> Could the unittest inject a mock sender that dervives from ThreadSafeSender
> instead of introducing a new class? I think it's be preferrable to add a
> protected empty ctor to the ThreadSafeSender class to allow tests to mock it
up.
> 
> The awkward ServiceWorkerMessageSender::thread_safe_sender() method seems like
a
> symptom of something being off about this composition.

Thank you for your comment.

I considered the option. Basically I'd prefer composition to derived class from
commonly used class because changes of such a base class could affect widely and
I've actually experienced some troubles due to that. Therefore, finally I
decided to use this way when starting to make a patch

But now, SWMessageSender is used only for mock and
SWMessageSender::thread_safe_sender() seems one of bad signals as you mentioned.
I'll consider making the base class mockable and reverting this.

[nhiroki, 2015-03-18 02:18:44]

On 2015/03/18 01:51:58, nhiroki wrote:
> On 2015/03/18 00:18:03, michaeln wrote:
> > drive by: I don't understand the needs for the new
ServiceWorkerMessageSender
> > class? The new class is threadsafe refcounted and has a virtual Send(msg)
> > method. It wraps a threadsafe refcounted class with a virtual Send(msg)
> method.
> > 
> > From the callers point of view, ServiceWorkerMessageSender looks just like a
> > ThreadSafeSender, so why the new class?
> > 
> > The CL description sayz this is for unittests to inject a mock sender impl.
> > Could the unittest inject a mock sender that dervives from ThreadSafeSender
> > instead of introducing a new class? I think it's be preferrable to add a
> > protected empty ctor to the ThreadSafeSender class to allow tests to mock it
> up.
> > 
> > The awkward ServiceWorkerMessageSender::thread_safe_sender() method seems
like
> a
> > symptom of something being off about this composition.
> 
> Thank you for your comment.
> 
> I considered the option. Basically I'd prefer composition to derived class
from
> commonly used class because changes of such a base class could affect widely
and
> I've actually experienced some troubles due to that. Therefore, finally I
> decided to use this way when starting to make a patch
> 
> But now, SWMessageSender is used only for mock and
> SWMessageSender::thread_safe_sender() seems one of bad signals as you
mentioned.
> I'll consider making the base class mockable and reverting this.

I made a WIP CL (https://codereview.chromium.org/1017013002/) and will revert
this.

[nhiroki, 2015-03-18 02:21:06]

A revert of this CL (patchset #3 id:120001) has been created in
https://codereview.chromium.org/1002393005/ by nhiroki@chromium.org.

The reason for reverting is: Decided to take a different way to mock IPC
messaging.
See review comments on the original CL for more details..