Trace_event upgrades

chrome/browser/resources/gpu_internals/tracing_controller.js

+// Copyright (c) 2010 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.
+cr.define('gpu', function() {
+  function TracingController() {
+    this.overlay_ = $('gpu-tracing-overlay');

[arv (Not doing code reviews), 2011/02/23 17:51:25]

I know we do this a lot in the options code but using a static (the DOM element)
in an instance always seems dirty to me. How about passing in an overlay
instance instead?

[nduca, 2011/02/23 23:34:13]

I agree, it smells bad. I think there are two 'wrong' cases that are worth
distinguishing:
1. Where we are trying to decorate an existing DOM node that was part of the
original page
2. Where the component has some private dom nodes that are used internally to
it.

I'm guilty of both of these, to be sure. I think the TracingController is case
2...

I notice in things like Closure, dom structures private to a component are just
created by building the DOM in script, rather than by having some inlined HTML
in the original document.

So what about one of the following:
1. don't inline, generate the overlay's div on the fly [eew]
2. Still inline the private nodes as a template and deepCopy them into a Overlay
instance during TracingController's init.



On 2011/02/23 17:51:25, arv wrote:

+    cr.ui.decorate(this.overlay_, gpu.Overlay);
+
+    this.tracingEnabled_ = false;

[arv (Not doing code reviews), 2011/02/23 17:51:25]

I would move this to the prototype.

[nduca, 2011/02/23 23:34:13]

Done.

+    this.traceEvents_ = [];
+  }
+  TracingController.prototype = {
+    __proto__: cr.EventTarget.prototype,
+
+    initUI : function(mainTabs) {

[arv (Not doing code reviews), 2011/02/23 17:51:25]

no ws before :

[nduca, 2011/02/23 23:34:13]

Done.

+      var startButton = $('gpu-tracing-start-button');
+      var stopButton = $('gpu-tracing-stop-button');
+      mainTabs.addCustomWidget(startButton);
+
+      startButton.onclick = function() {

[arv (Not doing code reviews), 2011/02/23 17:51:25]

startButton.onclick = this.beginTracing.bind(this);

[nduca, 2011/02/23 23:34:13]

Done.

+        this.beginTracing();
+      }.bind(this);
+      stopButton.onclick = function() {
+        this.endTracing();
+      }.bind(this);
+      this.onclick = function() {
+        this.endTracing();
+      }.bind(this);
+    },
+
+    /**
+     * Called by info_view to empty the trace buffer
+     */
+    beginTracing : function() {
+      if(this.tracingEnabled_) throw new Error("Tracing already begun.");

[arv (Not doing code reviews), 2011/02/23 17:51:25]

line break

[arv (Not doing code reviews), 2011/02/23 17:51:25]

new is not needed here

[nduca, 2011/02/23 23:34:13]

Done.

+
+      this.overlay_.show();
+
+      this.tracingEnabled_ = true;
+      console.log("Beginning to trace...");

[arv (Not doing code reviews), 2011/02/23 17:51:25]

Use single quotes

+
+      this.traceEvents_ = [];
+      if(!browserBridge.debugMode)
+        chrome.send('beginTracing');
+
+      this.tracingEnabled_ = true;
+
+      var e = new cr.Event('gpuTraceBegun');
+      e.events = this.traceEvents_;
+      this.dispatchEvent(e);
+
+      e = new cr.Event('gpuTraceEventsChanged');
+      e.numEvents = this.traceEvents_.length;
+      this.dispatchEvent(e);
+
+      this.currentlyBoundKeypressListener_ = this.onOverlayKeyPress_.bind(this);

[arv (Not doing code reviews), 2011/02/23 17:51:25]

I'm not sure this is cleaner but this can also be done as:

var self = this;
window.addEventListener('keypress', function f(e) {
  window.removeEventListener('keypress', f);
  self.onOverlayKeyPress_(e);
});

[nduca, 2011/02/23 23:34:13]

Thats pretty neat. I dislike that less than I would have first thought. Closure
seems to put a wrapper around major EventTargets such that a fourth parameter
for the addEventListener is the binding context for the callback,e.g:
 addEventListener('foo', myMethod, false, this);

I can't think of a clean way to make this work in cr.js land, though. If
anything jumps out to you, I'm happy to code it up.

On 2011/02/23 17:51:25, arv wrote:

+      window.addEventListener('keypress', this.currentlyBoundKeypressListener_);
+    },
+
+    onOverlayKeyPress_ : function(e) {
+      console.log(e);
+      if(e.charCode == 13) { // enter

[arv (Not doing code reviews), 2011/02/23 17:51:25]

if (e.keyIdentifier == 'Enter')

[arv (Not doing code reviews), 2011/02/23 17:51:25]

Whitespace after if

[nduca, 2011/02/23 23:34:13]

Done.

+        this.endTracing();
+      }
+    },
+
+    /**
+     * Checks whether gpu tracing is enabled
+     */
+    isGpuTracingEnabled : function() {

[arv (Not doing code reviews), 2011/02/23 17:51:25]

Maybe use a getter?

[nduca, 2011/02/23 23:34:13]

Done.

+      return this.tracingEnabled_;
+    },
+
+    /**
+     * Gets the currently traced events. If tracing is active, then
+     * this can change on the fly.
+     */
+    getGpuTraceEvents : function() {

[arv (Not doing code reviews), 2011/02/23 17:51:25]

getter?

[nduca, 2011/02/23 23:34:13]

Done.

+      return this.traceEvents_;
+    },
+
+    /**
+     * Callbed by gpu c++ code when new GPU trace data arrives.
+     */
+    onGpuTraceDataCollected: function(events) {
+      this.traceEvents_.push.apply(this.traceEvents_, events);
+    },
+
+    /**
+     * Called by info_view to finish tracing and update all views.
+     */
+    endTracing : function() {
+      if(!this.tracingEnabled_) throw new Error("Tracing not begun.");
+
+      window.removeEventListener('keypress',
+                                 this.currentlyBoundKeypressListener_);
+
+      console.log("Finishing trace");
+      if(!browserBridge.debugMode)

[arv (Not doing code reviews), 2011/02/23 17:51:25]

If else has curly braces then if should too

[nduca, 2011/02/23 23:34:13]

Done.

+        chrome.send('beginToEndTracing');
+      else {
+        var events = getTimelineTestData1();
+        this.onGpuTraceDataCollected(events);
+        window.setTimeout(this.onEndTracingComplete.bind(this), 250);
+      }
+    },
+    /* Called by the browser when all processes ack tracing having completed. */
+    onEndTracingComplete : function() {
+      this.overlay_.hide();
+      this.tracingEnabled_ = false;
+      console.log("onEndTracingComplete p1 with " +
+                  this.traceEvents_.length + " events.");
+      var e = new cr.Event('gpuTraceEnded');
+      e.events = this.traceEvents_;
+      this.dispatchEvent(e);
+    },
+
+    selfTest : function() {
+      this.beginTracing();
+      window.setTimeout(function() {
+                          this.endTracing();

[arv (Not doing code reviews), 2011/02/23 17:51:25]

this.endTracing.bind(this)

[nduca, 2011/02/23 23:34:13]

Done.

+                        }.bind(this),500);
+    }
+  };
+  return {
+    TracingController : TracingController
+  };
+});
+