DownloadManagerController now uses WebState

DownloadManagerController now uses WebState

Instead of using StoreKitLauncher, DownloadManagerController can now
get everything it needs from WebState.

BUG=684063
Review-Url: https://codereview.chromium.org/2740473002
Cr-Commit-Position: refs/heads/master@{#455538}
Committed: https://chromium.googlesource.com/chromium/src/+/998ad027faf59d366cd87996d3060e224d4609f1

[pkl (ping after 24h if needed), 2017-03-07 02:35:54]

pkl@chromium.org changed reviewers:
+ rohitrao@chromium.org, sdefresne@chromium.org

[pkl (ping after 24h if needed), 2017-03-07 02:35:54]

PTAL. This is a continuation to use StoreKitTabHelper for everything.

[rohitrao (ping after 24h), 2017-03-07 13:51:22]

lgtm with a note for sdefresne

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/tabs/tab.mm
File ios/chrome/browser/tabs/tab.mm (left):

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab.mm:1459: return nil;
There is a small logic change here, in that the old code would not open the
download controller at all if there was no storekit launcher.  The new code will
open the download controller but do nothing when you try to go to the app store.

Nothing needs to be changed here, we should just be aware of the change.

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/tabs/tab.mm
File ios/chrome/browser/tabs/tab.mm (right):

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab.mm:1455: [[[DownloadManagerController alloc]
initWithWebState:self.webState
This addresses the "followup" comments that I left on the other CL =)

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/ui/downl...
File ios/chrome/browser/ui/downloads/download_manager_controller_unittest.mm
(right):

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/ui/downl...
ios/chrome/browser/ui/downloads/download_manager_controller_unittest.mm:51:
class DownloadManagerControllerTest : public ChromeWebTest {
These changes end up making the test more heavyweight, because the controller
relies on objects that are buried inside WebState.

sdefresne@ does not like this, because it forces us to write "integration" tests
instead of unit tests.  Nothing to change in this CL, but we should explore ways
to restructure the helpers to make them more testable.

[sdefresne, 2017-03-07 17:15:49]

lgtm

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/ui/downl...
File ios/chrome/browser/ui/downloads/download_manager_controller.h (right):

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/ui/downl...
ios/chrome/browser/ui/downloads/download_manager_controller.h:27: -
(id)initWithWebState:(web::WebState*)webState downloadURL:(const GURL&)url;
nit: can you use the opportunity you are changing this line to also change the
return type to "instancetype"?

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/ui/downl...
File ios/chrome/browser/ui/downloads/download_manager_controller.mm (right):

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/ui/downl...
ios/chrome/browser/ui/downloads/download_manager_controller.mm:523: -
(id)initWithWebState:(web::WebState*)webState downloadURL:(const GURL&)url {
ditto

[pkl (ping after 24h if needed), 2017-03-07 22:07:19]

Thank you! Addressed comments.
Will land after https://codereview.chromium.org/2731553005/

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/tabs/tab.mm
File ios/chrome/browser/tabs/tab.mm (right):

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab.mm:1455: [[[DownloadManagerController alloc]
initWithWebState:self.webState
On 2017/03/07 13:51:22, rohitrao wrote:
> This addresses the "followup" comments that I left on the other CL =)

Acknowledged.

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/ui/downl...
File ios/chrome/browser/ui/downloads/download_manager_controller.h (right):

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/ui/downl...
ios/chrome/browser/ui/downloads/download_manager_controller.h:27: -
(id)initWithWebState:(web::WebState*)webState downloadURL:(const GURL&)url;
On 2017/03/07 17:15:49, sdefresne wrote:
> nit: can you use the opportunity you are changing this line to also change the
> return type to "instancetype"?

Done.

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/ui/downl...
File ios/chrome/browser/ui/downloads/download_manager_controller.mm (right):

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/ui/downl...
ios/chrome/browser/ui/downloads/download_manager_controller.mm:523: -
(id)initWithWebState:(web::WebState*)webState downloadURL:(const GURL&)url {
On 2017/03/07 17:15:49, sdefresne wrote:
> ditto

Done.

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/ui/downl...
File ios/chrome/browser/ui/downloads/download_manager_controller_unittest.mm
(right):

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/ui/downl...
ios/chrome/browser/ui/downloads/download_manager_controller_unittest.mm:51:
class DownloadManagerControllerTest : public ChromeWebTest {
On 2017/03/07 13:51:22, rohitrao wrote:
> These changes end up making the test more heavyweight, because the controller
> relies on objects that are buried inside WebState.
> 
> sdefresne@ does not like this, because it forces us to write "integration"
tests
> instead of unit tests.  Nothing to change in this CL, but we should explore
ways
> to restructure the helpers to make them more testable.

Acknowledged.

[pkl (ping after 24h if needed), 2017-03-07 22:22:02]

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/tabs/tab.mm
File ios/chrome/browser/tabs/tab.mm (left):

https://codereview.chromium.org/2740473002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab.mm:1459: return nil;
On 2017/03/07 13:51:22, rohitrao wrote:
> There is a small logic change here, in that the old code would not open the
> download controller at all if there was no storekit launcher.  The new code
will
> open the download controller but do nothing when you try to go to the app
store.
> 
> Nothing needs to be changed here, we should just be aware of the change.

I added a check to download manager controller to not bring up the UI if
StoreKitTabHelper is nil.

[pkl (ping after 24h if needed), 2017-03-08 17:46:56]

The CQ bit was checked by pkl@chromium.org

[pkl (ping after 24h if needed), 2017-03-08 17:46:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org, rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2740473002/#ps40001
(title: "handles nil StoreKitTabHelper")

[commit-bot: I haz the power, 2017-03-08 17:47: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, 2017-03-08 20:51:21]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488995216162610,
"parent_rev": "12ffb78327c4780b4bdc5384c759498072d8eaba", "commit_rev":
"998ad027faf59d366cd87996d3060e224d4609f1"}

[commit-bot: I haz the power, 2017-03-08 20:52:02]

Description was changed from

==========
DownloadManagerController now uses WebState

Instead of using StoreKitLauncher, DownloadManagerController can now
get everything it needs from WebState.

BUG=684063
==========

to

==========
DownloadManagerController now uses WebState

Instead of using StoreKitLauncher, DownloadManagerController can now
get everything it needs from WebState.

BUG=684063

Review-Url: https://codereview.chromium.org/2740473002
Cr-Commit-Position: refs/heads/master@{#455538}
Committed:
https://chromium.googlesource.com/chromium/src/+/998ad027faf59d366cd87996d306...
==========

[commit-bot: I haz the power, 2017-03-08 20:52:03]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/998ad027faf59d366cd87996d306...