It2Me native messaging: GYP and source refactoring

It2Me native messaging: GYP and source refactoring

1. Add "me2me" to the names of all the me2me specific GYP targets and source files. This makes it easier for my next CL that will add the It2Me native messaging host component.
2. Create a remoting_native_messaging_base GYP target that contains the common native messaging plumbing code shared between remoting_core, remoting_me2me_native_messaging_host and the to-be-created remoting_it2me_native_messaging_host.
3. Clean up some dependencies in remoting.gyp: e.g. the native messaging manifest should be depended on by the archive targets, not the native messaging host binary targets.
4. Rename It2MeImpl to It2MeHost.

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

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

[weitao, 2013-10-29 19:34:27]

I decided to start fresh so I am using a new issue for this CL.

This CL still contains a lot of changes but the most controversial part
(renaming the binaries and the name of the native messaging channel) is removed.
All the changes in this CL should be pretty straightforward.

[Lambros, 2013-10-29 20:09:40]

lgtm with comment

https://codereview.chromium.org/49113003/diff/80001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/49113003/diff/80001/remoting/remoting.gyp#new...
remoting/remoting.gyp:539: 'VERSION=<(version_full)',
This #define is not needed here.

[commit-bot: I haz the power, 2013-10-29 21:21:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/weitaosu@chromium.org/49113003/130001

[commit-bot: I haz the power, 2013-10-30 01:05:51]

Retried try job too often on linux_chromeos for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, chromeos_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
dbus_unittests, device_unittests, gpu_unittests, interactive_ui_tests,
ipc_tests, jingle_unittests, media_unittests, net_unittests, ppapi_unittests,
printing_unittests, sandbox_linux_unittests, sql_unittests, sync_unit_tests,
unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-10-30 18:01:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/weitaosu@chromium.org/49113003/460001

[commit-bot: I haz the power, 2013-10-30 18:55:50]

Retried try job too often on win_x64_rel for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[commit-bot: I haz the power, 2013-10-30 22:46:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/weitaosu@chromium.org/49113003/780001

[Sergey Ulanov, 2013-10-30 22:51:01]

https://codereview.chromium.org/49113003/diff/780001/remoting/host/it2me/it2m...
File remoting/host/it2me/it2me_host.h (right):

https://codereview.chromium.org/49113003/diff/780001/remoting/host/it2me/it2m...
remoting/host/it2me/it2me_host.h:5: #ifndef REMOTING_HOST_IT2ME__IT2ME_HOST_H_
s/__/_/

https://codereview.chromium.org/49113003/diff/780001/remoting/host/setup/me2m...
File remoting/host/setup/me2me_native_messaging_host.h (right):

https://codereview.chromium.org/49113003/diff/780001/remoting/host/setup/me2m...
remoting/host/setup/me2me_native_messaging_host.h:154: int
NativeMessagingHostMain();
Should this routine be renamed as well?

https://codereview.chromium.org/49113003/diff/780001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/49113003/diff/780001/remoting/remoting.gyp#ne...
remoting/remoting.gyp:635: 'target_name':
'remoting_me2me_native_messaging_host_static',
Do we really need this target? I think you can just include
me2me_native_messaging_host.* in remoting_host_setup_base and trust the linker
to strip that code from binaries that don't use it.

https://codereview.chromium.org/49113003/diff/780001/remoting/webapp/host_nat...
File remoting/webapp/host_native_messaging.js (right):

https://codereview.chromium.org/49113003/diff/780001/remoting/webapp/host_nat...
remoting/webapp/host_native_messaging.js:32: this.me2mePort_ = null;
I don't think you really need this change. It's better to keep
HostNativeMessaging dedicated to the me2me native messaging and add a spearate
class for it2me. The plugin is currently is instantiated separately for It2Me
and Me2Me. HostDispatcher which creates HostNativeMessaging is used only in
Me2Me case.
You will need to change host_session.js to abstract the plugin and use native
messaging when it's available.

[commit-bot: I haz the power, 2013-10-31 01:26:17]

List of reviewers changed. sergeyu@chromium.org did a drive-by without LGTM'ing!

[weitao, 2013-10-31 17:59:33]

Sergey PTAL.

https://codereview.chromium.org/49113003/diff/80001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/49113003/diff/80001/remoting/remoting.gyp#new...
remoting/remoting.gyp:539: 'VERSION=<(version_full)',
On 2013/10/29 20:09:40, Lambros wrote:
> This #define is not needed here.

Done.

https://codereview.chromium.org/49113003/diff/780001/remoting/host/it2me/it2m...
File remoting/host/it2me/it2me_host.h (right):

https://codereview.chromium.org/49113003/diff/780001/remoting/host/it2me/it2m...
remoting/host/it2me/it2me_host.h:5: #ifndef REMOTING_HOST_IT2ME__IT2ME_HOST_H_
On 2013/10/30 22:51:01, Sergey Ulanov wrote:
> s/__/_/

Done.

https://codereview.chromium.org/49113003/diff/780001/remoting/host/setup/me2m...
File remoting/host/setup/me2me_native_messaging_host.h (right):

https://codereview.chromium.org/49113003/diff/780001/remoting/host/setup/me2m...
remoting/host/setup/me2me_native_messaging_host.h:154: int
NativeMessagingHostMain();
I'll do that as well as renaming the NativeMessagingHost class in my next CL.

On 2013/10/30 22:51:01, Sergey Ulanov wrote:
> Should this routine be renamed as well?

https://codereview.chromium.org/49113003/diff/780001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/49113003/diff/780001/remoting/remoting.gyp#ne...
remoting/remoting.gyp:635: 'target_name':
'remoting_me2me_native_messaging_host_static',
I think this makes a cleaner componentization. setup_base includes all the
DaemonController functionality that is shared between the NPAPI plugin and the
native messaging host.

One good example is Jamie's fix for the Mac issue (293036). Since
native_messaging_host.cc is included in the setup_base target almost all
remoting targets need to be rebuilt. Now that it is moved
me2me_native_messaging_host_static, only the targets that include native
messaging (me2me_native_messaging_host and remoting_core) need to be rebuilt.

Initially I didn't have this me2me_native_messaging_host_static target and
included me2me_native_messaging_host.* directly in
remoting_me2me_native_messaging_host. But that broke the Windows build which
combines the me2me host and the NM host in one binary. So I created this static
target to accommodate that.

WDYT?

On 2013/10/30 22:51:01, Sergey Ulanov wrote:
> Do we really need this target? I think you can just include
> me2me_native_messaging_host.* in remoting_host_setup_base and trust the linker
> to strip that code from binaries that don't use it.

[Sergey Ulanov, 2013-10-31 19:20:42]

https://codereview.chromium.org/49113003/diff/780001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/49113003/diff/780001/remoting/remoting.gyp#ne...
remoting/remoting.gyp:635: 'target_name':
'remoting_me2me_native_messaging_host_static',
Problem is that each additional target adds complexity in this file making it
harder for humans to read and edit it. It's already freaking 3000 lines long!

On 2013/10/31 17:59:34, weitaosu wrote:
> I think this makes a cleaner componentization. setup_base includes all the
> DaemonController functionality that is shared between the NPAPI plugin and the
> native messaging host.
> 
> One good example is Jamie's fix for the Mac issue (293036). Since
> native_messaging_host.cc is included in the setup_base target almost all
> remoting targets need to be rebuilt. Now that it is moved
> me2me_native_messaging_host_static, only the targets that include native
> messaging (me2me_native_messaging_host and remoting_core) need to be rebuilt.

Having multiple targets may make build faster in some cases, but it makes it
slower in all other cases, so it's not clear if it is a win overall. Each
additional target requires separate library file to be generated. It also means
that GYP will take longer to process this file (e.g. a separate .ninja file is
generated for each build target).


> 
> Initially I didn't have this me2me_native_messaging_host_static target and
> included me2me_native_messaging_host.* directly in
> remoting_me2me_native_messaging_host. But that broke the Windows build which
> combines the me2me host and the NM host in one binary. So I created this
static
> target to accommodate that.

Right, that's why I think it should be part of remoting_host_setup_base.


> 
> WDYT?
> 
> On 2013/10/30 22:51:01, Sergey Ulanov wrote:
> > Do we really need this target? I think you can just include
> > me2me_native_messaging_host.* in remoting_host_setup_base and trust the
linker
> > to strip that code from binaries that don't use it.
>

[weitao, 2013-10-31 20:59:15]

PTAL.

https://codereview.chromium.org/49113003/diff/780001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/49113003/diff/780001/remoting/remoting.gyp#ne...
remoting/remoting.gyp:635: 'target_name':
'remoting_me2me_native_messaging_host_static',
On 2013/10/31 19:20:42, Sergey Ulanov wrote:
> Problem is that each additional target adds complexity in this file making it
> harder for humans to read and edit it. It's already freaking 3000 lines long!
> 
> On 2013/10/31 17:59:34, weitaosu wrote:
> > I think this makes a cleaner componentization. setup_base includes all the
> > DaemonController functionality that is shared between the NPAPI plugin and
the
> > native messaging host.
> > 
> > One good example is Jamie's fix for the Mac issue (293036). Since
> > native_messaging_host.cc is included in the setup_base target almost all
> > remoting targets need to be rebuilt. Now that it is moved
> > me2me_native_messaging_host_static, only the targets that include native
> > messaging (me2me_native_messaging_host and remoting_core) need to be
rebuilt.
> 
> Having multiple targets may make build faster in some cases, but it makes it
> slower in all other cases, so it's not clear if it is a win overall. Each
> additional target requires separate library file to be generated. It also
means
> that GYP will take longer to process this file (e.g. a separate .ninja file is
> generated for each build target).
> 
> 
> > 
> > Initially I didn't have this me2me_native_messaging_host_static target and
> > included me2me_native_messaging_host.* directly in
> > remoting_me2me_native_messaging_host. But that broke the Windows build which
> > combines the me2me host and the NM host in one binary. So I created this
> static
> > target to accommodate that.
> 
> Right, that's why I think it should be part of remoting_host_setup_base.
> 
> 
> > 
> > WDYT?
> > 
> > On 2013/10/30 22:51:01, Sergey Ulanov wrote:
> > > Do we really need this target? I think you can just include
> > > me2me_native_messaging_host.* in remoting_host_setup_base and trust the
> linker
> > > to strip that code from binaries that don't use it.
> > 
> 

Done.

[Sergey Ulanov, 2013-10-31 21:01:50]

lgtm. Thanks!

[commit-bot: I haz the power, 2013-10-31 21:11:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/weitaosu@chromium.org/49113003/830002

[commit-bot: I haz the power, 2013-10-31 23:30:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/weitaosu@chromium.org/49113003/830002

[commit-bot: I haz the power, 2013-11-01 11:43:42]

Change committed as 232331

[robertphillips, 2013-11-01 12:50:13]

I reverted this due to compilation failures on the following bots:

Mac
http://build.chromium.org/p/chromium/builders/Mac/builds/22699/steps/compile/...
Google Chrome Linux
http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux/...
Google Chrome Linux x64
http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux%...
Google Chrome Mac
http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Mac/bu...

Here is a typical failure (in case the above links rot):

FAILED: cd ../../remoting; export
BUILT_PRODUCTS_DIR=/Volumes/data/b/build/slave/Mac/build/src/out/Release; export
CONFIGURATION=Release; export PRODUCT_NAME=remoting_me2me_host_archive; export
SDKROOT=/Developer/SDKs/MacOSX10.6.sdk; export
SRCROOT=/Volumes/data/b/build/slave/Mac/build/src/out/Release/../../remoting;
export SOURCE_ROOT="${SRCROOT}"; export
TARGET_BUILD_DIR=/Volumes/data/b/build/slave/Mac/build/src/out/Release; export
TEMP_DIR="${TMPDIR}";python host/installer/build-installer-archive.py
../out/Release/gen/remoting/remoting-me2me-host
../out/Release/remoting-me2me-host-mac.zip --source-file-roots
host/installer/mac/ ../chrome/installer/mac/ --source-files
host/installer/mac/do_signing.sh host/installer/mac/do_signing.props
host/installer/mac/ChromotingHost.pkgproj
host/installer/mac/ChromotingHostService.pkgproj
host/installer/mac/ChromotingHostUninstaller.pkgproj
host/installer/mac/LaunchAgents/org.chromium.chromoting.plist
host/installer/mac/PrivilegedHelperTools/org.chromium.chromoting.me2me.sh
host/installer/mac/Config/org.chromium.chromoting.conf
host/installer/mac/Scripts/keystone_install.sh
host/installer/mac/Scripts/remoting_postflight.sh
host/installer/mac/Scripts/remoting_preflight.sh
host/installer/mac/Keystone/GoogleSoftwareUpdate.pkg
../chrome/installer/mac/pkg-dmg --generated-files
../out/Release/remoting_host_prefpane.prefPane
../out/Release/remoting_me2me_host.app
../out/Release/remoting_host_uninstaller.app
../out/Release/remoting_native_messaging_host
../out/Release/remoting/com.google.chrome.remote_desktop.json
--generated-files-dst PreferencePanes/org.chromium.chromoting.prefPane
PrivilegedHelperTools/org.chromium.chromoting.me2me_host.app
"Applications/Chromoting Host Uninstaller.app"
PrivilegedHelperTools/org.chromium.chromoting.me2me_host.app/Contents/MacOS/native_messaging_host
Config/com.google.chrome.remote_desktop.json --defs "VERSION=32.0.1689.0"
"VERSION_SHORT=32.0.1689" "VERSION_MAJOR=32" "VERSION_MINOR=0"
"HOST_NAME=Chromoting Host" "HOST_SERVICE_NAME=Chromoting Host Service"
"HOST_UNINSTALLER_NAME=Chromoting Host Uninstaller" "HOST_PKG=Chromoting Host"
"HOST_SERVICE_PKG=ChromotingHostService"
"HOST_UNINSTALLER_PKG=ChromotingHostUninstaller"
"BUNDLE_ID_HOST=org.chromium.pkg.ChromotingHost"
"BUNDLE_ID_HOST_SERVICE=org.chromium.pkg.ChromotingHostService"
"BUNDLE_ID_HOST_UNINSTALLER=org.chromium.pkg.ChromotingHostUninstaller"
"DMG_VOLUME_NAME=Chromoting Host 32.0.1689.0"
"DMG_FILE_NAME=ChromotingHost-32.0.1689.0"
Traceback (most recent call last):
  File "host/installer/build-installer-archive.py", line 292, in <module>
    sys.exit(main())
  File "host/installer/build-installer-archive.py", line 286, in main
    defs)
  File "host/installer/build-installer-archive.py", line 207, in
buildHostArchive
    shutil.copy2(bs, dst_file)
  File
"/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/shutil.py",
line 130, in copy2
    copyfile(src, dst)
  File
"/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/shutil.py",
line 82, in copyfile
    with open(src, 'rb') as fsrc:
IOError: [Errno 2] No such file or directory:
'../out/Release/remoting_native_messaging_host'
ninja: build stopped: subcommand failed.

[commit-bot: I haz the power, 2013-11-01 22:13:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/weitaosu@chromium.org/49113003/1260001

[commit-bot: I haz the power, 2013-11-02 01:27:04]

Change committed as 232543