Enable Error Console for ADT and <= Dev Channel

chrome/browser/ui/webui/extensions/extension_settings_handler.cc

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/ui/webui/extensions/extension_settings_handler.h"
 
 #include "apps/app_load_service.h"
 #include "apps/app_restore_service.h"
 #include "apps/app_window.h"
 #include "apps/app_window_registry.h"
@@ skipping 34 matching lines @@
 #include "chrome/browser/managed_mode/managed_user_service.h"
 #include "chrome/browser/managed_mode/managed_user_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/tab_contents/background_contents.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/chrome_select_file_policy.h"
 #include "chrome/browser/ui/extensions/application_launch.h"
 #include "chrome/browser/ui/webui/extensions/extension_basic_info.h"
 #include "chrome/browser/ui/webui/extensions/extension_icon_source.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/chrome_version_info.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/extensions/extension_icon_set.h"
 #include "chrome/common/extensions/manifest_url_handler.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "components/user_prefs/pref_registry_syncable.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_process_host.h"
@@ skipping 262 matching lines @@
     if (!warnings.empty()) {
       base::ListValue* warnings_list = new base::ListValue;
       for (std::vector<std::string>::const_iterator iter = warnings.begin();
            iter != warnings.end(); ++iter) {
         warnings_list->Append(base::Value::CreateStringValue(*iter));
       }
       extension_data->Set("warnings", warnings_list);
     }
   }
 
-  // If the ErrorConsole is enabled, get the errors for the extension and add
-  // them to the list. Otherwise, use the install warnings (using both is
-  // redundant).
+  // We display errors from the ErrorConsole if either the switch is enabled or
+  // we are in a development channel, and the error console itself is enabled
+  // (i.e., the user is in Developer Mode).
   ErrorConsole* error_console =
       ErrorConsole::Get(extension_service_->profile());
-  if (error_console->enabled()) {
+  if ((FeatureSwitch::error_console()->IsEnabled() ||
+       chrome::VersionInfo::GetChannel() <= chrome::VersionInfo::CHANNEL_DEV) &&
+      error_console->enabled()) {

[not at google - send to devlin, 2014/03/27 23:41:11]

It's a bit weird having the enabled checks only here, particularly since there's
an "enabled" property on error_console. Could you pull this logic into the
ErrorConsole itself? You'll probably want to rename it IsEnabled() since it'd be
doing more than trivial checks.

[Devlin, 2014/03/28 01:25:49]

I don't know if we can pull it into ErrorConsole, for the reason that this check
is different based on whether it's for chrome:extensions or for the Apps
Developer Tools [1].  Apps Developer Tools can just check ErrorConsole::enabled
(which is basically just chrome:extensions is in developer mode), but this one
has to check for the switch or the channel.  Moving these into ErrorConsole
would mean that either ADT and chrome:extensions are lock-step, or that ADT has
to do the inverse of these checks.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

[not at google - send to devlin, 2014/03/28 15:07:40]

Ok, thanks for the explanation. I still thing that "enabled" is confusing
though, it really means "enabled in the ADT" and it happens that "enabled in
chrome://extensions" is a subset of those situations.

Another point of oddity: there are a lot of checks in this file like
"(!enabled_) bail()" which is too simplistic. If the ADT isn't installed but
developer mode is on (and this is beta channel say) then this will record, which
it needn't.

Perhaps you could cut it like this:
- rename "enabled" in this class to "should record" and base that on the
heuristic (developer mode) &&
(ADT is installed || channel is dev || command line flag is on)
- ADT always listens
- chrome://extensions listens if dev || command line flag

that implies to me that those conditions should be brought inside this class
(IsEnabledForChromeExtensions() and IsEnabledForADT()) even though it's a bit
odd.

WDYT? Sorry for this patch turning out to be more than you bargained for :)

[Devlin, 2014/03/28 21:49:59]

Done.

     const ErrorList& errors =
         error_console->GetErrorsForExtension(extension->id());
     if (!errors.empty()) {
       scoped_ptr<base::ListValue> manifest_errors(new base::ListValue);
       scoped_ptr<base::ListValue> runtime_errors(new base::ListValue);
       for (ErrorList::const_iterator iter = errors.begin();
            iter != errors.end(); ++iter) {
         if ((*iter)->type() == ExtensionError::MANIFEST_ERROR) {
           manifest_errors->Append((*iter)->ToValue().release());
         } else {  // Handle runtime error.
@@ skipping 976 matching lines @@
     const base::FilePath& path) {
   if (IndexOfLoadingPath(path) == -1)
     return;  // Not an extension we're tracking.
   if (retry)
     LoadUnpackedExtension(path);
   else
     RemoveLoadingPath(path);
 }
 
 }  // namespace extensions