move chromeos bubble setup code to window.cc

move chromeos bubble setup code to window.cc, still under browser.
This code is also in the original bubble code.

BUG=98322
TEST=None

[alicet1, 2011-11-20 04:15:57]

hey ben, I noticed the TODO(alicet) to move this code was removed from the
setting_level_bubble.cc code, I think this is needed for chromeos bubbles, not
just the settings bubble.

thanx,
alice

[msw, 2011-11-20 10:30:43]

Drive by; please set BUG=98322.

http://codereview.chromium.org/8604012/diff/1/ui/views/bubble/bubble_delegate.cc
File ui/views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/8604012/diff/1/ui/views/bubble/bubble_delegate...
ui/views/bubble/bubble_delegate.cc:40:
params.push_back(bubble->show_while_screen_is_locked());
Do we still need to call SetWindowType when
(!bubble->show_while_screen_is_locked())?

http://codereview.chromium.org/8604012/diff/1/ui/views/bubble/bubble_delegate...
ui/views/bubble/bubble_delegate.cc:117: use_focusless_(false) {
Initialize show_while_screen_is_locked_(false) here too.

http://codereview.chromium.org/8604012/diff/1/ui/views/bubble/bubble_delegate.h
File ui/views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/8604012/diff/1/ui/views/bubble/bubble_delegate...
ui/views/bubble/bubble_delegate.h:155: // Whether to show this bubble when
screen is locked.
Add "on ChromeOS."?

[alicet1, 2011-11-20 16:21:45]

problem is, SetWindowType seems to be under chrome/browser/chromeos, so
ui/views/bubble cannot depend on that.

ben, is this code needed for chromeos bubbles?

thanx,
alice

http://codereview.chromium.org/8604012/diff/1/ui/views/bubble/bubble_delegate.cc
File ui/views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/8604012/diff/1/ui/views/bubble/bubble_delegate...
ui/views/bubble/bubble_delegate.cc:40:
params.push_back(bubble->show_while_screen_is_locked());
On 2011/11/20 10:30:44, msw wrote:
> Do we still need to call SetWindowType when
> (!bubble->show_while_screen_is_locked())?

the old code does.

http://codereview.chromium.org/8604012/diff/1/ui/views/bubble/bubble_delegate...
ui/views/bubble/bubble_delegate.cc:117: use_focusless_(false) {
On 2011/11/20 10:30:44, msw wrote:
> Initialize show_while_screen_is_locked_(false) here too.

Done.

http://codereview.chromium.org/8604012/diff/1/ui/views/bubble/bubble_delegate.h
File ui/views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/8604012/diff/1/ui/views/bubble/bubble_delegate...
ui/views/bubble/bubble_delegate.h:155: // Whether to show this bubble when
screen is locked.
On 2011/11/20 10:30:44, msw wrote:
> Add "on ChromeOS."?

Done.

[Ben Goodger (Google), 2011-11-21 16:53:37]

The SetWindowType call is most likely essential to prevent regressions on
existing Cros.

derat can tell you more about the specific types and what they're for.

-Ben

On Sun, Nov 20, 2011 at 8:21 AM, <alicet@chromium.org> wrote:

> problem is, SetWindowType seems to be under chrome/browser/chromeos, so
> ui/views/bubble cannot depend on that.
>
> ben, is this code needed for chromeos bubbles?
>
> thanx,
> alice
>
>
>
> http://codereview.chromium.**org/8604012/diff/1/ui/views/**
>
bubble/bubble_delegate.cc<http://codereview.chromium.org/8604012/diff/1/ui/views/bubble/bubble_delegate.cc>
> File ui/views/bubble/bubble_**delegate.cc (right):
>
> http://codereview.chromium.**org/8604012/diff/1/ui/views/**
>
bubble/bubble_delegate.cc#**newcode40<http://codereview.chromium.org/8604012/diff/1/ui/views/bubble/bubble_delegate.cc#newcode40>
> ui/views/bubble/bubble_**delegate.cc:40:
> params.push_back(bubble->show_**while_screen_is_locked());
> On 2011/11/20 10:30:44, msw wrote:
>
>> Do we still need to call SetWindowType when
>> (!bubble->show_while_screen_**is_locked())?
>>
>
> the old code does.
>
>
> http://codereview.chromium.**org/8604012/diff/1/ui/views/**
>
bubble/bubble_delegate.cc#**newcode117<http://codereview.chromium.org/8604012/diff/1/ui/views/bubble/bubble_delegate.cc#newcode117>
> ui/views/bubble/bubble_**delegate.cc:117: use_focusless_(false) {
> On 2011/11/20 10:30:44, msw wrote:
>
>> Initialize show_while_screen_is_locked_(**false) here too.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8604012/diff/1/ui/views/**
>
bubble/bubble_delegate.h<http://codereview.chromium.org/8604012/diff/1/ui/views/bubble/bubble_delegate.h>
> File ui/views/bubble/bubble_**delegate.h (right):
>
> http://codereview.chromium.**org/8604012/diff/1/ui/views/**
>
bubble/bubble_delegate.h#**newcode155<http://codereview.chromium.org/8604012/diff/1/ui/views/bubble/bubble_delegate.h#newcode155>
> ui/views/bubble/bubble_**delegate.h:155: // Whether to show this bubble
> when screen is locked.
> On 2011/11/20 10:30:44, msw wrote:
>
>> Add "on ChromeOS."?
>>
>
> Done.
>
>
http://codereview.chromium.**org/8604012/<http://codereview.chromium.org/8604...
>

[Daniel Erat, 2011-11-21 16:59:08]

Yes, you need to set the window type regardless of whether the bubble should be
shown while the screen is locked or not.

LGTM apart from a few nits.

http://codereview.chromium.org/8604012/diff/2002/ui/views/bubble/bubble_deleg...
File ui/views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/8604012/diff/2002/ui/views/bubble/bubble_deleg...
ui/views/bubble/bubble_delegate.cc:5: #include <vector>
bubble_delegate.h should be included before anything else

http://codereview.chromium.org/8604012/diff/2002/ui/views/bubble/bubble_deleg...
File ui/views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/8604012/diff/2002/ui/views/bubble/bubble_deleg...
ui/views/bubble/bubble_delegate.h:69: bool show_while_screen_is_locked() const {
#if defined(OS_CHROMEOS)

[alicet1, 2011-11-21 20:58:44]

hi dan,

updated with the wm_ipc methods move. I don't know where is a good place to put
this, can you suggest?

thanx,
alice

http://codereview.chromium.org/8604012/diff/2002/ui/views/bubble/bubble_deleg...
File ui/views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/8604012/diff/2002/ui/views/bubble/bubble_deleg...
ui/views/bubble/bubble_delegate.cc:5: #include <vector>
On 2011/11/21 16:59:09, Daniel Erat wrote:
> bubble_delegate.h should be included before anything else

Done.

http://codereview.chromium.org/8604012/diff/2002/ui/views/bubble/bubble_deleg...
File ui/views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/8604012/diff/2002/ui/views/bubble/bubble_deleg...
ui/views/bubble/bubble_delegate.h:69: bool show_while_screen_is_locked() const {
On 2011/11/21 16:59:09, Daniel Erat wrote:
> #if defined(OS_CHROMEOS)

Done.

[Daniel Erat, 2011-11-21 21:15:57]

Instead of moving a bunch of code around, could you add a virtual method to
BubbleDelegateView allowing the delegate to do initialization of its own?

http://codereview.chromium.org/8604012/diff/14007/ui/views/bubble/bubble_dele...
File ui/views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/8604012/diff/14007/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:69: #if defined(OS_CHROMEOS)
to be clear, i meant that everything screen-lock-related should be ifdef-ed so
it's only visible on chrome os

[alicet1, 2011-11-21 23:01:20]

updated after offline chat.

msw: you should take a look also, I'll need to make all the browser bubbles use
this new stepping function instead.

thanx,
alice

http://codereview.chromium.org/8604012/diff/14007/ui/views/bubble/bubble_dele...
File ui/views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/8604012/diff/14007/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:69: #if defined(OS_CHROMEOS)
On 2011/11/21 21:15:57, Daniel Erat wrote:
> to be clear, i meant that everything screen-lock-related should be ifdef-ed so
> it's only visible on chrome os

I left the member variable there, as it is rather ugly to add the ifdef to the
ctor also.

[Daniel Erat, 2011-11-21 23:10:50]

http://codereview.chromium.org/8604012/diff/20007/chrome/browser/ui/views/win...
File chrome/browser/ui/views/window.cc (right):

http://codereview.chromium.org/8604012/diff/20007/chrome/browser/ui/views/win...
chrome/browser/ui/views/window.cc:30: views::Widget* 
CreateViewsBubble(views::BubbleDelegateView* delegate) {
nit: one space after return type instead of two

http://codereview.chromium.org/8604012/diff/20007/chrome/browser/ui/views/win...
File chrome/browser/ui/views/window.h (right):

http://codereview.chromium.org/8604012/diff/20007/chrome/browser/ui/views/win...
chrome/browser/ui/views/window.h:25: // chromeos, use
views::BubbleDelgateView::CreateBubble() directly.
nit: s/Delgate/Delegate/

http://codereview.chromium.org/8604012/diff/20007/ui/views/bubble/bubble_dele...
File ui/views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/8604012/diff/20007/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:159: // Whether to show this bubble when
screen is locked on ChromeOS.
still seems weird to me to define this on platforms where it's not used, but i
guess that we already do the same thing for use_focusless_?  i suppose that the
ifdef around getter and setter should be removed for consistency (or
use_focusless_ should be ifdef-ed as well -- you could move both of these above
border_widget_ if the only objection is that it's difficult to put the ifdef in
the initialization list).

[Ben Goodger (Google), 2011-11-21 23:12:37]

http://codereview.chromium.org/8604012/diff/20007/ui/views/bubble/bubble_dele...
File ui/views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/8604012/diff/20007/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:160: bool show_while_screen_is_locked_;
indeed... I think this should be hoisted to the chokepoint I described in
private email. views shouldn't know about "lock screens"

[alicet1, 2011-11-22 00:23:53]

http://codereview.chromium.org/8604012/diff/20007/chrome/browser/ui/views/win...
File chrome/browser/ui/views/window.cc (right):

http://codereview.chromium.org/8604012/diff/20007/chrome/browser/ui/views/win...
chrome/browser/ui/views/window.cc:30: views::Widget* 
CreateViewsBubble(views::BubbleDelegateView* delegate) {
On 2011/11/21 23:10:50, Daniel Erat wrote:
> nit: one space after return type instead of two

Done.

http://codereview.chromium.org/8604012/diff/20007/ui/views/bubble/bubble_dele...
File ui/views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/8604012/diff/20007/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:159: // Whether to show this bubble when
screen is locked on ChromeOS.
On 2011/11/21 23:10:50, Daniel Erat wrote:
> still seems weird to me to define this on platforms where it's not used, but i
> guess that we already do the same thing for use_focusless_?  i suppose that
the
> ifdef around getter and setter should be removed for consistency (or
> use_focusless_ should be ifdef-ed as well -- you could move both of these
above
> border_widget_ if the only objection is that it's difficult to put the ifdef
in
> the initialization list).

removed.

http://codereview.chromium.org/8604012/diff/20007/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:160: bool show_while_screen_is_locked_;
On 2011/11/21 23:12:38, Ben Goodger (Google) wrote:
> indeed... I think this should be hoisted to the chokepoint I described in
> private email. views shouldn't know about "lock screens"

removed.

[Daniel Erat, 2011-11-22 00:31:35]

LGTM

http://codereview.chromium.org/8604012/diff/20010/chrome/browser/ui/views/win...
File chrome/browser/ui/views/window.h (right):

http://codereview.chromium.org/8604012/diff/20010/chrome/browser/ui/views/win...
chrome/browser/ui/views/window.h:33: views::Widget*
CreateViewsBubbleScreenLocked(
this name is a bit awkward.  CreatePersistentViewsBubble(), maybe?

[msw, 2011-11-22 00:31:52]

I'm curious why this window type needs to be set; have we noticed regressions on
bubbles that are running without this right now on CrOS? But presuming it's
necessary, I suppose this LGTM (with the comment nit).

http://codereview.chromium.org/8604012/diff/20010/chrome/browser/ui/views/win...
File chrome/browser/ui/views/window.h (right):

http://codereview.chromium.org/8604012/diff/20010/chrome/browser/ui/views/win...
chrome/browser/ui/views/window.h:25: // chromeos, use
views::BubbleDelgateView::CreateBubble() directly.
There's maybe 1-2 bubbles (CriticalNotificationBubble, ?) that don't run on
CrOS. Let's just run them all through this codepath, since it won't hurt
anything.

[Daniel Erat, 2011-11-22 00:36:10]

http://codereview.chromium.org/8604012/diff/20010/chrome/browser/ui/views/win...
File chrome/browser/ui/views/window.h (right):

http://codereview.chromium.org/8604012/diff/20010/chrome/browser/ui/views/win...
chrome/browser/ui/views/window.h:25: // chromeos, use
views::BubbleDelgateView::CreateBubble() directly.
s/Delgate/Delegate/

[alicet1, 2011-11-22 00:56:58]

thanks!

ben, can you take a look?

thanx,
alice

http://codereview.chromium.org/8604012/diff/20010/chrome/browser/ui/views/win...
File chrome/browser/ui/views/window.h (right):

http://codereview.chromium.org/8604012/diff/20010/chrome/browser/ui/views/win...
chrome/browser/ui/views/window.h:25: // chromeos, use
views::BubbleDelgateView::CreateBubble() directly.
On 2011/11/22 00:31:52, msw wrote:
> There's maybe 1-2 bubbles (CriticalNotificationBubble, ?) that don't run on
> CrOS. Let's just run them all through this codepath, since it won't hurt
> anything.

removed the last sentence.

http://codereview.chromium.org/8604012/diff/20010/chrome/browser/ui/views/win...
chrome/browser/ui/views/window.h:25: // chromeos, use
views::BubbleDelgateView::CreateBubble() directly.
On 2011/11/22 00:36:10, Daniel Erat wrote:
> s/Delgate/Delegate/

Done.

http://codereview.chromium.org/8604012/diff/20010/chrome/browser/ui/views/win...
chrome/browser/ui/views/window.h:33: views::Widget*
CreateViewsBubbleScreenLocked(
On 2011/11/22 00:31:35, Daniel Erat wrote:
> this name is a bit awkward.  CreatePersistentViewsBubble(), maybe?

Done.

[msw, 2011-11-22 07:38:28]

This CL is deterministically failing two of linux_chromeos's browser_tests:

1) LoginUserTest.UserPassed with error:
chrome/browser/chromeos/login/login_browsertest.cc:56: Failure
Mock function called more times than expected - returning directly.
    Function call: AddObserver(0x8493da8)
         Expected: to be called once
           Actual: called twice - over-saturated and active

2) LoginProfileTest.UserNotPassed with error:
unknown file: Failure
Uninteresting mock function call - returning directly.
    Function call: AddObserver(0x7fcd16f2e948)

LoginUserTest::SetUpInProcessBrowserTestFixture in
chrome/browser/chromeos/login/login_browsertest.cc creates the failing test
condition, and it was just recently disabled on USE_AURA. I'm not quite sure
what this condition is actually testing, but it may be appropriate to disable it
altogether (at least for now); adding nkostylev for more insight.

[msw, 2011-11-22 07:39:56]

+nkostylev: Please see my last message about SetUpInProcessBrowserTestFixture.

[alicet1, 2011-11-22 17:36:44]

I don't think the LoginUserTest and LoginProfileTest failures are related to
this CL. note that they are already failing in the main linux_chromeos builder:

http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromeOS

thanx,
alice

[msw, 2011-11-22 17:43:46]

You're right, sorry for the confusion.
Can you call browser::CreateViewsBubble for AvatarMenuBubbleView too? With that,
this LGTM!

[alicet1, 2011-11-22 17:58:24]

On 2011/11/22 17:43:46, msw wrote:
> You're right, sorry for the confusion.
> Can you call browser::CreateViewsBubble for AvatarMenuBubbleView too? With
that,
> this LGTM!

added.

ben?

[Ben Goodger (Google), 2011-11-22 20:29:02]

http://codereview.chromium.org/8604012/diff/29010/chrome/browser/ui/views/win...
File chrome/browser/ui/views/window.h (right):

http://codereview.chromium.org/8604012/diff/29010/chrome/browser/ui/views/win...
chrome/browser/ui/views/window.h:31: views::Widget* CreatePersistentViewsBubble(
I don't like the use of "persistent" here since it is so generic it could mean
any number of things.

Instead, CreateViewsBubbleAboveLockScreen()

[Ben Goodger (Google), 2011-11-22 20:31:08]

LGTM with the anme change.

[msw, 2011-11-22 20:51:15]

Alice left for vacation, I've made the rename at
http://codereview.chromium.org/8650001/

http://codereview.chromium.org/8604012/diff/29010/chrome/browser/ui/views/win...
File chrome/browser/ui/views/window.h (right):

http://codereview.chromium.org/8604012/diff/29010/chrome/browser/ui/views/win...
chrome/browser/ui/views/window.h:31: views::Widget* CreatePersistentViewsBubble(
On 2011/11/22 20:29:02, Ben Goodger (Google) wrote:
> I don't like the use of "persistent" here since it is so generic it could mean
> any number of things.
> 
> Instead, CreateViewsBubbleAboveLockScreen()

Done.