Make OnDataTimedout and OnDidPerformInstallableCheck mutually exclusive.

Make OnDataTimedout and OnDidPerformInstallableCheck mutually exclusive.

AddToHomescreenDataFetcher::OnDataTimedout() and
AddToHomescreenDataFetcher::OnDidPerformInstallableCheck() are the
success and timeout callbacks for fetching data. This CL makes them
mutually exclusive. As both functions are run on the same thread, locks
are unnecessary for this case.

BUG=692626
Review-Url: https://codereview.chromium.org/2720083002
Cr-Commit-Position: refs/heads/master@{#454092}
Committed: https://chromium.googlesource.com/chromium/src/+/3c6d0b9a7b969fbb957b9f5960ceff6a2cc37ce1

[F, 2017-02-27 21:09:09]

Description was changed from

==========
Make OnDataTimedout and OnDidPerformInstallableCheck mutually exclusive.

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

to

==========
Make OnDataTimedout and OnDidPerformInstallableCheck mutually exclusive.

AddToHomescreenDataFetcher::OnDataTimedout and
AddToHomescreenDataFetcher::OnDidPerformInstallableCheck are the
success and timeout callbacks for fetching data. This CL makes them
mutually exclusive. As both functions are run on the same thread, locks
are unnecessary for this case.

BUG=692626
==========

[F, 2017-02-27 21:09:51]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-27 21:10:43]

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

[F, 2017-02-27 21:10:46]

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

[F, 2017-02-27 21:10:47]

Hi Dominick & Peter, PTAL. Thanks!

[commit-bot: I haz the power, 2017-02-27 21:52:09]

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

[commit-bot: I haz the power, 2017-02-27 21:52:09]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-02-27 23:30:39]

Can you please add gtest for add_to_homescreen_data_fetcher.cc verifying the bug
that you fixed?
The behavior when a timeout occurs is super subtle.

In particular, can you test that during the buggy flow of events
Observer::OnDidDetermineWebApkCompatibility() Observer::OnUserTitleAvailable()
and Observer::OnDataAvailable() are called just once?

I suggest making AddToHomescreenDataFetcher::CreateLauncherIcon() virtual and
overwriting the method in your test class to do nothing (other than checking
that it is in fact called) and calling
AddToHomescreenDataFetcher::NotifyObserver() manually

https://codereview.chromium.org/2720083002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right):

https://codereview.chromium.org/2720083002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:216: return;
Nit:
You can remove the else

In Chrome we prefer
if (a) {
  // do work
  return;
}
// do work


Instead of
if (a) {
  // do work
  return;
} else {
  // do work
}

[F, 2017-02-28 23:09:02]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 23:09:33]

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

[F, 2017-02-28 23:09:42]

Thanks Peter, PTAL!

By offline discussion, we are going to revisit the tests in follow up CLs.

https://codereview.chromium.org/2720083002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right):

https://codereview.chromium.org/2720083002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:216: return;
On 2017/02/27 23:30:38, pkotwicz wrote:
> Nit:
> You can remove the else
> 
> In Chrome we prefer
> if (a) {
>   // do work
>   return;
> }
> // do work
> 
> 
> Instead of
> if (a) {
>   // do work
>   return;
> } else {
>   // do work
> }

Done.

[dominickn, 2017-02-28 23:17:13]

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

https://codereview.chromium.org/2720083002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:215: if
(is_installable_check_complete_)
Combine the two conditionals:

if (!web_contents() || !weak_observer_ || is_installable_check_complete_)
  return;

https://codereview.chromium.org/2720083002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc
(right):

https://codereview.chromium.org/2720083002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:36: //
TODO(697228): Fix slow & incomplete tests.
TODO(zpeng): Add tests for <things to add tests for>. See crbug.com/697228.

[F, 2017-02-28 23:21:54]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[F, 2017-02-28 23:22:07]

Thanks Dominick & Peter, PTAL!

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

https://codereview.chromium.org/2720083002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:215: if
(is_installable_check_complete_)
On 2017/02/28 23:17:12, dominickn wrote:
> Combine the two conditionals:
> 
> if (!web_contents() || !weak_observer_ || is_installable_check_complete_)
>   return;

Done.

https://codereview.chromium.org/2720083002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc
(right):

https://codereview.chromium.org/2720083002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:36: //
TODO(697228): Fix slow & incomplete tests.
On 2017/02/28 23:17:12, dominickn wrote:
> TODO(zpeng): Add tests for <things to add tests for>. See crbug.com/697228.

Done. Thanks!

[commit-bot: I haz the power, 2017-02-28 23:22:31]

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

[dominickn, 2017-02-28 23:23:15]

https://codereview.chromium.org/2720083002/diff/40001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right):

https://codereview.chromium.org/2720083002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:212: if
(!web_contents() || !weak_observer_ || !is_installable_check_complete_)
Should this be is_installable_check_complete_ (not negated)?

[commit-bot: I haz the power, 2017-03-01 00:36:16]

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

[commit-bot: I haz the power, 2017-03-01 00:36:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[F, 2017-03-01 15:05:55]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 15:06:08]

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

[F, 2017-03-01 15:35:21]

Thanks Dominick & Peter, PTAL!

Sorry about the mistake, I wasn't thinking clearly

https://codereview.chromium.org/2720083002/diff/40001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right):

https://codereview.chromium.org/2720083002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:212: if
(!web_contents() || !weak_observer_ || !is_installable_check_complete_)
On 2017/02/28 23:23:15, dominickn wrote:
> Should this be is_installable_check_complete_ (not negated)?

Done. Sorry

[commit-bot: I haz the power, 2017-03-01 15:39:23]

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

[commit-bot: I haz the power, 2017-03-01 15:39:24]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-03-01 20:36:38]

Description was changed from

==========
Make OnDataTimedout and OnDidPerformInstallableCheck mutually exclusive.

AddToHomescreenDataFetcher::OnDataTimedout and
AddToHomescreenDataFetcher::OnDidPerformInstallableCheck are the
success and timeout callbacks for fetching data. This CL makes them
mutually exclusive. As both functions are run on the same thread, locks
are unnecessary for this case.

BUG=692626
==========

to

==========
Make OnDataTimedout and OnDidPerformInstallableCheck mutually exclusive.

AddToHomescreenDataFetcher::OnDataTimedout() and
AddToHomescreenDataFetcher::OnDidPerformInstallableCheck() are the
success and timeout callbacks for fetching data. This CL makes them
mutually exclusive. As both functions are run on the same thread, locks
are unnecessary for this case.

BUG=692626
==========

[pkotwicz, 2017-03-01 20:40:37]

LGTM

[dominickn, 2017-03-01 23:11:41]

lgtm, thanks

[F, 2017-03-01 23:22:50]

The CQ bit was checked by zpeng@chromium.org

[commit-bot: I haz the power, 2017-03-01 23:23: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-03-01 23:29:27]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488410570117070,
"parent_rev": "136203f21978d0ff27553d36872ac67115ee9759", "commit_rev":
"3c6d0b9a7b969fbb957b9f5960ceff6a2cc37ce1"}

[commit-bot: I haz the power, 2017-03-01 23:30:11]

Description was changed from

==========
Make OnDataTimedout and OnDidPerformInstallableCheck mutually exclusive.

AddToHomescreenDataFetcher::OnDataTimedout() and
AddToHomescreenDataFetcher::OnDidPerformInstallableCheck() are the
success and timeout callbacks for fetching data. This CL makes them
mutually exclusive. As both functions are run on the same thread, locks
are unnecessary for this case.

BUG=692626
==========

to

==========
Make OnDataTimedout and OnDidPerformInstallableCheck mutually exclusive.

AddToHomescreenDataFetcher::OnDataTimedout() and
AddToHomescreenDataFetcher::OnDidPerformInstallableCheck() are the
success and timeout callbacks for fetching data. This CL makes them
mutually exclusive. As both functions are run on the same thread, locks
are unnecessary for this case.

BUG=692626

Review-Url: https://codereview.chromium.org/2720083002
Cr-Commit-Position: refs/heads/master@{#454092}
Committed:
https://chromium.googlesource.com/chromium/src/+/3c6d0b9a7b969fbb957b9f5960ce...
==========

[commit-bot: I haz the power, 2017-03-01 23:30:13]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/3c6d0b9a7b969fbb957b9f5960ce...