Make sure sideload wipeout doesn't interfere with the tests.

Make sure sideload wipeout doesn't interfere with the tests.
Make sure it does not run on other platforms besides Windows, where it was intended (for now).
Fix the problem with bubble appearing to the right of the browser.
Make sure the bubble does not appear on next run after explicitly dismissing it.
Enable sideload wipeout by default.

BUG=154624
TEST=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=163705

[Finnur, 2012-10-23 14:28:37]

Scott, can you take the three chrome/browser/ui/views files and Aaron, the rest?

[Aaron Boodman, 2012-10-23 15:20:20]

http://codereview.chromium.org/11232060/diff/1010/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/11232060/diff/1010/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:579: #if defined(OS_WIN)
See other comment about OS_WIN ifdef.

http://codereview.chromium.org/11232060/diff/1010/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:2189: #if !defined(OS_WIN)
Better to chokepoint this in the feature switch itself. See:
http://git.chromium.org/gitweb/?p=chromium.git;a=blob;f=chrome/common/extensi...

http://codereview.chromium.org/11232060/diff/1010/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/disabled_extensions_view.cc (right):

http://codereview.chromium.org/11232060/diff/1010/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/disabled_extensions_view.cc:63: #if
!defined(OS_WIN)
Shouldn't be necessary anymore, right?

http://codereview.chromium.org/11232060/diff/1010/chrome/common/extensions/fe...
File chrome/common/extensions/feature_switch.cc (right):

http://codereview.chromium.org/11232060/diff/1010/chrome/common/extensions/fe...
chrome/common/extensions/feature_switch.cc:36: FeatureSwitch::DEFAULT_ENABLED),
Leave this disabled for now. I'd like to test it myself and talk with Peter
about how to proceed.

[Aaron Boodman, 2012-10-23 15:20:29]

lgtm w/ changes

[Aaron Boodman, 2012-10-23 15:23:36]

(actually there's enough changes here that I should probably see the new patch
before you commit)

http://codereview.chromium.org/11232060/diff/1010/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/disabled_extensions_view.cc (right):

http://codereview.chromium.org/11232060/diff/1010/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/disabled_extensions_view.cc:140:
IntegerPrefMember sideload_wipeout_bubble_shown;
I think that it's only supposed to be shown once, so long as the user interacts
with the bubble in any way - clicking any of the buttons or links.

It only shows multiple times if the user completely ignores it and the browser
closes without any interaction.

[Finnur, 2012-10-23 15:43:10]

http://codereview.chromium.org/11232060/diff/1010/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/disabled_extensions_view.cc (right):

http://codereview.chromium.org/11232060/diff/1010/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/disabled_extensions_view.cc:140:
IntegerPrefMember sideload_wipeout_bubble_shown;
That makes sense for the buttons, but I'm not as sure about the 'Learn more'
link. Pressing the link closes the bubble and takes you to the help center. You
can't go back to the bubble if the help center convinces you that you should go
enable extensions. I don't feel strongly about it though and could be convinced
otherwise. Just want to make sure it is not overlooked.

On 2012/10/23 15:23:38, Aaron Boodman wrote:
> I think that it's only supposed to be shown once, so long as the user
interacts
> with the bubble in any way - clicking any of the buttons or links.
> 
> It only shows multiple times if the user completely ignores it and the browser
> closes without any interaction.

[Finnur, 2012-10-23 16:02:14]

Updated.

[Peter Ludwig, 2012-10-23 16:11:01]

http://codereview.chromium.org/11232060/diff/1010/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/disabled_extensions_view.cc (right):

http://codereview.chromium.org/11232060/diff/1010/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/disabled_extensions_view.cc:140:
IntegerPrefMember sideload_wipeout_bubble_shown;
It's fine for the bubble to dismiss (and to never appear again) after "Learn
more" is clicked.  The help article will describe how to get to the extensions
settings page from the hot dogs menu.

The butterbar should show the first time the user goes to the extension settings
page regardless of if they got there by clicking the "Extension settings" button
on the bubble, or by clicking "OK, great" and then later clicking though the hot
dogs menu to get to the extension settings page.

On 2012/10/23 15:43:10, Finnur wrote:
> That makes sense for the buttons, but I'm not as sure about the 'Learn more'
> link. Pressing the link closes the bubble and takes you to the help center.
You
> can't go back to the bubble if the help center convinces you that you should
go
> enable extensions. I don't feel strongly about it though and could be
convinced
> otherwise. Just want to make sure it is not overlooked.
> 
> On 2012/10/23 15:23:38, Aaron Boodman wrote:
> > I think that it's only supposed to be shown once, so long as the user
> interacts
> > with the bubble in any way - clicking any of the buttons or links.
> > 
> > It only shows multiple times if the user completely ignores it and the
browser
> > closes without any interaction.
>

[Aaron Boodman, 2012-10-23 16:18:06]

http://codereview.chromium.org/11232060/diff/2006/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/disabled_extensions_view.cc (right):

http://codereview.chromium.org/11232060/diff/2006/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/disabled_extensions_view.cc:73:
sideload_wipeout_bubble_shown.Init(
Since this is used in multiple methods and is low-cost, it makes sense to define
as a member of the class so that you don't have to repeat its definition and
initialization.

http://codereview.chromium.org/11232060/diff/2006/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/disabled_extensions_view.cc:145:
DontShowBubbleAgain();
Per Peter's comment, this should go in LinkClicked() too, right?

[Finnur, 2012-10-23 16:55:44]

Updated.

http://codereview.chromium.org/11232060/diff/1010/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/disabled_extensions_view.cc (right):

http://codereview.chromium.org/11232060/diff/1010/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/disabled_extensions_view.cc:140:
IntegerPrefMember sideload_wipeout_bubble_shown;
Sounds good.

> The butterbar should show the first time the user goes to the extension
settings
> page regardless of if they got there by clicking the "Extension settings"
button
> on the bubble, or by clicking "OK, great" and then later clicking though the
hot
> dogs menu to get to the extension settings page.

Yup, that's how it works today.

http://codereview.chromium.org/11232060/diff/2006/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/disabled_extensions_view.cc (right):

http://codereview.chromium.org/11232060/diff/2006/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/disabled_extensions_view.cc:73:
sideload_wipeout_bubble_shown.Init(
This function is static and this (would be) member is not one of the simple
types.

On 2012/10/23 16:18:06, Aaron Boodman wrote:
> Since this is used in multiple methods and is low-cost, it makes sense to
define
> as a member of the class so that you don't have to repeat its definition and
> initialization.

http://codereview.chromium.org/11232060/diff/2006/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/disabled_extensions_view.cc:145:
DontShowBubbleAgain();
Done.

On 2012/10/23 16:18:06, Aaron Boodman wrote:
> Per Peter's comment, this should go in LinkClicked() too, right?

[sky, 2012-10-23 16:57:09]

http://codereview.chromium.org/11232060/diff/10013/chrome/browser/ui/views/to...
File chrome/browser/ui/views/toolbar_view.cc (right):

http://codereview.chromium.org/11232060/diff/10013/chrome/browser/ui/views/to...
chrome/browser/ui/views/toolbar_view.cc:325: MessageLoop::current()->PostTask(
Why do you need to delay this? I would rather you wait for a specific event then
show rather than the delayed task which can have all sorts of timing issues.

[Finnur, 2012-10-23 17:04:59]

I had problems getting the bubble to anchor correctly, I think it is a race
condition because the toolbar is being shown and in that process the bubble is
being shown, anchored to it. If the bubble was first, then it wouldn't be
anchored correctly.

What event are you thinking of? I tried to do this in ViewHierarchyChanged when
the Toolbar view is added, but that didn't work.

On 2012/10/23 16:57:09, sky wrote:
>
http://codereview.chromium.org/11232060/diff/10013/chrome/browser/ui/views/to...
> File chrome/browser/ui/views/toolbar_view.cc (right):
> 
>
http://codereview.chromium.org/11232060/diff/10013/chrome/browser/ui/views/to...
> chrome/browser/ui/views/toolbar_view.cc:325: MessageLoop::current()->PostTask(
> Why do you need to delay this? I would rather you wait for a specific event
then
> show rather than the delayed task which can have all sorts of timing issues.

[sky, 2012-10-23 17:15:24]

Does WidgetObserver::OnWidgetVisibilityChanged do what you need?

  -Scott

On Tue, Oct 23, 2012 at 10:04 AM,  <finnur@chromium.org> wrote:
> I had problems getting the bubble to anchor correctly, I think it is a race
> condition because the toolbar is being shown and in that process the bubble
> is
> being shown, anchored to it. If the bubble was first, then it wouldn't be
> anchored correctly.
>
> What event are you thinking of? I tried to do this in ViewHierarchyChanged
> when
> the Toolbar view is added, but that didn't work.
>
>
> On 2012/10/23 16:57:09, sky wrote:
>
>
http://codereview.chromium.org/11232060/diff/10013/chrome/browser/ui/views/to...
>>
>> File chrome/browser/ui/views/toolbar_view.cc (right):
>
>
>
>
http://codereview.chromium.org/11232060/diff/10013/chrome/browser/ui/views/to...
>>
>> chrome/browser/ui/views/toolbar_view.cc:325:
>> MessageLoop::current()->PostTask(
>> Why do you need to delay this? I would rather you wait for a specific
>> event
>
> then
>>
>> show rather than the delayed task which can have all sorts of timing
>> issues.
>
>
>
>
> http://codereview.chromium.org/11232060/

[Finnur, 2012-10-23 17:16:56]

I'll give it a shot. 

On 2012/10/23 17:15:24, sky wrote:
> Does WidgetObserver::OnWidgetVisibilityChanged do what you need?
> 
>   -Scott
> 
> On Tue, Oct 23, 2012 at 10:04 AM,  <mailto:finnur@chromium.org> wrote:
> > I had problems getting the bubble to anchor correctly, I think it is a race
> > condition because the toolbar is being shown and in that process the bubble
> > is
> > being shown, anchored to it. If the bubble was first, then it wouldn't be
> > anchored correctly.
> >
> > What event are you thinking of? I tried to do this in ViewHierarchyChanged
> > when
> > the Toolbar view is added, but that didn't work.
> >
> >
> > On 2012/10/23 16:57:09, sky wrote:
> >
> >
>
http://codereview.chromium.org/11232060/diff/10013/chrome/browser/ui/views/to...
> >>
> >> File chrome/browser/ui/views/toolbar_view.cc (right):
> >
> >
> >
> >
>
http://codereview.chromium.org/11232060/diff/10013/chrome/browser/ui/views/to...
> >>
> >> chrome/browser/ui/views/toolbar_view.cc:325:
> >> MessageLoop::current()->PostTask(
> >> Why do you need to delay this? I would rather you wait for a specific
> >> event
> >
> > then
> >>
> >> show rather than the delayed task which can have all sorts of timing
> >> issues.
> >
> >
> >
> >
> > http://codereview.chromium.org/11232060/

[Finnur, 2012-10-23 17:48:32]

That seems like what I was looking for. Updated. PTAL.

[Aaron Boodman, 2012-10-23 18:13:41]

lgtm

[sky, 2012-10-23 19:46:13]

http://codereview.chromium.org/11232060/diff/4009/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar_view.cc (right):

http://codereview.chromium.org/11232060/diff/4009/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar_view.cc:227: GetWidget()->AddObserver(this);
I get nervous when there is an add with no corresponding remove. Is it possible
to remove this?

[sky, 2012-10-23 19:46:59]

http://codereview.chromium.org/11232060/diff/4009/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar_view.cc (right):

http://codereview.chromium.org/11232060/diff/4009/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar_view.cc:227: GetWidget()->AddObserver(this);
On 2012/10/23 19:46:14, sky wrote:
> I get nervous when there is an add with no corresponding remove. Is it
possible
> to remove this?

Sorry, this comment is a bit vague. I specifically mean you invoke
REmoveObserver(this) at some point?

[Finnur, 2012-10-23 21:37:30]

> I get nervous when there is an add with no corresponding remove.

And you should. Good catch! Fixed.

[sky, 2012-10-23 22:10:29]

LGTM

[Aaron Boodman, 2012-10-23 22:21:14]

lgtm