Enable Error Console for ADT and <= Dev Channel

Enable Error Console for ADT and <= Dev Channel

Turn Error Console on by default for users in Developer Mode, if they are using
Dev/Canary chrome.  Turn Error Console on by default for all users of the
Chrome Apps Developer Tools.

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

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

https://codereview.chromium.org/215623002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/215623002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:345:
error_console->enabled()) {
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]

https://codereview.chromium.org/215623002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/215623002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:345:
error_console->enabled()) {
On 2014/03/27 23:41:11, kalman wrote:
> 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.

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:39]

https://codereview.chromium.org/215623002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/215623002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:345:
error_console->enabled()) {

> 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.

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:58]

https://codereview.chromium.org/215623002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/215623002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:345:
error_console->enabled()) {
On 2014/03/28 15:07:40, kalman wrote:
> 
> > 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.
> 
> 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 :)
> 

Done.

[not at google - send to devlin, 2014-03-28 22:05:32]

https://codereview.chromium.org/215623002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/error_console/error_console.cc (right):

https://codereview.chromium.org/215623002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/error_console/error_console.cc:53: :
should_record_(false), default_mask_(kDefaultMask), profile_(profile) {
I thnk that enabled_ was better, sorry.

https://codereview.chromium.org/215623002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/error_console/error_console.cc:166:
kAppsDeveloperToolsExtensionId, ExtensionRegistry::EVERYTHING) != NULL;
Yes I think we had better only do this while the ADT is enabled, and listen for
ExtensionRegistry events. The Registry should outlive this class if a dependency
is declared on ExtensionSystem, but I suggest making the Registry an explicit
dependency on this class from the constructor.

Apart from that it shouldn't be too bad. All that OnLoaded/OnUnloaded need to do
is call OnPrefChanged.

https://codereview.chromium.org/215623002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/error_console/error_console.h (right):

https://codereview.chromium.org/215623002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/error_console/error_console.h:84: // enabled by
default in ADT, but only Dev Channel for chrome:extensoins (or
chrome://extensions

you misspelled extensions.

[Devlin, 2014-03-31 18:39:31]

https://codereview.chromium.org/215623002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/error_console/error_console.cc (right):

https://codereview.chromium.org/215623002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/error_console/error_console.cc:53: :
should_record_(false), default_mask_(kDefaultMask), profile_(profile) {
On 2014/03/28 22:05:32, kalman wrote:
> I thnk that enabled_ was better, sorry.

Haha okay, done.

https://codereview.chromium.org/215623002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/error_console/error_console.cc:166:
kAppsDeveloperToolsExtensionId, ExtensionRegistry::EVERYTHING) != NULL;
On 2014/03/28 22:05:32, kalman wrote:
> Yes I think we had better only do this while the ADT is enabled, and listen
for
> ExtensionRegistry events. The Registry should outlive this class if a
dependency
> is declared on ExtensionSystem, but I suggest making the Registry an explicit
> dependency on this class from the constructor.
> 
> Apart from that it shouldn't be too bad. All that OnLoaded/OnUnloaded need to
do
> is call OnPrefChanged.

ErrorConsole can't directly have dependencies - it's not a KeyedService. But as
long as ExtensionSystem stays dependent on ExtensionRegistry, it'll be fine. 
Implemented ADT enabled checks.

https://codereview.chromium.org/215623002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/error_console/error_console.h (right):

https://codereview.chromium.org/215623002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/error_console/error_console.h:84: // enabled by
default in ADT, but only Dev Channel for chrome:extensoins (or
On 2014/03/28 22:05:32, kalman wrote:
> chrome://extensions
> 
> you misspelled extensions.

Done.

[not at google - send to devlin, 2014-03-31 18:52:43]

lgtm

would be nice to test this logic too, but I'm not sure it's worth it.

if you use ScopedChannelOverride rather than the switch, you could have a test
which additionally scopes it to beta/stable channel and makes sure it's not
enabled.

Better yet would be to install/uninstall the ADT but whatever, only if it's
really easy to do so.

https://codereview.chromium.org/215623002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/error_console/error_console.cc (right):

https://codereview.chromium.org/215623002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/error_console/error_console.cc:52: ExtensionService*
extension_service)
this constructor argument isn't used

https://codereview.chromium.org/215623002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/error_console/error_console.cc:64:
ExtensionRegistry::Get(profile_)->AddObserver(this);
use ScopedObserver so that you don't need to remove:

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

https://codereview.chromium.org/215623002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/error_console/error_console.cc:152:
chrome::VersionInfo::GetChannel() <=
maybe use FeatureChannel::GetCurrentChannel() here instead, since this is
extensions code, and it's easier to test with.

https://codereview.chromium.org/215623002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/error_console/error_console.cc:165:
Enable(ExtensionSystem::Get(profile_)->extension_service());
just let Enable() fetch the ExtensionService from |profile_|.

https://codereview.chromium.org/215623002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/error_console/error_console.cc:187: const
ExtensionSet* extensions = extension_service->extensions();
actually all you need is an ExtensionRegistry, not an ExtensionService

https://codereview.chromium.org/215623002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/error_console/error_console.h (right):

https://codereview.chromium.org/215623002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/error_console/error_console.h:84: // chrome:extensions
page or the Chrome Apps Developer Tools.
nit: put a blank line here (with a //) to make it easier to read. I have a hard
time separating out the real docs from the TODO content.

https://codereview.chromium.org/215623002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/error_console/error_console_browsertest.cc
(right):

https://codereview.chromium.org/215623002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/error_console/error_console_browsertest.cc:207: //
sure (in case a test is running on, e.g., a beta channel).
you can use FeatureChannel::ScopedChannelOverride(..) here instead (well, as a
member field) and make this slightly less wordy. up to you, command line switch
works as well.

[Devlin, 2014-03-31 21:18:23]

Also added in a test, because we love tests.

https://codereview.chromium.org/215623002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/error_console/error_console.cc (right):

https://codereview.chromium.org/215623002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/error_console/error_console.cc:52: ExtensionService*
extension_service)
On 2014/03/31 18:52:43, kalman wrote:
> this constructor argument isn't used

Done.

https://codereview.chromium.org/215623002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/error_console/error_console.cc:64:
ExtensionRegistry::Get(profile_)->AddObserver(this);
On 2014/03/31 18:52:43, kalman wrote:
> use ScopedObserver so that you don't need to remove:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

Done.

https://codereview.chromium.org/215623002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/error_console/error_console.cc:152:
chrome::VersionInfo::GetChannel() <=
On 2014/03/31 18:52:43, kalman wrote:
> maybe use FeatureChannel::GetCurrentChannel() here instead, since this is
> extensions code, and it's easier to test with.

Done.

https://codereview.chromium.org/215623002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/error_console/error_console.cc:165:
Enable(ExtensionSystem::Get(profile_)->extension_service());
On 2014/03/31 18:52:43, kalman wrote:
> just let Enable() fetch the ExtensionService from |profile_|.

Done.

https://codereview.chromium.org/215623002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/error_console/error_console.cc:187: const
ExtensionSet* extensions = extension_service->extensions();
On 2014/03/31 18:52:43, kalman wrote:
> actually all you need is an ExtensionRegistry, not an ExtensionService

Done.

https://codereview.chromium.org/215623002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/error_console/error_console.h (right):

https://codereview.chromium.org/215623002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/error_console/error_console.h:84: // chrome:extensions
page or the Chrome Apps Developer Tools.
On 2014/03/31 18:52:43, kalman wrote:
> nit: put a blank line here (with a //) to make it easier to read. I have a
hard
> time separating out the real docs from the TODO content.

Done.

[Devlin, 2014-03-31 21:18:55]

+Finnur for small changes to settings_handler.cc

[Finnur, 2014-04-01 10:33:37]

LGTM

[Devlin, 2014-04-01 16:08:16]

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

[commit-bot: I haz the power, 2014-04-01 16:08:25]

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

[commit-bot: I haz the power, 2014-04-01 19:23:42]

Change committed as 260922