Migrate mac installer to delegate-driven model

Migrate mac installer to delegate-driven model

BUG=

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

[Anna Zeng, 2016-07-15 20:00:58]

Description was changed from

==========
Adjusted main.m for clarity & readability

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

to

==========
Adjusted main.m for clarity & readability

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

[Anna Zeng, 2016-07-15 20:00:58]

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

[Anna Zeng, 2016-07-15 20:08:06]

This is the progress Ivan and I have made so far in figuring out how to
restructure main.m to be more clean (we also moved this content to
AppDelegate.m, but that will be bundled with a future UI cl). If we continue
going in this direction, most of the code will be structured around blocks (this
may also include writing a wrapper around DownloadDelegate that uses blocks, as
Sidney kindly suggested). Is this the right way to go here? Or would using all
delegates or selectors be a better avenue to take?

[Anna Zeng, 2016-07-15 21:54:53]

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

[Anna Zeng, 2016-07-18 17:52:48]

On 2016/07/15 20:08:06, Anna Zeng wrote:
> This is the progress Ivan and I have made so far in figuring out how to
> restructure main.m to be more clean (we also moved this content to
> AppDelegate.m, but that will be bundled with a future UI cl). If we continue
> going in this direction, most of the code will be structured around blocks
(this
> may also include writing a wrapper around DownloadDelegate that uses blocks,
as
> Sidney kindly suggested). Is this the right way to go here? Or would using all
> delegates or selectors be a better avenue to take?

Ping hosts! Requesting review :D

[Sidney San Martín, 2016-07-18 18:28:22]

On 2016/07/15 20:08:06, Anna Zeng wrote:
> If we continue going in this direction, most of the code will be structured
> around blocks (this may also include writing a wrapper around
> DownloadDelegate that uses blocks, as Sidney kindly suggested). Is this the
> right way to go here? Or would using all delegates or selectors be a better
> avenue to take?

I think that, since the AppDelegate only ever has one Downloader and needs to
receive a couple of different kinds of feedback from it (progress, completion),
a delegate would actually make more sense! Downloader could have a
DownloaderDelegate protocol, with methods like:

- -[DownloaderDelegate downloadProgress:]
- -[DownloaderDelegate downloadFinished:]
- -[DownloaderDelegate downloadFailedWithError:]

[Sidney San Martín, 2016-07-18 18:28:52]

https://codereview.chromium.org/2148293005/diff/1/chrome/installer/mac/app/Do...
File chrome/installer/mac/app/DownloadDelegate.m (right):

https://codereview.chromium.org/2148293005/diff/1/chrome/installer/mac/app/Do...
chrome/installer/mac/app/DownloadDelegate.m:10: @implementation DownloadDelegate
: NSObject
Not strictly related to this CL, but you probably don't need this class.
Downloader should serve as the NSURLSessionDownloadDelegate.

https://codereview.chromium.org/2148293005/diff/1/chrome/installer/mac/app/Do...
chrome/installer/mac/app/DownloadDelegate.m:31: [manager copyItemAtURL:location
toURL:downloadsDirectory error:nil];
You should probably move the file, not copy it — else the downloaded copy will
hang out until the system cleans it up (e.g. on the next restart).

https://codereview.chromium.org/2148293005/diff/1/chrome/installer/mac/app/ma...
File chrome/installer/mac/app/main.m (right):

https://codereview.chromium.org/2148293005/diff/1/chrome/installer/mac/app/ma...
chrome/installer/mac/app/main.m:22: downloadChrome();
It's confusing that talkToOmaha() also starts the download. A middle ground
would be to still have it take a block (the function can still be called
talkToOmaha), and then in main():

    talkToOmaha(^{ downloadChrome(); });

Or, talkToOmaha needs a better name.

https://codereview.chromium.org/2148293005/diff/1/chrome/installer/mac/app/ma...
chrome/installer/mac/app/main.m:36: runUntilDate:[NSDate
dateWithTimeIntervalSinceNow:3]];
You should probably just call `run`, and then exit() when you want the app to
terminate. (When you have a GUI, `[NSApp terminate:nil]` will also do the job).

[Anna Zeng, 2016-07-19 19:23:48]

Requesting review! We have addressed all comments included thus far, and welcome
more perspectives.

[Mark Mentovai, 2016-07-19 21:07:40]

I like this!

Anna, if you wrote this, have you let Ivan look at for review? We should be
doing that too.

I’ll let Elly make a pass also, since she had a more in-depth discussion with
you this morning, but I think that this is a nice improvement.

https://codereview.chromium.org/2148293005/diff/40001/chrome/installer/mac/ap...
File chrome/installer/mac/app/Downloader.m (right):

https://codereview.chromium.org/2148293005/diff/40001/chrome/installer/mac/ap...
chrome/installer/mac/app/Downloader.m:6: #import <assert.h>
<assert.h> gets #included, not #imported. We generally sort headers without
regard to case, so assert < Foundation.

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

https://codereview.chromium.org/2148293005/diff/40001/chrome/installer/mac/ap...
chrome/installer/mac/app/MainDelegate.m:6: #import <Cocoa/Cocoa.h>
Sort ’em! But if you’re #importing Cocoa, there’s no need to also #import
Foundation.

[Sidney San Martín, 2016-07-19 23:19:20]

https://codereview.chromium.org/2148293005/diff/40001/chrome/installer/mac/ap...
File chrome/installer/mac/app/MainDelegate.h (right):

https://codereview.chromium.org/2148293005/diff/40001/chrome/installer/mac/ap...
chrome/installer/mac/app/MainDelegate.h:12: @end
main.m knows that it's dealing with an instance of MainDelegate, so
`talkToOmaha` can be a regular method. You don't need this protocol.

https://codereview.chromium.org/2148293005/diff/40001/chrome/installer/mac/ap...
chrome/installer/mac/app/MainDelegate.h:16: @end
In an app like this, delegate protocols are meant to help with separation of
concerns. MainDelegate needs to know about OmahaCommunication, but
OmahaCommunication and the other helper classes don't need to know about
MainDelegate (and shouldn't import MainDelegate.h).

Instead of this DownloadDelegate protocol, you might want one like this in
OmahaCommunication.h:

    @protocol OmahaCommunicationDelegate
    - (void)omahaRequestFinishedWithData:(NSData *);
    @end

It makes moving responsibility around easier: if you ever want a different class
to be an OmahaCommunicationDelegate, or you want something other than
downloading Chrome to happen after you get the response from Omaha, you don't
have to change OmahaCommunication.{h,m} at all.

It also makes control flow in MainDelegate clearer: It calls
`-[OmahaCommunication sendRequest]`, then later it gets called back with
`omahaRequestFinishedWithData:`. (You don't have to remember that `sendRequest`
causes `downloadChromeWithData:` to be called).

https://codereview.chromium.org/2148293005/diff/40001/chrome/installer/mac/ap...
chrome/installer/mac/app/MainDelegate.h:20: @end
Same thing here.

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

https://codereview.chromium.org/2148293005/diff/40001/chrome/installer/mac/ap...
chrome/installer/mac/app/MainDelegate.m:21: [messenger setDelegate:self];
Pretty trivial: you can just say `messenger.delegate = self;`, instead.

https://codereview.chromium.org/2148293005/diff/40001/chrome/installer/mac/ap...
chrome/installer/mac/app/MainDelegate.m:28: [download setDelegate:self];
Ditto, could be `download.delegate = self;`.

https://codereview.chromium.org/2148293005/diff/40001/chrome/installer/mac/ap...
File chrome/installer/mac/app/OmahaCommunication.m (left):

https://codereview.chromium.org/2148293005/diff/40001/chrome/installer/mac/ap...
chrome/installer/mac/app/OmahaCommunication.m:20: sessionHelper_ =
[[NetworkCommunication alloc] init];
IMHO the NetworkCommunication class works in a pretty confusing way. Let's chat
about it sometime (but don't worry about it right now).

[Anna Zeng, 2016-07-21 18:06:44]

https://codereview.chromium.org/2148293005/diff/40001/chrome/installer/mac/ap...
File chrome/installer/mac/app/MainDelegate.h (right):

https://codereview.chromium.org/2148293005/diff/40001/chrome/installer/mac/ap...
chrome/installer/mac/app/MainDelegate.h:16: @end
On 2016/07/19 23:19:20, Sidney San Martín wrote:
> In an app like this, delegate protocols are meant to help with separation of
> concerns. MainDelegate needs to know about OmahaCommunication, but
> OmahaCommunication and the other helper classes don't need to know about
> MainDelegate (and shouldn't import MainDelegate.h).
> 
> Instead of this DownloadDelegate protocol, you might want one like this in
> OmahaCommunication.h:
> 
>     @protocol OmahaCommunicationDelegate
>     - (void)omahaRequestFinishedWithData:(NSData *);
>     @end
> 
> It makes moving responsibility around easier: if you ever want a different
class
> to be an OmahaCommunicationDelegate, or you want something other than
> downloading Chrome to happen after you get the response from Omaha, you don't
> have to change OmahaCommunication.{h,m} at all.
> 
> It also makes control flow in MainDelegate clearer: It calls
> `-[OmahaCommunication sendRequest]`, then later it gets called back with
> `omahaRequestFinishedWithData:`. (You don't have to remember that
`sendRequest`
> causes `downloadChromeWithData:` to be called).

Let's talk about this in real time too!

https://codereview.chromium.org/2148293005/diff/40001/chrome/installer/mac/ap...
File chrome/installer/mac/app/OmahaCommunication.m (left):

https://codereview.chromium.org/2148293005/diff/40001/chrome/installer/mac/ap...
chrome/installer/mac/app/OmahaCommunication.m:20: sessionHelper_ =
[[NetworkCommunication alloc] init];
On 2016/07/19 23:19:20, Sidney San Martín wrote:
> IMHO the NetworkCommunication class works in a pretty confusing way. Let's
chat
> about it sometime (but don't worry about it right now).

Sure thing! Just so you know our mentality behind NetworkCommunication going
into that conversation, we decided to create this class to simplify setting up
NSURLSession since we use it in two different cases within the application. We'd
love to hear your perspective on the pros and cons of this decision and your
suggestion for what to do about it! :D Thanks Sidney.

[Anna Zeng, 2016-07-21 18:49:47]

Description was changed from

==========
Adjusted main.m for clarity & readability

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

to

==========
Migrate mac installer to delegate-driven model

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

[Anna Zeng, 2016-07-21 18:50:35]

Ready for another round of eyes!

[Sidney San Martín, 2016-07-22 02:29:45]

On 2016/07/21 18:50:35, Anna Zeng wrote:
> Ready for another round of eyes!

I have a few minor comments, but nothing blocking. So: LGTM.

[Sidney San Martín, 2016-07-22 02:30:17]

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

https://codereview.chromium.org/2148293005/diff/60001/chrome/installer/mac/ap...
chrome/installer/mac/app/MainDelegate.h:15: - (void)onDownloadSuccess;
This method is already in DownloaderDelegate, an UnpackDelegate would probably
have different methods — maybe drop this protocol for now?

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

https://codereview.chromium.org/2148293005/diff/60001/chrome/installer/mac/ap...
chrome/installer/mac/app/OmahaCommunication.h:22: @property(nonatomic, copy)
NetworkCommunication* sessionHelper;
Not part of this CL, so feel free to ignore for now: It doesn't look like these
two properties are used (OmahaCommunication.m just assigns directly to the
instance variables) — and NetworkCommunication isn't copyable, so it would crash
if you tried to assign to this property. Maybe drop them? I'm not 100% sure on
the convention here around classes using ivars directly.

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

https://codereview.chromium.org/2148293005/diff/60001/chrome/installer/mac/ap...
chrome/installer/mac/app/OmahaCommunication.m:47: [_dataCollected
appendData:data];
You might be better off just using -[NSURLSession
dataTaskWithURL:completionHandler:] — it calls back with a complete NSData,
instead of giving you the pieces as they come in. This class could still take a
delegate, it's just an internal change.

https://codereview.chromium.org/2148293005/diff/60001/chrome/installer/mac/ap...
chrome/installer/mac/app/OmahaCommunication.m:54: [delegate_
onOmahaSuccessWithData:_dataCollected];
The delegate should get the error, if there is one. For now, you could just
handle errors in MainDelegate with an NSLog(…); exit(1);.

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

https://codereview.chromium.org/2148293005/diff/60001/chrome/installer/mac/ap...
chrome/installer/mac/app/main.m:15: //     runUntilDate:[NSDate
dateWithTimeIntervalSinceNow:10]];
Maybe delete this, instead of leaving it commented out?

[Mark Mentovai, 2016-07-22 15:15:13]

Nice refactor. LGTM when the remaining feedback is addressed.

[Anna Zeng, 2016-07-22 16:19:15]

The CQ bit was checked by zengster@google.com

[Anna Zeng, 2016-07-22 16:19:15]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
sdy@chromium.org
Link to the patchset: https://codereview.chromium.org/2148293005/#ps80001
(title: "Last patch before CQ")

[Anna Zeng, 2016-07-22 16:19:23]

The CQ bit was checked by zengster@google.com

[commit-bot: I haz the power, 2016-07-22 16:19:32]

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-07-22 17:08:34]

Description was changed from

==========
Migrate mac installer to delegate-driven model

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

to

==========
Migrate mac installer to delegate-driven model

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

[commit-bot: I haz the power, 2016-07-22 17:08:35]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-22 17:10:27]

Description was changed from

==========
Migrate mac installer to delegate-driven model

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

to

==========
Migrate mac installer to delegate-driven model

BUG=

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

[commit-bot: I haz the power, 2016-07-22 17:10:28]

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