Call AddToHomescreenDataFetcher::Observer callbacks when manifest fetch times out

Call AddToHomescreenDataFetcher::Observer callbacks when manifest fetch times out

This CL ensures that all of the AddToHomescreenDataFetcher::Observer callbacks
still get called when the manifest fetch times out. Previously
Observer::OnDidDetermineWebApkCompatibility() and
Observer::OnDataAvailable() were not called.

Observer::OnDidDetermineWebApkCompatibility() shows the dialog when WebAPKs are
enabled and the manifest fetch times out.
Observer::OnUserTitleAvailable() enables the text field in the add-to-homescreen
dialog.
Observer::OnDataAvailable() enables the add button in the add-to-homescreen
dialog.

BUG=647873
TEST=AddToHomescreenManagerTest#testAddWebappShortcutManifestFetchTimesOut

Committed: https://crrev.com/67a9c9db813e7b963f0696dbad0b7c8da70c67be
Cr-Commit-Position: refs/heads/master@{#419796}

[pkotwicz, 2016-09-15 22:44:40]

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

[pkotwicz, 2016-09-15 22:44:41]

Xi can you please take a look?

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

https://codereview.chromium.org/2342183002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:233: if
(check_installable_ && is_waiting_for_installable_check_) {
I don't see how this if() statement evaluates to true.

FetchFavicon() is private. OnDidPerformInstallableCheck() is the only caller.

[pkotwicz, 2016-09-15 22:47:43]

My bad, I did not see the timer. Abandoning this CL

[pkotwicz, 2016-09-15 22:47:59]

Description was changed from

==========
Remove unneeded AddToHomescreenDataFetcher::is_waiting_for_installable_check_

BUG=None
==========

to

==========
Remove unneeded AddToHomescreenDataFetcher::is_waiting_for_installable_check_

BUG=None
==========

[pkotwicz, 2016-09-16 03:05:19]

Things are a bit more complicated. I have a CL which fixes things up locally.

First I am looking at fixing at an unrelated crash that I found while testing
"the fixup CL"

[pkotwicz, 2016-09-16 23:42:06]

Description was changed from

==========
Remove unneeded AddToHomescreenDataFetcher::is_waiting_for_installable_check_

BUG=None
==========

to

==========
Call AddToHomescreenDataFetcher::Observer callbacks when manifest fetch times
out

This CL ensures that all of the AddToHomescreenDataFetcher::Observer callbacks
still get called when the manifest fetch times out. Previously
Observer::OnDidDetermineWebApkCompatibility() and
Observer::OnDataAvailable() were not called.

Observer::OnDidDetermineWebApkCompatibility() shows the dialog when WebAPKs are
enabled and the manifest fetch times out.
Observer::OnUserTitleAvailable() enables the text field in the add-to-homescreen
dialog.
Observer::OnDataAvailable() enables the add button in the add-to-homescreen
dialog.

BUG=647873
TEST=AddToHomescreenManagerTest#testAddWebappShortcutManifestFetchTimesOut
==========

[pkotwicz, 2016-09-16 23:45:11]

Patchset #2 (id:20001) has been deleted

[pkotwicz, 2016-09-16 23:48:15]

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

[pkotwicz, 2016-09-16 23:48:15]

Dominick, can you please take a look?

[dominickn, 2016-09-19 05:41:44]

Thanks for adding this test. :)

https://codereview.chromium.org/2342183002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenManagerTest.java
(right):

https://codereview.chromium.org/2342183002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenManagerTest.java:333:
public void testAddWebappShortcutManifestFetchTimesOut() throws Exception {
Can you test both the non-WebAPK and WebAPK paths here (make this a non-test
method, and then have two test methods, once of which sets the command line flag
to enable WebAPKs)? The WebAPK path calls ShowDialog in
OnDidDetermineWebApkCompatibility.

Can you also check a non-PWA, and validate that none of the callbacks are
called?

https://codereview.chromium.org/2342183002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenManagerTest.java:338:
// Register handler for "slow?1000" URL.
Nit: it's slow?10000 in the test file

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

https://codereview.chromium.org/2342183002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:101: void
OnTimeout();
Nit: call this OnDataTimedout() to match data_timeout_timer and the general
naming scheme here.

https://codereview.chromium.org/2342183002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:118: void
CreateLauncherIcon(const SkBitmap& raw__icon);
Nit: two underscores in variable name.

Nit: make the comment match the variable name.

https://codereview.chromium.org/2342183002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:142: bool
did_installable_check_;
Nit: is_installable_check_complete_

[pkotwicz, 2016-09-19 23:34:02]

Dominick, can you please take another look?

https://codereview.chromium.org/2342183002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenManagerTest.java
(right):

https://codereview.chromium.org/2342183002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenManagerTest.java:333:
public void testAddWebappShortcutManifestFetchTimesOut() throws Exception {
I have added a test for the non-WebAPK case.

Shouldn't AddToHomescreenManager#Observer#onUserTitleAvailable() and
AddToHomescreenManager#Observer#onReadyToAdd() still be called for non-PWAs?

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

https://codereview.chromium.org/2342183002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:118: void
CreateLauncherIcon(const SkBitmap& raw__icon);
You've got good eyes!

__fixed__

[dominickn, 2016-09-20 00:15:12]

https://codereview.chromium.org/2342183002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenManagerTest.java
(right):

https://codereview.chromium.org/2342183002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenManagerTest.java:333:
public void testAddWebappShortcutManifestFetchTimesOut() throws Exception {
On 2016/09/19 23:34:02, pkotwicz wrote:
> I have added a test for the non-WebAPK case.
> 
> Shouldn't AddToHomescreenManager#Observer#onUserTitleAvailable() and
> AddToHomescreenManager#Observer#onReadyToAdd() still be called for non-PWAs?
> 

Sorry, mistyped. But OnDidDetermineWebApkCompatibility shouldn't be called - so
you should check for 2 callbacks, not 3 :)

https://codereview.chromium.org/2342183002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenManagerTest.java
(right):

https://codereview.chromium.org/2342183002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenManagerTest.java:360:
manager.waitForCallbacks(3, 10);
Shouldn't this wait for 2 callbacks in the non-WebAPK case, since
check_webapk_availability_ should be false?

[pkotwicz, 2016-09-20 01:52:35]

https://codereview.chromium.org/2342183002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenManagerTest.java
(right):

https://codereview.chromium.org/2342183002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenManagerTest.java:333:
public void testAddWebappShortcutManifestFetchTimesOut() throws Exception {
I have added a test for a non-PWA

https://codereview.chromium.org/2342183002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenManagerTest.java
(right):

https://codereview.chromium.org/2342183002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenManagerTest.java:360:
manager.waitForCallbacks(3, 10);
Java does not know about
AddToHomescreenManager::OnDidDetermineWebApkCompatibility().

AddToHomescreenManagerCallbackTracker considers
AddToHomescreenManager#showDialog() a "callback". Maybe it should not count.

It would be nice if we could have a browser test for
add_to_homescreen_data_fetcher.cc. Unfortunately our test framework does not
your support browser tests on Android

[pkotwicz, 2016-09-20 01:53:21]

Dominick can you please take another look?

[dominickn, 2016-09-20 03:23:23]

lgtm thanks!

https://codereview.chromium.org/2342183002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenManagerTest.java
(right):

https://codereview.chromium.org/2342183002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenManagerTest.java:360:
manager.waitForCallbacks(3, 10);
On 2016/09/20 01:52:35, pkotwicz wrote:
> Java does not know about
> AddToHomescreenManager::OnDidDetermineWebApkCompatibility().
> 
> AddToHomescreenManagerCallbackTracker considers
> AddToHomescreenManager#showDialog() a "callback". Maybe it should not count.
> 
> It would be nice if we could have a browser test for
> add_to_homescreen_data_fetcher.cc. Unfortunately our test framework does not
> your support browser tests on Android

Acknowledged.

[pkotwicz, 2016-09-20 14:37:59]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-09-20 14:38:18]

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

[pkotwicz, 2016-09-20 15:25:06]

The CQ bit was unchecked by pkotwicz@chromium.org

[pkotwicz, 2016-09-20 15:43:26]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-09-20 15:43:27]

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/2342183002/#ps100001
(title: "Merge branch 'master' into remove_unneeded_var")

[commit-bot: I haz the power, 2016-09-20 15:43:42]

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-09-20 17:03:13]

Description was changed from

==========
Call AddToHomescreenDataFetcher::Observer callbacks when manifest fetch times
out

This CL ensures that all of the AddToHomescreenDataFetcher::Observer callbacks
still get called when the manifest fetch times out. Previously
Observer::OnDidDetermineWebApkCompatibility() and
Observer::OnDataAvailable() were not called.

Observer::OnDidDetermineWebApkCompatibility() shows the dialog when WebAPKs are
enabled and the manifest fetch times out.
Observer::OnUserTitleAvailable() enables the text field in the add-to-homescreen
dialog.
Observer::OnDataAvailable() enables the add button in the add-to-homescreen
dialog.

BUG=647873
TEST=AddToHomescreenManagerTest#testAddWebappShortcutManifestFetchTimesOut
==========

to

==========
Call AddToHomescreenDataFetcher::Observer callbacks when manifest fetch times
out

This CL ensures that all of the AddToHomescreenDataFetcher::Observer callbacks
still get called when the manifest fetch times out. Previously
Observer::OnDidDetermineWebApkCompatibility() and
Observer::OnDataAvailable() were not called.

Observer::OnDidDetermineWebApkCompatibility() shows the dialog when WebAPKs are
enabled and the manifest fetch times out.
Observer::OnUserTitleAvailable() enables the text field in the add-to-homescreen
dialog.
Observer::OnDataAvailable() enables the add button in the add-to-homescreen
dialog.

BUG=647873
TEST=AddToHomescreenManagerTest#testAddWebappShortcutManifestFetchTimesOut
==========

[commit-bot: I haz the power, 2016-09-20 17:03:18]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-09-20 17:05:14]

Description was changed from

==========
Call AddToHomescreenDataFetcher::Observer callbacks when manifest fetch times
out

This CL ensures that all of the AddToHomescreenDataFetcher::Observer callbacks
still get called when the manifest fetch times out. Previously
Observer::OnDidDetermineWebApkCompatibility() and
Observer::OnDataAvailable() were not called.

Observer::OnDidDetermineWebApkCompatibility() shows the dialog when WebAPKs are
enabled and the manifest fetch times out.
Observer::OnUserTitleAvailable() enables the text field in the add-to-homescreen
dialog.
Observer::OnDataAvailable() enables the add button in the add-to-homescreen
dialog.

BUG=647873
TEST=AddToHomescreenManagerTest#testAddWebappShortcutManifestFetchTimesOut
==========

to

==========
Call AddToHomescreenDataFetcher::Observer callbacks when manifest fetch times
out

This CL ensures that all of the AddToHomescreenDataFetcher::Observer callbacks
still get called when the manifest fetch times out. Previously
Observer::OnDidDetermineWebApkCompatibility() and
Observer::OnDataAvailable() were not called.

Observer::OnDidDetermineWebApkCompatibility() shows the dialog when WebAPKs are
enabled and the manifest fetch times out.
Observer::OnUserTitleAvailable() enables the text field in the add-to-homescreen
dialog.
Observer::OnDataAvailable() enables the add button in the add-to-homescreen
dialog.

BUG=647873
TEST=AddToHomescreenManagerTest#testAddWebappShortcutManifestFetchTimesOut

Committed: https://crrev.com/67a9c9db813e7b963f0696dbad0b7c8da70c67be
Cr-Commit-Position: refs/heads/master@{#419796}
==========

[commit-bot: I haz the power, 2016-09-20 17:05:15]

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