Pass handle of the native view window to the native messaging host.

Pass handle of the native view window to the native messaging host.

Passing the handle allows the native messaging host to create native UI windows that will be correctly focused. The handle is passed on the command line as "--parent-window=<decimal handle value>".

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

[alexeypa (please no reviews), 2013-08-07 21:46:33]

PTAL.

[Sergey Ulanov, 2013-08-07 22:51:20]

Description should refer refer to bug 269068, not 173509

https://chromiumcodereview.appspot.com/22532011/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(right):

https://chromiumcodereview.appspot.com/22532011/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:82: int
source_process_id,
instead of passing process_id and routing_id pair, can the caller provide
native_view pointer instead?

https://chromiumcodereview.appspot.com/22532011/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/native_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/22532011/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/native_process_launcher.cc:191: // Pass
handle of the native view window to the native messaging host.
explain why it can be useful please.

https://chromiumcodereview.appspot.com/22532011/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/native_process_launcher.cc:195:
base::Int64ToString(window_handle));
Maybe use hexadecimal form instead of decimal number? Also, I suggest using
uintptr_t and uint64 - that will allow to avoid minus sign when formatting it to
string.

[Matt Perry, 2013-08-07 22:53:55]

This seems awfully specific to one use case. Is there a more general approach?
For example, maybe the renderer can send a message with its native view, or the
native messaging process can send a message requesting it.

[Sergey Ulanov, 2013-08-08 00:39:15]

On 2013/08/07 22:53:55, Matt Perry wrote:
> This seems awfully specific to one use case. Is there a more general approach?
> For example, maybe the renderer can send a message with its native view, or
the
> native messaging process can send a message requesting it.

I think it will be common for native messaging hosts to show UAC prompts, and
it's useful to have handle of the calling chrome window for that. I.e. this
problem is not specific to chrome remote desktop. Extensions have no way to get
native handle of the window they run in, so they can't just send a message. It's
the browser process who must provide this information to the native host. We
considered adding a "hello" message that would contain the handle to the native
host, but existing hosts may not support this message, so that would require
changes in the manifest file. It's not clear what it would give us compared to
passing this information in the host command line, but it would make native
messaging host implementation harder. We already use it to pass ID of the
calling extensions. It seems reasonable to pass additional information that a
host may need using command line parameters.

An alternative solution is to have an extension API that would allow
apps/extensions to get native window handle of the window they run in, but it
looks like overkill unless there are other use cases for such API.

[Matt Perry, 2013-08-08 00:43:51]

On 2013/08/08 00:39:15, Sergey Ulanov wrote:
> On 2013/08/07 22:53:55, Matt Perry wrote:
> > This seems awfully specific to one use case. Is there a more general
approach?
> > For example, maybe the renderer can send a message with its native view, or
> the
> > native messaging process can send a message requesting it.
> 
> I think it will be common for native messaging hosts to show UAC prompts, and
> it's useful to have handle of the calling chrome window for that. I.e. this
> problem is not specific to chrome remote desktop. Extensions have no way to
get
> native handle of the window they run in, so they can't just send a message.
It's
> the browser process who must provide this information to the native host. We
> considered adding a "hello" message that would contain the handle to the
native
> host, but existing hosts may not support this message, so that would require
> changes in the manifest file. It's not clear what it would give us compared to
> passing this information in the host command line, but it would make native
> messaging host implementation harder. We already use it to pass ID of the
> calling extensions. It seems reasonable to pass additional information that a
> host may need using command line parameters.
> 
> An alternative solution is to have an extension API that would allow
> apps/extensions to get native window handle of the window they run in, but it
> looks like overkill unless there are other use cases for such API.

OK, that's a good explanation. Thanks.

LGTM

[alexeypa (please no reviews), 2013-08-08 16:57:32]

https://chromiumcodereview.appspot.com/22532011/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(right):

https://chromiumcodereview.appspot.com/22532011/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:82: int
source_process_id,
On 2013/08/07 22:51:20, Sergey Ulanov wrote:
> instead of passing process_id and routing_id pair, can the caller provide
> native_view pointer instead?

Done.

https://chromiumcodereview.appspot.com/22532011/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/native_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/22532011/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/native_process_launcher.cc:191: // Pass
handle of the native view window to the native messaging host.
On 2013/08/07 22:51:20, Sergey Ulanov wrote:
> explain why it can be useful please.

Done.

https://chromiumcodereview.appspot.com/22532011/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/native_process_launcher.cc:195:
base::Int64ToString(window_handle));
On 2013/08/07 22:51:20, Sergey Ulanov wrote:
> Maybe use hexadecimal form instead of decimal number? Also, I suggest using

Parsing a decimal value is typically more common (and therefore more tested)
operation than parsing a hex value. For instance, many routines do not expect
"0x" prefix. Not adding the prefix is an option but than it will not be apparent
that it is a hexadecimal number. 

> uintptr_t and uint64 - that will allow to avoid minus sign when formatting it
to
> string.

The minus sign is understood by conversion routines. Given that the handles are
signed I'd like to avoid unnecessary casting.

I prefer to keep it decimal. The conversion routines expect decimal by default
and it requires less casting.

[Sergey Ulanov, 2013-08-08 18:22:37]

lgtm

https://chromiumcodereview.appspot.com/22532011/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/native_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/22532011/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/native_process_launcher.cc:195:
base::Int64ToString(window_handle));
On 2013/08/08 16:57:32, alexeypa wrote:
> On 2013/08/07 22:51:20, Sergey Ulanov wrote:
> > Maybe use hexadecimal form instead of decimal number? Also, I suggest using
> 
> Parsing a decimal value is typically more common (and therefore more tested)
> operation than parsing a hex value. For instance, many routines do not expect
> "0x" prefix. Not adding the prefix is an option but than it will not be
apparent
> that it is a hexadecimal number. 

Yes, I was thinking about doing it without the prefix. I don't think it would be
less apparent that it's hex, and it should be in documentation anyway. This
reminds me that we also need to update documentation for this change. It's in
chrome/common/extensions/docs/templates/articles/messaging.html . Probably can
be done in a separate CL.

> 
> > uintptr_t and uint64 - that will allow to avoid minus sign when formatting
it
> to
> > string.
> 
> The minus sign is understood by conversion routines. Given that the handles
are
> signed I'd like to avoid unnecessary casting.

How are the handles signed? I thought HWND is just void*, no? So it shouldn't
matter if you reinterpret_cast<> them to intptr_t or uintptr_t.

It's not a problem with chromium, but it could be with some other libraries,
which, for example, may not handle '-' properly when parsing command line.

> 
> I prefer to keep it decimal. The conversion routines expect decimal by default
> and it requires less casting.

ok, let's keep it decimal.

https://chromiumcodereview.appspot.com/22532011/diff/12001/chrome/browser/ext...
File chrome/browser/extensions/api/messaging/native_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/22532011/diff/12001/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_process_launcher.cc:27: const
char kNativeHostsDirectoryName[] = "native_hosts";
This is not used anywhere. Can you please remove it.

https://chromiumcodereview.appspot.com/22532011/diff/12001/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_process_launcher.cc:28: const
char kNativeViewSwitchName[] = "native_view";
Maybe call it "parent-hwnd" or something like that? "native_view" doesn't make
sense outside of chromium code. Also it's more common to use "-" instead of "_"
in parameter names.

https://chromiumcodereview.appspot.com/22532011/diff/12001/chrome/test/data/n...
File chrome/test/data/native_messaging/native_hosts/echo.py (right):

https://chromiumcodereview.appspot.com/22532011/diff/12001/chrome/test/data/n...
chrome/test/data/native_messaging/native_hosts/echo.py:28: for arg in
sys.argv[1:]:
I think it's better to keep this test as it was. Existing native hosts may
depend on order of arguments. Also this code only handles the case when args are
specified as "--arg=value", but not "--arg value".

[alexeypa (please no reviews), 2013-08-08 23:57:41]

https://chromiumcodereview.appspot.com/22532011/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/native_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/22532011/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/native_process_launcher.cc:195:
base::Int64ToString(window_handle));
> How are the handles signed? I thought HWND is just void*, no?

Pointers are signed too. (void*)0xffffffff is smaller than (void*)0.

> So it shouldn't
> matter if you reinterpret_cast<> them to intptr_t or uintptr_t.

It does not matter if you are careful enough (on the platforms we care about).
It might matter in some subtle cases or exotic CPU architectures. For instance
it is "safe" to do |int64_t i = (int32_t)handle| on a 64bit Windows machine.
Despite the handles being pointers, their values are guaranteed to be 32bit for
backward compatibility. 

Basically my point is that it is safer to worry only about type sizes, than to
worry about both type sizes and whether a value can be represented by an
unsigned type.

https://chromiumcodereview.appspot.com/22532011/diff/12001/chrome/browser/ext...
File chrome/browser/extensions/api/messaging/native_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/22532011/diff/12001/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_process_launcher.cc:27: const
char kNativeHostsDirectoryName[] = "native_hosts";
On 2013/08/08 18:22:37, Sergey Ulanov wrote:
> This is not used anywhere. Can you please remove it.

Done.

https://chromiumcodereview.appspot.com/22532011/diff/12001/chrome/browser/ext...
chrome/browser/extensions/api/messaging/native_process_launcher.cc:28: const
char kNativeViewSwitchName[] = "native_view";
On 2013/08/08 18:22:37, Sergey Ulanov wrote:
> Maybe call it "parent-hwnd" or something like that? "native_view" doesn't make
> sense outside of chromium code. Also it's more common to use "-" instead of
"_"
> in parameter names. 

Done. --parent-window

https://chromiumcodereview.appspot.com/22532011/diff/12001/chrome/test/data/n...
File chrome/test/data/native_messaging/native_hosts/echo.py (right):

https://chromiumcodereview.appspot.com/22532011/diff/12001/chrome/test/data/n...
chrome/test/data/native_messaging/native_hosts/echo.py:28: for arg in
sys.argv[1:]:
On 2013/08/08 18:22:37, Sergey Ulanov wrote:
> I think it's better to keep this test as it was. Existing native hosts may
> depend on order of arguments.

To recap our offline conversation. It is very convenient to use CommandLine. It
does proper quoting on Windows and required to start the process on Posix.
Unfortunately it reorders the switches and arguments such that switches go
first.

I believe changing the order of the command line arguments has minimal potential
to break existing hosts for two reasons:

1. Native messaging has been enabled in m29 that is not yet reached stable.
2. The command line format has not been documented yet.

> Also this code only handles the case when args are
> specified as "--arg=value", but not "--arg value".

That should be good enough for now, because we know that the command line is
produced by CommandLine.

[commit-bot: I haz the power, 2013-08-09 00:08:23]

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

[Sergey Ulanov, 2013-08-09 01:00:52]

lgtm

https://chromiumcodereview.appspot.com/22532011/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/native_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/22532011/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/native_process_launcher.cc:195:
base::Int64ToString(window_handle));
On 2013/08/08 23:57:41, alexeypa wrote:
> > How are the handles signed? I thought HWND is just void*, no?
> 
> Pointers are signed too. (void*)0xffffffff is smaller than (void*)0.

AFAIK pointer ordering is undefined, so it depends on compiler. GCC seems to
compare them as unsigned values:

$ cat a.cc 
#include <iostream>

int main() {
  void* a = reinterpret_cast<void*>(-1LL);
  void* b = reinterpret_cast<void*>(0);
  if (a < b)
    std::cout << "a < b";
  else
    std::cout << "a >= b";
  std::cout << std::endl;
}
$ g++ a.cc
$ ./a.out 
a >= b


> 
> > So it shouldn't
> > matter if you reinterpret_cast<> them to intptr_t or uintptr_t.
> 
> It does not matter if you are careful enough (on the platforms we care about).
> It might matter in some subtle cases or exotic CPU architectures. For instance
> it is "safe" to do |int64_t i = (int32_t)handle| on a 64bit Windows machine.
> Despite the handles being pointers, their values are guaranteed to be 32bit
for
> backward compatibility. 
> 
> Basically my point is that it is safer to worry only about type sizes, than to
> worry about both type sizes and whether a value can be represented by an
> unsigned type.

[commit-bot: I haz the power, 2013-08-09 05:13:12]

Change committed as 216595