Add UI for RuntimeErrors in the ErrorConsole

chrome/browser/resources/extensions/extension_error.js

Index: chrome/browser/resources/extensions/extension_error.js
diff --git a/chrome/browser/resources/extensions/extension_error.js b/chrome/browser/resources/extensions/extension_error.js
index fb2699c73b5ada3ce8fd87809d3f315d56e8149b..3db991b779d01f66354c54d39510443ebcbfdcdb 100644
--- a/chrome/browser/resources/extensions/extension_error.js
+++ b/chrome/browser/resources/extensions/extension_error.js
@@ -41,13 +41,14 @@ cr.define('extensions', function() {
    * Creates a new ExtensionError HTMLElement; this is used to show a
    * notification to the user when an error is caused by an extension.
    * @param {Object} error The error the element should represent.
+   * @param {string} templateName The name of the template to clone for the
+   *     error ('extension-error-[detailed|simple]-wrapper').
    * @constructor
    * @extends {HTMLDivElement}
    */
-  function ExtensionError(error) {
-    var div = document.createElement('div');
+  function ExtensionError(error, templateName) {
+    var div = cloneTemplate(templateName);
     div.__proto__ = ExtensionError.prototype;
-    div.className = 'extension-error-simple-wrapper';
     div.error_ = error;
     div.decorate();
     return div;
@@ -84,7 +85,19 @@ cr.define('extensions', function() {
           getRelativeUrl(this.error_.source, this.extensionUrl_),
           this.error_.source));
 
-      this.appendChild(metadata);
+      if (this.className == 'extension-error-simple-wrapper')
+        this.appendChild(metadata);
+      else
+        this.querySelector('summary').appendChild(metadata);

[not at google - send to devlin, 2013/09/06 17:05:19]

You could do this in a semi-template-agnostic way (less magic would be to
declare where the metadata should go, but that seems like overkill at the
moment):

// The template may specify a <summary> to put template metadata in.
// If not, just append it to the top-level template element.
var metadataContainer = this.querySelector('summary') || this;
metadataContainer.appendChild(metadata);

[Devlin, 2013/09/06 18:18:02]

Oooh, that's much cleaner. Thanks :)

+
+      if (this.error_.contextUrl) {
+        this.querySelector('.extension-error-details').
+            appendChild(this.getContextNode_());
+      }
+      if (this.error_.stackTrace) {
+        this.querySelector('.extension-error-details').
+            appendChild(this.getStackNode_());
+      }

[not at google - send to devlin, 2013/09/06 17:05:19]

instead:

var errorDetails = this.querySelector('.extension-error-details');
if (this.error_.contextUrl)
  errorDetails.appendChild(this.getContextNode_());
if (this.error_.stackTrace)
  errorDetails.appendChild(this.getStackNode_());

[Devlin, 2013/09/06 18:18:02]

Done (s/errorDetails/detailsNode to keep consistent with most others).

     },
 
     /**
@@ -93,12 +106,13 @@ cr.define('extensions', function() {
      * @param {string} description a human-friendly description the location
      *     (e.g., filename, line).
      * @param {string} url The url of the resource to view.
+     * @param {?number} line An optional line number of the resource.
      * @return {HTMLElement} The created node, either a link or plaintext.
      * @private
      */
-    getViewSourceOrPlain_: function(description, url) {
+    getViewSourceOrPlain_: function(description, url, line) {
       if (this.canViewSource_(url))
-        var node = this.getViewSourceLink_(url);
+        var node = this.getViewSourceLink_(url, line);
       else
         var node = document.createElement('div');
       node.className = 'extension-error-view-source';
@@ -122,32 +136,100 @@ cr.define('extensions', function() {
      * @return {HTMLElement} The clickable node to view the source.
      * @private
      */
-    getViewSourceLink_: function(url) {
+    getViewSourceLink_: function(url, line) {
       var node = document.createElement('a');
       var relativeUrl = getRelativeUrl(url, this.extensionUrl_);
+      var dictionary = { 'extensionId': this.error_.extensionId,

[not at google - send to devlin, 2013/09/06 17:05:19]

s/dictionary/requestFileSourceArgs?

[Devlin, 2013/09/06 18:18:02]

Done.

+                         'message': this.error_.message,
+                         'pathSuffix': relativeUrl };
+      if (relativeUrl === 'manifest.json') {

[not at google - send to devlin, 2013/09/06 17:05:19]

just ==. No point ===ing for a comparison to a non-empty string.

That said can't you get the C++ to figure all of this out? Why does it need to
be at the JS layer?

[Devlin, 2013/09/06 18:18:02]

s/===/==.  We have to do the check on the JS side because we change the
arguments depending on if it's a manifest file or not. But I suppose the
fileType property is redundant, so removed that.

+        dictionary.fileType = 'manifest';
+        dictionary.manifestKey = this.error_.manifestKey;
+        dictionary.manifestSpecific = this.error_.manifestSpecific;
+      } else {
+        dictionary.fileType = 'source';
+        // If we specify a line in the function call, use that. Otherwise, use
+        // the line of the last stack frame, or 0 if we don't have a stack (0
+        // results in no highlighting).
+        dictionary.lineNumber =
+            line ? line : this.error_.stackTrace ?
+                this.error_.stackTrace[0].lineNumber : 0;

[not at google - send to devlin, 2013/09/06 17:05:19]

mmmm nested ternary-if confusing.

[Devlin, 2013/09/06 18:18:02]

Yeah, I was afraid of that (hence the comment)... but an if-else chain is
definitely more readable. Done.

+      }
 
       node.addEventListener('click', function(e) {
-        chrome.send('extensionErrorRequestFileSource',
-                    [{'extensionId': this.error_.extensionId,
-                      'message': this.error_.message,
-                      'fileType': 'manifest',
-                      'pathSuffix': relativeUrl,
-                      'manifestKey': this.error_.manifestKey,
-                      'manifestSpecific': this.error_.manifestSpecific}]);
-      }.bind(this));
+        chrome.send('extensionErrorRequestFileSource', [dictionary]);
+      });
+      node.title = loadTimeData.getString('extensionErrorViewSource');
+      return node;
+    },
+
+    /**
+     * Get the context node for this error. This will attempt to link to the
+     * context in which the error occurred, and can be either an extension page
+     * or an external page.
+     * @return {HTMLDivElement} The context node for the error, including the
+     *     label and a link to the context.
+     * @private
+     */
+    getContextNode_: function() {
+      var node = cloneTemplate('extension-error-context-wrapper');
+      var linkNode = node.querySelector('a');
+      if (isExtensionUrl(this.error_.contextUrl, this.extensionUrl_)) {
+        linkNode.innerText = getRelativeUrl(this.error_.contextUrl,
+                                            this.extensionUrl_);

[not at google - send to devlin, 2013/09/06 17:05:19]

I believe that innerText is deprecated in favour of textContent these days; a
fair number of cases in this file where you should change that.

Casual top-google-result searching yields:
http://kellegous.com/j/2013/02/27/innertext-vs-textcontent/

[Devlin, 2013/09/06 18:18:02]

Hadn't heard that before. Good to know. Done.

+      } else {
+        linkNode.innerText = this.error_.contextUrl;
+        linkNode.href = this.error_.contextUrl;
+        linkNode.target = '_blank';

[not at google - send to devlin, 2013/09/06 17:05:19]

I don't understand why the <a>s get hrefs here, while the isExtensionUrl case
doesn't. Could you explain?

[Devlin, 2013/09/06 18:18:02]

I didn't actually realize we could navigate directly to an extension url (e.g.,
a background page).  Apparently, we can (even if doing so isn't very exciting). 
Changed.

+      }
+      return node;
+    },
+
+    /**
+     * Get a node for the stack trace for this error. Each stack frame will
+     * include a resource url, line number, and function name (possibly
+     * anonymous). If possible, these frames will also be linked for viewing the
+     * source.
+     * @return {HTMLDetailsElement} The stack trace node for this error, with
+     *     all stack frames nested in a details-summary object.
+     * @private
+     */
+    getStackNode_: function() {
+      var node = cloneTemplate('extension-error-stack-trace');
+      var listNode = node.querySelector('.extension-error-stack-trace-list');
+      this.error_.stackTrace.forEach(function(frame) {
+        var frameNode = document.createElement('div');
+        var description = getRelativeUrl(frame.url, this.extensionUrl_) +
+                          ':' + frame.lineNumber;
+        if (frame.functionName) {
+          var functionName = frame.functionName == '(anonymous function)' ?
+              loadTimeData.getString('extensionErrorAnonymousFunction') :
+              frame.functionName;
+          description += ' (' + functionName + ')';

[not at google - send to devlin, 2013/09/06 17:05:19]

small point, but + concatenation is inefficient; better to append to an array
then use .join('') on it.

[Devlin, 2013/09/06 18:18:02]

Is it, though?
http://stackoverflow.com/questions/7299010/why-is-string-concatenation-faster...
http://jsperf.com/string-connect-vs-array-join
http://jsperf.com/join-concat/2

:)

[not at google - send to devlin, 2013/09/06 19:00:15]

Heh ok. So in Python it makes a huge, huge difference, and *that* I can back up
with data :)

+        }
+        frameNode.appendChild(this.getViewSourceOrPlain_(
+            description, frame.url, frame.lineNumber));
+        listNode.appendChild(
+            document.createElement('li')).appendChild(frameNode);
+      }, this);
+
       return node;
     },
   };
 
   /**
    * A variable length list of runtime or manifest errors for a given extension.
+   * @param {Array.<Object>} errors The list of extension errors with which
+   *     to populate the list.
+   * @param {string} type The type of errors, either 'manifest' or 'runtime'.
    * @constructor
    * @extends {HTMLDivElement}
    */
-  function ExtensionErrorList(errors) {
+  function ExtensionErrorList(errors, type) {
     var div = cloneTemplate('extension-error-list');
     div.__proto__ = ExtensionErrorList.prototype;
     div.errors_ = errors;
+    div.type_ = type;
     div.decorate();
     return div;
   }
@@ -164,10 +246,18 @@ cr.define('extensions', function() {
 
     /** @override */
     decorate: function() {
+      var title = this.type_ == 'manifest' ? 'extensionErrorsManifestErrors' :
+          'extensionErrorsRuntimeErrors';
+      this.querySelector('.extension-error-list-title').innerText =
+          loadTimeData.getString(title);
+
       this.contents_ = this.querySelector('.extension-error-list-contents');
       this.errors_.forEach(function(error) {
         this.contents_.appendChild(document.createElement('li')).appendChild(
-            new ExtensionError(error));
+            new ExtensionError(error,
+                               error.contextUrl || error.stackTrace ?
+                                   'extension-error-detailed-wrapper' :
+                                   'extension-error-simple-wrapper'));
       }, this);
 
       if (this.contents_.children.length > this.MAX_ERRORS_TO_SHOW_) {