Restart Mac pref-pane if version mismatch detected

Restart Mac pref-pane if version mismatch detected


BUG=129226
TEST=manual

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

[Jamie, 2012-05-23 01:17:17]

lgtm, but I'd like Daniel to take a look at the ObjC code for a second opinion.

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

http://codereview.chromium.org/10413065/diff/1/remoting/host/installer/mac/Pr...
remoting/host/installer/mac/PrivilegedHelperTools/org.chromium.chromoting.me2me.sh:89:
elif [[ "$1" = "--launch-prefpane" ]]; then
Nit: relaunch-prefpane?

http://codereview.chromium.org/10413065/diff/1/remoting/host/installer/mac/Pr...
remoting/host/installer/mac/PrivilegedHelperTools/org.chromium.chromoting.me2me.sh:92:
cat
Yuck :(

Also, we don't currently use "set -e" in this script, but "cat | true" would
make this safe in case we ever did (and this code path could easily be missed
otherwise). I'm assuming here that cat returns an error when the parent quits.
Similarly, you might want to pipe its stderr to /dev/null.

http://codereview.chromium.org/10413065/diff/1/remoting/host/me2me_preference...
File remoting/host/me2me_preference_pane.mm (right):

http://codereview.chromium.org/10413065/diff/1/remoting/host/me2me_preference...
remoting/host/me2me_preference_pane.mm:541: if (![this_version
isEqualToString:disk_version]) {
Optional: My ObjC knowledge is not enough to know whether or not this is a
genuine concern, but it occurs to me that you're using two different methods to
obtain the version string, and assuming they're going to return the same thing
in the case that nothing has changed. On the face of it, this seems like a
reasonable assumption, but you could avoid it if you use the on-disk method to
read the version number at start-up and cache it.

[dcaiafa, 2012-05-23 17:16:19]

http://codereview.chromium.org/10413065/diff/1/remoting/host/me2me_preference...
File remoting/host/me2me_preference_pane.mm (right):

http://codereview.chromium.org/10413065/diff/1/remoting/host/me2me_preference...
remoting/host/me2me_preference_pane.mm:553: [NSApp terminate:nil];
I don't think terminate: is like exit(). The code following restartPanel will
still run. Is this intended?

[Lambros, 2012-05-24 00:54:49]

I will do an alert message in a separate CL - it's a little too complex to do
here.

https://chromiumcodereview.appspot.com/10413065/diff/1/remoting/host/installe...
File
remoting/host/installer/mac/PrivilegedHelperTools/org.chromium.chromoting.me2me.sh
(right):

https://chromiumcodereview.appspot.com/10413065/diff/1/remoting/host/installe...
remoting/host/installer/mac/PrivilegedHelperTools/org.chromium.chromoting.me2me.sh:89:
elif [[ "$1" = "--launch-prefpane" ]]; then
On 2012/05/23 01:17:17, Jamie wrote:
> Nit: relaunch-prefpane?

Done.

https://chromiumcodereview.appspot.com/10413065/diff/1/remoting/host/installe...
remoting/host/installer/mac/PrivilegedHelperTools/org.chromium.chromoting.me2me.sh:92:
cat
On 2012/05/23 01:17:17, Jamie wrote:
> Yuck :(
> 
> Also, we don't currently use "set -e" in this script, but "cat | true" would
> make this safe in case we ever did (and this code path could easily be missed
> otherwise). I'm assuming here that cat returns an error when the parent quits.
> Similarly, you might want to pipe its stderr to /dev/null.

Done.

https://chromiumcodereview.appspot.com/10413065/diff/1/remoting/host/me2me_pr...
File remoting/host/me2me_preference_pane.mm (right):

https://chromiumcodereview.appspot.com/10413065/diff/1/remoting/host/me2me_pr...
remoting/host/me2me_preference_pane.mm:541: if (![this_version
isEqualToString:disk_version]) {
On 2012/05/23 01:17:17, Jamie wrote:
> Optional: My ObjC knowledge is not enough to know whether or not this is a
> genuine concern, but it occurs to me that you're using two different methods
to
> obtain the version string, and assuming they're going to return the same thing
> in the case that nothing has changed. On the face of it, this seems like a
> reasonable assumption, but you could avoid it if you use the on-disk method to
> read the version number at start-up and cache it.
I'm not sure which method is better, so I'll leave it for now.

https://chromiumcodereview.appspot.com/10413065/diff/1/remoting/host/me2me_pr...
remoting/host/me2me_preference_pane.mm:553: [NSApp terminate:nil];
On 2012/05/23 17:16:19, dcaiafa wrote:
> I don't think terminate: is like exit(). The code following restartPanel will
> still run. Is this intended? 
Good catch!  The following code would've loaded and deleted the config file,
before the pref-pane got restarted - not what we want!
I've fixed this by having this method return BOOL, and having the caller abort
without loading the config.

[dcaiafa, 2012-05-24 16:58:47]

lgtm