in-chrome viewer for metro

Listen in the browser for a connection from a metro viewer process.

Render to the surface it provides to us.

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

[cpu_(ooo_6.6-7.5), 2012-09-24 23:31:33]

https://codereview.chromium.org/10983015

[jam, 2012-09-27 21:18:40]

(drive by nits)

https://codereview.chromium.org/10984007/diff/7002/chrome/browser/viewer/view...
File chrome/browser/viewer/viewer_process_host.cc (right):

https://codereview.chromium.org/10984007/diff/7002/chrome/browser/viewer/view...
chrome/browser/viewer/viewer_process_host.cc:16: DVLOG(ERROR) << __FUNCTION__ <<
", ViewerProcessHost";
error?

https://codereview.chromium.org/10984007/diff/7002/chrome/browser/viewer/view...
chrome/browser/viewer/viewer_process_host.cc:19:
&ViewerProcessHost::CreateChannel, base::Unretained(this)));
how is this safe (i.e. that this object is deleted before this runs)? why not
use a WeakPtrFactory?

https://codereview.chromium.org/10984007/diff/7002/chrome/browser/viewer/view...
chrome/browser/viewer/viewer_process_host.cc:26: return false;
nit: return channel_->Send(msg)?

https://codereview.chromium.org/10984007/diff/7002/chrome/browser/viewer/view...
chrome/browser/viewer/viewer_process_host.cc:40: NOTIMPLEMENTED();
nit: if you don't need these, no need to override them yet

https://codereview.chromium.org/10984007/diff/7002/chrome/browser/viewer/view...
File chrome/browser/viewer/viewer_process_host.h (right):

https://codereview.chromium.org/10984007/diff/7002/chrome/browser/viewer/view...
chrome/browser/viewer/viewer_process_host.h:14: struct ChannelHandle;
nit: not needed

https://codereview.chromium.org/10984007/diff/7002/chrome/browser/viewer/view...
chrome/browser/viewer/viewer_process_host.h:17: namespace viewer {
nit: just keep everything in the "chrome" namespace. or even no namespace if you
wish, since src\chrome doesn't need a namespace

https://codereview.chromium.org/10984007/diff/7002/chrome/common/viewer_messa...
File chrome/common/viewer_messages.h (right):

https://codereview.chromium.org/10984007/diff/7002/chrome/common/viewer_messa...
chrome/common/viewer_messages.h:10: #include "ipc/ipc_platform_file.h"
nit: don't need this (also the traits one as well)

https://codereview.chromium.org/10984007/diff/7002/chrome/common/viewer_messa...
chrome/common/viewer_messages.h:18: uint32 /* target hwnd */)
should probably be a NativeViewId?

[scottmg, 2012-09-27 21:30:46]

On 2012/09/27 21:18:40, John Abd-El-Malek wrote:
> (drive by nits)

Thanks, sorry I should have said it's all big hacks just to see if we could get
it working.

In addition to all the bugs you point out, I don't think I can't really check
this in until we have the "two worlds" ash/desktop separation; the ui/surface
code just arbitrarily overwrites the first window since it doesn't have the
separate hidden context to use.

[scottmg, 2012-09-28 04:50:45]

Thanks John, changed to ChannelProxy so it can talk to AcceleratedSurface.

https://chromiumcodereview.appspot.com/10984007/diff/7002/chrome/browser/view...
File chrome/browser/viewer/viewer_process_host.cc (right):

https://chromiumcodereview.appspot.com/10984007/diff/7002/chrome/browser/view...
chrome/browser/viewer/viewer_process_host.cc:16: DVLOG(ERROR) << __FUNCTION__ <<
", ViewerProcessHost";
On 2012/09/27 21:18:40, John Abd-El-Malek wrote:
> error?

Done. (deleted)

https://chromiumcodereview.appspot.com/10984007/diff/7002/chrome/browser/view...
chrome/browser/viewer/viewer_process_host.cc:19:
&ViewerProcessHost::CreateChannel, base::Unretained(this)));
On 2012/09/27 21:18:40, John Abd-El-Malek wrote:
> how is this safe (i.e. that this object is deleted before this runs)? why not
> use a WeakPtrFactory?

Done.

https://chromiumcodereview.appspot.com/10984007/diff/7002/chrome/browser/view...
chrome/browser/viewer/viewer_process_host.cc:26: return false;
On 2012/09/27 21:18:40, John Abd-El-Malek wrote:
> nit: return channel_->Send(msg)?

Done.

https://chromiumcodereview.appspot.com/10984007/diff/7002/chrome/browser/view...
chrome/browser/viewer/viewer_process_host.cc:40: NOTIMPLEMENTED();
On 2012/09/27 21:18:40, John Abd-El-Malek wrote:
> nit: if you don't need these, no need to override them yet

Done.

https://chromiumcodereview.appspot.com/10984007/diff/7002/chrome/browser/view...
File chrome/browser/viewer/viewer_process_host.h (right):

https://chromiumcodereview.appspot.com/10984007/diff/7002/chrome/browser/view...
chrome/browser/viewer/viewer_process_host.h:14: struct ChannelHandle;
On 2012/09/27 21:18:40, John Abd-El-Malek wrote:
> nit: not needed

Done.

https://chromiumcodereview.appspot.com/10984007/diff/7002/chrome/browser/view...
chrome/browser/viewer/viewer_process_host.h:17: namespace viewer {
On 2012/09/27 21:18:40, John Abd-El-Malek wrote:
> nit: just keep everything in the "chrome" namespace. or even no namespace if
you
> wish, since src\chrome doesn't need a namespace

Done.

https://chromiumcodereview.appspot.com/10984007/diff/7002/chrome/common/viewe...
File chrome/common/viewer_messages.h (right):

https://chromiumcodereview.appspot.com/10984007/diff/7002/chrome/common/viewe...
chrome/common/viewer_messages.h:10: #include "ipc/ipc_platform_file.h"
On 2012/09/27 21:18:40, John Abd-El-Malek wrote:
> nit: don't need this (also the traits one as well)

Done.

https://chromiumcodereview.appspot.com/10984007/diff/7002/chrome/common/viewe...
chrome/common/viewer_messages.h:18: uint32 /* target hwnd */)
On 2012/09/27 21:18:40, John Abd-El-Malek wrote:
> should probably be a NativeViewId?

Done.

[jam, 2012-09-28 17:07:54]

https://codereview.chromium.org/10984007/diff/2013/chrome/browser/viewer/view...
File chrome/browser/viewer/viewer_process_host.cc (right):

https://codereview.chromium.org/10984007/diff/2013/chrome/browser/viewer/view...
chrome/browser/viewer/viewer_process_host.cc:16: :
ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
nit: not needed now that you use channelproxy

[cpu_(ooo_6.6-7.5), 2012-09-28 21:37:36]

https://codereview.chromium.org/10984007/diff/4025/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main_win.h (right):

https://codereview.chromium.org/10984007/diff/4025/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_win.h:72:
removable_device_notifications_window_;
have this member only #if defined aura ?

https://codereview.chromium.org/10984007/diff/4025/chrome/browser/viewer/view...
File chrome/browser/viewer/viewer_process_host.h (right):

https://codereview.chromium.org/10984007/diff/4025/chrome/browser/viewer/view...
chrome/browser/viewer/viewer_process_host.h:3: // found in the LICENSE file.
there is already a viewer in chrome (the chromoting) viewer so we want to rename
this to metro_viewer

https://codereview.chromium.org/10984007/diff/4025/chrome/common/viewer_messa...
File chrome/common/viewer_messages.h (right):

https://codereview.chromium.org/10984007/diff/4025/chrome/common/viewer_messa...
chrome/common/viewer_messages.h:17: gfx::NativeViewId /* target hwnd */)
this should be an async message. That might have be the problem on the
metro_driver that forced the posttask.

https://codereview.chromium.org/10984007/diff/4025/win8/delegate_execute/comm...
File win8/delegate_execute/command_execute_impl.cc (right):

https://codereview.chromium.org/10984007/diff/4025/win8/delegate_execute/comm...
win8/delegate_execute/command_execute_impl.cc:147: AtlTrace("Using Shift-F11
debug hook, returning AHE_IMMERSIVE");
move block ahead of line 140

https://codereview.chromium.org/10984007/diff/4025/win8/delegate_execute/comm...
win8/delegate_execute/command_execute_impl.cc:147: AtlTrace("Using Shift-F11
debug hook, returning AHE_IMMERSIVE");
\n at end of atltrace

https://codereview.chromium.org/10984007/diff/4025/win8/metro_driver/chrome_a...
File win8/metro_driver/chrome_app_view.cc (right):

https://codereview.chromium.org/10984007/diff/4025/win8/metro_driver/chrome_a...
win8/metro_driver/chrome_app_view.cc:28: 
remove this 29 - 60 block.

https://codereview.chromium.org/10984007/diff/4025/win8/metro_driver/chrome_a...
win8/metro_driver/chrome_app_view.cc:841: IPC::ChannelProxy chan("viewer",
IPC::Channel::MODE_NAMED_CLIENT,
this is the reason the pipe closes, methinks.

https://codereview.chromium.org/10984007/diff/4025/win8/metro_driver/chrome_a...
win8/metro_driver/chrome_app_view.cc:868: 
if we go back to my code with async message I think it would work.

[scottmg, 2012-09-28 22:14:28]

Thanks

https://codereview.chromium.org/10984007/diff/4025/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main_win.h (right):

https://codereview.chromium.org/10984007/diff/4025/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_win.h:72:
removable_device_notifications_window_;
On 2012/09/28 21:37:37, cpu wrote:
> have this member only #if defined aura ?

Done.

https://codereview.chromium.org/10984007/diff/4025/chrome/browser/viewer/view...
File chrome/browser/viewer/viewer_process_host.h (right):

https://codereview.chromium.org/10984007/diff/4025/chrome/browser/viewer/view...
chrome/browser/viewer/viewer_process_host.h:3: // found in the LICENSE file.
On 2012/09/28 21:37:37, cpu wrote:
> there is already a viewer in chrome (the chromoting) viewer so we want to
rename
> this to metro_viewer

Done.

https://codereview.chromium.org/10984007/diff/4025/chrome/common/viewer_messa...
File chrome/common/viewer_messages.h (right):

https://codereview.chromium.org/10984007/diff/4025/chrome/common/viewer_messa...
chrome/common/viewer_messages.h:17: gfx::NativeViewId /* target hwnd */)
On 2012/09/28 21:37:37, cpu wrote:
> this should be an async message. That might have be the problem on the
> metro_driver that forced the posttask.

Done.

https://codereview.chromium.org/10984007/diff/4025/win8/delegate_execute/comm...
File win8/delegate_execute/command_execute_impl.cc (right):

https://codereview.chromium.org/10984007/diff/4025/win8/delegate_execute/comm...
win8/delegate_execute/command_execute_impl.cc:147: AtlTrace("Using Shift-F11
debug hook, returning AHE_IMMERSIVE");
On 2012/09/28 21:37:37, cpu wrote:
> move block ahead of line 140

Done.

https://codereview.chromium.org/10984007/diff/4025/win8/delegate_execute/comm...
win8/delegate_execute/command_execute_impl.cc:147: AtlTrace("Using Shift-F11
debug hook, returning AHE_IMMERSIVE");
On 2012/09/28 21:37:37, cpu wrote:
> \n at end of atltrace

Done.

https://codereview.chromium.org/10984007/diff/4025/win8/metro_driver/chrome_a...
File win8/metro_driver/chrome_app_view.cc (right):

https://codereview.chromium.org/10984007/diff/4025/win8/metro_driver/chrome_a...
win8/metro_driver/chrome_app_view.cc:28: 
On 2012/09/28 21:37:37, cpu wrote:
> remove this 29 - 60 block.

moved to metro_viewer_messages.cc

https://codereview.chromium.org/10984007/diff/4025/win8/metro_driver/chrome_a...
win8/metro_driver/chrome_app_view.cc:841: IPC::ChannelProxy chan("viewer",
IPC::Channel::MODE_NAMED_CLIENT,
On 2012/09/28 21:37:37, cpu wrote:
> this is the reason the pipe closes, methinks.

Done.

https://codereview.chromium.org/10984007/diff/4025/win8/metro_driver/chrome_a...
win8/metro_driver/chrome_app_view.cc:868: 
On 2012/09/28 21:37:37, cpu wrote:
> if we go back to my code with async message I think it would work.

Done.

https://codereview.chromium.org/10984007/diff/4025/win8/metro_driver/chrome_a...
win8/metro_driver/chrome_app_view.cc:988: if (GetAsyncKeyState(VK_SHIFT) &&
GetAsyncKeyState(VK_F11)) {
Made it #if !defined(USE_AURA) instead

[cpu_(ooo_6.6-7.5), 2012-09-28 22:40:45]

lgtm

[scottmg, 2012-09-28 22:42:06]

+ben for OWNERS (and reviewing any bits you care about)

[Ben Goodger (Google), 2012-10-01 16:30:54]

https://chromiumcodereview.appspot.com/10984007/diff/19018/chrome/browser/met...
File chrome/browser/metro_viewer/metro_viewer_process_host_win.cc (right):

https://chromiumcodereview.appspot.com/10984007/diff/19018/chrome/browser/met...
chrome/browser/metro_viewer/metro_viewer_process_host_win.cc:13: #if
defined(USE_AURA)
and we use gyp not to build these files

https://chromiumcodereview.appspot.com/10984007/diff/19018/chrome/browser/met...
File chrome/browser/metro_viewer/metro_viewer_process_host_win.h (right):

https://chromiumcodereview.appspot.com/10984007/diff/19018/chrome/browser/met...
chrome/browser/metro_viewer/metro_viewer_process_host_win.h:8: #if
defined(USE_AURA)
we tend not to do things like this, rather, we wrap the #include

https://chromiumcodereview.appspot.com/10984007/diff/19018/chrome/browser/met...
chrome/browser/metro_viewer/metro_viewer_process_host_win.h:39: };
disallow_cop

https://chromiumcodereview.appspot.com/10984007/diff/19018/chrome/common/metr...
File chrome/common/metro_viewer_messages.h (right):

https://chromiumcodereview.appspot.com/10984007/diff/19018/chrome/common/metr...
chrome/common/metro_viewer_messages.h:1: // Copyright 2012 The Chromium Authors.
All rights reserved.
seems like this file should live somewhere outside of chrome, since win8 isn't
allowed to include from src/chrome.

https://chromiumcodereview.appspot.com/10984007/diff/19018/win8/metro_driver/...
File win8/metro_driver/metro_viewer_messages.cc (right):

https://chromiumcodereview.appspot.com/10984007/diff/19018/win8/metro_driver/...
win8/metro_driver/metro_viewer_messages.cc:7: #include
"chrome/common/metro_viewer_messages.h"
oof this seems undesirable

[scottmg, 2012-10-01 17:30:24]

Thanks. I moved the message definitions to ui/ and deleted the old attempt at a
(non-multiprocess) viewer there.

https://chromiumcodereview.appspot.com/10984007/diff/19018/chrome/browser/met...
File chrome/browser/metro_viewer/metro_viewer_process_host_win.cc (right):

https://chromiumcodereview.appspot.com/10984007/diff/19018/chrome/browser/met...
chrome/browser/metro_viewer/metro_viewer_process_host_win.cc:13: #if
defined(USE_AURA)
On 2012/10/01 16:30:54, Ben Goodger (Google) wrote:
> and we use gyp not to build these files

Done.

https://chromiumcodereview.appspot.com/10984007/diff/19018/chrome/browser/met...
File chrome/browser/metro_viewer/metro_viewer_process_host_win.h (right):

https://chromiumcodereview.appspot.com/10984007/diff/19018/chrome/browser/met...
chrome/browser/metro_viewer/metro_viewer_process_host_win.h:8: #if
defined(USE_AURA)
On 2012/10/01 16:30:54, Ben Goodger (Google) wrote:
> we tend not to do things like this, rather, we wrap the #include

Done.

https://chromiumcodereview.appspot.com/10984007/diff/19018/chrome/browser/met...
chrome/browser/metro_viewer/metro_viewer_process_host_win.h:39: };
On 2012/10/01 16:30:54, Ben Goodger (Google) wrote:
> disallow_cop

Done.

https://chromiumcodereview.appspot.com/10984007/diff/19018/chrome/common/metr...
File chrome/common/metro_viewer_messages.h (right):

https://chromiumcodereview.appspot.com/10984007/diff/19018/chrome/common/metr...
chrome/common/metro_viewer_messages.h:1: // Copyright 2012 The Chromium Authors.
All rights reserved.
On 2012/10/01 16:30:54, Ben Goodger (Google) wrote:
> seems like this file should live somewhere outside of chrome, since win8 isn't
> allowed to include from src/chrome.

Ah, OK, I didn't realize that. [ Apparently we should add a DEPS:
http://code.google.com/searchframe#search/&exact_package=chromium&q=#include\...
].

ui/ I guess makes sense? Moved.

[Ben Goodger (Google), 2012-10-01 21:08:52]

lgtm