Enable extensions error button test that was failing on stable.

Enable extensions error button test that was failing on stable.

Error buttons should not be visible in stable.
Error buttons should be visible in dev.

BUG=463245
Committed: https://crrev.com/cfaf5dcfb0371f395a9b8cfb9c835f6690e0ecbd
Cr-Commit-Position: refs/heads/master@{#329465}

[hcarmona, 2015-05-07 21:18:37]

hcarmona@chromium.org changed reviewers:
+ dbeam@chromium.org, kalman@chromium.org, rdevlin.cronin@chromium.org

[hcarmona, 2015-05-07 21:18:37]

PTAL

Updated existing test + added new one.

[Devlin, 2015-05-07 21:33:56]

Thanks for getting to the bottom of this! :)

https://codereview.chromium.org/1132493004/diff/1/chrome/browser/ui/webui/ext...
File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js
(right):

https://codereview.chromium.org/1132493004/diff/1/chrome/browser/ui/webui/ext...
chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:337: //    
This means there will be a single error button.
This seems weird to me.  The number and type of errors doesn't affect whether or
not the errors link shows (it shows with > 0 of either type).  So I think we can
remove this comment (and it's sibling above).  Also, it'd be good to double
check that the errors link shows up on the right extension (i.e., not the "good"
one).

For extra bonus points, we could check that the class on the icon is the "warn"
one, since we have log and warn level errors.  (You don't have to, but if it's
an easy fix - I think it should be - it could be nice.)

[Dan Beam, 2015-05-07 21:47:34]

https://codereview.chromium.org/1132493004/diff/1/chrome/browser/ui/webui/ext...
File chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc
(right):

https://codereview.chromium.org/1132493004/diff/1/chrome/browser/ui/webui/ext...
chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc:97:
chrome::VersionInfo::CHANNEL_DEV));
i don't like doing these things because it changes the browser's channel in the
middle of a test...

can't we just do the typical test-only hacking where we make a method virtual
and derive it for testing, e.g. 

real_thing.cc

  class RealThing {
   public:
    virtual bool CanThisDoSomething() { return false; }
  };

real_thing_test.cc

  class TestThing : public RealThing {
   public:
    bool CanThisDoSomething() override { return true; }
  };

or just make a RealThing::OverrideForTesting() (and allow only the test to
access it, preferably).

[Devlin, 2015-05-07 21:53:36]

https://codereview.chromium.org/1132493004/diff/1/chrome/browser/ui/webui/ext...
File chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc
(right):

https://codereview.chromium.org/1132493004/diff/1/chrome/browser/ui/webui/ext...
chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc:97:
chrome::VersionInfo::CHANNEL_DEV));
On 2015/05/07 21:47:34, Dan Beam wrote:
> i don't like doing these things because it changes the browser's channel in
the
> middle of a test...
> 
> can't we just do the typical test-only hacking where we make a method virtual
> and derive it for testing, e.g. 
> 
> real_thing.cc
> 
>   class RealThing {
>    public:
>     virtual bool CanThisDoSomething() { return false; }
>   };
> 
> real_thing_test.cc
> 
>   class TestThing : public RealThing {
>    public:
>     bool CanThisDoSomething() override { return true; }
>   };
> 
> or just make a RealThing::OverrideForTesting() (and allow only the test to
> access it, preferably).

I'm not super keen on the channels either, but I don't think we need a virtual
class here.  Instead, what about just having ErrorConsole check for if the
feature switch is deliberately disabled (i.e.,
FeatureSwitch::error_console()->GetOverrideValue() == OVERRIDE_DISABLED).

[hcarmona, 2015-05-07 22:49:53]

https://codereview.chromium.org/1132493004/diff/1/chrome/browser/ui/webui/ext...
File chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc
(right):

https://codereview.chromium.org/1132493004/diff/1/chrome/browser/ui/webui/ext...
chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc:97:
chrome::VersionInfo::CHANNEL_DEV));
On 2015/05/07 21:53:36, Devlin wrote:
> On 2015/05/07 21:47:34, Dan Beam wrote:
> > i don't like doing these things because it changes the browser's channel in
> the
> > middle of a test...
> > 
> > can't we just do the typical test-only hacking where we make a method
virtual
> > and derive it for testing, e.g. 
> > 
> > real_thing.cc
> > 
> >   class RealThing {
> >    public:
> >     virtual bool CanThisDoSomething() { return false; }
> >   };
> > 
> > real_thing_test.cc
> > 
> >   class TestThing : public RealThing {
> >    public:
> >     bool CanThisDoSomething() override { return true; }
> >   };
> > 
> > or just make a RealThing::OverrideForTesting() (and allow only the test to
> > access it, preferably).
> 
> I'm not super keen on the channels either, but I don't think we need a virtual
> class here.  Instead, what about just having ErrorConsole check for if the
> feature switch is deliberately disabled (i.e.,
> FeatureSwitch::error_console()->GetOverrideValue() == OVERRIDE_DISABLED).

Updated the ErrorConsole check and removed the channel override.

https://codereview.chromium.org/1132493004/diff/1/chrome/browser/ui/webui/ext...
File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js
(right):

https://codereview.chromium.org/1132493004/diff/1/chrome/browser/ui/webui/ext...
chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:337: //    
This means there will be a single error button.
On 2015/05/07 21:33:56, Devlin wrote:
> This seems weird to me.  The number and type of errors doesn't affect whether
or
> not the errors link shows (it shows with > 0 of either type).  So I think we
can
> remove this comment (and it's sibling above).  Also, it'd be good to double
> check that the errors link shows up on the right extension (i.e., not the
"good"
> one).
> 
> For extra bonus points, we could check that the class on the icon is the
"warn"
> one, since we have log and warn level errors.  (You don't have to, but if it's
> an easy fix - I think it should be - it could be nice.)

Comments were getting confusing: removed.
Updated test with additional checks for visible link.

[Devlin, 2015-05-07 23:04:14]

lgtm

https://codereview.chromium.org/1132493004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/error_console/error_console.cc (right):

https://codereview.chromium.org/1132493004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/error_console/error_console.cc:172: FeatureSwitch*
error_console = FeatureSwitch::error_console();
I think this boolean result is right, but it's hard to read. :)

Let's do:
if (error_console->GetOverrideValue() == FeatureSwitch::OVERRIDE_DISABLED)
  return false;  // Explicitly disabled for testing.
if (!profile_->GetPrefs()->GetBoolean(prefs::kExtensionsUIDeveloperMode)
  return false;  // Only enabled in developer mode.
if (GetCurrentChannel() > chrome::VersionInfo::CHANNEL_DEV &&
    !error_console->IsEnabled())
  return false;  // Restricted to dev channel or opt-in.

return true;

It's a few more lines, but I think the readability is worthwhile.

[Dan Beam, 2015-05-07 23:08:45]

lgtm

https://codereview.chromium.org/1132493004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/error_console/error_console.cc (right):

https://codereview.chromium.org/1132493004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/error_console/error_console.cc:172: FeatureSwitch*
error_console = FeatureSwitch::error_console();
On 2015/05/07 23:04:14, Devlin wrote:
> I think this boolean result is right, but it's hard to read. :)
> 
> Let's do:
> if (error_console->GetOverrideValue() == FeatureSwitch::OVERRIDE_DISABLED)
>   return false;  // Explicitly disabled for testing.
> if (!profile_->GetPrefs()->GetBoolean(prefs::kExtensionsUIDeveloperMode)
>   return false;  // Only enabled in developer mode.
> if (GetCurrentChannel() > chrome::VersionInfo::CHANNEL_DEV &&
>     !error_console->IsEnabled())
>   return false;  // Restricted to dev channel or opt-in.
> 
> return true;
> 
> It's a few more lines, but I think the readability is worthwhile.

+10

[hcarmona, 2015-05-08 21:01:51]

Removed test to make sure that the buttons are hidden. This is tested by
ErrorConsoleUnitTest.EnableAndDisableErrorConsole (which breaks if we
force the error console off).

[Dan Beam, 2015-05-12 01:34:13]

https://codereview.chromium.org/1132493004/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/error_console/error_console.cc (right):

https://codereview.chromium.org/1132493004/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/error_console/error_console.cc:172: FeatureSwitch*
error_console = FeatureSwitch::error_console();
put |error_console| as close as possible to its use

https://codereview.chromium.org/1132493004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_settings_browsertest.h
(right):

https://codereview.chromium.org/1132493004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_settings_browsertest.h:45: void
EnableErrorConsole();
set a better standard than the rest of us and put a doc comment here

https://codereview.chromium.org/1132493004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js
(right):

https://codereview.chromium.org/1132493004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:11: var
GOOD_EXTENSION_ID = 'ldnnhddmnhbkjipkidpdiheffobcpfmf';
\n

https://codereview.chromium.org/1132493004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:283: 
why the \n?

[hcarmona, 2015-05-12 17:20:13]

Applied feedback.

https://codereview.chromium.org/1132493004/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/error_console/error_console.cc (right):

https://codereview.chromium.org/1132493004/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/error_console/error_console.cc:172: FeatureSwitch*
error_console = FeatureSwitch::error_console();
On 2015/05/12 01:34:13, Dan Beam wrote:
> put |error_console| as close as possible to its use

Done.

https://codereview.chromium.org/1132493004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_settings_browsertest.h
(right):

https://codereview.chromium.org/1132493004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_settings_browsertest.h:45: void
EnableErrorConsole();
On 2015/05/12 01:34:13, Dan Beam wrote:
> set a better standard than the rest of us and put a doc comment here

Done.

https://codereview.chromium.org/1132493004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js
(right):

https://codereview.chromium.org/1132493004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:11: var
GOOD_EXTENSION_ID = 'ldnnhddmnhbkjipkidpdiheffobcpfmf';
On 2015/05/12 01:34:13, Dan Beam wrote:
> \n

Done.

https://codereview.chromium.org/1132493004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:283: 
On 2015/05/12 01:34:13, Dan Beam wrote:
> why the \n?

It was to separate enabling the error console and installing the
extensions. Not really necessary. Removed.

[Dan Beam, 2015-05-12 17:52:21]

lgtm

[hcarmona, 2015-05-12 17:55:07]

The CQ bit was checked by hcarmona@chromium.org

[hcarmona, 2015-05-12 17:55:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1132493004/#ps100001
(title: "Apply feedback")

[commit-bot: I haz the power, 2015-05-12 17:55:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1132493004/100001

[commit-bot: I haz the power, 2015-05-12 19:51:50]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-12 19:53:39]

Patchset 6 (id:??) landed as
https://crrev.com/cfaf5dcfb0371f395a9b8cfb9c835f6690e0ecbd
Cr-Commit-Position: refs/heads/master@{#329465}