[Android Webapps] Make AddToHomescreenDataFetcher easier to test

[Android Webapps] Make AddToHomescreenDataFetcher easier to test

This CL makes AddToHomescreenDataFetcher use a WeakPtrFactory. This enables
cancelling all pending asynchronous operations when AddToHomescreenDataFetcher
times out.

BUG=721881
Review-Url: https://codereview.chromium.org/2968693003
Cr-Commit-Position: refs/heads/master@{#484411}
Committed: https://chromium.googlesource.com/chromium/src/+/484d04841e5e80857f49625cdb5d647c7ddcc6de

[pkotwicz, 2017-06-30 21:44:48]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2017-06-30 21:49:45]

pkotwicz@chromium.org changed reviewers:
+ dominickn@chromium.org

[pkotwicz, 2017-06-30 21:49:45]

Dominick, can you please take a look?

[pkotwicz, 2017-06-30 21:51:16]

Dominick, can you please take a look?

I hope that this CL will make the tests that you are writing simpler. In
particular, I think that with this CL (and arguably without) we no longer need
to test the behavior of what happens when the InstallabeManager::GetData()
callback gets called after AddToHomescreenDataFetcher times out

[dominickn, 2017-07-03 00:43:16]

This mostly looks fine. I'll be honest though, this makes my CL harder. It's a
big rebase and retest cycle straight after I sunk most of my last week on
rebasing and restesting to fix this mess up (and taking suggestions like testing
the InstallableManager::GetData calls after the time out).

https://codereview.chromium.org/2968693003/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h (right):

https://codereview.chromium.org/2968693003/diff/20001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:51: //
OnDidGetWebApplicationInfo is expected to be called when finished.
Nit: document that |observer| must outlive this object.

[pkotwicz, 2017-07-04 15:37:41]

Patchset #2 (id:40001) has been deleted

[pkotwicz, 2017-07-04 16:04:25]

Dominick, can you please take another look?

I don't mind waiting for your CL to land prior to landing this CL. I did not
intend on giving you extra work.

The reason that I wrote the CL is because it was hard to keep track of the 5
scenarios. In particular, it was hard to associate a specific scenario with
comments. For instance, the comment for the test on lines 527-553 in your CL
says that it is testing Scenario#1 when it is in fact testing Scenario#2


Scenario | Service worker | Fetch Handler | AddToHomescreenDataFetcher |Error
code after        |
_________|_registered_____|_______________|_times
out__________________|GetData() returns_______|
#1_______|_N______________|_______________|____________________________|GetData()
never returns |
#2_______|_Y______________|_N_____________|_Y__________________________|NOT_OFFLINE_CAPABLE_____|
#3_______|_Y______________|_N_____________|_N__________________________|________________________|
#4_______|_Y______________|_Y_____________|_Y__________________________|NO_ERROR_DETECTED_______|
#5_______|_Y______________|_Y_____________|_N__________________________|________________________|

For the sake of completeness, I don't think that it is necessary to test the "no
fetch handler in the service worker" case because the behavior is the same as
when the "Web Manifest is not installable (e.g. because both the name and the
short_name are empty)

[dominickn, 2017-07-05 00:16:09]

lgtm

This definitely makes the code easier to reason about. I was just saying it
really doesn't make my CL any easier right now since someone is going to have to
have a painful rebase, recompile, retest, and strip-out-redundancy cycle.

[pkotwicz, 2017-07-05 02:10:49]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2017-07-05 02:10:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org
Link to the patchset: https://codereview.chromium.org/2968693003/#ps80001
(title: "Merge branch 'master' into homescreen_fetcher_weak_ptr2")

[commit-bot: I haz the power, 2017-07-05 02:11:02]

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-07-05 02:13:13]

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

[commit-bot: I haz the power, 2017-07-05 02:13:14]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  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_...)

[dominickn, 2017-07-05 03:04:40]

https://codereview.chromium.org/2968693003/diff/80001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h (right):

https://codereview.chromium.org/2968693003/diff/80001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:10: #include
"base/macros.h"
Nit: #include "base/memory/weak_ptr.h"

[pkotwicz, 2017-07-05 04:36:52]

Patchset #4 (id:100001) has been deleted

[pkotwicz, 2017-07-05 04:37:07]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2017-07-05 04:37:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org
Link to the patchset: https://codereview.chromium.org/2968693003/#ps120001
(title: "Merge branch 'master' into homescreen_fetcher_weak_ptr2")

[commit-bot: I haz the power, 2017-07-05 04:37:16]

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-07-05 06:22:22]

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

[commit-bot: I haz the power, 2017-07-05 06:22:23]

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...)

[pkotwicz, 2017-07-05 20:10:52]

Patchset #4 (id:120001) has been deleted

[pkotwicz, 2017-07-05 20:11:11]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2017-07-05 20:11:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org
Link to the patchset: https://codereview.chromium.org/2968693003/#ps140001
(title: "Merge branch 'master' into homescreen_fetcher_weak_ptr2")

[commit-bot: I haz the power, 2017-07-05 20:11:34]

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-07-05 20:13:39]

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

[commit-bot: I haz the power, 2017-07-05 20:13:41]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pkotwicz, 2017-07-05 21:24:17]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2017-07-05 21:24:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org
Link to the patchset: https://codereview.chromium.org/2968693003/#ps160001
(title: "Merge branch 'master' into homescreen_fetcher_weak_ptr2")

[commit-bot: I haz the power, 2017-07-05 21:24:44]

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-07-05 23:25:42]

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

[commit-bot: I haz the power, 2017-07-05 23:25:43]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_TIMED_OUT, build has not
started yet; builder either lacks capacity or does not exist (misspelled?))
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[pkotwicz, 2017-07-06 00:14:20]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2017-07-06 00:14:48]

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-07-06 00:25:35]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1499300060998400,
"parent_rev": "10b4b19297da4a3cd28944da3134df517946b50b", "commit_rev":
"484d04841e5e80857f49625cdb5d647c7ddcc6de"}

[commit-bot: I haz the power, 2017-07-06 00:25:57]

Description was changed from

==========
[Android Webapps] Make AddToHomescreenDataFetcher easier to test

This CL makes AddToHomescreenDataFetcher use a WeakPtrFactory. This enables
cancelling all pending asynchronous operations when AddToHomescreenDataFetcher
times out.

BUG=721881
==========

to

==========
[Android Webapps] Make AddToHomescreenDataFetcher easier to test

This CL makes AddToHomescreenDataFetcher use a WeakPtrFactory. This enables
cancelling all pending asynchronous operations when AddToHomescreenDataFetcher
times out.

BUG=721881

Review-Url: https://codereview.chromium.org/2968693003
Cr-Commit-Position: refs/heads/master@{#484411}
Committed:
https://chromium.googlesource.com/chromium/src/+/484d04841e5e80857f49625cdb5d...
==========

[commit-bot: I haz the power, 2017-07-06 00:25:58]

Committed patchset #5 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/484d04841e5e80857f49625cdb5d...