[Extensions] Update Error Console UI

[Extensions] Update Error Console UI

The current Error Console UI is a little lacking. Most egregiously, there's no
way to clear errors. Update the UI by moving more of the error console into its
dedicated overlay, which
- Minimizes its footprint on the main page.
- Makes it easy to switch between errors easily.
Additionally, add support for removing specific (or all) errors associated with
extensions.

Screenshots are up at
https://drive.google.com/open?id=0BxjK6_MzZ8UPfndZcElPVGxxdWxXQmN3WkVtSFhNUkN3SlFVYVQtaGlpWTJaOU52YTRfOGs&authuser=0
(Internal only for now; will post externally soon)

BUG=482526
Committed: https://crrev.com/20bf10bc2f45275a9473b8121447b44810bb4909
Cr-Commit-Position: refs/heads/master@{#327509}

[Devlin, 2015-03-26 21:00:21]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-03-26 21:00:28]

Patchset #1 (id:20001) has been deleted

[Devlin, 2015-03-26 21:00:35]

Patchset #1 (id:40001) has been deleted

[Devlin, 2015-03-26 21:00:40]

Patchset #1 (id:60001) has been deleted

[Devlin, 2015-03-26 21:00:45]

Patchset #1 (id:80001) has been deleted

[Devlin, 2015-03-26 21:00:52]

Patchset #1 (id:100001) has been deleted

[Devlin, 2015-03-26 21:29:48]

Patchset #1 (id:120001) has been deleted

[Devlin, 2015-03-26 21:39:18]

Patchset #1 (id:140001) has been deleted

[Devlin, 2015-03-26 21:40:06]

Patchset #1 (id:160001) has been deleted

[Devlin, 2015-03-26 21:41:59]

rdevlin.cronin@chromium.org changed reviewers:
+ dbeam@chromium.org

[Devlin, 2015-03-26 21:42:01]

Hey Dan - sorry for the size.  Please take a look at the HTML/CSS/JS when you
can.

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error.js (right):

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:197: * @private {?}
Making this @private {ExtensionError} results in "ERROR - Bad type annotation.
Unknown type ExtensionError", even though the function on 37 is marked as a
constructor and the prototype is defined on line 44...any ideas?

[Devlin, 2015-03-30 23:16:15]

Friendly ping :)

[Dan Beam, 2015-03-31 00:33:50]

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:432:
ExtensionInfoGenerator(browser_context()).CreateExtensionInfo(params->id);
+\s\s

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/developer_private/extension_info_generator.cc
(right):

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:368:
(extension = registry->disabled_extensions().GetByID(id)) != nullptr) {
:_(

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error.css (right):

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.css:34:
.extension-error-list-contents:empty + #no-errors-span {
what exactly are you trying to accomplish here?

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error.js (right):

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:50: return this;
nit: no if/else after return, i.e.

  if (element.classList.contains('extension-error-metadata'))
    return this;

  if (element.classList.contains('error-delete-button'))
    return /** !HTMLElement */(this.querySelector('.error-delete-button'));

  assertNotReached();
  return element;

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:72: this.error = error;
I couldn't really tell, but can this be @private?

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:108: { bubbles: true,
detail: this.error }));
{\s -> {
\s} -> }

(everywhere)

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:116: if (e.keyCode == 13
/* enter */)
e.keyIdentifier == 'Enter'

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:116: if (e.keyCode == 13
/* enter */)
isn't there already a click dispatched on Enter?

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:197: * @private {?}
On 2015/03/26 21:42:00, Devlin wrote:
> Making this @private {ExtensionError} results in "ERROR - Bad type annotation.
> Unknown type ExtensionError", even though the function on 37 is marked as a
> constructor and the prototype is defined on line 44...any ideas?

extensions.ExtensionError

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:233: index =
Math.min(index, this.errors_.length - 1);
what if this.errors_.length == 0?  does -1 work?

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:249: updateErrors_:
function(newErrors) {
how often is this called?  currently O(N) of M*N it seems :/ (M=list size,
N=newErrors size).  how big can M*N get?

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:273: var node = null;
var node = errorList.children[index];
if (node)
  this.setActiveErrorNode_(node.querySelector('.extension-error-metadata));

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:289:
this.errors_.forEach(function(error) { ids.push(error.id); });
var ids = this.errors_.map(function(error) { return error.id; });

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:296:
this.errors_.splice(0, this.errors_.length);
this.errors_.length = 0;

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:299:
errorList.removeChild(errorList.firstChild);
this.querySelector('.extension-error-list-contents').innerHTML = '';

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:309:
this.activeError_.classList.remove('extension-error-active');
nit: \n

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:311:
node.classList.add('extension-error-active');
nit: \n

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error_overlay.js (right):

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error_overlay.js:374: 
nit: /** @private */ at least

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extensions.css (right):

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extensions.css:126: line-height: 100px;  /*
vertically centers text and serves as min-height. */
nit: Vertically

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extensions.html (right):

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extensions.html:164: </span>
nit: </span> on previous line (because it fits)

https://codereview.chromium.org/1016413004/diff/180001/extensions/browser/err...
File extensions/browser/error_map.h (right):

https://codereview.chromium.org/1016413004/diff/180001/extensions/browser/err...
extensions/browser/error_map.h:63: // Removes errors that match the given
|filter| from the map.
nit: update to include |affected_ids|

[Devlin, 2015-03-31 16:16:03]

Patchset #2 (id:200001) has been deleted

[Devlin, 2015-03-31 16:17:51]

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:432:
ExtensionInfoGenerator(browser_context()).CreateExtensionInfo(params->id);
On 2015/03/31 00:33:49, Dan Beam wrote:
> +\s\s

Done.

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/developer_private/extension_info_generator.cc
(right):

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:368:
(extension = registry->disabled_extensions().GetByID(id)) != nullptr) {
On 2015/03/31 00:33:49, Dan Beam wrote:
> :_(

Why sad face?  I think it actually makes sense, as opposed to:
if (registry->disabled_extensions().GetByID(id))
  extension = registry->disabled_extensions().GetByID(id);
else if (registry->terminated_extensions().GetByID(id))
  extension = registry->terminated_extensions().GetByID(id);

not to mention, it saves a look-up.  Also might be worth noting this is a direct
copy over of the code from developer_private_api.cc; it's not new. :)

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error.css (right):

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.css:34:
.extension-error-list-contents:empty + #no-errors-span {
On 2015/03/31 00:33:49, Dan Beam wrote:
> what exactly are you trying to accomplish here?

If the extension error list is empty, I want to show the no-errors-span. 
Otherwise, I want to hide the no-errors-span (handled by line 31).

Would you prefer this done in JS?

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error.js (right):

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:50: return this;
On 2015/03/31 00:33:49, Dan Beam wrote:
> nit: no if/else after return, i.e.
> 
>   if (element.classList.contains('extension-error-metadata'))
>     return this;
> 
>   if (element.classList.contains('error-delete-button'))
>     return /** !HTMLElement */(this.querySelector('.error-delete-button'));
> 
>   assertNotReached();
>   return element;

Done.

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:72: this.error = error;
On 2015/03/31 00:33:50, Dan Beam wrote:
> I couldn't really tell, but can this be @private?

It's used by the ExtensionErrorList (in this patch set, line 316).  We could
make it private, and then expose a getter, but figured that might be overkill. 
WDYT?

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:108: { bubbles: true,
detail: this.error }));
On 2015/03/31 00:33:49, Dan Beam wrote:
> {\s -> {
> \s} -> }
> 
> (everywhere)

Done.

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:116: if (e.keyCode == 13
/* enter */)
On 2015/03/31 00:33:50, Dan Beam wrote:
> isn't there already a click dispatched on Enter?

I would have thought so, but in testing, no.  I wonder if this is because it's
not typical button?  (It's a row in a focus grid)

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:116: if (e.keyCode == 13
/* enter */)
On 2015/03/31 00:33:49, Dan Beam wrote:
> e.keyIdentifier == 'Enter'

Thought keyIdentifier was from an old draft...  Done.

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:197: * @private {?}
On 2015/03/31 00:33:50, Dan Beam wrote:
> On 2015/03/26 21:42:00, Devlin wrote:
> > Making this @private {ExtensionError} results in "ERROR - Bad type
annotation.
> > Unknown type ExtensionError", even though the function on 37 is marked as a
> > constructor and the prototype is defined on line 44...any ideas?
> 
> extensions.ExtensionError

Thought of that and tried it, same error (well, "Unknown type
extensions.ExtensionError")

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:233: index =
Math.min(index, this.errors_.length - 1);
On 2015/03/31 00:33:50, Dan Beam wrote:
> what if this.errors_.length == 0?  does -1 work?

Yep, line 274 (in this patch set) checks the index.

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:249: updateErrors_:
function(newErrors) {
On 2015/03/31 00:33:49, Dan Beam wrote:
> how often is this called?  currently O(N) of M*N it seems :/ (M=list size,
> N=newErrors size).  how big can M*N get?

Yeah, right now it's M*N.  M and N are each capped at 100.  It can be called
each time an extension throws an error, or each time an error is deleted (the
latter is gated on user action, the former is not).

We could, technically, reduce this to O(N) = N trivially by deleting the current
error list and completely recreating it, but I think the cost of deleting 100 UI
elements and recreating 100 UI elements could be >100x more expensive than
comparing an id attribute.  Additionally, this way we preserve the existing DOM,
which I think is important for a11y.

Note also that in the "normal" case, this will not reach 100 or be called
terribly frequently - that would be a very broken extension (which we should
handle, but for it to be a little rough, I think, is reasonable).

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:273: var node = null;
On 2015/03/31 00:33:49, Dan Beam wrote:
> var node = errorList.children[index];
> if (node)
>   this.setActiveErrorNode_(node.querySelector('.extension-error-metadata));

That's actually different.  setActiveErrorNode_() treats a null "active node" as
setting the active node to none, which is important (in the case when we need to
not have one).

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:289:
this.errors_.forEach(function(error) { ids.push(error.id); });
On 2015/03/31 00:33:50, Dan Beam wrote:
> var ids = this.errors_.map(function(error) { return error.id; });

Done.

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:296:
this.errors_.splice(0, this.errors_.length);
On 2015/03/31 00:33:49, Dan Beam wrote:
> this.errors_.length = 0;

How have I never seen this?  Awesome.

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:299:
errorList.removeChild(errorList.firstChild);
On 2015/03/31 00:33:50, Dan Beam wrote:
> this.querySelector('.extension-error-list-contents').innerHTML = '';

Pushing back on this.  Two reasons:
- *** It seems to be so much faster.  There are dozens (insert meme here) if not
hundreds of perfs on this, but here's one:
http://jsperf.com/innerhtml-vs-removechild/290 (there are a lot in that same
series).  I have no idea what we're doing with innerHTML in blink, but it's 100x
slower.
- innerHTML is just kind of yucky in general, and I wouldn't be surprised if we
disallow it in chrome at some point out of principle (we already do this in
other projects, and I think there may even be a closure_compiler rule to enforce
it :))

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:309:
this.activeError_.classList.remove('extension-error-active');
On 2015/03/31 00:33:49, Dan Beam wrote:
> nit: \n

Done.

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:311:
node.classList.add('extension-error-active');
On 2015/03/31 00:33:50, Dan Beam wrote:
> nit: \n

Done.

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error_overlay.js (right):

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error_overlay.js:374: 
On 2015/03/31 00:33:50, Dan Beam wrote:
> nit: /** @private */ at least

Whoops!  Added jsdocs.

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extensions.css (right):

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extensions.css:126: line-height: 100px;  /*
vertically centers text and serves as min-height. */
On 2015/03/31 00:33:50, Dan Beam wrote:
> nit: Vertically

Done.

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extensions.html (right):

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extensions.html:164: </span>
On 2015/03/31 00:33:50, Dan Beam wrote:
> nit: </span> on previous line (because it fits)

Done.

https://codereview.chromium.org/1016413004/diff/180001/extensions/browser/err...
File extensions/browser/error_map.h (right):

https://codereview.chromium.org/1016413004/diff/180001/extensions/browser/err...
extensions/browser/error_map.h:63: // Removes errors that match the given
|filter| from the map.
On 2015/03/31 00:33:50, Dan Beam wrote:
> nit: update to include |affected_ids|

Done.

[Dan Beam, 2015-04-22 21:02:39]

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/developer_private/extension_info_generator.cc
(right):

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:368:
(extension = registry->disabled_extensions().GetByID(id)) != nullptr) {
On 2015/03/31 16:17:50, Devlin wrote:
> On 2015/03/31 00:33:49, Dan Beam wrote:
> > :_(
> 
> Why sad face?  I think it actually makes sense, as opposed to:
> if (registry->disabled_extensions().GetByID(id))
>   extension = registry->disabled_extensions().GetByID(id);
> else if (registry->terminated_extensions().GetByID(id))
>   extension = registry->terminated_extensions().GetByID(id);
> 
> not to mention, it saves a look-up.  Also might be worth noting this is a
direct
> copy over of the code from developer_private_api.cc; it's not new. :)

eh, it's all awkward. whatevs.

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error.css (right):

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.css:34:
.extension-error-list-contents:empty + #no-errors-span {
On 2015/03/31 16:17:50, Devlin wrote:
> On 2015/03/31 00:33:49, Dan Beam wrote:
> > what exactly are you trying to accomplish here?
> 
> If the extension error list is empty, I want to show the no-errors-span. 
> Otherwise, I want to hide the no-errors-span (handled by line 31).
> 
> Would you prefer this done in JS?

yeah, because :empty doesn't match if there's whitespace
https://developer.mozilla.org/en-US/docs/Web/CSS/:empty

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:333: if
(!extension_ids_.count(id)) {
extension_ids vs extension_ids_ is pretty dubious

maybe |removed| or |removed_ids| instead?

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/api/developer_private/extension_info_generator.cc
(right):

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:378:
registry->terminated_extensions().GetByID(id)) != nullptr) {
mildly scary but ok...

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/api/developer_private/extension_info_generator.h
(right):

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/developer_private/extension_info_generator.h:21:
class RuntimeError;
Y U ADD?

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/error_console/error_console.cc (right):

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/error_console/error_console.cc:57: }
why not just {} in the .h?  is this preferred for some reason?

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error.css (right):

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.css:23: cursor: pointer;
^ why cursor: pointer; ?  to know that you can click?

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.css:81: background-repeat:
no-repeat;
nit: isn't this the same as

  background: url(chrome://theme/IDR_CLOSE_DIALOG) no-repeat center;

?

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error.js (right):

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:65: decorate:
function(error, boundary) {
this code is out of date

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:116: if (e.keyIdentifier
== 'Enter')
probably need target != deleteButton here as well

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:223: var index =
this.errors_.indexOf(error);
using references as IDs is kinda dubious.  will errors never be copied or
regenerated?

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:226: var wasActive =
this.activeError_ && this.activeError_.error == error;
same dubiousness with == across objects

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:274: if (index >= 0 &&
index < errorList.children.length) {
nit:

  var item = errorList.children[index];
  if (item)
    this.setActiveErrorNode_(item.querySelector('.extension-error-metadata'));

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:276:
'.extension-error-metadata');
nit:

  node = errorList.children[index].querySelector(
      '.extension-error-metadata');

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:296: var errorList =
this.querySelector('.extension-error-list-contents');
nit:

  this.querySelector('.extension-error-list-contents').innerHTML = '';

?

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error_overlay.css (right):

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error_overlay.css:17: overflow-y:
auto;
what's constricting the height here?

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error_overlay.js (right):

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error_overlay.js:119: if
(this.error_)
is this if() necessary?

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_list.js:455: var newEx =
this.extensions_.filter(function(e) {
what if this.extensions_ is empty?

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_list.js:663: 
var errors = extension.runtimeErrors.length || extension.runtimeErrors.length;
this.updateVisibility_(row, '.errors-link', errors, function() {

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_list.js:681: },
extension.manifestErrors.length ? Level.WARN : Level.LOG);
i have no idea what this does

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_list.js:685: icon.className =
'extension-error-icon';
and the point of this is... to remove other classes?

[Devlin, 2015-04-22 23:17:32]

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error.css (right):

https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.css:34:
.extension-error-list-contents:empty + #no-errors-span {
On 2015/04/22 21:02:37, Dan Beam wrote:
> On 2015/03/31 16:17:50, Devlin wrote:
> > On 2015/03/31 00:33:49, Dan Beam wrote:
> > > what exactly are you trying to accomplish here?
> > 
> > If the extension error list is empty, I want to show the no-errors-span. 
> > Otherwise, I want to hide the no-errors-span (handled by line 31).
> > 
> > Would you prefer this done in JS?
> 
> yeah, because :empty doesn't match if there's whitespace
> https://developer.mozilla.org/en-US/docs/Web/CSS/:empty

Haha yeah, I learned that the hard way :P  (Notice there wasn't any in the
.html).  Moved to JS

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:333: if
(!extension_ids_.count(id)) {
On 2015/04/22 21:02:37, Dan Beam wrote:
> extension_ids vs extension_ids_ is pretty dubious
> 
> maybe |removed| or |removed_ids| instead?

Fair, done.

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/api/developer_private/extension_info_generator.h
(right):

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/developer_private/extension_info_generator.h:21:
class RuntimeError;
On 2015/04/22 21:02:37, Dan Beam wrote:
> Y U ADD?

Removed.

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/error_console/error_console.cc (right):

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/error_console/error_console.cc:57: }
On 2015/04/22 21:02:37, Dan Beam wrote:
> why not just {} in the .h?  is this preferred for some reason?

I think there was some rule that it was preferred to have virtual functions
declared in the .cc, but we are pretty lax, especially when that rule would mean
adding a new file.  So I pretty much go with whatever the convention in that
file is (and here, it was to put them in the .cc).  I have no preference,
though, so happy to move them if you like.

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error.css (right):

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.css:23: cursor: pointer;
On 2015/04/22 21:02:38, Dan Beam wrote:
> ^ why cursor: pointer; ?  to know that you can click?

Exactly.

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.css:81: background-repeat:
no-repeat;
On 2015/04/22 21:02:38, Dan Beam wrote:
> nit: isn't this the same as
> 
>   background: url(chrome://theme/IDR_CLOSE_DIALOG) no-repeat center;
> 
> ?

background: url(chrome://theme/IDR_CLOSE_DIALOG) no-repeat center transparent;

but yes.  Is there logic to when to prefer the shorthand (with background) vs
verbose (with, e.g., margins)?  Just because the attributes for background all
take different possible values?

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error.js (right):

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:65: decorate:
function(error, boundary) {
On 2015/04/22 21:02:38, Dan Beam wrote:
> this code is out of date

Resynced.

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:116: if (e.keyIdentifier
== 'Enter')
On 2015/04/22 21:02:38, Dan Beam wrote:
> probably need target != deleteButton here as well

Done.

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:223: var index =
this.errors_.indexOf(error);
On 2015/04/22 21:02:38, Dan Beam wrote:
> using references as IDs is kinda dubious.  will errors never be copied or
> regenerated?

Hmm... in theory, no, but in a pass-by-reference language, I'm not willing to
bet on it.  Changed.

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:226: var wasActive =
this.activeError_ && this.activeError_.error == error;
On 2015/04/22 21:02:38, Dan Beam wrote:
> same dubiousness with == across objects

Done.

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:274: if (index >= 0 &&
index < errorList.children.length) {
On 2015/04/22 21:02:38, Dan Beam wrote:
> nit:
> 
>   var item = errorList.children[index];
>   if (item)
>     this.setActiveErrorNode_(item.querySelector('.extension-error-metadata'));

We also want to call setActiveErrorNode_ with null if there isn't one, so
adjusted slightly to
this.setActiveErrorNode_(
    item ? item.querySelector('.extension-error-metadata') : null);

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:276:
'.extension-error-metadata');
On 2015/04/22 21:02:38, Dan Beam wrote:
> nit:
> 
>   node = errorList.children[index].querySelector(
>       '.extension-error-metadata');

Moot with above, but noted. :)

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:296: var errorList =
this.querySelector('.extension-error-list-contents');
On 2015/04/22 21:02:38, Dan Beam wrote:
> nit:
> 
>   this.querySelector('.extension-error-list-contents').innerHTML = '';
> 
> ?

Where'd that comment go....
https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...

But that definitely proves you are consistent. ;)

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error_overlay.css (right):

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error_overlay.css:17: overflow-y:
auto;
On 2015/04/22 21:02:38, Dan Beam wrote:
> what's constricting the height here?

It's restricted because it's in an overlay, which has a max height.  Would you
prefer a more explicit restriction?

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error_overlay.js (right):

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error_overlay.js:119: if
(this.error_)
On 2015/04/22 21:02:38, Dan Beam wrote:
> is this if() necessary?

If your asking if this.error_ is ever null, the answer is yes.  If you're asking
if anything bad happens if we clear a non-existent error, I think the answer is
no.  If removed.

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_list.js:455: var newEx =
this.extensions_.filter(function(e) {
On 2015/04/22 21:02:38, Dan Beam wrote:
> what if this.extensions_ is empty?

I don't think it can be - there'd be no node to create or click on.  Let's
assert it.

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_list.js:663: 
On 2015/04/22 21:02:38, Dan Beam wrote:
> var errors = extension.runtimeErrors.length || extension.runtimeErrors.length;
> this.updateVisibility_(row, '.errors-link', errors, function() {

Done.

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_list.js:681: },
extension.manifestErrors.length ? Level.WARN : Level.LOG);
On 2015/04/22 21:02:38, Dan Beam wrote:
> i have no idea what this does

Comment on line 675. :)

It goes through and looks for the error with the highest severity, passing in an
initial value of WARN if there's a manifest error (since all manifest errors
have WARN level) or LOG (the lowest) otherwise.

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_list.js:685: icon.className =
'extension-error-icon';
On 2015/04/22 21:02:38, Dan Beam wrote:
> and the point of this is... to remove other classes?

Exactly.  We could also remove all the possibles, if that's more clear?
i.e.
Object.keys(map).forEach(function(key) {
  icon.classList.remove(map[key].name);
});
icon.classList.add(map[highSeverity].name);

Preferred?  Or option c?

[Devlin, 2015-04-24 19:56:17]

Ping?

[Dan Beam, 2015-04-28 03:47:04]

lgtm

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_list.js:670: var Level =
chrome.developerPrivate.ErrorLevel;
\n

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_list.js:674: map[Level.ERROR] =
{weight: 2, name: 'extension-error-fatal-icon'};
\n

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_list.js:677: var highSeverity =
extension.runtimeErrors.reduce(
nit: highestSeverity or mostSevere

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_list.js:685: icon.className =
'extension-error-icon';
On 2015/04/22 23:17:32, Devlin wrote:
> On 2015/04/22 21:02:38, Dan Beam wrote:
> > and the point of this is... to remove other classes?
> 
> Exactly.  We could also remove all the possibles, if that's more clear?
> i.e.
> Object.keys(map).forEach(function(key) {
>   icon.classList.remove(map[key].name);
> });
> icon.classList.add(map[highSeverity].name);
> 
> Preferred?  Or option c?

leave as is, maybe add "  // Remove other classes."

https://codereview.chromium.org/1016413004/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error.css (right):

https://codereview.chromium.org/1016413004/diff/240001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.css:73: background:
url(chrome://theme/IDR_CLOSE_DIALOG) center no-repeat transparent;
can't you omit transparent because it's the default?

https://codereview.chromium.org/1016413004/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error.js (right):

https://codereview.chromium.org/1016413004/diff/240001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:115:
this.addEventListener('keydown', function(e) {
isn't there already a synthetic click being dispatched if you press Enter on a
tabable node?

https://codereview.chromium.org/1016413004/diff/240001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:272: };
nit: make a static method to do this:

  /**
    * @param {Array<(ManifestError|RuntimeError}>} errors
    * @param {string} id
  findErrorById: function(errors, id) {
    for (var i = 0; i < errors.length; ++i) {
      if (errors[i].id == id)
        return i;
    }
    return -1;
  },

and call from various places

[Dan Beam, 2015-04-28 17:08:08]

https://codereview.chromium.org/1016413004/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error_overlay.js (left):

https://codereview.chromium.org/1016413004/diff/240001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error_overlay.js:406: * @suppress
{missingProperties}
add this back

[Devlin, 2015-04-29 16:08:37]

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_list.js:670: var Level =
chrome.developerPrivate.ErrorLevel;
On 2015/04/28 03:47:03, Dan Beam wrote:
> \n

Done.

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_list.js:674: map[Level.ERROR] =
{weight: 2, name: 'extension-error-fatal-icon'};
On 2015/04/28 03:47:03, Dan Beam wrote:
> \n

Done.

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_list.js:677: var highSeverity =
extension.runtimeErrors.reduce(
On 2015/04/28 03:47:03, Dan Beam wrote:
> nit: highestSeverity or mostSevere

Done.

https://codereview.chromium.org/1016413004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_list.js:685: icon.className =
'extension-error-icon';
On 2015/04/28 03:47:03, Dan Beam wrote:
> On 2015/04/22 23:17:32, Devlin wrote:
> > On 2015/04/22 21:02:38, Dan Beam wrote:
> > > and the point of this is... to remove other classes?
> > 
> > Exactly.  We could also remove all the possibles, if that's more clear?
> > i.e.
> > Object.keys(map).forEach(function(key) {
> >   icon.classList.remove(map[key].name);
> > });
> > icon.classList.add(map[highSeverity].name);
> > 
> > Preferred?  Or option c?
> 
> leave as is, maybe add "  // Remove other classes."

Done.

https://codereview.chromium.org/1016413004/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error.css (right):

https://codereview.chromium.org/1016413004/diff/240001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.css:73: background:
url(chrome://theme/IDR_CLOSE_DIALOG) center no-repeat transparent;
On 2015/04/28 03:47:03, Dan Beam wrote:
> can't you omit transparent because it's the default?

Thought I tried that, and it didn't look right (I assumed it was inheriting from
somewhere, but it may have just been a faulty property elsewhere). But
apparently it's fine now.  Removed.

https://codereview.chromium.org/1016413004/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error.js (right):

https://codereview.chromium.org/1016413004/diff/240001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:115:
this.addEventListener('keydown', function(e) {
On 2015/04/28 03:47:04, Dan Beam wrote:
> isn't there already a synthetic click being dispatched if you press Enter on a
> tabable node?

In testing, no - I think only controls (buttons, etc) have click events
generated from an enter, but I could be wrong.

https://codereview.chromium.org/1016413004/diff/240001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error.js:272: };
On 2015/04/28 03:47:03, Dan Beam wrote:
> nit: make a static method to do this:
> 
>   /**
>     * @param {Array<(ManifestError|RuntimeError}>} errors
>     * @param {string} id
>   findErrorById: function(errors, id) {
>     for (var i = 0; i < errors.length; ++i) {
>       if (errors[i].id == id)
>         return i;
>     }
>     return -1;
>   },
> 
> and call from various places

Done.

https://codereview.chromium.org/1016413004/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_error_overlay.js (left):

https://codereview.chromium.org/1016413004/diff/240001/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_error_overlay.js:406: * @suppress
{missingProperties}
On 2015/04/28 17:08:07, Dan Beam wrote:
> add this back

Done.

[Devlin, 2015-04-29 16:12:11]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-04-29 16:12:11]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1016413004/#ps280001
(title: " ")

[commit-bot: I haz the power, 2015-04-29 16:12:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1016413004/280001

[commit-bot: I haz the power, 2015-04-29 16:51:51]

Committed patchset #5 (id:280001)

[commit-bot: I haz the power, 2015-04-29 16:53:31]

Patchset 5 (id:??) landed as
https://crrev.com/20bf10bc2f45275a9473b8121447b44810bb4909
Cr-Commit-Position: refs/heads/master@{#327509}