Adds a monotonically increasing id to the frame tree

This is needed so that clients can identify when they have applied a
particular change. Also, I'm making it so the originator of a
structure change gets the notification on the client interface of the
change.

Here's an example of what this fixes:
An HTMLViewer with with two local roots, A and B.
B1 is added as a child of B and then removed.
OnFrameAdded() is seen on A and HTMLViewer attempts to add B1
back. But B1 was already removed, so that it shouldn't added it back.

The monotonically increasing id allows html viewer to identify this by
the following. When a frame is removed HTMLFrameTreeManager now adds
the id to a set. The id is removed when OnFrameRemoved is seen (this
necessitates calling OnFrameRemoved on the originator). In
OnFrameAdded if the id is in the removed set the add is ignored.

BUG=479172,490221
TEST=none
R=fsamuel@chromium.org

Review URL: https://codereview.chromium.org/1280393002

Cr-Commit-Position: refs/heads/master@{#342735}

6 files changed, 64 insertions, 36 deletions

diff --git a/mandoline/tab/frame.cc b/mandoline/tab/frame.cc
index 70e5880..b120c93 100644
--- a/mandoline/tab/frame.cc
+++ b/mandoline/tab/frame.cc
@@ -6,6 +6,7 @@
 
 #include <algorithm>
 
+#include "base/auto_reset.h"
 #include "base/stl_util.h"
 #include "components/view_manager/public/cpp/view.h"
 #include "components/view_manager/public/cpp/view_property.h"
@@ -75,8 +76,14 @@ Frame::~Frame() {
 }
 
 void Frame::Init(Frame* parent) {
-  if (parent)
-    parent->Add(this);
+  {
+    // Set the FrameTreeClient to null so that we don't notify the client of the
+    // add before OnConnect().
+    base::AutoReset<FrameTreeClient*> frame_tree_client_resetter(
+        &frame_tree_client_, nullptr);
+    if (parent)
+      parent->Add(this);
+  }
 
   InitClient();
 }
@@ -150,8 +157,10 @@ void Frame::InitClient() {
   // TODO(sky): error handling.
   FrameTreeServerPtr frame_tree_server_ptr;
   frame_tree_server_binding_.Bind(GetProxy(&frame_tree_server_ptr).Pass());
-  if (frame_tree_client_)
-    frame_tree_client_->OnConnect(frame_tree_server_ptr.Pass(), array.Pass());
+  if (frame_tree_client_) {
+    frame_tree_client_->OnConnect(frame_tree_server_ptr.Pass(),
+                                  tree_->change_id(), array.Pass());
+  }
 }
 
 void Frame::SetView(mojo::View* view) {
@@ -174,7 +183,7 @@ void Frame::Add(Frame* node) {
   node->parent_ = this;
   children_.push_back(node);
 
-  tree_->root()->NotifyAdded(this, node);
+  tree_->root()->NotifyAdded(this, node, tree_->AdvanceChangeID());
 }
 
 void Frame::Remove(Frame* node) {
@@ -184,7 +193,7 @@ void Frame::Remove(Frame* node) {
   node->parent_ = nullptr;
   children_.erase(iter);
 
-  tree_->root()->NotifyRemoved(this, node);
+  tree_->root()->NotifyRemoved(this, node, tree_->AdvanceChangeID());
 }
 
 void Frame::LoadingStartedImpl() {
@@ -241,26 +250,24 @@ Frame* Frame::FindTargetFrame(uint32_t frame_id) {
   return frame;
 }
 
-void Frame::NotifyAdded(const Frame* source, const Frame* added_node) {
-  if (added_node == this)
-    return;
-
-  if (source != this && frame_tree_client_)
-    frame_tree_client_->OnFrameAdded(FrameToFrameData(added_node));
+void Frame::NotifyAdded(const Frame* source,
+                        const Frame* added_node,
+                        uint32_t change_id) {
+  if (frame_tree_client_)
+    frame_tree_client_->OnFrameAdded(change_id, FrameToFrameData(added_node));
 
   for (Frame* child : children_)
-    child->NotifyAdded(source, added_node);
+    child->NotifyAdded(source, added_node, change_id);
 }
 
-void Frame::NotifyRemoved(const Frame* source, const Frame* removed_node) {
-  if (removed_node == this)
-    return;
-
-  if (source != this && frame_tree_client_)
-    frame_tree_client_->OnFrameRemoved(removed_node->id());
+void Frame::NotifyRemoved(const Frame* source,
+                          const Frame* removed_node,
+                          uint32_t change_id) {
+  if (frame_tree_client_)
+    frame_tree_client_->OnFrameRemoved(change_id, removed_node->id());
 
   for (Frame* child : children_)
-    child->NotifyRemoved(source, removed_node);
+    child->NotifyRemoved(source, removed_node, change_id);
 }
 
 void Frame::NotifyClientPropertyChanged(const Frame* source,
diff --git a/mandoline/tab/frame.h b/mandoline/tab/frame.h
index 7d288b5..8d051fe 100644
--- a/mandoline/tab/frame.h
+++ b/mandoline/tab/frame.h
@@ -123,8 +123,12 @@ class Frame : public mojo::ViewObserver, public FrameTreeServer {
   Frame* FindTargetFrame(uint32_t frame_id);
 
   // Notifies the client and all descendants as appropriate.
-  void NotifyAdded(const Frame* source, const Frame* added_node);
-  void NotifyRemoved(const Frame* source, const Frame* removed_node);
+  void NotifyAdded(const Frame* source,
+                   const Frame* added_node,
+                   uint32_t change_id);
+  void NotifyRemoved(const Frame* source,
+                     const Frame* removed_node,
+                     uint32_t change_id);
   void NotifyClientPropertyChanged(const Frame* source,
                                    const mojo::String& name,
                                    const mojo::Array<uint8_t>& value);
diff --git a/mandoline/tab/frame_apptest.cc b/mandoline/tab/frame_apptest.cc
index 3459e64..32685a7 100644
--- a/mandoline/tab/frame_apptest.cc
+++ b/mandoline/tab/frame_apptest.cc
@@ -140,15 +140,16 @@ class TestFrameTreeClient : public FrameTreeClient {
 
   // TestFrameTreeClient:
   void OnConnect(FrameTreeServerPtr server,
+                 uint32_t change_id,
                  mojo::Array<FrameDataPtr> frames) override {
     connect_count_++;
     connect_frames_ = frames.Pass();
     server_ = server.Pass();
   }
-  void OnFrameAdded(FrameDataPtr frame) override {
+  void OnFrameAdded(uint32_t change_id, FrameDataPtr frame) override {
     adds_.push_back(frame.Pass());
   }
-  void OnFrameRemoved(uint32_t frame_id) override {}
+  void OnFrameRemoved(uint32_t change_id, uint32_t frame_id) override {}
   void OnFrameClientPropertyChanged(uint32_t frame_id,
                                     const mojo::String& name,
                                     mojo::Array<uint8_t> new_data) override {}
@@ -201,8 +202,8 @@ TEST_F(FrameTest, SingleChild) {
   EXPECT_EQ(child_frame->view()->id(), frames_in_child[1]->frame_id);
   EXPECT_EQ(tree.root()->view()->id(), frames_in_child[1]->parent_id);
 
-  // The root did the add, so it shouldn't get an add.
-  EXPECT_EQ(0u, root_client.adds().size());
+  // We should have gotten notification of the add.
+  EXPECT_EQ(1u, root_client.adds().size());
 }
 
 }  // namespace mandoline
diff --git a/mandoline/tab/frame_tree.cc b/mandoline/tab/frame_tree.cc
index a6da90c..aca4edf 100644
--- a/mandoline/tab/frame_tree.cc
+++ b/mandoline/tab/frame_tree.cc
@@ -22,7 +22,8 @@ FrameTree::FrameTree(mojo::View* view,
             root_client,
             user_data.Pass(),
             Frame::ClientPropertyMap()),
-      progress_(0.f) {
+      progress_(0.f),
+      change_id_(1u) {
   root_.Init(nullptr);
 }
 
@@ -66,6 +67,10 @@ void FrameTree::CreateSharedFrame(
                         client_properties);
 }
 
+uint32_t FrameTree::AdvanceChangeID() {
+  return ++change_id_;
+}
+
 Frame* FrameTree::CreateAndAddFrameImpl(
     mojo::View* view,
     uint32_t frame_id,
diff --git a/mandoline/tab/frame_tree.h b/mandoline/tab/frame_tree.h
index b4bd590..4422a2b 100644
--- a/mandoline/tab/frame_tree.h
+++ b/mandoline/tab/frame_tree.h
@@ -35,6 +35,8 @@ class FrameTree {
 
   Frame* root() { return &root_; }
 
+  uint32_t change_id() const { return change_id_; }
+
   Frame* CreateAndAddFrame(mojo::View* view,
                            Frame* parent,
                            FrameTreeClient* client,
@@ -60,6 +62,9 @@ class FrameTree {
  private:
   friend class Frame;
 
+  // Increments the change id, returning the new value.
+  uint32_t AdvanceChangeID();
+
   Frame* CreateAndAddFrameImpl(
       mojo::View* view,
       uint32_t frame_id,
@@ -82,6 +87,8 @@ class FrameTree {
 
   double progress_;
 
+  uint32_t change_id_;
+
   DISALLOW_COPY_AND_ASSIGN(FrameTree);
 };
 
diff --git a/mandoline/tab/public/interfaces/frame_tree.mojom b/mandoline/tab/public/interfaces/frame_tree.mojom
index d25c6ee..f49612c 100644
--- a/mandoline/tab/public/interfaces/frame_tree.mojom
+++ b/mandoline/tab/public/interfaces/frame_tree.mojom
@@ -17,8 +17,12 @@ import "network/public/interfaces/url_loader.mojom";
 // frame_ids are the same as views ids. This means that when a client creates
 // a new view to be part of the frame tree it immediately knows the id to use
 // for FrameTreeServer calls.
-// TODO(sky): there are likely timing issues here, figure out how to resolve
-// that.
+//
+// The server provides an id that may be used to identify the state of the
+// tree. The change id is an integer that is incremented every time the
+// structure of the tree changes. The change id is not used by the server; the
+// server only updates the change id and notifies clients of the new id (by
+// way of structure change functions such as OnFrameAdded()).
 
 // Expresses a preference for where a navigation should occur.
 enum NavigationTargetType {
@@ -90,15 +94,15 @@ interface FrameTreeServer {
 
 interface FrameTreeClient {
   // Called once per client. |frame_data| gives the contents of the tree.
-  OnConnect(FrameTreeServer server, array<FrameData> frame_data);
+  OnConnect(FrameTreeServer server,
+            uint32 change_id,
+            array<FrameData> frame_data);
 
-  // Called when a new frame is added to the tree. This is not called on the
-  // originator of the change.
-  OnFrameAdded(FrameData frame_data);
+  // Called when a new frame is added to the tree.
+  OnFrameAdded(uint32 change_id, FrameData frame_data);
 
-  // Called when a frame is removed from the tree. This is not called on the
-  // originator of the change.
-  OnFrameRemoved(uint32 frame_id);
+  // Called when a frame is removed from the tree.
+  OnFrameRemoved(uint32 change_id, uint32 frame_id);
 
   // Called when a client property changes.
   OnFrameClientPropertyChanged(uint32 frame_id,