Make extensions developer mode warning controllable by experiments

Make extensions developer mode warning controllable by experiments

Before this patch, the warning was only shown on windows stable/beta
channels. This patch changes that so it can be enabled on mac as well,
and whether it is shown or not can be controlled by our server-side
experiment framework.

BUG=485300
Committed: https://crrev.com/a772c58af2ebac3670c3b5e1cb13df883863a335
Cr-Commit-Position: refs/heads/master@{#329002}

[asargent_no_longer_on_chrome, 2015-05-06 21:13:53]

asargent@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Devlin, 2015-05-06 21:21:32]

https://codereview.chromium.org/1125353002/diff/1/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/extension_message_bubble_factory.cc (right):

https://codereview.chromium.org/1125353002/diff/1/chrome/browser/ui/extension...
chrome/browser/ui/extensions/extension_message_bubble_factory.cc:26: #if
defined(OS_WIN) || defined(OS_MACOSX)
This will enable all of these bubbles, not just the dev mode warning one.

I wonder if it would be better to have a centralized experiment for all of them,
since I think we're discussing having them all on mac anyway.  But I'm not sure
how that would work with turning on a subset.

[asargent_no_longer_on_chrome, 2015-05-06 23:41:25]

https://codereview.chromium.org/1125353002/diff/1/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/extension_message_bubble_factory.cc (right):

https://codereview.chromium.org/1125353002/diff/1/chrome/browser/ui/extension...
chrome/browser/ui/extensions/extension_message_bubble_factory.cc:26: #if
defined(OS_WIN) || defined(OS_MACOSX)
On 2015/05/06 21:21:32, Devlin wrote:
> This will enable all of these bubbles, not just the dev mode warning one.
> 
> I wonder if it would be better to have a centralized experiment for all of
them,
> since I think we're discussing having them all on mac anyway.  But I'm not
sure
> how that would work with turning on a subset.

Jake and I touched base today and I mentioned this, and his take was that this
was ok.

We definitely want the SuspiciousExtensionBubbleController turned on, because
that's important UI for the webstore lockdown (it's what tells you about any
non-webstore extensions that have been disabled). The settings API is currently
only available on windows, so for now that bubble can't display for mac, but if
we bring the API to mac (I suggested to Jake that we should consider doing that
in the near future), we'd probably want the same warnings as windows. That just
leaves the proxy override warning one, and the same reasoning applies to mac as
windows for why we'd want to warn users. 

Does this seem sufficient to you for us to move forward? Or should we start an
email thread with Jake to discuss further?

[Devlin, 2015-05-06 23:51:56]

https://codereview.chromium.org/1125353002/diff/1/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/extension_message_bubble_factory.cc (right):

https://codereview.chromium.org/1125353002/diff/1/chrome/browser/ui/extension...
chrome/browser/ui/extensions/extension_message_bubble_factory.cc:26: #if
defined(OS_WIN) || defined(OS_MACOSX)
On 2015/05/06 23:41:24, Antony Sargent wrote:
> On 2015/05/06 21:21:32, Devlin wrote:
> > This will enable all of these bubbles, not just the dev mode warning one.
> > 
> > I wonder if it would be better to have a centralized experiment for all of
> them,
> > since I think we're discussing having them all on mac anyway.  But I'm not
> sure
> > how that would work with turning on a subset.
> 
> Jake and I touched base today and I mentioned this, and his take was that this
> was ok.
> 
> We definitely want the SuspiciousExtensionBubbleController turned on, because
> that's important UI for the webstore lockdown (it's what tells you about any
> non-webstore extensions that have been disabled). The settings API is
currently
> only available on windows, so for now that bubble can't display for mac, but
if
> we bring the API to mac (I suggested to Jake that we should consider doing
that
> in the near future), we'd probably want the same warnings as windows. That
just
> leaves the proxy override warning one, and the same reasoning applies to mac
as
> windows for why we'd want to warn users. 
> 
> Does this seem sufficient to you for us to move forward? Or should we start an
> email thread with Jake to discuss further? 
> 

In principle, I'm fine with launching all of these.  But I do think it's a
little weird that we put the dev mode behind an experiment, but just turn on the
others to 100% - especially since the suspicious bubble controller's incarnation
on mac hasn't gone through launch/UI review, AFAIK.

Could we put them all in one user-protection-bubble experiment?  Then we could
dial it up when we're sure they're ready?

Also, IIRC, I tested out the suspicious bubble on mac, and it *mostly* worked,
but there was a line or two I had to change.  I might have done that as part of
a separate CL, but I'm not 100% sure - I can check tomorrow if there are any
changes that need to happen in that respect.

[Devlin, 2015-05-07 15:44:17]

https://codereview.chromium.org/1125353002/diff/1/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/extension_message_bubble_factory.cc (right):

https://codereview.chromium.org/1125353002/diff/1/chrome/browser/ui/extension...
chrome/browser/ui/extensions/extension_message_bubble_factory.cc:26: #if
defined(OS_WIN) || defined(OS_MACOSX)
On 2015/05/06 23:51:56, Devlin wrote:
> On 2015/05/06 23:41:24, Antony Sargent wrote:
> > On 2015/05/06 21:21:32, Devlin wrote:
> > > This will enable all of these bubbles, not just the dev mode warning one.
> > > 
> > > I wonder if it would be better to have a centralized experiment for all of
> > them,
> > > since I think we're discussing having them all on mac anyway.  But I'm not
> > sure
> > > how that would work with turning on a subset.
> > 
> > Jake and I touched base today and I mentioned this, and his take was that
this
> > was ok.
> > 
> > We definitely want the SuspiciousExtensionBubbleController turned on,
because
> > that's important UI for the webstore lockdown (it's what tells you about any
> > non-webstore extensions that have been disabled). The settings API is
> currently
> > only available on windows, so for now that bubble can't display for mac, but
> if
> > we bring the API to mac (I suggested to Jake that we should consider doing
> that
> > in the near future), we'd probably want the same warnings as windows. That
> just
> > leaves the proxy override warning one, and the same reasoning applies to mac
> as
> > windows for why we'd want to warn users. 
> > 
> > Does this seem sufficient to you for us to move forward? Or should we start
an
> > email thread with Jake to discuss further? 
> > 
> 
> In principle, I'm fine with launching all of these.  But I do think it's a
> little weird that we put the dev mode behind an experiment, but just turn on
the
> others to 100% - especially since the suspicious bubble controller's
incarnation
> on mac hasn't gone through launch/UI review, AFAIK.
> 
> Could we put them all in one user-protection-bubble experiment?  Then we could
> dial it up when we're sure they're ready?
> 
> Also, IIRC, I tested out the suspicious bubble on mac, and it *mostly* worked,
> but there was a line or two I had to change.  I might have done that as part
of
> a separate CL, but I'm not 100% sure - I can check tomorrow if there are any
> changes that need to happen in that respect.

Just confirmed that the suspicious bubble does WAI.  Still have the same
concerns re launch/ui review, but at least I'm pretty sure this won't cause epic
crashes. :)

[asargent_no_longer_on_chrome, 2015-05-07 19:10:20]

On 2015/05/06 23:51:56, Devlin wrote:
>
https://codereview.chromium.org/1125353002/diff/1/chrome/browser/ui/extension...
> File chrome/browser/ui/extensions/extension_message_bubble_factory.cc (right):
> 
>
https://codereview.chromium.org/1125353002/diff/1/chrome/browser/ui/extension...
> chrome/browser/ui/extensions/extension_message_bubble_factory.cc:26: #if
> defined(OS_WIN) || defined(OS_MACOSX)
> On 2015/05/06 23:41:24, Antony Sargent wrote:
> > On 2015/05/06 21:21:32, Devlin wrote:
> > > This will enable all of these bubbles, not just the dev mode warning one.
> > > 
> > > I wonder if it would be better to have a centralized experiment for all of
> > them,
> > > since I think we're discussing having them all on mac anyway.  But I'm not
> > sure
> > > how that would work with turning on a subset.
> > 
> > Jake and I touched base today and I mentioned this, and his take was that
this
> > was ok.
> > 
> > We definitely want the SuspiciousExtensionBubbleController turned on,
because
> > that's important UI for the webstore lockdown (it's what tells you about any
> > non-webstore extensions that have been disabled). The settings API is
> currently
> > only available on windows, so for now that bubble can't display for mac, but
> if
> > we bring the API to mac (I suggested to Jake that we should consider doing
> that
> > in the near future), we'd probably want the same warnings as windows. That
> just
> > leaves the proxy override warning one, and the same reasoning applies to mac
> as
> > windows for why we'd want to warn users. 
> > 
> > Does this seem sufficient to you for us to move forward? Or should we start
an
> > email thread with Jake to discuss further? 
> > 
> 
> In principle, I'm fine with launching all of these.  But I do think it's a
> little weird that we put the dev mode behind an experiment, but just turn on
the
> others to 100% - especially since the suspicious bubble controller's
incarnation
> on mac hasn't gone through launch/UI review, AFAIK.
> 
> Could we put them all in one user-protection-bubble experiment?  Then we could
> dial it up when we're sure they're ready?
> 
> Also, IIRC, I tested out the suspicious bubble on mac, and it *mostly* worked,
> but there was a line or two I had to change.  I might have done that as part
of
> a separate CL, but I'm not 100% sure - I can check tomorrow if there are any
> changes that need to happen in that respect.

Ok, here's what I think makes sense to do:

-Make the suspicious bubble dependent on whether the webstore lockdown is turned
on or not. It seems like it would be pretty bad to have the lockdown turned on
with no UI to tell users when a non-webstore extension was disabled.

-Keep the experiment for the dev mode extensions bubble (but also make it show
if the lockdown is turned on, since we never want the lockdown without the dev
mode warning)

-Add an experiment for the proxy override dialog.

I'll see if I can double-check this with Jake

[asargent_no_longer_on_chrome, 2015-05-07 23:34:39]

Ok, new version up, PTAL.

One thing I did differently after talking with Jake was leaving the dev mode
extensions warning as a separate experiment and *not* tying it to whether the
webstore lockdown is turned on. His take was that there is a slim chance we'd
actually want to control that independently.

[Devlin, 2015-05-08 16:06:35]

lgtm

https://codereview.chromium.org/1125353002/diff/20001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/extension_message_bubble_factory.cc (right):

https://codereview.chromium.org/1125353002/diff/20001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_message_bubble_factory.cc:30: bool
g_enabled = false;
Since this is only ever used in testing now, it should probably be renamed
g_enabled_for_testing.

[asargent_no_longer_on_chrome, 2015-05-08 16:29:48]

https://codereview.chromium.org/1125353002/diff/20001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/extension_message_bubble_factory.cc (right):

https://codereview.chromium.org/1125353002/diff/20001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_message_bubble_factory.cc:30: bool
g_enabled = false;
On 2015/05/08 16:06:35, Devlin wrote:
> Since this is only ever used in testing now, it should probably be renamed
> g_enabled_for_testing.

Done. I used 'g_enabled_for_tests' because that is a few characters shorter and
so helps prevent global warming.

[asargent_no_longer_on_chrome, 2015-05-08 16:30:17]

The CQ bit was checked by asargent@chromium.org

[asargent_no_longer_on_chrome, 2015-05-08 16:30:17]

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/1125353002/#ps40001
(title: "s/g_enabled/g_enabled_for_tests/")

[commit-bot: I haz the power, 2015-05-08 16:32:33]

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

[commit-bot: I haz the power, 2015-05-08 17:14:53]

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

[commit-bot: I haz the power, 2015-05-08 17:14:55]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[asargent_no_longer_on_chrome, 2015-05-08 17:59:48]

The CQ bit was checked by asargent@chromium.org

[asargent_no_longer_on_chrome, 2015-05-08 17:59:48]

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/1125353002/#ps80001
(title: "fix win compile problem with unreached code")

[commit-bot: I haz the power, 2015-05-08 18:01:08]

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

[commit-bot: I haz the power, 2015-05-08 18:59:10]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-08 19:00:50]

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