Webapps: Clear AppBannerManagerAndroid::native_app_data_ prior to native app check

Webapps: Clear AppBannerManagerAndroid::native_app_data_ prior to native app check

This CL fixes a bug where
1) Navigating to a web page which has a Web Manifest with
"related_applications" specified.
2) Navigating to a web page with a web app compatible Web Manifest
would show a native app banner for the second page.

Whether |AppBannerManagerAndroid::native_app_data_| is set is used to determine
which type of banner to create (native app or webapp). Previously
|AppBannerManagerAndroid::native_app_data_| was only cleared if there was a
native app associated with the Web Manifest. This CL clears
|AppBannerManagerAndroid::native_app_data_| prior to checking
whether there is a native app associated with the Web Manifest.

BUG=687024
Review-Url: https://codereview.chromium.org/2671653002
Cr-Commit-Position: refs/heads/master@{#448218}
Committed: https://chromium.googlesource.com/chromium/src/+/de6f8b587176d94643869d0f28aee52b92ad00b8

[pkotwicz, 2017-02-02 04:41:51]

Description was changed from

==========
Webapps: Clear AppBannerManagerAndroid::native_app_data_ prior to native app
check

This CL fixes a bug where
1) Navigating to a web page which has a Web Manifest with "related_applications"
specified.
2) Navigating to a web page with a web app compatible Web Manifest
would show a native app banner for the second page.

Whether |AppBannerManagerAndroid::native_app_data_| is set is used to determine
which type of banner to create. Previously
|AppBannerManagerAndroid::native_app_data_| was only cleared if there was a
native app associated with the Web Manifest. This CL clears
|AppBannerManagerAndroid::native_app_data_| prior to checking
whether there is a native app associated with the Web Manifest.

BUG=687024
==========

to

==========
Webapps: Clear AppBannerManagerAndroid::native_app_data_ prior to native app
check

This CL fixes a bug where
1) Navigating to a web page which has a Web Manifest with
"related_applications" specified.
2) Navigating to a web page with a web app compatible Web Manifest
would show a native app banner for the second page.

Whether |AppBannerManagerAndroid::native_app_data_| is set is used to determine
which type of banner to create (native app or webapp). Previously
|AppBannerManagerAndroid::native_app_data_| was only cleared if there was a
native app associated with the Web Manifest. This CL clears
|AppBannerManagerAndroid::native_app_data_| prior to checking
whether there is a native app associated with the Web Manifest.

BUG=687024
==========

[pkotwicz, 2017-02-02 04:54:52]

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

[pkotwicz, 2017-02-02 04:54:53]

mgiuca@ can you please take a look? The reset code should likely be in a
ResetState() method called from AppBannerManager::RequestAppBanner() but I'll
save that for a follow up CL

[pkotwicz, 2017-02-02 04:56:24]

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

[Matt Giuca, 2017-02-02 05:43:38]

Err, I don't know this code at all (I just reported the bug). You should wait
for Dom to look at it (and I'm not an OWNER anyway). Sorry.

[Matt Giuca, 2017-02-02 05:43:59]

mgiuca@chromium.org changed reviewers:
- mgiuca@chromium.org

[dominickn, 2017-02-02 06:53:38]

Not sure why this wasn't sent to me first?

https://codereview.chromium.org/2671653002/diff/1/chrome/browser/android/bann...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/2671653002/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_manager_android.cc:161:
native_app_data_.Reset();
We should implement this correctly first up rather than in a follow-up.

Let's say we visit a page which triggers a native app banner successfully. We
will carry the native_app_data_ around incorrectly all the way up until this
method is next called.

AppBannerManager is designed such that everything is fetched once per main frame
navigation, and remains valid for the entirety of the length of time the user
spends on the page once they navigate (even if the banner is shown, the data is
still valid, and if we want to allow reshowing the banner in the future we need
to hang on to the data). That means the right place to clean up is in
AppBannerManager::DidFinishNavigation.

You should

1. Add the protected virtual AppBannerManager::ResetData() method, which has:

manifest_ = content::Manifest();
manifest_url_ = GURL();
validated_url_ = GURL();
referrer_.erase();

2. Remove the last two lines above from Stop()

3. Call ResetData() in AppBannerManager::DidFinishNavigation()

4. Override ResetData() in AppBannerManagerAndroid to add:

native_app_data_.Reset();
native_app_package_ = "";

This guarantees that once we navigate, we are back in an unfetched state until
the next fetch succeeds. We have to do a little more work but we ensure that the
banner manager is internally consistent.

[Matt Giuca, 2017-02-02 07:22:27]

On 2017/02/02 06:53:38, dominickn wrote:
> Not sure why this wasn't sent to me first?

It was sent to both of us (but addressed to me, I guess time zones).

[pkotwicz, 2017-02-06 01:07:31]

Patchset #3 (id:40001) has been deleted

[pkotwicz, 2017-02-06 01:08:51]

Dominick, can you please take another look? I also reset
|page_requested_prompt_| because it might not be reset after a navigation. I
added a test for this case

[dominickn, 2017-02-06 02:16:53]

https://codereview.chromium.org/2671653002/diff/60001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2671653002/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:308: if (was_canceled_by_page_ &&
!page_requested_prompt_) {
ResetData() should not be recording metrics. It should only reset data that was
downloaded from the page.

For instance, if the user closes the tab without navigating away,
DidFinishNavigation will never be called and these metrics will not be recorded.
This is why they were recorded in Stop(), which is explicitly called by every
path through the banner pipeline that doesn't show a banner.

We also shouldn't be resetting internal manager state like was_canceled_by_page_
or page_requested_prompt_ in ResetData(). All ResetData() should do is reset
manifest_, manifest_url_, validated_url_, and referrer_. Everything else should
stay in Stop().

https://codereview.chromium.org/2671653002/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:320: page_requested_prompt_ =
false;
Good catch on these flag variables, but I think page_requested_prompt_ and
was_canceled_by_page_ should be reset in RequestAppBanner() (since they both
have to do with the beforeinstallprompt event).

https://codereview.chromium.org/2671653002/diff/60001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/2671653002/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.h:164: // Resets the manager's state.
The comment should be:

"Resets all fetched data for the current page".

Since we actually remember some of the manager's state for metrics purposes.

[pkotwicz, 2017-02-06 03:00:11]

Dominick, can you please take another look?

https://codereview.chromium.org/2671653002/diff/60001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2671653002/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:308: if (was_canceled_by_page_ &&
!page_requested_prompt_) {
Good point

https://codereview.chromium.org/2671653002/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:320: page_requested_prompt_ =
false;
I will do this in a follow up CL. It is good to keep CLs which are merged back
small

https://codereview.chromium.org/2671653002/diff/60001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/2671653002/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.h:164: // Resets the manager's state.
Done. I have also renamed the method to ResetCurrentPageData()

[dominickn, 2017-02-06 03:05:51]

lgtm

[pkotwicz, 2017-02-06 03:13:19]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2017-02-06 03:13:24]

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

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486350799127800,
"parent_rev": "9bbca673dfd88e645d81253b890826cbab4c3704", "commit_rev":
"de6f8b587176d94643869d0f28aee52b92ad00b8"}

[commit-bot: I haz the power, 2017-02-06 07:04:07]

Description was changed from

==========
Webapps: Clear AppBannerManagerAndroid::native_app_data_ prior to native app
check

This CL fixes a bug where
1) Navigating to a web page which has a Web Manifest with
"related_applications" specified.
2) Navigating to a web page with a web app compatible Web Manifest
would show a native app banner for the second page.

Whether |AppBannerManagerAndroid::native_app_data_| is set is used to determine
which type of banner to create (native app or webapp). Previously
|AppBannerManagerAndroid::native_app_data_| was only cleared if there was a
native app associated with the Web Manifest. This CL clears
|AppBannerManagerAndroid::native_app_data_| prior to checking
whether there is a native app associated with the Web Manifest.

BUG=687024
==========

to

==========
Webapps: Clear AppBannerManagerAndroid::native_app_data_ prior to native app
check

This CL fixes a bug where
1) Navigating to a web page which has a Web Manifest with
"related_applications" specified.
2) Navigating to a web page with a web app compatible Web Manifest
would show a native app banner for the second page.

Whether |AppBannerManagerAndroid::native_app_data_| is set is used to determine
which type of banner to create (native app or webapp). Previously
|AppBannerManagerAndroid::native_app_data_| was only cleared if there was a
native app associated with the Web Manifest. This CL clears
|AppBannerManagerAndroid::native_app_data_| prior to checking
whether there is a native app associated with the Web Manifest.

BUG=687024

Review-Url: https://codereview.chromium.org/2671653002
Cr-Commit-Position: refs/heads/master@{#448218}
Committed:
https://chromium.googlesource.com/chromium/src/+/de6f8b587176d94643869d0f28ae...
==========

[commit-bot: I haz the power, 2017-02-06 07:04:09]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/de6f8b587176d94643869d0f28ae...