ErrorConsole Switch, Duplicate Error Detection, Dev Mode Check

ErrorConsole Switch, Duplicate Error Detection, Dev Mode Check

Add in a command-line switch for ErrorConsole ('enable-error-console').
Add in duplicate-error detection, so that multiple copies of the same error aren't both stored.
Add in a check so that the ErrorConsole only stores errors if the user is in Developer Mode.
Rename ManifestParsingError to ManifestError, JavascriptRuntimeError to RuntimeError

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

[Devlin, 2013-08-26 21:33:13]

Hey Yoyo, please take a look when you get a chance. The duplicate error/dev mode
checking is taken from the manifest errors patch to reduce its size, and also
get it in before runtime errors.

[Yoyo Zhou, 2013-08-27 00:41:07]

https://codereview.chromium.org/23311011/diff/9001/chrome/browser/extensions/...
File chrome/browser/extensions/error_console/error_console.cc (right):

https://codereview.chromium.org/23311011/diff/9001/chrome/browser/extensions/...
chrome/browser/extensions/error_console/error_console.cc:56: // If we don't have
the necessary CommandLine switch enabled, then return
As I mentioned earlier today, take a look at FeatureSwitch:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

One of the benefits that didn't come to my mind at the time is it greatly
simplifies what you have to do when the switch goes from default-off to
default-on.

https://codereview.chromium.org/23311011/diff/9001/chrome/browser/extensions/...
chrome/browser/extensions/error_console/error_console.cc:96: // If we find a
duplicate error, replace the error with the new version.
There's no way to see how often a duplicate error has occurred, right? It might
be nice to show a count, although I'm not sure what the UI should look like.

https://codereview.chromium.org/23311011/diff/9001/chrome/browser/extensions/...
chrome/browser/extensions/error_console/error_console.cc:101:
extension_errors->at(i) = error.release();
Could this violate our expectation that the error list contains the most recent
errors? e.g. say we're just showing the 3 most recent errors. We might not be
showing an error that repeated an older error for some reason. It seems to me
that adding the new error to the end would be preferable.

[Devlin, 2013-08-27 17:13:56]

https://codereview.chromium.org/23311011/diff/9001/chrome/browser/extensions/...
File chrome/browser/extensions/error_console/error_console.cc (right):

https://codereview.chromium.org/23311011/diff/9001/chrome/browser/extensions/...
chrome/browser/extensions/error_console/error_console.cc:56: // If we don't have
the necessary CommandLine switch enabled, then return
On 2013/08/27 00:41:07, Yoyo Zhou wrote:
> As I mentioned earlier today, take a look at FeatureSwitch:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
> 
> One of the benefits that didn't come to my mind at the time is it greatly
> simplifies what you have to do when the switch goes from default-off to
> default-on.

Done.

https://codereview.chromium.org/23311011/diff/9001/chrome/browser/extensions/...
chrome/browser/extensions/error_console/error_console.cc:96: // If we find a
duplicate error, replace the error with the new version.
On 2013/08/27 00:41:07, Yoyo Zhou wrote:
> There's no way to see how often a duplicate error has occurred, right? It
might
> be nice to show a count, although I'm not sure what the UI should look like.

Originally, I was holding off on any of the implementation until I had an idea
what the UI would be, but it's a simple enough addition to add in. Occurrence
count added.

https://codereview.chromium.org/23311011/diff/9001/chrome/browser/extensions/...
chrome/browser/extensions/error_console/error_console.cc:101:
extension_errors->at(i) = error.release();
On 2013/08/27 00:41:07, Yoyo Zhou wrote:
> Could this violate our expectation that the error list contains the most
recent
> errors? e.g. say we're just showing the 3 most recent errors. We might not be
> showing an error that repeated an older error for some reason. It seems to me
> that adding the new error to the end would be preferable.

Ooh, good point. Fixed.

[Yoyo Zhou, 2013-08-27 17:28:00]

LGTM

https://codereview.chromium.org/23311011/diff/18002/chrome/browser/extensions...
File chrome/browser/extensions/error_console/error_console.cc (right):

https://codereview.chromium.org/23311011/diff/18002/chrome/browser/extensions...
chrome/browser/extensions/error_console/error_console.cc:137:
FeatureSwitch::error_console()->IsEnabled();
It doesn't make sense to test for the FeatureSwitch here if the listener isn't
registered if it's disabled. But if you choose to, you should return early if
it's disabled.

https://codereview.chromium.org/23311011/diff/18002/chrome/browser/extensions...
File chrome/browser/extensions/error_console/error_console_unittest.cc (right):

https://codereview.chromium.org/23311011/diff/18002/chrome/browser/extensions...
chrome/browser/extensions/error_console/error_console_unittest.cc:197: // The
more-recently reported error should be in the slot of the error it
this comment is obsolete

[Devlin, 2013-08-27 18:25:55]

https://codereview.chromium.org/23311011/diff/18002/chrome/browser/extensions...
File chrome/browser/extensions/error_console/error_console.cc (right):

https://codereview.chromium.org/23311011/diff/18002/chrome/browser/extensions...
chrome/browser/extensions/error_console/error_console.cc:137:
FeatureSwitch::error_console()->IsEnabled();
On 2013/08/27 17:28:00, Yoyo Zhou wrote:
> It doesn't make sense to test for the FeatureSwitch here if the listener isn't
> registered if it's disabled. But if you choose to, you should return early if
> it's disabled.

My thought was to protect a little against ScopedOverrides for FeatureSwitches,
but that kinda opens a whole new can of worms, and since there's no
FeatureSwitch::Observer, it'd be hard to handle all the cases, so there's no
reason to handle just one.

https://codereview.chromium.org/23311011/diff/18002/chrome/browser/extensions...
File chrome/browser/extensions/error_console/error_console_unittest.cc (right):

https://codereview.chromium.org/23311011/diff/18002/chrome/browser/extensions...
chrome/browser/extensions/error_console/error_console_unittest.cc:197: // The
more-recently reported error should be in the slot of the error it
On 2013/08/27 17:28:00, Yoyo Zhou wrote:
> this comment is obsolete

Dang. Missed one. Done.

[Yoyo Zhou, 2013-08-27 18:48:43]

https://codereview.chromium.org/23311011/diff/18002/chrome/browser/extensions...
File chrome/browser/extensions/error_console/error_console.cc (right):

https://codereview.chromium.org/23311011/diff/18002/chrome/browser/extensions...
chrome/browser/extensions/error_console/error_console.cc:137:
FeatureSwitch::error_console()->IsEnabled();
On 2013/08/27 18:25:55, D Cronin wrote:
> On 2013/08/27 17:28:00, Yoyo Zhou wrote:
> > It doesn't make sense to test for the FeatureSwitch here if the listener
isn't
> > registered if it's disabled. But if you choose to, you should return early
if
> > it's disabled.
> 
> My thought was to protect a little against ScopedOverrides for
FeatureSwitches,
> but that kinda opens a whole new can of worms, and since there's no
> FeatureSwitch::Observer, it'd be hard to handle all the cases, so there's no
> reason to handle just one.

Yeah, as you noted, because of the lack of observer, it can't be done.

[commit-bot: I haz the power, 2013-08-27 18:54:39]

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

[commit-bot: I haz the power, 2013-08-27 19:07:01]

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

[commit-bot: I haz the power, 2013-08-27 19:22:56]

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

[commit-bot: I haz the power, 2013-08-27 19:48:56]

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

[commit-bot: I haz the power, 2013-08-28 00:57:26]

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

[commit-bot: I haz the power, 2013-08-28 16:02:47]

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...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[Devlin, 2013-08-28 16:25:26]

On 2013/08/28 16:02:47, I haz the power (commit-bot) wrote:
> 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...
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

Doesn't work on Android because Developer Mode is undefined -- I can disable the
unittest for Android in the gypi, but do we start up ExtensionSystem (and thus
ErrorConsole) on Android?

[Yoyo Zhou, 2013-08-28 16:32:38]

On 2013/08/28 16:25:26, D Cronin wrote:
> On 2013/08/28 16:02:47, I haz the power (commit-bot) wrote:
> > 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...
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
> 
> Doesn't work on Android because Developer Mode is undefined -- I can disable
the
> unittest for Android in the gypi, but do we start up ExtensionSystem (and thus
> ErrorConsole) on Android?

Yes, I think we actually do. (Note the #if defined(ENABLE_EXTENSIONS) in
extension_system.cc.)

Yeah, I think we should disable the unit test.

[commit-bot: I haz the power, 2013-08-28 17:43:56]

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

[commit-bot: I haz the power, 2013-08-28 21:59:24]

Change committed as 220108