Issue 9958083: Initial version of the Chromoting installer for Mac.

Issue 9958083: Initial version of the Chromoting installer for Mac. (Closed)

Created:
8 years, 8 months ago by garykac

Modified:
8 years, 8 months ago

Reviewers:
Lambros

CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, pam+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org

Base URL:
svn://chrome-svn/chrome/trunk/src/

Visibility:
Public.

More Reviews

Description

Initial version of the Chromoting installer for Mac. This must be built manually (run build.sh) and isn't integrated into the gyp build system yet. It also requires that you have Iceberg installed on your local machine. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=131416

Patch Set 1 #

Total comments: 26

Patch Set 2 : #

Patch Set 3 : #

Created: 8 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1696 lines, --1 lines)			Patch
A	remoting/host/installer/mac/Chrome Remote Desktop.packproj	View	1	1 chunk	+778 lines, -0 lines	0 comments	Download
A	remoting/host/installer/mac/Chromoting.packproj	View	1	1 chunk	+676 lines, -0 lines	0 comments	Download
A	remoting/host/installer/mac/LaunchAgents/org.chromium.chromoting.plist	View		1 chunk	+27 lines, -0 lines	0 comments	Download
A	remoting/host/installer/mac/PrivilegedHelperTools/org.chromium.chromoting.json	View		0 chunks	+0 lines, --1 lines	0 comments	Download
A	remoting/host/installer/mac/PrivilegedHelperTools/org.chromium.chromoting.me2me.sh	View	1	1 chunk	+27 lines, -0 lines	0 comments	Download
A	remoting/host/installer/mac/Scripts/keystone_install.sh	View	1	1 chunk	+9 lines, -0 lines	0 comments	Download
A	remoting/host/installer/mac/Scripts/remoting_postflight.sh	View	1	1 chunk	+47 lines, -0 lines	0 comments	Download
A	remoting/host/installer/mac/Scripts/uninstall.sh	View	1	1 chunk	+45 lines, -0 lines	0 comments	Download
A	remoting/host/installer/mac/build.sh	View	1 2	1 chunk	+56 lines, -0 lines	0 comments	Download
A	remoting/host/installer/mac/tools/ksregister.sh	View	1 2	1 chunk	+18 lines, -0 lines	0 comments	Download
A	remoting/host/installer/mac/tools/ksunregister.sh	View	1 2	1 chunk	+13 lines, -0 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Lambros

Most of these files should have copyright/license headers. org.chromium.chromoting.json is a config file, so it ...

8 years, 8 months ago (2012-04-03 00:49:17 UTC) #2

garykac

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/Chrome%20Remote%20Desktop.packproj File remoting/host/installer/mac/Chrome Remote Desktop.packproj (right): http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/Chrome%20Remote%20Desktop.packproj#newcode1 remoting/host/installer/mac/Chrome Remote Desktop.packproj:1: <?xml version="1.0" encoding="UTF-8"?> On 2012/04/03 ...

8 years, 8 months ago (2012-04-09 19:04:16 UTC) #3

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/Chr...
File remoting/host/installer/mac/Chrome Remote Desktop.packproj (right):

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/Chr...
remoting/host/installer/mac/Chrome Remote Desktop.packproj:1: <?xml
version="1.0" encoding="UTF-8"?>
On 2012/04/03 00:49:17, Lambros wrote:
> Does this file need to be in this CL?  You haven't included the Google-branded
> version of the .plist or any other files (perhaps you were planning to
generate
> them programmatically?)

There is no separate Google-branded version of the service - we use the
'chromoting' service for both ChromeRemoteDesktop and Chromoting.

We do this because it's not user-facing. We only brand user-visible parts of the
installer UI.

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/Chr...
remoting/host/installer/mac/Chrome Remote Desktop.packproj:80:
<string>/Users/garykac/Desktop/Mac_me2me_Install/Keystone/GoogleSoftwareUpdate.pkg</string>
On 2012/04/03 00:49:17, Lambros wrote:
> Please avoid these absolute locations if you can :)  Ideally, this should be
> build-able on arbitrary machines. 

Done.  The installer likes to convert relative paths into absolute paths when
importing. Grr..

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/Chr...
remoting/host/installer/mac/Chrome Remote Desktop.packproj:474:
<string>Chromoting</string>
On 2012/04/03 00:49:17, Lambros wrote:
> Chrome Remote Desktop?

The Chrome Remote Desktop metapackage includes a Chromoting package. The user
doesn't see the sub-package info.

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/Chr...
remoting/host/installer/mac/Chrome Remote Desktop.packproj:488:
<string>org.chromium.pkg.Chromoting</string>
On 2012/04/03 00:49:17, Lambros wrote:
> Chrome Remote Desktop?

As above.

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/Chr...
remoting/host/installer/mac/Chrome Remote Desktop.packproj:527:
<string>Chromoting</string>
On 2012/04/03 00:49:17, Lambros wrote:
> Chrome Remote Desktop?

As above.

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/Chr...
File remoting/host/installer/mac/Chromoting.packproj (right):

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/Chr...
remoting/host/installer/mac/Chromoting.packproj:613: <string>Chrome Remote
Desktop</string>
On 2012/04/03 00:49:17, Lambros wrote:
> Chromoting?

Done.

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/Pri...
File
remoting/host/installer/mac/PrivilegedHelperTools/org.chromium.chromoting.me2me.sh
(right):

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/Pri...
remoting/host/installer/mac/PrivilegedHelperTools/org.chromium.chromoting.me2me.sh:4:
HOST_EXE=$CONFIG_DIR/org.chromium.chromoting.me2me_host
On 2012/04/03 00:49:17, Lambros wrote:
> Maybe add NAME=org.chromium.chromoting and use it instead of writing out the
> name in full?

Done.

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/Pri...
remoting/host/installer/mac/PrivilegedHelperTools/org.chromium.chromoting.me2me.sh:8:
echo $$
On 2012/04/03 00:49:17, Lambros wrote:
> Please could you add a comment before the first "echo $$"?  Something along
> these lines:
> This script is executed from base::mac::ExecuteWithPrivilegesAndWait(), which
> requires the child process to write its PID to stdout before anything else -
see
> authorization_util.h.

Done.

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/Scr...
File remoting/host/installer/mac/Scripts/remoting_postflight.sh (right):

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/Scr...
remoting/host/installer/mac/Scripts/remoting_postflight.sh:28: # However, since
we're installing as an admin, the launchctl command is run
On 2012/04/03 00:49:17, Lambros wrote:
> If we're installing as an admin, do we need "sudo" in this script to run
> commands as root?

I verified that they're not needed when inside the installer.

Removed.

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/Scr...
File remoting/host/installer/mac/Scripts/uninstall.sh (right):

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/Scr...
remoting/host/installer/mac/Scripts/uninstall.sh:15: # Cleanup files from old
versions of installer.
On 2012/04/03 00:49:17, Lambros wrote:
> Is this necessary now?  I think a package uninstaller should only remove files
> that were installed by the package.

this is temporary code that will be replaced by a standalone uninstaller app in
a following cl.

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/Scr...
remoting/host/installer/mac/Scripts/uninstall.sh:16: sudo rm -f
"$LAUNCHAGENTS/com.google.chrome_remote_desktop.plist"
On 2012/04/03 00:49:17, Lambros wrote:
> "sudo" calls should be minimized. They are annoying for people who've
configured
> a zero-length timeout for sudo, so each one would prompt for a password! :)  I
> would suggest something like this:
> 
> sudo rm -rf \
>   "file1" \
>   "file2" \
>   "directory3" \
>   "file4"

See above. This is all going away soon.

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/bui...
File remoting/host/installer/mac/build.sh (right):

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/bui...
remoting/host/installer/mac/build.sh:2: 
On 2012/04/03 00:49:17, Lambros wrote:
> Add comment explaining that Icebox needs to be installed for this to work.
> 
> Just a suggestion: have this script create either the branded or the
un-branded
> package, depending on a command-line switch.  That way, the official Chrome
and
> unofficial Chromium build systems could each produce just one package.

We're only building an installer for "Chrome Remote Desktop".  It uses a
"Chromoting" package internally which can be used to install just "Chromoting",
but we're not packaging that into a .dmg.

http://codereview.chromium.org/9958083/diff/1/remoting/host/installer/mac/bui...
remoting/host/installer/mac/build.sh:16: rm -f "$DMG_FILENAME"
On 2012/04/03 00:49:17, Lambros wrote:
> Maybe remove $DMG_TEMP as well, in case a previous run was aborted?

Done.

lgtm

Expand Messages | Collapse Messages