Introduce base classes for blame contexts

tracing/tracing/extras/chrome/blame_context/frame_tree_node.html

Index: tracing/tracing/extras/chrome/blame_context/frame_tree_node.html
diff --git a/tracing/tracing/extras/chrome/frame_tree_node.html b/tracing/tracing/extras/chrome/blame_context/frame_tree_node.html
similarity index 46%
rename from tracing/tracing/extras/chrome/frame_tree_node.html
rename to tracing/tracing/extras/chrome/blame_context/frame_tree_node.html
index dde8fc28d1f8bc6a061fa07169cf7f27660fe872..eee0dc13df87bab41c32c909183d6138b8bf3f93 100644
--- a/tracing/tracing/extras/chrome/frame_tree_node.html
+++ b/tracing/tracing/extras/chrome/blame_context/frame_tree_node.html
@@ -5,28 +5,34 @@ Use of this source code is governed by a BSD-style license that can be
 found in the LICENSE file.
 -->
 
-<link rel="import" href="/tracing/model/object_instance.html">
+<link rel="import" href="/tracing/extras/chrome/blame_context/blame_context.html">
 
 <script>
 'use strict';
 
+/**
+ * @fileoverview Trace Viewer side's correspondence of Chrome's
+ * content::FrameTreeNode class.
+ *
+ */
 tr.exportTo('tr.e.chrome', function() {
-  var constants = tr.e.cc.constants;
-
-  var ObjectSnapshot = tr.model.ObjectSnapshot;
-  var ObjectInstance = tr.model.ObjectInstance;
+  var BlameContextSnapshot = tr.e.chrome.BlameContextSnapshot;
+  var BlameContextInstance = tr.e.chrome.BlameContextInstance;
 
   function FrameTreeNodeSnapshot() {
-    ObjectSnapshot.apply(this, arguments);
+    BlameContextSnapshot.apply(this, arguments);
   }
 
   FrameTreeNodeSnapshot.prototype = {
-    __proto__: ObjectSnapshot.prototype,
+    __proto__: BlameContextSnapshot.prototype,
 
-    preInitialize: function() {
+    get crossProcessCounterpart() {

[Sami, 2016/06/01 16:02:11]

My preference would be to call these renderer/browserProcessCounterpart. Feel
free to leave as is if you think it makes the visualization easier.

[benjhayden, 2016/06/01 20:23:22]

These getters should specify their type: renderFrameSnapshot,
frameTreeNodeSnapshot.
"Counterpart" is not a type name.

[Xiaocheng, 2016/06/02 06:54:55]

FrameTreeNodeSnapshot and RenderFrameSnapshot should share a getter of the same
name for retrieving each other, so that we can avoid type-based conditional
statements. So I prefer not changing this part.

[benjhayden, 2016/06/02 20:07:01]

Right, I think I see what you're doing. You're saving some typing, and
expressing a close 1:1 relationship, right?
I know it's hard to imagine now while we're thinking about it explicitly, but I
promise you that this is the kind of naming that will be confusing down the
road. I still try to do clever stuff like this sometimes (it's fun!), but
explicitness is much more important in this code base than cleverness or
brevity.

The reason that statically-typed languages like Java and C++ are so obnoxiously
verbose is that explicitness (and explicit types especially) makes it easier for
large teams that frequently gain new members to understand what programs are
doing. Catapult is a large, distributed team that frequently gains new members
who don't already know how everything works, and it's much easier for new
members to see that FTNs and RFs are connected if FTN says "here's my RF" and RF
says "here's my FTN", instead of both of them saying "here's my counterpart, but
I won't tell you what that is". Even renderer-process vs browser-process isn't
enough information to tell new members that this getter returns an RF or FTN
specifically. Code is written once, and these getters will be called a few,
maybe several times (I think I see one caller in the url getter that won't need
a type conditional, and one in the side panel that will), but the callers will
be read many many times, so it's a good investment to optimize for new members
reading, and tell them the return type in the getter name.

Does that help? This is roughly how this was explained to me. I hope I don't
sound like a jerk. :-)

[Xiaocheng, 2016/06/03 05:38:38]

Yeah, you do have a point.

Done.

+      if (this.args.renderFrame instanceof tr.e.chrome.RenderFrameSnapshot)
+        return this.args.renderFrame;
     },
 
-    initialize: function() {
+    get url() {
+      return this.args.url;
     },
 
     get userFriendlyName() {
@@ -34,19 +40,27 @@ tr.exportTo('tr.e.chrome', function() {
     }
   };
 
-  ObjectSnapshot.register(
+  tr.model.ObjectSnapshot.register(
       FrameTreeNodeSnapshot,
       {typeName: 'FrameTreeNode'});
 
   function FrameTreeNodeInstance() {
-    ObjectInstance.apply(this, arguments);
+    BlameContextInstance.apply(this, arguments);
   }
 
   FrameTreeNodeInstance.prototype = {
-    __proto__: ObjectInstance.prototype
+    __proto__: BlameContextInstance.prototype,
+
+    get isTracedByRenderer() {

[benjhayden, 2016/06/01 20:23:22]

Why is this necessary?
Snapshots have a 'snapshottedOnThread' field that contains this information.

[Xiaocheng, 2016/06/02 06:54:55]

They do, but the thread/process itself doesn't tell whether it's browser or
renderer.

We don't want to have too many type-based conditions, or comparison against
hard-coded process/thread names ('CrBrowserMain', 'CrRendererMain', ...). So
this getter should be useful.

[benjhayden, 2016/06/02 20:07:01]

Sure it does: Process.name = 'Browser'/'Renderer', Thread.name similar, but the
right way to tell whether a thread/process is a Chrome browser/renderer is to
use the helpers in model/helpers/ because the distinction between
browser/renderer is complicated (webview, multi-browser traces). The helpers are
a bit buggy at the moment, but those bugs are in the helpers, not in
{RenderFrame,FrameTreeNode}{Snapshot,Instance}.
Why are type-based conditions bad?
They're a bit more typing, but they're explicit, which is tremendously valuable.

[Xiaocheng, 2016/06/03 05:38:38]

TIL model/helpers/.

Done.

+      return false;
+    },
+
+    get blameContextType() {
+      return 'Frame';
+    }
   };
 
-  ObjectInstance.register(
+  tr.model.ObjectInstance.register(
       FrameTreeNodeInstance,
       {typeName: 'FrameTreeNode'});