Improve UI for unpacked extensions failing to load

Improve UI for unpacked extensions failing to load.

TBR=jochen@chromium.org for chrome/browser/chrome_notification_types.h (*removing* a notification type).
BUG=358710
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266395

[Devlin, 2014-04-25 16:19:52]

Finnur, can you take a look at c/b/ui/webui?  The JS/CSS still need a bit of
work, but C++ should be mostly stable.

[Finnur, 2014-04-25 16:47:21]

I only have two minor nits.

I also tested this on Linux and like what I see. :)

So, c/b/ui/webui LGTM (with the two nits below).

https://codereview.chromium.org/252593003/diff/10001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right):

https://codereview.chromium.org/252593003/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_loader_handler.cc:194: size_t
column = 1u;
Would probably make sense to have these be invalid values (say... 0, not 1)?

https://codereview.chromium.org/252593003/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_loader_handler.cc:202: LOG(WARNING)
<< "Regex: " << regex;
There are a lot of LOG(WARNING) statements in this file that I suspect are for
debugging. Remove?

[Devlin, 2014-04-25 16:54:08]

Ben, c/b/r/ is still in heavy flux, but c/b/e should be pretty stable, if you
want a head start.

[not at google - send to devlin, 2014-04-25 17:10:20]

cpp lgtm, haven't looked at resources.

https://codereview.chromium.org/252593003/diff/10001/chrome/browser/extension...
File chrome/browser/extensions/unpacked_installer.cc (right):

https://codereview.chromium.org/252593003/diff/10001/chrome/browser/extension...
chrome/browser/extensions/unpacked_installer.cc:300: return;
this file is quite broken, but I'm scared to touch it.

one tiny cleanup here is to remove the early-return since it doesn't add
anything (if (service_weak_.get()) service_weak_->...;

https://codereview.chromium.org/252593003/diff/10001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_load_error.css (right):

https://codereview.chromium.org/252593003/diff/10001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_load_error.css:1: /* Copyright (c)
2012 The Chromium Authors. All rights reserved.
2014 :)

https://codereview.chromium.org/252593003/diff/10001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_load_error.css:4: 
i'm going tro trust you on this one.

https://codereview.chromium.org/252593003/diff/10001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_load_error.css:16: position:
relative;
shouldn't need this

[Devlin, 2014-04-25 18:13:12]

https://codereview.chromium.org/252593003/diff/10001/chrome/browser/extension...
File chrome/browser/extensions/unpacked_installer.cc (right):

https://codereview.chromium.org/252593003/diff/10001/chrome/browser/extension...
chrome/browser/extensions/unpacked_installer.cc:300: return;
On 2014/04/25 17:10:20, kalman wrote:
> this file is quite broken, but I'm scared to touch it.
> 
> one tiny cleanup here is to remove the early-return since it doesn't add
> anything (if (service_weak_.get()) service_weak_->...;

Done.

https://codereview.chromium.org/252593003/diff/10001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_load_error.css (right):

https://codereview.chromium.org/252593003/diff/10001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_load_error.css:1: /* Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2014/04/25 17:10:20, kalman wrote:
> 2014 :)

Done.

https://codereview.chromium.org/252593003/diff/10001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_load_error.css:16: position:
relative;
On 2014/04/25 17:10:20, kalman wrote:
> shouldn't need this

Done.

https://codereview.chromium.org/252593003/diff/10001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right):

https://codereview.chromium.org/252593003/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_loader_handler.cc:194: size_t
column = 1u;
On 2014/04/25 16:47:21, Finnur wrote:
> Would probably make sense to have these be invalid values (say... 0, not 1)?

Sure, done.

https://codereview.chromium.org/252593003/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_loader_handler.cc:202: LOG(WARNING)
<< "Regex: " << regex;
On 2014/04/25 16:47:21, Finnur wrote:
> There are a lot of LOG(WARNING) statements in this file that I suspect are for
> debugging. Remove?

Whoops, sorry (remnant of rushing to get the review up).

[not at google - send to devlin, 2014-04-25 20:15:51]

basically lg though you may want a second opinion - and note the rather more
significant change away from a util class.

https://codereview.chromium.org/252593003/diff/70001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_load_error.html (right):

https://codereview.chromium.org/252593003/diff/70001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_load_error.html:3: Copyright 2013
The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/252593003/diff/70001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_loader.js (right):

https://codereview.chromium.org/252593003/diff/70001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_loader.js:79: this.__proto__ =
ExtensionLoader.prototype;
don't need this here

https://codereview.chromium.org/252593003/diff/70001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_loader.js:80: this.init();
and it seems like you might as well inline init() in here?

https://codereview.chromium.org/252593003/diff/70001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_util.js (right):

https://codereview.chromium.org/252593003/diff/70001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_util.js:12: 
util is a bit of a misnomer at the moment since it's specifically used for
augmenting the shared functionality between the manifest and the error console.

can you attach this logic to a webui-custom-element-type-thing and not have it
be a util at all?

[Devlin, 2014-04-25 20:48:39]

https://codereview.chromium.org/252593003/diff/70001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_load_error.html (right):

https://codereview.chromium.org/252593003/diff/70001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_load_error.html:3: Copyright 2013
The Chromium Authors. All rights reserved.
On 2014/04/25 20:15:52, kalman wrote:
> 2014

Done.

https://codereview.chromium.org/252593003/diff/70001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_loader.js (right):

https://codereview.chromium.org/252593003/diff/70001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_loader.js:79: this.__proto__ =
ExtensionLoader.prototype;
On 2014/04/25 20:15:52, kalman wrote:
> don't need this here

Done.

https://codereview.chromium.org/252593003/diff/70001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_loader.js:80: this.init();
On 2014/04/25 20:15:52, kalman wrote:
> and it seems like you might as well inline init() in here?

It's small enough. Sure.

https://codereview.chromium.org/252593003/diff/70001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_util.js (right):

https://codereview.chromium.org/252593003/diff/70001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_util.js:12: 
On 2014/04/25 20:15:52, kalman wrote:
> util is a bit of a misnomer at the moment since it's specifically used for
> augmenting the shared functionality between the manifest and the error
console.
> 
> can you attach this logic to a webui-custom-element-type-thing and not have it
> be a util at all?

Done.

[not at google - send to devlin, 2014-04-25 21:00:34]

lgtm

https://codereview.chromium.org/252593003/diff/90001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_error_overlay.js (right):

https://codereview.chromium.org/252593003/diff/90001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_error_overlay.js:15: };
no ;

https://codereview.chromium.org/252593003/diff/90001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_error_overlay.js:420:
clearElement(this.codeDiv_);
this should be "this.codeDiv_.clear()" right?

https://codereview.chromium.org/252593003/diff/90001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_load_error.html (right):

https://codereview.chromium.org/252593003/diff/90001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_load_error.html:7: <div
id="extension-load-error" hidden>
it would be better if the "hidden" property were on where this is used

[Devlin, 2014-04-25 22:19:08]

https://codereview.chromium.org/252593003/diff/90001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_error_overlay.js (right):

https://codereview.chromium.org/252593003/diff/90001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_error_overlay.js:15: };
On 2014/04/25 21:00:34, kalman wrote:
> no ;

whoops, done.

https://codereview.chromium.org/252593003/diff/90001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_error_overlay.js:420:
clearElement(this.codeDiv_);
On 2014/04/25 21:00:34, kalman wrote:
> this should be "this.codeDiv_.clear()" right?

They do the same thing, but probably cleaner that way, yes. Done.

https://codereview.chromium.org/252593003/diff/90001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_load_error.html (right):

https://codereview.chromium.org/252593003/diff/90001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_load_error.html:7: <div
id="extension-load-error" hidden>
On 2014/04/25 21:00:34, kalman wrote:
> it would be better if the "hidden" property were on where this is used

I don't think it can be, since it is done through an "include" tag. :/

[Devlin, 2014-04-25 22:19:15]

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

[commit-bot: I haz the power, 2014-04-25 22:28:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/252593003/...

[commit-bot: I haz the power, 2014-04-25 23:45:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-25 23:45:45]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[Devlin, 2014-04-25 23:47:38]

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

[commit-bot: I haz the power, 2014-04-25 23:50:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/252593003/...

[commit-bot: I haz the power, 2014-04-26 08:12:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-26 08:12:34]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[Devlin, 2014-04-26 13:46:56]

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

[commit-bot: I haz the power, 2014-04-26 13:47:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/252593003/...

[commit-bot: I haz the power, 2014-04-26 22:44:37]

Change committed as 266395