Fix Regression in Unpacked Extension Load Error UI

Fix Regression in Unpacked Extension Load Error UI

A regression where we stopped showing the manifest error was introduced in
r283852. Fix it.

TBR=finnur@chromium.org

BUG=397467
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285770

[not at google - send to devlin, 2014-07-25 16:03:10]

https://codereview.chromium.org/422533002/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_loader.js (left):

https://codereview.chromium.org/422533002/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_loader.js:20: * Construct a
Failure.
what was wrong with a Failure type?

[Devlin, 2014-07-25 16:15:41]

https://codereview.chromium.org/422533002/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_loader.js (left):

https://codereview.chromium.org/422533002/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_loader.js:20: * Construct a
Failure.
On 2014/07/25 16:03:10, kalman wrote:
> what was wrong with a Failure type?

Well, depends on your semantics ;)  Since JS is "typeless" (I know it's not
really true), there *is* no failure type.  So, really, the problem was we had an
unused function (since we never once _constructed_ a failure.  All pertinent
documentation on the failure object was moved to the failures_ array comment.

[not at google - send to devlin, 2014-07-25 16:19:09]

https://codereview.chromium.org/422533002/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_loader.js (left):

https://codereview.chromium.org/422533002/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_loader.js:20: * Construct a
Failure.
On 2014/07/25 16:15:41, Devlin wrote:
> On 2014/07/25 16:03:10, kalman wrote:
> > what was wrong with a Failure type?
> 
> Well, depends on your semantics ;)  Since JS is "typeless" (I know it's not
> really true), there *is* no failure type.  So, really, the problem was we had
an
> unused function (since we never once _constructed_ a failure.  All pertinent
> documentation on the failure object was moved to the failures_ array comment.

if we never constructed a Failure that is a problem. we should construct
Failures instead of appending objects to |failures_| :)

JS resources are going to start being type-checked with closure (there is a
chromium-dev thread about it) so type information is nice.

[Devlin, 2014-07-25 16:22:44]

https://codereview.chromium.org/422533002/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_loader.js (left):

https://codereview.chromium.org/422533002/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_loader.js:20: * Construct a
Failure.
On 2014/07/25 16:19:09, kalman wrote:
> On 2014/07/25 16:15:41, Devlin wrote:
> > On 2014/07/25 16:03:10, kalman wrote:
> > > what was wrong with a Failure type?
> > 
> > Well, depends on your semantics ;)  Since JS is "typeless" (I know it's not
> > really true), there *is* no failure type.  So, really, the problem was we
had
> an
> > unused function (since we never once _constructed_ a failure.  All pertinent
> > documentation on the failure object was moved to the failures_ array
comment.
> 
> if we never constructed a Failure that is a problem. we should construct
> Failures instead of appending objects to |failures_| :)
> 
> JS resources are going to start being type-checked with closure (there is a
> chromium-dev thread about it) so type information is nice.

Well, the failure object is primarily constructed in C++ and sent to us as an
object with 3/4 params (filePath, error, manifest).  Would you prefer to
explicitly construct a failure from an existing object, instead of appending the
missing param?  Might be worth it, for the closure compiler, but also seems
kinda... wasteful.

[not at google - send to devlin, 2014-07-25 16:44:13]

https://codereview.chromium.org/422533002/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_loader.js (left):

https://codereview.chromium.org/422533002/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_loader.js:20: * Construct a
Failure.
On 2014/07/25 16:22:44, Devlin wrote:
> On 2014/07/25 16:19:09, kalman wrote:
> > On 2014/07/25 16:15:41, Devlin wrote:
> > > On 2014/07/25 16:03:10, kalman wrote:
> > > > what was wrong with a Failure type?
> > > 
> > > Well, depends on your semantics ;)  Since JS is "typeless" (I know it's
not
> > > really true), there *is* no failure type.  So, really, the problem was we
> had
> > an
> > > unused function (since we never once _constructed_ a failure.  All
pertinent
> > > documentation on the failure object was moved to the failures_ array
> comment.
> > 
> > if we never constructed a Failure that is a problem. we should construct
> > Failures instead of appending objects to |failures_| :)
> > 
> > JS resources are going to start being type-checked with closure (there is a
> > chromium-dev thread about it) so type information is nice.
> 
> Well, the failure object is primarily constructed in C++ and sent to us as an
> object with 3/4 params (filePath, error, manifest).  Would you prefer to
> explicitly construct a failure from an existing object, instead of appending
the
> missing param?  Might be worth it, for the closure compiler, but also seems
> kinda... wasteful.

Yes I think that would be better. why is it wasteful?

[Devlin, 2014-07-25 17:17:25]

https://codereview.chromium.org/422533002/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_loader.js (left):

https://codereview.chromium.org/422533002/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_loader.js:20: * Construct a
Failure.
On 2014/07/25 16:44:13, kalman wrote:
> On 2014/07/25 16:22:44, Devlin wrote:
> > On 2014/07/25 16:19:09, kalman wrote:
> > > On 2014/07/25 16:15:41, Devlin wrote:
> > > > On 2014/07/25 16:03:10, kalman wrote:
> > > > > what was wrong with a Failure type?
> > > > 
> > > > Well, depends on your semantics ;)  Since JS is "typeless" (I know it's
> not
> > > > really true), there *is* no failure type.  So, really, the problem was
we
> > had
> > > an
> > > > unused function (since we never once _constructed_ a failure.  All
> pertinent
> > > > documentation on the failure object was moved to the failures_ array
> > comment.
> > > 
> > > if we never constructed a Failure that is a problem. we should construct
> > > Failures instead of appending objects to |failures_| :)
> > > 
> > > JS resources are going to start being type-checked with closure (there is
a
> > > chromium-dev thread about it) so type information is nice.
> > 
> > Well, the failure object is primarily constructed in C++ and sent to us as
an
> > object with 3/4 params (filePath, error, manifest).  Would you prefer to
> > explicitly construct a failure from an existing object, instead of appending
> the
> > missing param?  Might be worth it, for the closure compiler, but also seems
> > kinda... wasteful.
> 
> Yes I think that would be better. why is it wasteful?

Because of all the extra work we do to copy those expensive references into a
new object!! ;)  Mostly just because it looks a bit excessive in the code, to
me.  But I don't feel strongly.  Done.

[not at google - send to devlin, 2014-07-25 17:18:46]

lgtm!

[Devlin, 2014-07-25 17:42:56]

TBR Finnur for the micro webui regex change.

[Devlin, 2014-07-25 17:43:06]

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

[commit-bot: I haz the power, 2014-07-25 17:44:23]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/422533002/...

[commit-bot: I haz the power, 2014-07-25 21:23:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-26 00:06:30]

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

[commit-bot: I haz the power, 2014-07-26 00:06:31]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Devlin, 2014-07-26 14:01:46]

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

[commit-bot: I haz the power, 2014-07-26 14:03:50]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/422533002/...

[commit-bot: I haz the power, 2014-07-26 18:17:24]

Change committed as 285770