Add UI for RuntimeErrors in the ErrorConsole

chrome/browser/resources/extensions/extension_error.js

 // Copyright 2013 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('extensions', function() {
   'use strict';
 
   /**
    * Returns whether or not a given |url| is associated with an extension.
    * @param {string} url The url to examine.
@@ skipping 23 matching lines @@
    */
   function cloneTemplate(templateName) {
     return $('template-collection-extension-error').
         querySelector('.' + templateName).cloneNode(true);
   }
 
   /**
    * 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;
   }
 
   ExtensionError.prototype = {
     __proto__: HTMLDivElement.prototype,
 
     /** @override */
     decorate: function() {
@@ skipping 16 matching lines @@
       this.extensionUrl_ =
           'chrome-extension://' + this.error_.extensionId + '/';
 
       metadata.querySelector('.extension-error-message').innerText =
           this.error_.message;
 
       metadata.appendChild(this.getViewSourceOrPlain_(
           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).

     },
 
     /**
      * Return a div with text |description|. If it's possible to view the source
      * for |url|, linkify the div to do so.
      * @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';
       node.innerText = description;
       return node;
     },
 
     /**
      * Determine whether we can view the source of a given url.
      * @param {string} url The url of the resource to view.
      * @return {boolean} Whether or not we can view the source for the url.
      * @private
      */
     canViewSource_: function(url) {
       return isExtensionUrl(url, this.extensionUrl_) || url == 'manifest.json';
     },
 
     /**
      * Create a clickable node to view the source for the given url.
      * @param {string} url The url to the resource to view.
      * @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;
   }
 
   ExtensionErrorList.prototype = {
     __proto__: HTMLDivElement.prototype,
 
     /**
      * @private
      * @const
      * @type {number}
      */
     MAX_ERRORS_TO_SHOW_: 3,
 
     /** @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_) {
         for (var i = this.MAX_ERRORS_TO_SHOW_;
              i < this.contents_.children.length; ++i) {
           this.contents_.children[i].hidden = true;
         }
         this.initShowMoreButton_();
       }
     },
@@ skipping 17 matching lines @@
         button.innerText = loadTimeData.getString(message);
         button.isShowingAll = !button.isShowingAll;
       }.bind(this));
     }
   };
 
   return {
     ExtensionErrorList: ExtensionErrorList
   };
 });