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

chrome/browser/banners/app_banner_manager.cc

Index: chrome/browser/banners/app_banner_manager.cc
diff --git a/chrome/browser/banners/app_banner_manager.cc b/chrome/browser/banners/app_banner_manager.cc
index 5344e0da7c102e943527fdc861327d25458944c6..789970e7d8a125776409b288339913090c6ee81e 100644
--- a/chrome/browser/banners/app_banner_manager.cc
+++ b/chrome/browser/banners/app_banner_manager.cc
@@ -304,13 +304,23 @@ void AppBannerManager::ReportStatus(content::WebContents* web_contents,
   }
 }
 
-void AppBannerManager::Stop() {
+void AppBannerManager::ResetData() {
   if (was_canceled_by_page_ && !page_requested_prompt_) {

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

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

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

Good point

     TrackBeforeInstallEvent(
         BEFORE_INSTALL_EVENT_PROMPT_NOT_CALLED_AFTER_PREVENT_DEFAULT);
     ReportStatus(web_contents(), RENDERER_CANCELLED);
   }
 
+  active_media_players_.clear();
+  manifest_ = content::Manifest();
+  manifest_url_ = GURL();
+  validated_url_ = GURL();
+  referrer_.erase();
+  was_canceled_by_page_ = false;
+  page_requested_prompt_ = false;

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

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

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

I will do this in a follow up CL. It is good to keep CLs which are merged back
small

+}
+
+void AppBannerManager::Stop() {
   // In every non-debug run through the banner pipeline, we should have called
   // ReportStatus() and set need_to_log_status_ to false. The only case where
   // we don't is if we're still active and waiting for a callback from the
@@ -324,11 +334,7 @@ void AppBannerManager::Stop() {
   event_.reset();
 
   is_active_ = false;
-  was_canceled_by_page_ = false;
-  page_requested_prompt_ = false;
   need_to_log_status_ = false;
-  validated_url_ = GURL();
-  referrer_.erase();
 }
 
 void AppBannerManager::SendBannerPromptRequest() {
@@ -363,7 +369,7 @@ void AppBannerManager::DidStartNavigation(content::NavigationHandle* handle) {
 void AppBannerManager::DidFinishNavigation(content::NavigationHandle* handle) {
   if (handle->IsInMainFrame() && handle->HasCommitted() &&
       !handle->IsSamePage()) {
-    active_media_players_.clear();
+    ResetData();
     if (is_active_)
       Stop();
   }