The metro_driver should not block the UI thread while waiting for the Chrome browser IPC channel to…

The metro_driver should not block the UI thread while waiting for the Chrome browser IPC channel to be initialized.

This causes the taskbar to become unresponsive for a bit on Windows 7. Windows 8 also has the same
problem. However it is not as visible as on Windows 7.

Fix is to do the IPC channel dance in a timer.

BUG=417100
R=cpu

Committed: https://crrev.com/b1f98f2be2a9b791754eaebbefe4ffa599aa8c0b
Cr-Commit-Position: refs/heads/master@{#297058}

[ananta, 2014-09-24 00:39:35]

ananta@chromium.org changed reviewers:
+ scottmg@chromium.org

[ananta, 2014-09-24 00:39:42]

+scottmg

[scottmg, 2014-09-24 02:40:33]

https://codereview.chromium.org/593353003/diff/1/win8/metro_driver/chrome_app...
File win8/metro_driver/chrome_app_view_ash.cc (right):

https://codereview.chromium.org/593353003/diff/1/win8/metro_driver/chrome_app...
win8/metro_driver/chrome_app_view_ash.cc:1414: if
(!IPC::Channel::IsNamedServerInitialized(
WaitForChromeIPCConnection isn't used any more, should be deleted

https://codereview.chromium.org/593353003/diff/1/win8/metro_driver/chrome_app...
win8/metro_driver/chrome_app_view_ash.cc:1426: ::RemoveProp(core_window,
kChromeAppViewAshInstanceProp);
why do we need window props instead of plain globals, when e.g. ms_elapsed is
just a static?

https://codereview.chromium.org/593353003/diff/1/win8/metro_driver/chrome_app...
win8/metro_driver/chrome_app_view_ash.cc:1428: ::KillTimer(core_window,
kChromeChannelPollTimerId);
killtimer here and below can move out above the if

https://codereview.chromium.org/593353003/diff/1/win8/metro_driver/chrome_app...
File win8/metro_driver/chrome_app_view_ash.h (right):

https://codereview.chromium.org/593353003/diff/1/win8/metro_driver/chrome_app...
win8/metro_driver/chrome_app_view_ash.h:254: scoped_ptr<base::Thread>
io_thread_;
maybe move this above the ui_channel_ since it's supposed to outlive that. (i
know it's manually reset)

[ananta, 2014-09-24 18:22:33]

https://codereview.chromium.org/593353003/diff/1/win8/metro_driver/chrome_app...
File win8/metro_driver/chrome_app_view_ash.cc (right):

https://codereview.chromium.org/593353003/diff/1/win8/metro_driver/chrome_app...
win8/metro_driver/chrome_app_view_ash.cc:1414: if
(!IPC::Channel::IsNamedServerInitialized(
On 2014/09/24 02:40:32, scottmg wrote:
> WaitForChromeIPCConnection isn't used any more, should be deleted

Done.

https://codereview.chromium.org/593353003/diff/1/win8/metro_driver/chrome_app...
win8/metro_driver/chrome_app_view_ash.cc:1426: ::RemoveProp(core_window,
kChromeAppViewAshInstanceProp);
On 2014/09/24 02:40:32, scottmg wrote:
> why do we need window props instead of plain globals, when e.g. ms_elapsed is
> just a static?

Good point. Done.

https://codereview.chromium.org/593353003/diff/1/win8/metro_driver/chrome_app...
win8/metro_driver/chrome_app_view_ash.cc:1428: ::KillTimer(core_window,
kChromeChannelPollTimerId);
On 2014/09/24 02:40:32, scottmg wrote:
> killtimer here and below can move out above the if

Done.

https://codereview.chromium.org/593353003/diff/1/win8/metro_driver/chrome_app...
File win8/metro_driver/chrome_app_view_ash.h (right):

https://codereview.chromium.org/593353003/diff/1/win8/metro_driver/chrome_app...
win8/metro_driver/chrome_app_view_ash.h:254: scoped_ptr<base::Thread>
io_thread_;
On 2014/09/24 02:40:32, scottmg wrote:
> maybe move this above the ui_channel_ since it's supposed to outlive that. (i
> know it's manually reset)

Done.

[scottmg, 2014-09-24 19:33:35]

I'll defer to Carlos on the larger question of whether it's a good idea to be
doing this in a timer in the first place.

Otherwise lgtm, with below. Seems unfortuante that we need to check for a
non-null channel everywhere.

https://codereview.chromium.org/593353003/diff/40001/win8/metro_driver/chrome...
File win8/metro_driver/chrome_app_view_ash.cc (right):

https://codereview.chromium.org/593353003/diff/40001/win8/metro_driver/chrome...
win8/metro_driver/chrome_app_view_ash.cc:354: ChromeChannelListener*
g_ui_channel_listener = NULL;
could this be a member of ChromeAppViewAsh too so there's only 1 global?

https://codereview.chromium.org/593353003/diff/40001/win8/metro_driver/chrome...
win8/metro_driver/chrome_app_view_ash.cc:953: if (!input_source_)
ui_channel_ guard?

https://codereview.chromium.org/593353003/diff/40001/win8/metro_driver/chrome...
win8/metro_driver/chrome_app_view_ash.cc:986:
MetroViewerHostMsg_MouseButtonParams params;
ui_channel_ guard?

https://codereview.chromium.org/593353003/diff/40001/win8/metro_driver/chrome...
win8/metro_driver/chrome_app_view_ash.cc:1365: ui_channel_->Send(new
MetroViewerHostMsg_EdgeGesture());
does this need if guarding now too?

https://codereview.chromium.org/593353003/diff/40001/win8/metro_driver/chrome...
win8/metro_driver/chrome_app_view_ash.cc:1370: DCHECK(ui_channel_);
same

https://codereview.chromium.org/593353003/diff/40001/win8/metro_driver/chrome...
win8/metro_driver/chrome_app_view_ash.cc:1375: DCHECK(ui_channel_);
same

[ananta, 2014-09-24 19:57:18]

https://codereview.chromium.org/593353003/diff/40001/win8/metro_driver/chrome...
File win8/metro_driver/chrome_app_view_ash.cc (right):

https://codereview.chromium.org/593353003/diff/40001/win8/metro_driver/chrome...
win8/metro_driver/chrome_app_view_ash.cc:354: ChromeChannelListener*
g_ui_channel_listener = NULL;
On 2014/09/24 19:33:35, scottmg wrote:
> could this be a member of ChromeAppViewAsh too so there's only 1 global?

Done.

https://codereview.chromium.org/593353003/diff/40001/win8/metro_driver/chrome...
win8/metro_driver/chrome_app_view_ash.cc:953: if (!input_source_)
On 2014/09/24 19:33:35, scottmg wrote:
> ui_channel_ guard?

Not needed here because this is called after the channel is initialized. Added a
DCHECK

https://codereview.chromium.org/593353003/diff/40001/win8/metro_driver/chrome...
win8/metro_driver/chrome_app_view_ash.cc:986:
MetroViewerHostMsg_MouseButtonParams params;
On 2014/09/24 19:33:35, scottmg wrote:
> ui_channel_ guard?

Done.

https://codereview.chromium.org/593353003/diff/40001/win8/metro_driver/chrome...
win8/metro_driver/chrome_app_view_ash.cc:1365: ui_channel_->Send(new
MetroViewerHostMsg_EdgeGesture());
On 2014/09/24 19:33:35, scottmg wrote:
> does this need if guarding now too?

Done.

https://codereview.chromium.org/593353003/diff/40001/win8/metro_driver/chrome...
win8/metro_driver/chrome_app_view_ash.cc:1370: DCHECK(ui_channel_);
On 2014/09/24 19:33:35, scottmg wrote:
> same

Done.

https://codereview.chromium.org/593353003/diff/40001/win8/metro_driver/chrome...
win8/metro_driver/chrome_app_view_ash.cc:1375: DCHECK(ui_channel_);
On 2014/09/24 19:33:35, scottmg wrote:
> same

Done. Thanks for catching these.

[cpu_(ooo_6.6-7.5), 2014-09-25 23:00:08]

https://codereview.chromium.org/593353003/diff/60001/win8/metro_driver/chrome...
File win8/metro_driver/chrome_app_view_ash.cc (right):

https://codereview.chromium.org/593353003/diff/60001/win8/metro_driver/chrome...
win8/metro_driver/chrome_app_view_ash.cc:690: UINT_PTR channel_setup_timer_id =
change to base::timer, since in line 698 we run our message loop, it should
work. Then you can use base::bind so you should not need globals or touch the
core_window_hwnd_

[ananta, 2014-09-26 02:29:02]

https://codereview.chromium.org/593353003/diff/60001/win8/metro_driver/chrome...
File win8/metro_driver/chrome_app_view_ash.cc (right):

https://codereview.chromium.org/593353003/diff/60001/win8/metro_driver/chrome...
win8/metro_driver/chrome_app_view_ash.cc:690: UINT_PTR channel_setup_timer_id =
On 2014/09/25 23:00:08, cpu wrote:
> change to base::timer, since in line 698 we run our message loop, it should
> work. Then you can use base::bind so you should not need globals or touch the
> core_window_hwnd_

Good point. Done.

[cpu_(ooo_6.6-7.5), 2014-09-26 22:07:26]

lgtm

[ananta, 2014-09-26 22:26:43]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-09-26 22:27:50]

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

[commit-bot: I haz the power, 2014-09-26 22:32:20]

Committed patchset #6 (id:100001) as 5332e295e979f4bc2bb21a5c65beae590db491ba

[commit-bot: I haz the power, 2014-09-26 22:33:12]

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