Added unpacking step

Added unpacking step

BUG=

Committed: https://crrev.com/d0136826964ad0e135f71b18661b78e1a3cf1c71
Cr-Commit-Position: refs/heads/master@{#415213}

[Anna Zeng, 2016-08-01 21:08:11]

zengster@google.com changed reviewers:
+ ellyjones@chromium.org, ivanhernandez@google.com, mark@chromium.org

[Anna Zeng, 2016-08-01 21:08:12]

The code as it stands now is still a bit raw, but we wanted to show you our
progress in writing the unpacking step of the installer! Currently the open
issue of interest is that the unmounting will not work without the CFRunLoop
(this is captured in a TODO: inside Unpacker.m). The second issue that we can
currently focus on (among many) is to make testing this code more convenient.
This may result in a unittest file of some sort in the near future, so watch out
for that!

Suggestions / feedback are welcome.

[Anna Zeng, 2016-08-01 21:08:39]

Description was changed from

==========
Added unpacking step

BUG=
==========

to

==========
Added unpacking step

BUG=
==========

[Anna Zeng, 2016-08-01 21:08:40]

zengster@google.com changed reviewers:
+ sdy@chromium.org

[Mark Mentovai, 2016-08-02 19:01:33]

I think that this is the most important aspect of what’s going on here right
now. Since it looks like we’re running things in threads other than what we were
expecting, fixing this up is a good chance to make sure that we understand the
threads that everything is running on, and to make sure that they’re as we want
them.

https://codereview.chromium.org/2203583002/diff/20001/chrome/installer/mac/ap...
File chrome/installer/mac/app/Unpacker.m (right):

https://codereview.chromium.org/2203583002/diff/20001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:37: unmountTask.terminationHandler =
^void(NSTask* task) {
Aha! This is what’s causing all of the trouble with the run loop stuff below.

The termination handler isn’t guaranteed to run on the same thread as
-unpackDMG. In fact, it probably doesn’t run on the same thread at all. These
block completion handlers run stuff on a libdispatch worker thread. That’s
mostly fine until you expect something to happen on your run loop. Each thread
has its own run loop, and the run loop you’re expecting things to run on. When
you schedule the DiskArbitration callback with a run loop, presently, you’re
scheduling it with the CURRENT thread’s run loop, but you’re EXPECTING it to be
scheduled with the MAIN thread’s run loop.

For that matter, -unpackDMG isn’t running on the main thread either. Must be
another worker thread!

Fortunately, it’s pretty easy to send a block over to the main thread. You can
write

dispatch_async(dispatch_get_main_queue(), ^{
    stuff_to_run_on_main_thread();
});

You can do this from within your termination handler. That should get stuff back
on track.

[Mark Mentovai, 2016-08-02 19:03:37]

Another alternative to the termination handler with redispatch would be to
listen for the 
NSTaskDidTerminateNotification notification. See
https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundat...

[Anna Zeng, 2016-08-02 23:58:22]

https://codereview.chromium.org/2203583002/diff/20001/chrome/installer/mac/ap...
File chrome/installer/mac/app/Unpacker.m (right):

https://codereview.chromium.org/2203583002/diff/20001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:37: unmountTask.terminationHandler =
^void(NSTask* task) {
On 2016/08/02 19:01:32, Mark Mentovai wrote:
> Aha! This is what’s causing all of the trouble with the run loop stuff below.
> 
> The termination handler isn’t guaranteed to run on the same thread as
> -unpackDMG. In fact, it probably doesn’t run on the same thread at all. These
> block completion handlers run stuff on a libdispatch worker thread. That’s
> mostly fine until you expect something to happen on your run loop. Each thread
> has its own run loop, and the run loop you’re expecting things to run on. When
> you schedule the DiskArbitration callback with a run loop, presently, you’re
> scheduling it with the CURRENT thread’s run loop, but you’re EXPECTING it to
be
> scheduled with the MAIN thread’s run loop.
> 
> For that matter, -unpackDMG isn’t running on the main thread either. Must be
> another worker thread!
> 
> Fortunately, it’s pretty easy to send a block over to the main thread. You can
> write
> 
> dispatch_async(dispatch_get_main_queue(), ^{
>     stuff_to_run_on_main_thread();
> });
> 
> You can do this from within your termination handler. That should get stuff
back
> on track.

Awesome! This helped immensely. I ended up using a dispatch queue though, to
make sure that the main thread's UI isn't hanging while we do this work.

[Sidney San Martín, 2016-08-05 20:35:04]

https://codereview.chromium.org/2203583002/diff/20001/chrome/installer/mac/ap...
File chrome/installer/mac/app/Unpacker.m (right):

https://codereview.chromium.org/2203583002/diff/20001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:15: stringByResolvingSymlinksInPath]
This should be a pretty quick change, any reason not to do it now?

https://codereview.chromium.org/2203583002/diff/20001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:118: Unpacker* obj_self = (__bridge
Unpacker*)context;
These functions aren't really part of Unpacker, so maybe don't pretend they are?

- You should move them outside of the @implmentation/@end block (near the top of
the file is common).
- The variable should be called something like `unpacker`, not `*_self`.

https://codereview.chromium.org/2203583002/diff/20001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:119: [obj_self callbackFollowDisk:disk];
Could you include error info in the callback?

[Sidney San Martín, 2016-08-05 21:13:41]

https://codereview.chromium.org/2203583002/diff/20001/chrome/installer/mac/ap...
File chrome/installer/mac/app/Unpacker.m (right):

https://codereview.chromium.org/2203583002/diff/20001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:118: Unpacker* obj_self = (__bridge
Unpacker*)context;
You should also use `static` on these functions to indicate that they're only
used within this file.

[Anna Zeng, 2016-08-05 21:48:40]

https://codereview.chromium.org/2203583002/diff/20001/chrome/installer/mac/ap...
File chrome/installer/mac/app/Unpacker.m (right):

https://codereview.chromium.org/2203583002/diff/20001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:15: stringByResolvingSymlinksInPath]
On 2016/08/05 20:35:04, Sidney San Martín wrote:
> This should be a pretty quick change, any reason not to do it now?

I plan to include progress on this in the other refactor CL, as that one
includes a new class dedicated to keeping track of where files of interest are.
Acknowledged!

https://codereview.chromium.org/2203583002/diff/20001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:118: Unpacker* obj_self = (__bridge
Unpacker*)context;
On 2016/08/05 20:35:04, Sidney San Martín wrote:
> These functions aren't really part of Unpacker, so maybe don't pretend they
are?
> 
> - You should move them outside of the @implmentation/@end block (near the top
of
> the file is common).
> - The variable should be called something like `unpacker`, not `*_self`.

Done.

https://codereview.chromium.org/2203583002/diff/20001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:118: Unpacker* obj_self = (__bridge
Unpacker*)context;
On 2016/08/05 21:13:40, Sidney San Martín wrote:
> You should also use `static` on these functions to indicate that they're only
> used within this file.

Done.

https://codereview.chromium.org/2203583002/diff/20001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:119: [obj_self callbackFollowDisk:disk];
On 2016/08/05 20:35:04, Sidney San Martín wrote:
> Could you include error info in the callback?

Done, with more progress to come!

[Sidney San Martín, 2016-08-08 18:43:18]

https://codereview.chromium.org/2203583002/diff/60001/chrome/installer/mac/ap...
File chrome/installer/mac/app/MainDelegate.m (right):

https://codereview.chromium.org/2203583002/diff/60001/chrome/installer/mac/ap...
chrome/installer/mac/app/MainDelegate.m:9: extern dispatch_semaphore_t
mount_semaphore;
As discussed: a random extern definitely shouldn't be part of Unpacker's API,
you can just use delegate callbacks.

https://codereview.chromium.org/2203583002/diff/60001/chrome/installer/mac/ap...
chrome/installer/mac/app/MainDelegate.m:40:
dispatch_semaphore_wait(mount_semaphore, DISPATCH_TIME_FOREVER);
As discussed:
1. mountDMG and unmountDMG are implementation details, they shouldn't be part of
Unpacker's API. Instead, it should just take a URL and a destination and call
back when it's done.
2. You shouldn't hold onto a thread for this. Use delegate callbacks.

https://codereview.chromium.org/2203583002/diff/60001/chrome/installer/mac/ap...
File chrome/installer/mac/app/Unpacker.m (right):

https://codereview.chromium.org/2203583002/diff/60001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:20: dispatch_queue_t unpack_dq;
These should be instance variables, not globals (like functions, this is parsed
as regular C code even though it's in an @implementation section, because it's
not ObjC).

https://codereview.chromium.org/2203583002/diff/60001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:60: // TODO: how to find this from the PATH
env variable?
TBH, since this is a system utility I think that hard-coding it is best. There's
no risk of a user modifying their PATH and confusing the installer.

https://codereview.chromium.org/2203583002/diff/60001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:94: toPath:@"/Applications/Google
Chromo.app"
This path should come from outside (property, argument to init, etc.). It would
let tests use a different path, and reduce the risk of forgetting to change it
to "Chrome" later in the real installer, because it won't be buried here inside
Unpacker.

[Anna Zeng, 2016-08-12 22:56:18]

Ready for more review! Please note there are still TODOs that indicate areas of
improvement we are already aware of.

https://codereview.chromium.org/2203583002/diff/60001/chrome/installer/mac/ap...
File chrome/installer/mac/app/MainDelegate.m (right):

https://codereview.chromium.org/2203583002/diff/60001/chrome/installer/mac/ap...
chrome/installer/mac/app/MainDelegate.m:9: extern dispatch_semaphore_t
mount_semaphore;
On 2016/08/08 18:43:17, Sidney San Martín wrote:
> As discussed: a random extern definitely shouldn't be part of Unpacker's API,
> you can just use delegate callbacks.

Done.

https://codereview.chromium.org/2203583002/diff/60001/chrome/installer/mac/ap...
chrome/installer/mac/app/MainDelegate.m:40:
dispatch_semaphore_wait(mount_semaphore, DISPATCH_TIME_FOREVER);
On 2016/08/08 18:43:17, Sidney San Martín wrote:
> As discussed:
> 1. mountDMG and unmountDMG are implementation details, they shouldn't be part
of
> Unpacker's API. Instead, it should just take a URL and a destination and call
> back when it's done.
> 2. You shouldn't hold onto a thread for this. Use delegate callbacks.

#2 is taken care of; #1 is acknowledged and still being worked on.

https://codereview.chromium.org/2203583002/diff/60001/chrome/installer/mac/ap...
File chrome/installer/mac/app/Unpacker.m (right):

https://codereview.chromium.org/2203583002/diff/60001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:20: dispatch_queue_t unpack_dq;
On 2016/08/08 18:43:17, Sidney San Martín wrote:
> These should be instance variables, not globals (like functions, this is
parsed
> as regular C code even though it's in an @implementation section, because it's
> not ObjC).

Done.

https://codereview.chromium.org/2203583002/diff/60001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:60: // TODO: how to find this from the PATH
env variable?
On 2016/08/08 18:43:17, Sidney San Martín wrote:
> TBH, since this is a system utility I think that hard-coding it is best.
There's
> no risk of a user modifying their PATH and confusing the installer.

Done.

https://codereview.chromium.org/2203583002/diff/60001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:94: toPath:@"/Applications/Google
Chromo.app"
On 2016/08/08 18:43:18, Sidney San Martín wrote:
> This path should come from outside (property, argument to init, etc.). It
would
> let tests use a different path, and reduce the risk of forgetting to change it
> to "Chrome" later in the real installer, because it won't be buried here
inside
> Unpacker.

Done.

[Sidney San Martín, 2016-08-13 12:37:17]

LGTM with nits.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/AppDelegate.m (right):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/AppDelegate.m:125: if (![[NSWorkspace sharedWorkspace]
launchApplication:@"Google Chromo"]) {
Pass the full path or use `launchApplicationAtURL:options:configuration:error:`,
which takes an NSURL. Otherwise, if the user has another copy of Chrome on their
computer, it might get launched instead of the one you just installed.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/AppDelegate.m:135: [self displayError:extractError];
Is an unmount failure fatal/worth showing the user? Chrome is installed, so
might as well launch it and exit. The image will no longer be mounted after the
next reboot and the temp folder will be cleaned up by the OS eventually, so no
worries.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/Downloader.h (right):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/Downloader.h:13:
didDownloadData:(double)downloadProgressPercentage;
`downloader:didDownloadData:` suggests that the second parameter is some data.
I'd look for a clearer name — `downloader:progress:` would be fine, but there
are other good ones too.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/Downloader.h:24: // Downloads Chrome from the
|chromeImageURL| to the local hard drive.
No "the".

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/OmahaCommunication.h (right):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/OmahaCommunication.h:15:
onOmahaFailureWithError:(NSError*)error;
Since you added `omahaCommunication:` to these names, the second part doesn't
have to mention Omaha too.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/OmahaXMLRequest.m (right):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/OmahaXMLRequest.m:27:
objectForInfoDictionaryKey:@"CFBundleShortVersionString"]];
I just realized, if we'll always pass "3.0" as the protocol, that's definitely
not the installer app version and hard coding it makes sense.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/Unpacker.m (right):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:19: NSString* mountPath;
I believe instance variables should always have trailing underscores.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:29: static void unmount_callback(DADiskRef
disk,
These functions should not be in the @implementation section.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:32: if (dissenter) {
There's a similar block of code in eject_callback. I'd just call back to
Unpacker with the dissenter as quickly as possible to keep these functions short
and DRY.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:157: DADiskUnmount(whole_disk,
kDADiskUnmountOptionWhole, unmount_callback,
I might also pass `kDADiskUnmountOptionForce`. If some OS service is looking at
the disk image, that shouldn't trip up the installer.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/testing/Unpacker_test.mm (right):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/testing/Unpacker_test.mm:37:
initWithFinalAppPath:@"/Applications/Google Chromo.app"];
It would be better if the test used a temporary folder instead of putting stuff
in /Applications.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/testing/Unpacker_test.mm:53:
ASSERT_TRUE(CFBooleanGetValue((CFBooleanRef)mediaEjectableKey));
It would be way simpler to just look for a file (like "Google Chrome.app")
inside the mountpoint, no DA calls needed.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/testing/Unpacker_test.mm:63: EXPECT_FALSE(disk_check);
Ditto.

[Elly Fong-Jones, 2016-08-16 15:26:18]

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/AppDelegate.m (right):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/AppDelegate.m:82: Unpacker* unpacker = [[Unpacker
alloc]
who owns the unpacker? it looks like we don't store it anywhere

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/AppDelegate.m:125: if (![[NSWorkspace sharedWorkspace]
launchApplication:@"Google Chromo"]) {
Also, we're not called "Chromo" :)

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/BUILD.gn (left):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/BUILD.gn:35: "AppKit.framework",
I'm surprised we no longer need these frameworks?

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/NSAlert+ChromeInstallerAdditions.h (left):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/NSAlert+ChromeInstallerAdditions.h:12: -
(NSModalResponse)quitButton;
This isn't a button. Maybe (NSModalResponse)userResponse or something?

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/OmahaXMLParser.m (left):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/OmahaXMLParser.m:37: *error = [NSError
errorWithDomain:@"ChromeErrorDomain" code:1 userInfo:nil];
what's code 1?

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/Unpacker.m (right):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:116: // unpack_dq =
dispatch_queue_create("com.google.chrome.unpack",
does this commented code need to be here?

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:124:
dispatch_semaphore_wait(mount_semaphore, DISPATCH_TIME_FOREVER);
is this running on a work queue or somesuch? otherwise this could block the UI

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/testing/Unpacker_test.mm (right):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/testing/Unpacker_test.mm:20: if ([fileManager
removeItemAtPath:@"/Applications/Google Chromo.app"
Chrome :)

[Anna Zeng, 2016-08-16 23:07:40]

Ready for more feedback!

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/AppDelegate.m (right):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/AppDelegate.m:82: Unpacker* unpacker = [[Unpacker
alloc]
On 2016/08/16 15:26:18, Elly Jones wrote:
> who owns the unpacker? it looks like we don't store it anywhere

Currently, nobody owns it, just the same as how we treat the other classes.
However, since the unpacker will care about privilege escalation, we'll be
making Unpacker an instance variable in AppDelegate.m. Oh, what luck! You might
see it in the next patch here.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/AppDelegate.m:125: if (![[NSWorkspace sharedWorkspace]
launchApplication:@"Google Chromo"]) {
On 2016/08/13 12:37:18, Sidney San Martín wrote:
> Pass the full path or use
`launchApplicationAtURL:options:configuration:error:`,
> which takes an NSURL. Otherwise, if the user has another copy of Chrome on
their
> computer, it might get launched instead of the one you just installed.

Done.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/AppDelegate.m:125: if (![[NSWorkspace sharedWorkspace]
launchApplication:@"Google Chromo"]) {
On 2016/08/16 15:26:18, Elly Jones wrote:
> Also, we're not called "Chromo" :)

I made a note of this, and added a TODO to make this a command-line flag.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/AppDelegate.m:135: [self displayError:extractError];
On 2016/08/13 12:37:18, Sidney San Martín wrote:
> Is an unmount failure fatal/worth showing the user? Chrome is installed, so
> might as well launch it and exit. The image will no longer be mounted after
the
> next reboot and the temp folder will be cleaned up by the OS eventually, so no
> worries.

This is true! I have now moved the unmount step to be something that happens
after launching Chrome.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/BUILD.gn (left):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/BUILD.gn:35: "AppKit.framework",
On 2016/08/16 15:26:18, Elly Jones wrote:
> I'm surprised we no longer need these frameworks?

The Cocoa Framework includes AppKit, CoreFoundation, and Foundation; since we
use all three, it felt redundant to write those three separately when one could
simply write Cocoa.framework.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/Downloader.h (right):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/Downloader.h:13:
didDownloadData:(double)downloadProgressPercentage;
On 2016/08/13 12:37:18, Sidney San Martín wrote:
> `downloader:didDownloadData:` suggests that the second parameter is some data.
> I'd look for a clearer name — `downloader:progress:` would be fine, but there
> are other good ones too.

Done.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/Downloader.h:24: // Downloads Chrome from the
|chromeImageURL| to the local hard drive.
On 2016/08/13 12:37:18, Sidney San Martín wrote:
> No "the".

Done.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/NSAlert+ChromeInstallerAdditions.h (left):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/NSAlert+ChromeInstallerAdditions.h:12: -
(NSModalResponse)quitButton;
On 2016/08/16 15:26:18, Elly Jones wrote:
> This isn't a button. Maybe (NSModalResponse)userResponse or something?

Done.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/OmahaCommunication.h (right):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/OmahaCommunication.h:15:
onOmahaFailureWithError:(NSError*)error;
On 2016/08/13 12:37:18, Sidney San Martín wrote:
> Since you added `omahaCommunication:` to these names, the second part doesn't
> have to mention Omaha too.

Done.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/OmahaXMLParser.m (left):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/OmahaXMLParser.m:37: *error = [NSError
errorWithDomain:@"ChromeErrorDomain" code:1 userInfo:nil];
On 2016/08/16 15:26:18, Elly Jones wrote:
> what's code 1?

As the newly added TODO indicates: the error created here doesn't matter -- as
long as an error gets passed up to OmahaCommunication, we have been able to
communicate that the parsing did not succeed in getting what we wanted (a usable
URL to download Chrome from). It'd be helpful in the future to have error play a
bigger role in determining what should be done to take care of the error, but
for now it remains a TODO.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/OmahaXMLRequest.m (right):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/OmahaXMLRequest.m:27:
objectForInfoDictionaryKey:@"CFBundleShortVersionString"]];
On 2016/08/13 12:37:18, Sidney San Martín wrote:
> I just realized, if we'll always pass "3.0" as the protocol, that's definitely
> not the installer app version and hard coding it makes sense.

Done.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/Unpacker.m (right):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:19: NSString* mountPath;
On 2016/08/13 12:37:18, Sidney San Martín wrote:
> I believe instance variables should always have trailing underscores.

Done.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:29: static void unmount_callback(DADiskRef
disk,
On 2016/08/13 12:37:19, Sidney San Martín wrote:
> These functions should not be in the @implementation section.

Done.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:32: if (dissenter) {
On 2016/08/13 12:37:19, Sidney San Martín wrote:
> There's a similar block of code in eject_callback. I'd just call back to
> Unpacker with the dissenter as quickly as possible to keep these functions
short
> and DRY.

Done.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:116: // unpack_dq =
dispatch_queue_create("com.google.chrome.unpack",
On 2016/08/16 15:26:18, Elly Jones wrote:
> does this commented code need to be here?

This code was not completely deleted in case I needed to use the dispatch queue
again.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:124:
dispatch_semaphore_wait(mount_semaphore, DISPATCH_TIME_FOREVER);
On 2016/08/16 15:26:18, Elly Jones wrote:
> is this running on a work queue or somesuch? otherwise this could block the UI

Currently, it is not. Would you recommend running this method call in a work
queue in that case?

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/Unpacker.m:157: DADiskUnmount(whole_disk,
kDADiskUnmountOptionWhole, unmount_callback,
On 2016/08/13 12:37:18, Sidney San Martín wrote:
> I might also pass `kDADiskUnmountOptionForce`. If some OS service is looking
at
> the disk image, that shouldn't trip up the installer.

Done.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/testing/Unpacker_test.mm (right):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/testing/Unpacker_test.mm:20: if ([fileManager
removeItemAtPath:@"/Applications/Google Chromo.app"
On 2016/08/16 15:26:18, Elly Jones wrote:
> Chrome :)

Acknowledged.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/testing/Unpacker_test.mm:37:
initWithFinalAppPath:@"/Applications/Google Chromo.app"];
On 2016/08/13 12:37:19, Sidney San Martín wrote:
> It would be better if the test used a temporary folder instead of putting
stuff
> in /Applications.

Good point! Done.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/testing/Unpacker_test.mm:53:
ASSERT_TRUE(CFBooleanGetValue((CFBooleanRef)mediaEjectableKey));
On 2016/08/13 12:37:19, Sidney San Martín wrote:
> It would be way simpler to just look for a file (like "Google Chrome.app")
> inside the mountpoint, no DA calls needed.

Done.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/testing/Unpacker_test.mm:63: EXPECT_FALSE(disk_check);
On 2016/08/13 12:37:19, Sidney San Martín wrote:
> Ditto.

Done.

[Sidney San Martín, 2016-08-17 22:09:06]

Still just nits.

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
File chrome/installer/mac/app/AppDelegate.m (right):

https://codereview.chromium.org/2203583002/diff/80001/chrome/installer/mac/ap...
chrome/installer/mac/app/AppDelegate.m:82: Unpacker* unpacker = [[Unpacker
alloc]
On 2016/08/16 15:26:18, Elly Jones wrote:
> who owns the unpacker? it looks like we don't store it anywhere

ellyjones@: Just curious, is the plan to use ARC for the installer?

https://codereview.chromium.org/2203583002/diff/100001/chrome/installer/mac/a...
File chrome/installer/mac/app/Unpacker.h (right):

https://codereview.chromium.org/2203583002/diff/100001/chrome/installer/mac/a...
chrome/installer/mac/app/Unpacker.h:23: @property(nonatomic, assign)
NSObject<UnpackDelegate>* delegate;
Instead of this, you can just declare your delegate protocol like this:

    @protocol UnpackDelegate : NSObject
    …
    @end

https://codereview.chromium.org/2203583002/diff/100001/chrome/installer/mac/a...
File chrome/installer/mac/app/Unpacker.m (right):

https://codereview.chromium.org/2203583002/diff/100001/chrome/installer/mac/a...
chrome/installer/mac/app/Unpacker.m:20: WithDissenter:(DADissenterRef)dissenter;
W → w

https://codereview.chromium.org/2203583002/diff/100001/chrome/installer/mac/a...
File chrome/installer/mac/app/testing/Unpacker_test.mm (right):

https://codereview.chromium.org/2203583002/diff/100001/chrome/installer/mac/a...
chrome/installer/mac/app/testing/Unpacker_test.mm:85: test_semaphore =
dispatch_semaphore_create(0);
I think it would be better to make test_semaphore an instance variable on
TestDelegate, and add a `wait` method for the test to call, instead of having
them both access this global variable.

https://codereview.chromium.org/2203583002/diff/100001/chrome/installer/mac/a...
chrome/installer/mac/app/testing/Unpacker_test.mm:89: NSDownloadsDirectory,
NSUserDomainMask, YES) firstObject];
Does this test only work if you already have GoogleChrome.dmg in your downloads
folder? If so, we should talk about removing that requirement.

[Sidney San Martín, 2016-08-18 19:33:00]

https://codereview.chromium.org/2203583002/diff/100001/chrome/installer/mac/a...
File chrome/installer/mac/app/Unpacker.h (right):

https://codereview.chromium.org/2203583002/diff/100001/chrome/installer/mac/a...
chrome/installer/mac/app/Unpacker.h:23: @property(nonatomic, assign)
NSObject<UnpackDelegate>* delegate;
On 2016/08/17 22:09:06, Sidney San Martín wrote:
> Instead of this, you can just declare your delegate protocol like this:
> 
>     @protocol UnpackDelegate : NSObject
>     …
>     @end

Just for posterity…

I got the syntax wrong (by mixing in some Swift). It's actually:

     @protocol UnpackDelegate<NSObject>
     …
     @end

[Anna Zeng, 2016-08-18 19:41:09]

https://codereview.chromium.org/2203583002/diff/100001/chrome/installer/mac/a...
File chrome/installer/mac/app/Unpacker.h (right):

https://codereview.chromium.org/2203583002/diff/100001/chrome/installer/mac/a...
chrome/installer/mac/app/Unpacker.h:23: @property(nonatomic, assign)
NSObject<UnpackDelegate>* delegate;
On 2016/08/17 at 22:09:06, Sidney San Martín wrote:
> Instead of this, you can just declare your delegate protocol like this:
> 
>     @protocol UnpackDelegate : NSObject
>     …
>     @end

Done

https://codereview.chromium.org/2203583002/diff/100001/chrome/installer/mac/a...
File chrome/installer/mac/app/Unpacker.m (right):

https://codereview.chromium.org/2203583002/diff/100001/chrome/installer/mac/a...
chrome/installer/mac/app/Unpacker.m:20: WithDissenter:(DADissenterRef)dissenter;
On 2016/08/17 at 22:09:06, Sidney San Martín wrote:
> W → w

Done

https://codereview.chromium.org/2203583002/diff/100001/chrome/installer/mac/a...
File chrome/installer/mac/app/testing/Unpacker_test.mm (right):

https://codereview.chromium.org/2203583002/diff/100001/chrome/installer/mac/a...
chrome/installer/mac/app/testing/Unpacker_test.mm:85: test_semaphore =
dispatch_semaphore_create(0);
On 2016/08/17 at 22:09:06, Sidney San Martín wrote:
> I think it would be better to make test_semaphore an instance variable on
TestDelegate, and add a `wait` method for the test to call, instead of having
them both access this global variable.

Good point! Done.

https://codereview.chromium.org/2203583002/diff/100001/chrome/installer/mac/a...
chrome/installer/mac/app/testing/Unpacker_test.mm:89: NSDownloadsDirectory,
NSUserDomainMask, YES) firstObject];
On 2016/08/17 at 22:09:06, Sidney San Martín wrote:
> Does this test only work if you already have GoogleChrome.dmg in your
downloads folder? If so, we should talk about removing that requirement.

Doing.

[Anna Zeng, 2016-08-19 19:21:19]

Ready for review! Hosts, please leave feedback!

[Sidney San Martín, 2016-08-19 22:06:05]

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
File chrome/installer/mac/app/AppDelegate.m (right):

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
chrome/installer/mac/app/AppDelegate.m:47: // not terminate when we call
windowOut: to hide the installer during its
orderOut:

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
chrome/installer/mac/app/AppDelegate.m:110: // By disabling closing the window,
we can guarantee that the user will not
I don't think guarantee is the right word :). They could still force quit the
installer or unplug their computer, or the installer could crash. Another word
might be better.

We should def. look into doing an atomic move, so that it's less likely we
overwrite a working Chrome with a broken one… but that doesn't have to happen
now.

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
chrome/installer/mac/app/AppDelegate.m:113: window_.styleMask &=
~NSClosableWindowMask;
You also need to disable quit. Implementing applicationShouldTerminate: to
return NO might be enough.

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
chrome/installer/mac/app/AppDelegate.m:135: [NSURL
fileURLWithPath:@"/Applications/Google Chromo.app"
All of these `@"/Applications/Google Chromo.app"`s should probably point to one
`const NSString*` defined at the top of the file.

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
File chrome/installer/mac/app/BUILD.gn (right):

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
chrome/installer/mac/app/BUILD.gn:19: configs += [
"//build/config/compiler:enable_arc" ]
\o/

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
File chrome/installer/mac/app/Info.plist (right):

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
chrome/installer/mac/app/Info.plist:8: <string>mac_installer_app</string>
I'd probably hold off on changing this until you/Ivan can fix them all, instead
of just hard-coding this one.

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
File chrome/installer/mac/app/InstallerWindowController.m (right):

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
chrome/installer/mac/app/InstallerWindowController.m:119:
progressBar_.usesThreadedAnimation = YES;
Looks like this is on by default (on my machine, at least).

[Anna Zeng, 2016-08-23 03:13:43]

Again, looking for feedback! Thanks Sidney for the insightful comments.

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
File chrome/installer/mac/app/AppDelegate.m (right):

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
chrome/installer/mac/app/AppDelegate.m:47: // not terminate when we call
windowOut: to hide the installer during its
On 2016/08/19 22:06:04, Sidney San Martín wrote:
> orderOut:

Done.

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
chrome/installer/mac/app/AppDelegate.m:110: // By disabling closing the window,
we can guarantee that the user will not
On 2016/08/19 22:06:04, Sidney San Martín wrote:
> I don't think guarantee is the right word :). They could still force quit the
> installer or unplug their computer, or the installer could crash. Another word
> might be better.
> 
> We should def. look into doing an atomic move, so that it's less likely we
> overwrite a working Chrome with a broken one… but that doesn't have to
happen
> now.

Done.

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
chrome/installer/mac/app/AppDelegate.m:113: window_.styleMask &=
~NSClosableWindowMask;
On 2016/08/19 22:06:04, Sidney San Martín wrote:
> You also need to disable quit. Implementing applicationShouldTerminate: to
> return NO might be enough.

Done.

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
chrome/installer/mac/app/AppDelegate.m:135: [NSURL
fileURLWithPath:@"/Applications/Google Chromo.app"
On 2016/08/19 22:06:04, Sidney San Martín wrote:
> All of these `@"/Applications/Google Chromo.app"`s should probably point to
one
> `const NSString*` defined at the top of the file.

This portion of the code will be taken care of by another CL currently in
progress. Acknowledged!

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
File chrome/installer/mac/app/BUILD.gn (right):

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
chrome/installer/mac/app/BUILD.gn:19: configs += [
"//build/config/compiler:enable_arc" ]
On 2016/08/19 22:06:04, Sidney San Martín wrote:
> \o/

Acknowledged.

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
File chrome/installer/mac/app/Info.plist (right):

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
chrome/installer/mac/app/Info.plist:8: <string>mac_installer_app</string>
On 2016/08/19 22:06:04, Sidney San Martín wrote:
> I'd probably hold off on changing this until you/Ivan can fix them all,
instead
> of just hard-coding this one.

Done.

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
File chrome/installer/mac/app/InstallerWindowController.m (right):

https://codereview.chromium.org/2203583002/diff/120001/chrome/installer/mac/a...
chrome/installer/mac/app/InstallerWindowController.m:119:
progressBar_.usesThreadedAnimation = YES;
On 2016/08/19 22:06:04, Sidney San Martín wrote:
> Looks like this is on by default (on my machine, at least).

Done.

[Anna Zeng, 2016-08-23 17:06:52]

Added in the verification Mark mentioned should take the place of `hdiutil`
checksumming. Looking for review!

[Elly Fong-Jones, 2016-08-23 21:57:40]

lgtm! Looking really good :D

https://codereview.chromium.org/2203583002/diff/160001/chrome/installer/mac/a...
File chrome/installer/mac/app/AppDelegate.m (right):

https://codereview.chromium.org/2203583002/diff/160001/chrome/installer/mac/a...
chrome/installer/mac/app/AppDelegate.m:52: [self exit];
does orderOut constitute a window close?

https://codereview.chromium.org/2203583002/diff/160001/chrome/installer/mac/a...
chrome/installer/mac/app/AppDelegate.m:127:
fileExistsAtPath:@"/Applications/Google Chromo.app"]) {
This constant should probably not be repeated all over the place :)

[Anna Zeng, 2016-08-24 01:30:10]

Submitting!

https://codereview.chromium.org/2203583002/diff/160001/chrome/installer/mac/a...
File chrome/installer/mac/app/AppDelegate.m (right):

https://codereview.chromium.org/2203583002/diff/160001/chrome/installer/mac/a...
chrome/installer/mac/app/AppDelegate.m:52: [self exit];
On 2016/08/23 21:57:40, Elly Jones wrote:
> does orderOut constitute a window close?

The window is hidden when we call orderOut. To the user the window is
effectively gone, but the window isn't actually released until the app itself
terminates.

https://codereview.chromium.org/2203583002/diff/160001/chrome/installer/mac/a...
chrome/installer/mac/app/AppDelegate.m:127:
fileExistsAtPath:@"/Applications/Google Chromo.app"]) {
On 2016/08/23 21:57:40, Elly Jones wrote:
> This constant should probably not be repeated all over the place :)

Acknowledged. Fixed temporarily; this code would soon be replaced with a more
fully-written class.

[Anna Zeng, 2016-08-24 01:30:15]

The CQ bit was checked by zengster@google.com

[Anna Zeng, 2016-08-24 01:30:15]

The patchset sent to the CQ was uploaded after l-g-t-m from sdy@chromium.org,
ellyjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2203583002/#ps180001
(title: "Brushing up before CQ")

[commit-bot: I haz the power, 2016-08-24 01:31:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 02:13:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 02:14:00]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Anna Zeng, 2016-08-24 02:57:53]

The CQ bit was checked by zengster@google.com

[Anna Zeng, 2016-08-24 02:57:54]

The patchset sent to the CQ was uploaded after l-g-t-m from sdy@chromium.org,
ellyjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2203583002/#ps200001
(title: "Pleasing the commitbots!")

[commit-bot: I haz the power, 2016-08-24 02:58:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 04:26:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 04:26:49]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Anna Zeng, 2016-08-24 12:59:46]

The CQ bit was checked by zengster@google.com

[Anna Zeng, 2016-08-24 12:59:47]

The patchset sent to the CQ was uploaded after l-g-t-m from sdy@chromium.org,
ellyjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2203583002/#ps220001
(title: "Correcting the BUILD file to point to the correct location of test
files")

[commit-bot: I haz the power, 2016-08-24 13:00:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 13:06:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 13:06:35]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Anna Zeng, 2016-08-24 13:41:38]

The CQ bit was checked by zengster@google.com

[commit-bot: I haz the power, 2016-08-24 13:41:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 13:47:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 13:47:59]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Anna Zeng, 2016-08-24 14:28:41]

The CQ bit was checked by zengster@google.com

[Anna Zeng, 2016-08-24 14:28:42]

The patchset sent to the CQ was uploaded after l-g-t-m from sdy@chromium.org,
ellyjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2203583002/#ps240001
(title: "Commitbots, be pleased!")

[commit-bot: I haz the power, 2016-08-24 14:29:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 15:30:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 15:30:01]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Anna Zeng, 2016-08-24 18:00:58]

The CQ bit was checked by zengster@google.com

[Anna Zeng, 2016-08-24 18:00:58]

The patchset sent to the CQ was uploaded after l-g-t-m from sdy@chromium.org,
ellyjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2203583002/#ps260001
(title: "Fixed a segfault!")

[commit-bot: I haz the power, 2016-08-24 18:01:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 18:07:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 18:07:06]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Anna Zeng, 2016-08-24 21:17:21]

The CQ bit was checked by zengster@google.com

[Anna Zeng, 2016-08-24 21:17:22]

The patchset sent to the CQ was uploaded after l-g-t-m from sdy@chromium.org,
ellyjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2203583002/#ps280001
(title: "Rebased")

[commit-bot: I haz the power, 2016-08-24 21:17:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 22:05:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 22:05:26]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Anna Zeng, 2016-08-26 20:47:14]

The CQ bit was checked by zengster@google.com

[Anna Zeng, 2016-08-26 20:47:15]

The patchset sent to the CQ was uploaded after l-g-t-m from sdy@chromium.org,
ellyjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2203583002/#ps300001
(title: "Rebased again!")

[commit-bot: I haz the power, 2016-08-26 20:48:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 22:24:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 22:24:08]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Anna Zeng, 2016-08-29 21:10:35]

The CQ bit was checked by zengster@google.com

[Anna Zeng, 2016-08-29 21:10:36]

The patchset sent to the CQ was uploaded after l-g-t-m from sdy@chromium.org,
ellyjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2203583002/#ps360001
(title: "adjusted a small error")

[commit-bot: I haz the power, 2016-08-29 21:43:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Anna Zeng, 2016-08-30 01:01:14]

The CQ bit was checked by zengster@google.com

[Anna Zeng, 2016-08-30 01:01:14]

The patchset sent to the CQ was uploaded after l-g-t-m from sdy@chromium.org,
ellyjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2203583002/#ps380001
(title: "Added DMG to build file")

[commit-bot: I haz the power, 2016-08-30 01:18:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 04:37:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 04:37:55]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Anna Zeng, 2016-08-30 04:50:45]

The CQ bit was checked by zengster@google.com

[commit-bot: I haz the power, 2016-08-30 04:53:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 06:19:30]

Description was changed from

==========
Added unpacking step

BUG=
==========

to

==========
Added unpacking step

BUG=
==========

[commit-bot: I haz the power, 2016-08-30 06:19:32]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2016-08-30 06:22:07]

Description was changed from

==========
Added unpacking step

BUG=
==========

to

==========
Added unpacking step

BUG=

Committed: https://crrev.com/d0136826964ad0e135f71b18661b78e1a3cf1c71
Cr-Commit-Position: refs/heads/master@{#415213}
==========

[commit-bot: I haz the power, 2016-08-30 06:22:08]

Patchset 20 (id:??) landed as
https://crrev.com/d0136826964ad0e135f71b18661b78e1a3cf1c71
Cr-Commit-Position: refs/heads/master@{#415213}