First step at synchronizing client model changes with service.

mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc

+// Copyright 2014 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.
+
+#include "mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.h"
+
+#include "base/bind.h"
+#include "base/message_loop/message_loop.h"
+#include "mojo/public/cpp/bindings/allocation_scope.h"
+#include "mojo/public/cpp/shell/application.h"
+#include "mojo/public/interfaces/shell/shell.mojom.h"
+#include "mojo/services/public/cpp/view_manager/lib/view_manager_private.h"
+
+namespace mojo {
+namespace services {
+namespace view_manager {
+
+class ViewManagerTransaction {
+ public:
+  virtual ~ViewManagerTransaction() {}
+
+  virtual bool Equals(ViewManagerTransaction* other) {

[sky, 2014/04/28 23:04:19]

const ViewManagerTransaction* or const ViewManagerTransaction&

+    return other->type_ == type_;
+  }
+
+  virtual void Commit() = 0;
+
+  bool committed() const { return committed_; }
+  int change_id() const { return change_id_; }

[sky, 2014/04/28 23:04:19]

uint32_t

+
+ protected:
+  enum TransactionType {
+    TYPE_CREATE_VIEW_TREE_NODE,
+    TYPE_HIERARCHY

[sky, 2014/04/28 23:04:19]

Add description for TYPE_HIERARCH as it isn't obvious what is is.

+  };
+
+  ViewManagerTransaction(TransactionType type,
+                         ViewManagerSynchronizer* synchronizer)
+      : type_(type),
+        change_id_(synchronizer->GetNextChangeId()),
+        committed_(false),
+        synchronizer_(synchronizer) {
+  }
+
+  IViewManager* service() { return synchronizer_->service_.get(); }
+
+  uint32_t MakeTransportId(uint16_t id) {
+    return (synchronizer_->connection_id_ << 16) | id;
+  }
+
+  int type_;

[sky, 2014/04/28 23:04:19]

TransactionType?

[sky, 2014/04/28 23:04:19]

Style guide says no protected members.

+  int change_id_;

[sky, 2014/04/28 23:04:19]

Can this be const? And uint32_t.

+  bool committed_;
+  ViewManagerSynchronizer* synchronizer_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ViewManagerTransaction);
+};
+
+class CreateViewTreeNodeTransaction
+    : public ViewManagerTransaction {
+ public:
+  CreateViewTreeNodeTransaction(int node_id,
+                                ViewManagerSynchronizer* synchronizer)
+      : ViewManagerTransaction(TYPE_CREATE_VIEW_TREE_NODE, synchronizer),
+        node_id_(node_id) {}
+  virtual ~CreateViewTreeNodeTransaction() {}
+
+ private:
+  // Overridden from ViewManagerTransaction:
+  virtual bool Equals(ViewManagerTransaction* other) OVERRIDE {
+    return ViewManagerTransaction::Equals(other) &&
+        static_cast<CreateViewTreeNodeTransaction*>(other)->node_id_ ==
+            node_id_;
+  }
+  virtual void Commit() OVERRIDE {
+    DCHECK(!committed());

[sky, 2014/04/28 23:04:19]

Maybe the DCHECK and boolean should be part of the base class and have a pure
virtual DoCommit() or something function.

+    committed_ = true;
+    service()->CreateNode(
+        node_id_,
+        base::Bind(&CreateViewTreeNodeTransaction::OnActionCompleted,
+                   base::Unretained(this)));
+  }
+
+  void OnActionCompleted(bool success) {
+    DCHECK(success);
+    // TODO(beng): Failure means we tried to create with an extant id for this
+    //             connection. Figure out what to do.
+  }
+
+  int node_id_;

[sky, 2014/04/28 23:04:19]

uint16_t I believe.

+
+  DISALLOW_COPY_AND_ASSIGN(CreateViewTreeNodeTransaction);
+};
+
+class HierarchyTransaction : public ViewManagerTransaction {
+ public:
+  enum HierarchyChangeType {
+    TYPE_ADD,
+    TYPE_REMOVE
+  };
+  HierarchyTransaction(HierarchyChangeType hierarchy_change_type,
+                       int child_id,
+                       int parent_id,
+                       ViewManagerSynchronizer* synchronizer)
+      : ViewManagerTransaction(TYPE_HIERARCHY, synchronizer),
+        hierarchy_change_type_(hierarchy_change_type),
+        child_id_(child_id),
+        parent_id_(parent_id) {}
+  virtual ~HierarchyTransaction() {}
+
+ private:
+  // Overridden from ViewManagerTransaction:
+  virtual bool Equals(ViewManagerTransaction* other) OVERRIDE {
+    if (!ViewManagerTransaction::Equals(other))
+      return false;
+
+    HierarchyTransaction* hier_txn = static_cast<HierarchyTransaction*>(other);
+    return hier_txn->hierarchy_change_type_ == hierarchy_change_type_ &&
+        hier_txn->child_id_ == child_id_ &&
+        hier_txn->parent_id_ == parent_id_;
+  }
+  virtual void Commit() OVERRIDE {
+    DCHECK(!committed());
+    committed_ = true;
+    switch (hierarchy_change_type_) {
+    case TYPE_ADD:

[sky, 2014/04/28 23:04:19]

nit: indentation is off for the switch statement.

+    service()->AddNode(MakeTransportId(parent_id_),
+                       MakeTransportId(child_id_),
+                       change_id_,
+                       base::Bind(&HierarchyTransaction::OnActionCompleted,
+                                  base::Unretained(this)));
+      break;
+    case TYPE_REMOVE:
+    service()->RemoveNodeFromParent(
+        MakeTransportId(child_id_),
+        change_id_,
+        base::Bind(&HierarchyTransaction::OnActionCompleted,
+                   base::Unretained(this)));
+      break;
+    default:

[sky, 2014/04/28 23:04:19]

You should be able to remove this so that you get a compile error if a new type
is added without updating this switch statement.

+      NOTREACHED();
+    }
+  }
+
+  void OnActionCompleted(bool success) {
+    DCHECK(success);
+    // TODO(beng): Failure means either one of the nodes specified didn't exist,
+    //             or we passed the same node id for both params. Roll back?
+  }
+
+  HierarchyChangeType hierarchy_change_type_;

[sky, 2014/04/28 23:04:19]

const

+  int child_id_;

[sky, 2014/04/28 23:04:19]

uint16_t for both of these.

+  int parent_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(HierarchyTransaction);
+};
+
+ViewManagerSynchronizer::ViewManagerSynchronizer(ViewManager* view_manager)
+    : view_manager_(view_manager),
+      connected_(false),
+      connection_id_(0),
+      next_change_id_(0),

[sky, 2014/04/28 23:04:19]

order doesn't match header.

+      next_id_(0) {
+  InterfacePipe<services::view_manager::IViewManager, AnyInterface>
+      view_manager_pipe;
+  AllocationScope scope;
+  ViewManagerPrivate(view_manager_).application()->shell()->Connect(
+      "mojo:mojo_view_manager", view_manager_pipe.handle_to_peer.Pass());
+  service_.reset(view_manager_pipe.handle_to_self.Pass(), this);
+}
+
+ViewManagerSynchronizer::~ViewManagerSynchronizer() {
+}
+
+int ViewManagerSynchronizer::CreateViewTreeNode() {
+  int id = next_id_++;
+  pending_transactions_.push_back(new CreateViewTreeNodeTransaction(id, this));
+  ScheduleSync();
+  return id;
+}
+
+void ViewManagerSynchronizer::AddChild(int child_id, int parent_id) {
+  pending_transactions_.push_back(
+      new HierarchyTransaction(HierarchyTransaction::TYPE_ADD,
+                               child_id,
+                               parent_id,
+                               this));
+  ScheduleSync();
+}
+
+void ViewManagerSynchronizer::RemoveChild(int child_id, int parent_id) {
+  pending_transactions_.push_back(
+      new HierarchyTransaction(HierarchyTransaction::TYPE_REMOVE,
+                               child_id,
+                               parent_id,
+                               this));
+  ScheduleSync();
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// ViewManagerSynchronizer, IViewManagerClient implementation:
+
+void ViewManagerSynchronizer::OnConnectionEstablished(uint16 connection_id) {
+  connected_ = true;
+  connection_id_ = connection_id;
+  ScheduleSync();
+}
+
+void ViewManagerSynchronizer::OnNodeHierarchyChanged(uint32 node,
+                                                     uint32 new_parent,
+                                                     uint32 old_parent,
+                                                     int32 change_id) {
+  // The service will still notify us about changes made from this connection,
+  // but we use our transaction queue as a record of what those changes were.
+  // We use this ack as an opportunity to remove these entries from our queue.
+  // Non-zero change_id is the service's way of telling us the change originated
+  // from this connection.
+  if (RemovedFromPendingQueue(change_id))

[sky, 2014/04/28 23:04:19]

One problematic area is if a transaction generates multiple notifications. This
is currently possible for OnNodeViewReplaced. There needs to be a better way for
you to identify this on your side. I'll add that to my list.

[Ben Goodger (Google), 2014/04/29 00:00:54]

Hrm and probably for nested hiers.

+    return;
+
+  if (change_id == 0) {
+    // TODO(beng): Apply changes from another client.
+  }
+
+  // TODO(beng): possible that changes from this client are never ack'ed? e.g.
+  //             node creates are treated as txns & there is no ack.
+}
+
+void ViewManagerSynchronizer::OnNodeViewReplaced(uint32_t node,
+                                                 uint32_t new_view_id,
+                                                 uint32_t old_view_id,
+                                                 int32_t change_id) {
+  // ..
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// ViewManagerSynchronizer, private:
+
+void ViewManagerSynchronizer::ScheduleSync() {
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&ViewManagerSynchronizer::DoSync, base::Unretained(this)));
+}
+
+void ViewManagerSynchronizer::DoSync() {
+  // The service connection may not be set up yet. OnConnectionEstablished()
+  // will schedule another sync when it is.
+  if (!connected_)
+    return;
+
+  Transactions::const_iterator it = pending_transactions_.begin();
+  for (; it != pending_transactions_.end(); ++it) {
+    if (!(*it)->committed())
+      (*it)->Commit();
+  }
+}
+
+int ViewManagerSynchronizer::GetNextChangeId() {
+  // TODO(beng): update once change id becomes uint.
+  // TODO(beng): deal with change id collisions? Important in the "never ack'ed
+  //             change" case mentioned in OnNodeHierarchyChanged().
+  // "0" is a special value passed to other connected clients, so we can't use
+  // it.
+  next_change_id_ = std::max(1, ++next_change_id_);
+  return next_change_id_;
+}
+
+bool ViewManagerSynchronizer::RemovedFromPendingQueue(int change_id) {
+  if (change_id != 0) {
+    Transactions::iterator it = pending_transactions_.begin();

[sky, 2014/04/28 23:04:19]

I think you should only have to check the first transaction. To do otherwise
implies you could get notifications out of order, which isn't the case.

[Ben Goodger (Google), 2014/04/29 00:00:54]

Not quite. I currently have transactions for node creations. I have no ack for
those. That's a separate issue I need to talk to you about. But I need to
advance past them in the queue.

+    for (; it != pending_transactions_.end(); ++it) {
+      if ((*it)->change_id() == change_id) {
+        pending_transactions_.erase(it);
+        return true;
+      }
+    }
+  }
+  return false;
+}
+
+}  // namespace view_manager
+}  // namespace services
+}  // namespace mojo