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

chrome/browser/banners/app_banner_manager.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/banners/app_banner_manager.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 286 matching lines @@
   if (IsDebugMode()) {
     LogErrorToConsole(web_contents, code, GetStatusParam(code));
   } else {
     // Ensure that we haven't yet logged a status code for this page.
     DCHECK(need_to_log_status_);
     TrackInstallableStatusCode(code);
     need_to_log_status_ = false;
   }
 }
 
-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
   // InstallableManager (e.g. the renderer crashes or the browser is shutting
   // down). These situations are explicitly not logged.
   DCHECK(!need_to_log_status_ || is_active_);
 
   weak_factory_.InvalidateWeakPtrs();
   binding_.Close();
   controller_.reset();
   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() {
   RecordCouldShowBanner();
 
   TrackBeforeInstallEvent(BEFORE_INSTALL_EVENT_CREATED);
   event_request_id_ = ++gCurrentRequestID;
 
   web_contents()->GetMainFrame()->GetRemoteInterfaces()->GetInterface(
       mojo::MakeRequest(&controller_));
@@ skipping 14 matching lines @@
     // start. This may be the first navigation, or we may have stopped
     // observing if the banner flow was triggered on the previous page.
     SiteEngagementObserver::Observe(SiteEngagementService::Get(
         Profile::FromBrowserContext(web_contents()->GetBrowserContext())));
   }
 }
 
 void AppBannerManager::DidFinishNavigation(content::NavigationHandle* handle) {
   if (handle->IsInMainFrame() && handle->HasCommitted() &&
       !handle->IsSamePage()) {
-    active_media_players_.clear();
+    ResetData();
     if (is_active_)
       Stop();
   }
 }
 
 void AppBannerManager::DidFinishLoad(
     content::RenderFrameHost* render_frame_host,
     const GURL& validated_url) {
   // Don't start the banner flow unless the main frame has finished loading.
   if (render_frame_host->GetParent())
@@ skipping 154 matching lines @@
     // Simulate a non-canceled OnBannerPromptReply to show the delayed banner.
     // Don't reset |was_canceled_by_page_| yet for metrics purposes.
     OnBannerPromptReply(blink::mojom::AppBannerPromptReply::NONE, referrer_);
   } else {
     // Log that the prompt request was made for when we get the prompt reply.
     page_requested_prompt_ = true;
   }
 }
 
 }  // namespace banners