arc-bridge: Introduce the ArcService class

components/arc/arc_service.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_ARC_ARC_SERVICE_H_
+#define COMPONENTS_ARC_ARC_SERVICE_H_
+
+#include "base/macros.h"
+
+namespace arc {
+
+class ArcBridgeService;
+
+// Abstract class whose lifecycle will be managed by the ArcServiceManager.  It
+// is guaranteed that once the ownership of an ArcService has been transferred
+// to ArcServiceManager, it will outlive the ArcBridgeService, so it is safe to
+// keep a weak reference to it.
+class ArcService {
+ public:
+  ArcService() {}

[hidehiko, 2016/01/18 01:26:45]

Can we move {Add,Remove}Observer(this) to this class, so we do not in inherited
classes?
Also, you may want to move CallValidThread() check function to here, so that we
can share the implementation?

[Luis Héctor Chávez, 2016/01/19 17:43:54]

The problem with moving AddObserver(this) to this class is that ArcService would
need to be an ArcBridgeService::Observer (but would not implement any methods,
which is weird). Also, there is a subclass-specific check that needs to happen
just after adding the observer, which would make it error-prone to omit since it
would work some of the times (most of the interfaces did not follow the best
practices doc and ended up not including it). See what happened with the app
list (crbug.com/575873) for an example of why I'm a bit against doing this.

If I don't add AddObserver to this class, adding RemoveObserver feels
inconsistent.

For CalledOnValidThread, we probably should instead make the subclasses inherit
from NonThreadSafe instead of making ArcService take on responsibilities it
shouldn't. And even that is discouraged in favor of keeping a
ThreadChecker+calling CalledOnValidThread:
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr...

[hidehiko, 2016/01/20 16:04:50]

My understanding is that the goal of this CL is to de-dup the implementation
boilerplate. Then, using skeleton implementation (what I'm suggesting) can
reduce
the code more than a marker interface (the current code), IMHO.

For a skeleton implementation, inheriting ArcBridgeService::Observer looks
straightforward. Invoking OnSomethingReady() method if necessary in a
constructor
followed by AddObserver(this) does not sound problematic, to me. (You can call
all the necessary callback then).

With the implementation, subclasses would not have a problem like
crbug.com/575873.
(Of course, it is worth to be documented anyway).

CalledOnValidThread related stuff is optional actually, but IMHO, again assuming
we move to skeleton implementation concept, it looks clearer if we have the
function
in this class with clear documentation why and when the thread check is needed.

Bonus, if you have a ptr to ArcBridgeService, you can provide a simple getter
with clear document that the object pointing by the pointer is alive while the
instance is alive.

WDYT?

[Luis Héctor Chávez, 2016/01/20 18:40:46]

Turns out this doesn't work in C++:
https://isocpp.org/wiki/faq/strange-inheritance#calling-virtuals-from-ctors

If I were to move AddObserver/RemoveObserver to this class, only the
implementations of the OnSomethingReady() from ArcService would be called, not
the ones from the subclass, which would cause them to do nothing.

+  virtual ~ArcService() {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ArcService);
+};
+
+}  // namespace arc
+
+#endif  // COMPONENTS_ARC_ARC_SERVICE_H_