The continue window is owned by the desktop environment now.

The continue window is owned by the desktop environment now.

This CL completely removes HostUserInterface and It2MeHostUserInterface classes, making the desktop enviroment (It2MeDesktopEnvironment) responsible for creation of the UI. Platform-specific implementations of ContinueWindow have been rewritten on top of HostWindow class.

Collateral changes:
  - HostScriptObject creates a instance of base::ThreadTaskRunnerHandle so that timers could run on the plugin thread.
  - No more "uninteresting mock" messages caused by the continue and disconnect windows when running remoting_unittests.

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

[alexeypa (please no reviews), 2013-04-04 20:33:24]

PTAL.

[Sergey Ulanov, 2013-04-04 21:04:25]

Thank you for refactoring this code. Looks mostly good.

https://codereview.chromium.org/13461029/diff/1/remoting/host/basic_desktop_e...
File remoting/host/basic_desktop_environment.h (right):

https://codereview.chromium.org/13461029/diff/1/remoting/host/basic_desktop_e...
remoting/host/basic_desktop_environment.h:123: const UiStrings ui_strings_;
Not directly related to this CL, but maybe worth cleaning it up: ui_strings are
used in the constructor only so there is no store them here. Also better to pass
them as reference instead of pointer since the object is copied anyway.

https://codereview.chromium.org/13461029/diff/1/remoting/host/continue_window.cc
File remoting/host/continue_window.cc (right):

https://codereview.chromium.org/13461029/diff/1/remoting/host/continue_window...
remoting/host/continue_window.cc:11: const int kSessionExpirationTimeoutMinutes
= 10;
move these constants to anonymous namespace

https://codereview.chromium.org/13461029/diff/1/remoting/host/continue_window...
remoting/host/continue_window.cc:20: DCHECK(CalledOnValidThread());
NonThreadSafe destructor has the same DCHECK, no need to duplicate it here.

https://codereview.chromium.org/13461029/diff/1/remoting/host/continue_window...
remoting/host/continue_window.cc:53: void ContinueWindow::DisconnectSession() {
Maybe OnDisconnectTimer()?

https://codereview.chromium.org/13461029/diff/1/remoting/host/continue_window...
File remoting/host/continue_window_gtk.cc (right):

https://codereview.chromium.org/13461029/diff/1/remoting/host/continue_window...
remoting/host/continue_window_gtk.cc:33: base::Callback<void(bool)>
continue_callback_;
This is not used.

https://codereview.chromium.org/13461029/diff/1/remoting/host/continue_window...
File remoting/host/continue_window_win.cc (right):

https://codereview.chromium.org/13461029/diff/1/remoting/host/continue_window...
remoting/host/continue_window_win.cc:45: UiStrings ui_strings_;
Can this be moved to the base class?

https://codereview.chromium.org/13461029/diff/1/remoting/host/plugin/host_scr...
File remoting/host/plugin/host_script_object.h (right):

https://codereview.chromium.org/13461029/diff/1/remoting/host/plugin/host_scr...
remoting/host/plugin/host_script_object.h:261: base::ThreadTaskRunnerHandle
plugin_task_runner_handle_;
Awesome. Does this work properly? I remember there were some problems with
adding it here. Specifically if you use component build then libbase.so is
shared between chrome and plugin, which means ThreadTaskRunnerHandle DCHECKs
when you try to initialize it in the plugin - it's already initialized in chrome
for regular MessageLoop that runs on that thread. Maybe that problem is not
worth worrying about: it should still be possible to test component plugin
builds in statically-compiled chrome.
Also I'm not sure why you need this here - the timers in ContinueWindow should
run on the UI thread anyway.

[alexeypa (please no reviews), 2013-04-06 18:07:56]

PTAL.

https://chromiumcodereview.appspot.com/13461029/diff/1/remoting/host/basic_de...
File remoting/host/basic_desktop_environment.h (right):

https://chromiumcodereview.appspot.com/13461029/diff/1/remoting/host/basic_de...
remoting/host/basic_desktop_environment.h:123: const UiStrings ui_strings_;
On 2013/04/04 21:04:26, sergeyu wrote:
> Not directly related to this CL, but maybe worth cleaning it up: ui_strings
are
> used in the constructor only so there is no store them here. Also better to
pass
> them as reference instead of pointer since the object is copied anyway.

Done.

https://chromiumcodereview.appspot.com/13461029/diff/1/remoting/host/continue...
File remoting/host/continue_window.cc (right):

https://chromiumcodereview.appspot.com/13461029/diff/1/remoting/host/continue...
remoting/host/continue_window.cc:11: const int kSessionExpirationTimeoutMinutes
= 10;
On 2013/04/04 21:04:26, sergeyu wrote:
> move these constants to anonymous namespace

Constants have local scope by default. There is not need to place them into a
anonymous namespace. We use the same approach in other files too.

https://chromiumcodereview.appspot.com/13461029/diff/1/remoting/host/continue...
remoting/host/continue_window.cc:20: DCHECK(CalledOnValidThread());
On 2013/04/04 21:04:26, sergeyu wrote:
> NonThreadSafe destructor has the same DCHECK, no need to duplicate it here.

Done.

https://chromiumcodereview.appspot.com/13461029/diff/1/remoting/host/continue...
remoting/host/continue_window.cc:53: void ContinueWindow::DisconnectSession() {
On 2013/04/04 21:04:26, sergeyu wrote:
> Maybe OnDisconnectTimer()?

The primary purpose if this API is to be called by platform-specific
implementations of the continue window. Hence the name. 

PS. It has to be public, because on Mac the caller is not derived from
ContinueWindow.

https://chromiumcodereview.appspot.com/13461029/diff/1/remoting/host/continue...
File remoting/host/continue_window_gtk.cc (right):

https://chromiumcodereview.appspot.com/13461029/diff/1/remoting/host/continue...
remoting/host/continue_window_gtk.cc:33: base::Callback<void(bool)>
continue_callback_;
On 2013/04/04 21:04:26, sergeyu wrote:
> This is not used.

Done.

https://chromiumcodereview.appspot.com/13461029/diff/1/remoting/host/continue...
File remoting/host/continue_window_win.cc (right):

https://chromiumcodereview.appspot.com/13461029/diff/1/remoting/host/continue...
remoting/host/continue_window_win.cc:45: UiStrings ui_strings_;
On 2013/04/04 21:04:26, sergeyu wrote:
> Can this be moved to the base class?

Done.

https://chromiumcodereview.appspot.com/13461029/diff/1/remoting/host/plugin/h...
File remoting/host/plugin/host_script_object.h (right):

https://chromiumcodereview.appspot.com/13461029/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_script_object.h:261: base::ThreadTaskRunnerHandle
plugin_task_runner_handle_;
On 2013/04/04 21:04:26, sergeyu wrote:
> Awesome. Does this work properly?

Nope. :-( It breaks when Chrome and Chromoting shared the same base.dll. What do
you think about the patch #3 as we the way to fix it? 

> Also I'm not sure why you need this here - the timers in
> ContinueWindow should run on the UI thread anyway.

The UI thread is the plugin thread.

[Sergey Ulanov, 2013-04-08 23:56:13]

lgtm

https://chromiumcodereview.appspot.com/13461029/diff/14001/remoting/host/plug...
File remoting/host/plugin/host_script_object.cc (right):

https://chromiumcodereview.appspot.com/13461029/diff/14001/remoting/host/plug...
remoting/host/plugin/host_script_object.cc:670: if
(!base::ThreadTaskRunnerHandle::Get()) {
This is a hack. Main problem that we'll be effectively be hijacking Chrome's
message loop. E.g. what if message loop still runs after the plugin is shut
down. Also tasks posted using plugin_task_runner_ and ThreadTaskRunnerHandle may
be executed in unexpected order. Anyway, I think it's fine to keep it because it
doesn't affect static builds and the plugin will go away in the future, but
please make it clear in the comment that this is a hack.

[Wez, 2013-04-09 00:58:11]

https://chromiumcodereview.appspot.com/13461029/diff/14001/remoting/host/plug...
File remoting/host/plugin/host_script_object.cc (right):

https://chromiumcodereview.appspot.com/13461029/diff/14001/remoting/host/plug...
remoting/host/plugin/host_script_object.cc:666: // When using a componnet build,
the plugin can share base.dll with Chrome.
typo: component

[alexeypa (please no reviews), 2013-04-09 17:40:11]

+darin@

Could you please take a look at base/thread_task_runner_handle.cc? This is a
hack to allow the Chromoting plugin to work if both Chrome and the plugin are
built with "component=shared_library". base::ThreadTaskRunnerHandle object is
created in remoting/host/plugin/host_script_object.cc.

[alexeypa (please no reviews), 2013-04-10 22:08:28]

On 2013/04/09 17:40:11, alexeypa wrote:
> +darin@
> 
> Could you please take a look at base/thread_task_runner_handle.cc? This is a
> hack to allow the Chromoting plugin to work if both Chrome and the plugin are
> built with "component=shared_library". base::ThreadTaskRunnerHandle object is
> created in remoting/host/plugin/host_script_object.cc.

Darin, could you please take a look at base/* changes? Thanks.

[darin (slow to review), 2013-04-10 23:29:24]

On 2013/04/10 22:08:28, alexeypa wrote:
> On 2013/04/09 17:40:11, alexeypa wrote:
> > +darin@
> > 
> > Could you please take a look at base/thread_task_runner_handle.cc? This is a
> > hack to allow the Chromoting plugin to work if both Chrome and the plugin
are
> > built with "component=shared_library". base::ThreadTaskRunnerHandle object
is
> > created in remoting/host/plugin/host_script_object.cc.
> 
> Darin, could you please take a look at base/* changes? Thanks.

Sorry for the slow response.  That sure does seem like a hack.  I looked at the
callsites of ThreadTaskRunnerHandle::Get(), and they don't look prepared for it
to return NULL.

In the component=shared_library build, base should be its own DLL, so this
problem shouldn't exist.  What's going on?  Does this issue occur on Windows? 
Or, do you only see it happening on Linux?

More info, please :-)

[alexeypa (please no reviews), 2013-04-11 17:13:39]

On 2013/04/10 23:29:24, darin wrote:
> Sorry for the slow response.  That sure does seem like a hack.  I looked at
the
> callsites of ThreadTaskRunnerHandle::Get(), and they don't look prepared for
it
> to return NULL.
> 
> In the component=shared_library build, base should be its own DLL, so this
> problem shouldn't exist.  What's going on?  Does this issue occur on Windows? 
> Or, do you only see it happening on Linux?

It is indeed a hack but I think it is a safe one and here is why. Normally
Chrome and NPAPI plugin (remoting_host_plugin in this case) have each own copy
of |base::<anonymous>::lazy_tls_ptr| (from thread_task_runner_handle.cc). The
plugin can create it own message loop or register its own
|base::ThreadTaskRunnerHandle|.

When component build is used Chrome and the plugin could share one instance of
base.dll. The script that builds the webapp is not aware of the component build
so it does not copy base.dll along with remoting_host_plugin.dll. When
remoting_host_plugin.dll is loaded, the DLL loading logic picks up Chrome's
base.dll instead of the plugin's one (because the latter wasn't copied along).
As the result Chrome and the plugin end up sharing base.dll (and a bunch of
other DLLs).

This configuration can only work as long as Chrome and the plugin are compiled
from the same sources. This is exactly what happens during development. That is
why this hack is needed. Another way of solving it would be fixing the script
using to build the webapp. It is tricky because the list of dependent DLLs tend
to change all the time.  

> I looked at the
> callsites of ThreadTaskRunnerHandle::Get(), and they don't look prepared for
it
> to return NULL.

They weren't prepared to handle base::ThreadTaskRunnerHandle::Get() returning
NULL before this CL either. The assumption is that any caller should run on a
message loop or it will break.

> Does this issue occur on Windows?

I tried it only on Windows but the issue is not Windows-specific.

[darin (slow to review), 2013-04-11 18:40:50]

On 2013/04/11 17:13:39, alexeypa wrote:
> On 2013/04/10 23:29:24, darin wrote:
> > Sorry for the slow response.  That sure does seem like a hack.  I looked at
> the
> > callsites of ThreadTaskRunnerHandle::Get(), and they don't look prepared for
> it
> > to return NULL.
> > 
> > In the component=shared_library build, base should be its own DLL, so this
> > problem shouldn't exist.  What's going on?  Does this issue occur on
Windows? 
> > Or, do you only see it happening on Linux?
> 
> It is indeed a hack but I think it is a safe one and here is why. Normally
> Chrome and NPAPI plugin (remoting_host_plugin in this case) have each own copy
> of |base::<anonymous>::lazy_tls_ptr| (from thread_task_runner_handle.cc). The
> plugin can create it own message loop or register its own
> |base::ThreadTaskRunnerHandle|.
> 
> When component build is used Chrome and the plugin could share one instance of
> base.dll. The script that builds the webapp is not aware of the component
build
> so it does not copy base.dll along with remoting_host_plugin.dll. When
> remoting_host_plugin.dll is loaded, the DLL loading logic picks up Chrome's
> base.dll instead of the plugin's one (because the latter wasn't copied along).
> As the result Chrome and the plugin end up sharing base.dll (and a bunch of
> other DLLs).
> 
> This configuration can only work as long as Chrome and the plugin are compiled
> from the same sources. This is exactly what happens during development. That
is
> why this hack is needed. Another way of solving it would be fixing the script
> using to build the webapp. It is tricky because the list of dependent DLLs
tend
> to change all the time.  
> 

Fixing the scripts to package up a base.dll with the plugin sounds like the
right solution to me.


> > I looked at the
> > callsites of ThreadTaskRunnerHandle::Get(), and they don't look prepared for
> it
> > to return NULL.
> 
> They weren't prepared to handle base::ThreadTaskRunnerHandle::Get() returning
> NULL before this CL either. The assumption is that any caller should run on a
> message loop or it will break.

It seems like we could reasonably change the code to CHECK that the value is
non-null.

The point is that users of this class are not expected to null check.  They are
expected to setup their environment properly (i.e., allocate a MessageLoop). 
This approach is common in Chromium.  Don't litter the code with gratuitous null
checks and early returns.  We similarly do not null check malloc failure.  We
crash instead.

> 
> > Does this issue occur on Windows?
> 
> I tried it only on Windows but the issue is not Windows-specific.

The issue can be more painful on Linux.  There, due the wonders of ELF, Chrome
will have already loaded its own libbase.so, and those symbols would probably be
seen by your plugin regardless of whether or not you have your own libbase.so. 
I suspect it may be painful to make the component build work on Linux w/ a
plugin that is also using libbase.so.

[alexeypa (please no reviews), 2013-04-11 23:19:53]

On 2013/04/11 18:40:50, darin wrote:
> Fixing the scripts to package up a base.dll with the plugin sounds like the
> right solution to me.

It is not just base.dll. It is an unknown set of DLLs: all DLLs
remoting_host_plugin.dll directly depends on plus an unknown number of indirect
dependencies. The script that prepares the Chrome installation when using the
component build has the same problem. The way it was "solved" was to grab all
DLLs in out/Debug and then to the installation. This "fix" has already caused
problems on win_x64 bots. I'd like to avoid going the same route, since that the
only affected scenario is development.   

> > They weren't prepared to handle base::ThreadTaskRunnerHandle::Get()
returning
> > NULL before this CL either. The assumption is that any caller should run on
a
> > message loop or it will break.
> 
> It seems like we could reasonably change the code to CHECK that the value is
> non-null.

There is no point to CHECK this pointer. The code that uses the pointer will
crash on the NULL pointer just fine. That will be the intended behaviour.  

> The point is that users of this class are not expected to null check.  They
are
> expected to setup their environment properly (i.e., allocate a MessageLoop). 
> This approach is common in Chromium.  Don't litter the code with gratuitous
null
> checks and early returns.  We similarly do not null check malloc failure.  We
> crash instead.

I guess I didn't express myself clearly. I don't expect the callers of
ThreadTaskRunnerHandle::Get() to check the value of the returned pointer. The
callers should crash with Access Violation when the pointer is NULL. This is
100% equivalent to CHECKing that pointer.

There is one exception though. When base.dll is shared, I'd like to be able to
avoid creating the 2nd instance of ThreadTaskRunnerHandle, because only one is
allowed on any thread.

> The issue can be more painful on Linux.  There, due the wonders of ELF, Chrome
> will have already loaded its own libbase.so, and those symbols would probably
be
> seen by your plugin regardless of whether or not you have your own libbase.so.

> I suspect it may be painful to make the component build work on Linux w/ a
> plugin that is also using libbase.so.

That is another reason to implement this hack, since it cannot be avoided by
fixing the packaging script.

[alexeypa (please no reviews), 2013-04-11 23:21:10]

> The way it was "solved" was to grab all
> DLLs in out/Debug and then to the installation.

.. and then add them to the installation.

[alexeypa (please no reviews), 2013-04-12 15:46:58]

I slightly reworked the patch to emphasize that semantics of
ThreadTaskRunnerHandle::Get() does not change at all.

The hack is all about allowing the plugin to use Chrome's message loop when base
binary is shared.

[alexeypa (please no reviews), 2013-04-15 20:10:53]

Darin, ping?

[alexeypa (please no reviews), 2013-04-16 22:15:24]

On 2013/04/15 20:10:53, alexeypa wrote:
> Darin, ping?

ping^2. :-)

[alexeypa (please no reviews), 2013-04-17 20:39:33]

+willchan@

Could you please take a look at base/* changes and at our  discussion with Darin
in the comments? I pinged Darin a few times but got no response. I'd like to get
this CL going. Does this hack (affecting only the development environment) look
reasonable to you?

[darin (slow to review), 2013-04-17 20:59:55]

Sorry for the review latency.  Please send direct email in the future to get a
speedier response from me.  Sorry, Gmail just coalesces pings on code reviews
with existing unread mail :-(

I'm not convinced that this is a healthy change for the code.  I think we should
discuss alternatives.

At the very least, it seems like you should be able to avoid changes to src/base
by setting a pre-processor define in the build of chromoting.  Then, based on
that pre-processor define you can know to always initialize
plugin_task_runner_handle_ in the HostNPScriptObject constructor (or perhaps at
some earlier stage).

[alexeypa (please no reviews), 2013-04-17 21:12:49]

On 2013/04/17 20:59:55, darin wrote:
> Sorry for the review latency.  Please send direct email in the future to get a
> speedier response from me.  Sorry, Gmail just coalesces pings on code reviews
> with existing unread mail :-(

Understood.

> At the very least, it seems like you should be able to avoid changes to
src/base
> by setting a pre-processor define in the build of chromoting.  Then, based on
> that pre-processor define you can know to always initialize
> plugin_task_runner_handle_ in the HostNPScriptObject constructor (or perhaps
at
> some earlier stage).

The preprocessor define will not work when Chrome is built statically, but the
plugin uses the component build.

I considered adding some preprocessor magic the but could not come up with any
practical approach. The problem is that the consumers of ThreadTaskRunnerHandle
could reside in the same base.dll. Take times for instance. Ideally the times
created by the plugin should use the plugin's task runner. The timers created by
Chrome should use Chrome's task runner. 

Another approach could be that the plugin will properly switch between Chrome's
and its own ThreadTaskRunnerHandle when crossing NPAPI boundary. This seems to
be too involving change (too many entry points) for the scenario that happens
only during development.

[darin (slow to review), 2013-04-17 21:43:13]

On Wed, Apr 17, 2013 at 2:12 PM, <alexeypa@chromium.org> wrote:

> On 2013/04/17 20:59:55, darin wrote:
>
>> Sorry for the review latency.  Please send direct email in the future to
>> get a
>> speedier response from me.  Sorry, Gmail just coalesces pings on code
>> reviews
>> with existing unread mail :-(
>>
>
> Understood.
>
>
>  At the very least, it seems like you should be able to avoid changes to
>>
> src/base
>
>> by setting a pre-processor define in the build of chromoting.  Then,
>> based on
>> that pre-processor define you can know to always initialize
>> plugin_task_runner_handle_ in the HostNPScriptObject constructor (or
>> perhaps
>>
> at
>
>> some earlier stage).
>>
>
> The preprocessor define will not work when Chrome is built statically, but
> the
> plugin uses the component build.
>

Oh?  In this case, you would not be sharing a base.dll with Chrome.  I
think I'm confused about something.



>
> I considered adding some preprocessor magic the but could not come up with
> any
> practical approach. The problem is that the consumers of
> ThreadTaskRunnerHandle
> could reside in the same base.dll. Take times for instance. Ideally the
> times
> created by the plugin should use the plugin's task runner. The timers
> created by
> Chrome should use Chrome's task runner.
>
> Another approach could be that the plugin will properly switch between
> Chrome's
> and its own ThreadTaskRunnerHandle when crossing NPAPI boundary. This
> seems to
> be too involving change (too many entry points) for the scenario that
> happens
> only during development.
>
>
>
https://chromiumcodereview.**appspot.com/13461029/<https://chromiumcodereview...
>

[alexeypa (please no reviews), 2013-04-17 22:20:03]

> > The preprocessor define will not work when Chrome is built statically, but
> > the plugin uses the component build.
> 
> Oh?  In this case, you would not be sharing a base.dll with Chrome.  I
> think I'm confused about something.

I thought you were talking about enabling ThreadTaskRunnerHandle sharing when
'component=="shared_library"', but I guess you meant a dedicated define.

If ThreadTaskRunnerHandle sharing is enabled in the plugin when
'component=="shared_library"' then such a plugin cannot be tested with say
Canary builds, because they are built statically. Which means GYP_DEFINES should
be changed causing an eventual rebuild of Chrome which is pain on Windows.

If ThreadTaskRunnerHandle sharing is enabled by a dedicated define the situation
is better, since the plugin does not take as much time to rebuilt as Chrome
does. It is still inconvenient. The developer will need to remember to set the
define and so on...

Checking whether ThreadTaskRunnerHandle exists in runtime is free from the above
problems. It does not change behavior of existing code. The danger of misusing
of ThreadTaskRunnerHandle::IsSet() is minimal because most of Chrome's code is
running on threads with message loops. So ThreadTaskRunnerHandle::IsSet() can
only really be misused in quite special (for Chrome) situation when message loop
is not created on a thread at all.

[alexeypa (please no reviews), 2013-04-17 22:20:03]

> > The preprocessor define will not work when Chrome is built statically, but
> > the plugin uses the component build.
> 
> Oh?  In this case, you would not be sharing a base.dll with Chrome.  I
> think I'm confused about something.

I thought you were talking about enabling ThreadTaskRunnerHandle sharing when
'component=="shared_library"', but I guess you meant a dedicated define.

If ThreadTaskRunnerHandle sharing is enabled in the plugin when
'component=="shared_library"' then such a plugin cannot be tested with say
Canary builds, because they are built statically. Which means GYP_DEFINES should
be changed causing an eventual rebuild of Chrome which is pain on Windows.

If ThreadTaskRunnerHandle sharing is enabled by a dedicated define the situation
is better, since the plugin does not take as much time to rebuilt as Chrome
does. It is still inconvenient. The developer will need to remember to set the
define and so on...

Checking whether ThreadTaskRunnerHandle exists in runtime is free from the above
problems. It does not change behavior of existing code. The danger of misusing
of ThreadTaskRunnerHandle::IsSet() is minimal because most of Chrome's code is
running on threads with message loops. So ThreadTaskRunnerHandle::IsSet() can
only really be misused in quite special (for Chrome) situation when message loop
is not created on a thread at all.

[alexeypa (please no reviews), 2013-04-22 21:28:52]

Darin, ping?

[darin (slow to review), 2013-04-23 21:18:01]

alexeypa and I discussed this change over a hangout, and LGTM.

[commit-bot: I haz the power, 2013-04-24 18:51:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/13461029/38001

[commit-bot: I haz the power, 2013-04-24 20:44:46]

Change committed as 196226