Switch Windows to use the MessagePopupCollection

Switch Windows to use the MessagePopupCollection (reland)

Windows is currently using the old-style MessagePopupBubble which
couldn't display transparent space between the messages.
Moves MessageCenterTrayWinTests to interactive_ui_tests.

r=mukai@chromium.org
TBR=ben@chromium.org
TEST=WebNotificationTrayWinTest.*
BUG=181235
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188217

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

[Jun Mukai, 2013-03-12 00:49:54]

https://codereview.chromium.org/12667018/diff/4001/chrome/browser/ui/views/me...
File chrome/browser/ui/views/message_center/web_notification_tray_win.cc
(right):

https://codereview.chromium.org/12667018/diff/4001/chrome/browser/ui/views/me...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:124: new
message_center::MessagePopupCollection(NULL, message_center()));
You should specify the first argument, otherwise (I guess) it will crash on
win-aura.
It can be any of aura window, so getting the active browser window from
g_browser_process would be fine.

see comments in ui/views/widget/widget.h: "NULL is not allowed if you are using
aura."
and message_popup_collection.cc specifies params.context from this.

[dewittj, 2013-03-12 00:57:10]

https://codereview.chromium.org/12667018/diff/4001/chrome/browser/ui/views/me...
File chrome/browser/ui/views/message_center/web_notification_tray_win.cc
(right):

https://codereview.chromium.org/12667018/diff/4001/chrome/browser/ui/views/me...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:124: new
message_center::MessagePopupCollection(NULL, message_center()));
On 2013/03/12 00:49:54, Jun Mukai wrote:
> You should specify the first argument, otherwise (I guess) it will crash on
> win-aura.
The #ifdef inside mesage_popup_collection.cc prevents the crash.  I have run
this on win-aura, so I know it isn't crashing :)
> It can be any of aura window, so getting the active browser window from
> g_browser_process would be fine.
There isn't guaranteed to be an active browser window - suppose they're all
closed!  Additionally, I don't know of a method of g_browser_process that gives
you the default window.  Could you point me there?

[dewittj, 2013-03-12 00:59:51]

On 2013/03/12 00:57:10, dewittj wrote:
>
https://codereview.chromium.org/12667018/diff/4001/chrome/browser/ui/views/me...
> File chrome/browser/ui/views/message_center/web_notification_tray_win.cc
> (right):
> 
>
https://codereview.chromium.org/12667018/diff/4001/chrome/browser/ui/views/me...
> chrome/browser/ui/views/message_center/web_notification_tray_win.cc:124: new
> message_center::MessagePopupCollection(NULL, message_center()));
> On 2013/03/12 00:49:54, Jun Mukai wrote:
> > You should specify the first argument, otherwise (I guess) it will crash on
> > win-aura.
> The #ifdef inside mesage_popup_collection.cc prevents the crash.  I have run
> this on win-aura, so I know it isn't crashing :)
> > It can be any of aura window, so getting the active browser window from
> > g_browser_process would be fine.
> There isn't guaranteed to be an active browser window - suppose they're all
> closed!  Additionally, I don't know of a method of g_browser_process that
gives
> you the default window.  Could you point me there?

Scratch that, it doesn't crash when Aura is enabled but you're running the
normal Windows desktop, but it does crash when Aura is enabled and you're
running the Ash desktop.  I will need some expert advice.

[tfarina, 2013-03-12 01:15:38]

You can try asking Scott (sky), Elliot or Scott (mg).

[Jun Mukai, 2013-03-12 01:27:10]

https://codereview.chromium.org/12667018/diff/4001/chrome/browser/ui/views/me...
File chrome/browser/ui/views/message_center/web_notification_tray_win.cc
(right):

https://codereview.chromium.org/12667018/diff/4001/chrome/browser/ui/views/me...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:124: new
message_center::MessagePopupCollection(NULL, message_center()));
On 2013/03/12 00:57:10, dewittj wrote:
> On 2013/03/12 00:49:54, Jun Mukai wrote:
> > You should specify the first argument, otherwise (I guess) it will crash on
> > win-aura.
> The #ifdef inside mesage_popup_collection.cc prevents the crash.  I have run
> this on win-aura, so I know it isn't crashing :)
> > It can be any of aura window, so getting the active browser window from
> > g_browser_process would be fine.
> There isn't guaranteed to be an active browser window - suppose they're all
> closed!  Additionally, I don't know of a method of g_browser_process that
gives
> you the default window.  Could you point me there?

Hmm, I still don't get the point why win-aura is safe in CreateWidget (because
it passes NULL to params.context), but anyway that's fine if no crash happens.

[Jun Mukai, 2013-03-12 01:29:43]

On 2013/03/12 00:59:51, dewittj wrote:
> On 2013/03/12 00:57:10, dewittj wrote:
> >
>
https://codereview.chromium.org/12667018/diff/4001/chrome/browser/ui/views/me...
> > File chrome/browser/ui/views/message_center/web_notification_tray_win.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/12667018/diff/4001/chrome/browser/ui/views/me...
> > chrome/browser/ui/views/message_center/web_notification_tray_win.cc:124: new
> > message_center::MessagePopupCollection(NULL, message_center()));
> > On 2013/03/12 00:49:54, Jun Mukai wrote:
> > > You should specify the first argument, otherwise (I guess) it will crash
on
> > > win-aura.
> > The #ifdef inside mesage_popup_collection.cc prevents the crash.  I have run
> > this on win-aura, so I know it isn't crashing :)
> > > It can be any of aura window, so getting the active browser window from
> > > g_browser_process would be fine.
> > There isn't guaranteed to be an active browser window - suppose they're all
> > closed!  Additionally, I don't know of a method of g_browser_process that
> gives
> > you the default window.  Could you point me there?
> 
> Scratch that, it doesn't crash when Aura is enabled but you're running the
> normal Windows desktop, but it does crash when Aura is enabled and you're
> running the Ash desktop.  I will need some expert advice.

Doe win-ash use this class?  instead of ash/system/web_notification?
If win-ash crashes because of the lack of |context|, I think probably you can
use ash::Shell::GetPrimaryRootWindow().

[dewittj, 2013-03-12 05:36:54]

On 2013/03/12 01:29:43, Jun Mukai wrote:
> On 2013/03/12 00:59:51, dewittj wrote:
> > On 2013/03/12 00:57:10, dewittj wrote:
> > >
> >
>
https://codereview.chromium.org/12667018/diff/4001/chrome/browser/ui/views/me...
> > > File chrome/browser/ui/views/message_center/web_notification_tray_win.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/12667018/diff/4001/chrome/browser/ui/views/me...
> > > chrome/browser/ui/views/message_center/web_notification_tray_win.cc:124:
new
> > > message_center::MessagePopupCollection(NULL, message_center()));
> > > On 2013/03/12 00:49:54, Jun Mukai wrote:
> > > > You should specify the first argument, otherwise (I guess) it will crash
> on
> > > > win-aura.
> > > The #ifdef inside mesage_popup_collection.cc prevents the crash.  I have
run
> > > this on win-aura, so I know it isn't crashing :)
> > > > It can be any of aura window, so getting the active browser window from
> > > > g_browser_process would be fine.
> > > There isn't guaranteed to be an active browser window - suppose they're
all
> > > closed!  Additionally, I don't know of a method of g_browser_process that
> > gives
> > > you the default window.  Could you point me there?
> > 
> > Scratch that, it doesn't crash when Aura is enabled but you're running the
> > normal Windows desktop, but it does crash when Aura is enabled and you're
> > running the Ash desktop.  I will need some expert advice.
> 
> Doe win-ash use this class?  instead of ash/system/web_notification?
> If win-ash crashes because of the lack of |context|, I think probably you can
> use ash::Shell::GetPrimaryRootWindow().

The issue is that (on windows 7 at least) you can have the Ash desktop and the
regular Windows desktop running at the same time, so we will probably need to
define in the constructor which desktop the instance is running for.

[dewittj, 2013-03-13 18:38:15]

mukai: PTAL
erg made a comment over email clarifying that context==NULL is okay provided
top_level is true.

https://codereview.chromium.org/12667018/diff/4001/chrome/browser/ui/views/me...
File chrome/browser/ui/views/message_center/web_notification_tray_win.cc
(right):

https://codereview.chromium.org/12667018/diff/4001/chrome/browser/ui/views/me...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:124: new
message_center::MessagePopupCollection(NULL, message_center()));
On 2013/03/12 01:27:11, Jun Mukai wrote:
> On 2013/03/12 00:57:10, dewittj wrote:
> > On 2013/03/12 00:49:54, Jun Mukai wrote:
> > > You should specify the first argument, otherwise (I guess) it will crash
on
> > > win-aura.
> > The #ifdef inside mesage_popup_collection.cc prevents the crash.  I have run
> > this on win-aura, so I know it isn't crashing :)
> > > It can be any of aura window, so getting the active browser window from
> > > g_browser_process would be fine.
> > There isn't guaranteed to be an active browser window - suppose they're all
> > closed!  Additionally, I don't know of a method of g_browser_process that
> gives
> > you the default window.  Could you point me there?
> 
> Hmm, I still don't get the point why win-aura is safe in CreateWidget (because
> it passes NULL to params.context), but anyway that's fine if no crash happens.

Elliot's feedback was that the comment is wrong - NULL is okay, so long as
top_level is true.

[Jun Mukai, 2013-03-14 02:28:14]

lgtm

[commit-bot: I haz the power, 2013-03-14 16:32:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/12667018/9001

[commit-bot: I haz the power, 2013-03-14 22:36:46]

Change committed as 188217

[dewittj, 2013-03-17 01:17:16]

This was reverted because the tests were failing on the bots.  I suspect it's
because they were running in browser_tests so I moved them to
interactive_ui_tests.

[Jun Mukai, 2013-03-18 09:01:20]

lgtm++
please verify win7_aura passes.

[commit-bot: I haz the power, 2013-03-18 18:16:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/12667018/8002

[commit-bot: I haz the power, 2013-03-18 20:46:38]

Change committed as 188811