Switch the about:gpu implementation from an about handler to dom_ui....

chrome/browser/resources/gpu_internals/info_view.js

Index: chrome/browser/resources/gpu_internals/info_view.js
===================================================================
--- chrome/browser/resources/gpu_internals/info_view.js	(revision 0)
+++ chrome/browser/resources/gpu_internals/info_view.js	(revision 0)
@@ -0,0 +1,124 @@
+// 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.
+
+/**
+ * This view displays options for importing/exporting the captured data. Its
+ * primarily usefulness is to allow users to copy-paste their data in an easy
+ * to read format for bug reports.
+ *
+ *   - Has a button to generate a text report.
+ *
+ *   - Shows how many events have been captured.
+ *  @constructor

[arv (Not doing code reviews), 2010/12/06 19:45:57]

remove @constructor here

[nduca, 2010/12/07 01:03:40]

Done.

+ */
+cr.define('gpu', function() {
+  /**
+   * Provides information on the GPU process and underlying graphics hardware.
+   * @constructor
+   */
+  function InfoView(mainBoxId) {
+    DivView.call(this, mainBoxId);
+
+    this.textDiv_ = document.getElementById('info-text');
+    this.gpuInfo_ = undefined;
+    this.clientInfo_ = undefined;
+
+    var self = this;
+
+    // This function begins a request for the ClientInfo. If it comes back
+    // as undefined, then we will issue the request again in 250ms.
+    function beginRequestClientInfo() {

[arv (Not doing code reviews), 2010/12/06 19:45:57]

Move out of constructor and call with self as argument?

[nduca, 2010/12/07 01:03:40]

Done.

+      browser.callAsync('requestClientInfo', undefined, function(data) {
+        self.clientInfo_ = data;
+        self.refresh();
+        if(data === undefined) { // try again in 250 ms
+          window.setTimeout(beginRequestClientInfo, 250);
+        }
+      });
+    }
+    beginRequestClientInfo();
+
+    // This function begins a request for the GpuInfo. If it comes back
+    // as undefined, then we will issue the request again in 250ms.
+    function beginRequestGpuInfo() {

[arv (Not doing code reviews), 2010/12/06 19:45:57]

same here

[nduca, 2010/12/07 01:03:40]

Done.

+      browser.callAsync('requestGpuInfo', undefined, function(data) {
+        self.gpuInfo_ = data;
+        self.refresh();
+        if(data === undefined) { // try again in 250 ms
+          window.setTimeout(beginRequestGpuInfo, 250);
+        }
+      });
+    }
+    beginRequestGpuInfo();
+    this.refresh();
+  }
+
+  InfoView.prototype = {
+    __proto__: DivView,
+
+    /**
+    * Updates the view based on its currently known data
+    */
+    refresh: function(data) {
+      var html = [];

[arv (Not doing code reviews), 2010/12/06 19:45:57]

This thing might have been a lot cleaner using js templates. See
chrome://extensions for example

[nduca, 2010/12/07 01:03:40]

I'll go take a peek at this and see if I can switch to that approach instead.
Sending out this round of replies first to get your feedback.

On 2010/12/06 19:45:57, arv wrote:

+      function outputObject(obj) {

[arv (Not doing code reviews), 2010/12/06 19:45:57]

Move out of refresh. I don't think there is any point in having to recreate this
function on every call to refresh

[nduca, 2010/12/07 01:03:40]

Done.

[nduca, 2010/12/07 01:03:40]

Done.

+        var all_values = true;

[arv (Not doing code reviews), 2010/12/06 19:45:57]

no underscores

[nduca, 2010/12/07 01:03:40]

Done.

+        for (var k in obj) { // output values
+          var v = obj[k];
+          if(!(typeof(v) === 'number' || typeof(v) === "string")) {

[arv (Not doing code reviews), 2010/12/06 19:45:57]

whitespace after if

[arv (Not doing code reviews), 2010/12/06 19:45:57]

typeof is not a function, it is an unary operator and should not have
parentheses

[arv (Not doing code reviews), 2010/12/06 19:45:57]

Use single quotes

[nduca, 2010/12/07 01:03:40]

Done.

+            all_values = false;
+          }
+        }
+        if (all_values)
+          html.push("<table class='border'>");
+        else
+          html.push("<table>");
+        for (var k in obj) { // output values

[arv (Not doing code reviews), 2010/12/06 19:45:57]

2 spaces before //

+          var v = obj[k];
+          if (typeof(v) === 'number' || typeof(v) === "string") {
+            html.push("<tr><td><strong>");

[arv (Not doing code reviews), 2010/12/06 19:45:57]

Remove strong and use a class name + css

[arv (Not doing code reviews), 2010/12/06 19:45:57]

push takes var args.

html.push('<tr><td>', k, ...)

[nduca, 2010/12/07 01:03:40]

Going to skip the css-ization until I can look at jstemplate.
On 2010/12/06 19:45:57, arv wrote:
Done.

+            html.push(k);
+            html.push("</strong></td><td>");
+            html.push(v);
+            html.push("</td></tr>");
+          } else {
+            html.push("<tr><td colspan=2><strong>" + k + ":</strong>");
+            html.push("<div style=\"padding-left: 8px;\">");

[arv (Not doing code reviews), 2010/12/06 19:45:57]

-webkit-padding-start and move to css

[nduca, 2010/12/07 01:03:40]

Done.

+            outputObject(v);
+            html.push("</div>");
+            html.push("</td></tr>");
+          }
+        }
+        html.push("</table>");
+      }
+
+      html.push('<h2>Chrome information</h2>');
+      if (this.clientInfo_) {
+        var chromeVersion = this.clientInfo_.version +
+          ' (' + this.clientInfo_.official +
+          ' ' + this.clientInfo_.cl +
+          ') ' + this.clientInfo_.version_mod + "</p>";
+        var data = {'Data exported on' : (new Date()).toLocaleString(),

[arv (Not doing code reviews), 2010/12/06 19:45:57]

no ws before :

[nduca, 2010/12/07 01:03:40]

Done.

+                    'Chrome version: ' : chromeVersion};
+        outputObject(data);
+      } else {
+        html.push('... refreshing data ...');
+      }
+
+      html.push('<h2>GPU information</h2>');
+      if (this.gpuInfo_) {
+        outputObject(this.gpuInfo_);
+      } else {
+        html.push('... refreshing data ...');
+      }
+
+      // Open a new window to display this text.
+      this.textDiv_.innerHTML = html.join("\n");

[arv (Not doing code reviews), 2010/12/06 19:45:57]

XSS? You are not escaping all your text and some of that text comes from the
outside. It might not be an issue because the data is gotten from chrome but
using innerHTML in DOMUI is discouraged.

[nduca, 2010/12/07 01:03:40]

Hopefully I can get out of this trap with jst.

On 2010/12/06 19:45:57, arv wrote:

+    }
+  };
+
+  return {
+    InfoView: InfoView
+  };
+});
Property changes on: chrome/browser/resources/gpu_internals/info_view.js
___________________________________________________________________
Added: svn:eol-style
   + LF