Combine AppBannerManager::Did{Start,Finish}Navigation and reset state there.

Resets state_ ABM to INACTIVE in ResetCurrentPageData (called in
DidFinishNavigation).
This fixes a race possible when there are multiple navigations in quick
succession.

More info:
There is no guarantee about the ordering of Did{Start,Finish}Navigation and
 DidFinishLoad for multiple navigations [1], so it is possible to get the order:
 [DidStartNav(url1), DidFinishNav(url1), DidStartNav(url2), DidFinishLoad(url1),
 DidFinishNav(url2), DidFinishLoad(url2)]
The state_ in ABM is reset to INACTIVE on DidStartNavigation and checked
 (expected to still be INACTIVE) in DidFinishLoad.
In the case of unusual call order, the state was no longer INACTIVE (usually it
 is COMPLETE) in DidFinishLoad(url2).

This fix wouldn't help in the even-more-unusual order:
[DidFinishNav({url1,url2}), DidFinishLoad({url1,url2})]
I'm not sure how possible/likely that is.


[1]: https://cs.chromium.org/chromium/src/content/public/browser/web_contents_observer.h?rcl=c954302bbe18174ce8a54242282eace7f25644e3&l=166-168

Bug: 991832
Change-Id: I0f04a5b26ee396557b56d9e1dca7570d3355f0da
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1797527
Commit-Queue: Glen Robertson <glenrob@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697027}

chrome/browser/banners/app_banner_manager.cc

@@ -436,10 +436,13 @@ void AppBannerManager::ResetBindings() {
 }
 
 void AppBannerManager::ResetCurrentPageData() {
+  load_finished_ = false;
+  has_sufficient_engagement_ = false;
   active_media_players_.clear();
   manifest_ = blink::Manifest();
   manifest_url_ = GURL();
   validated_url_ = GURL();
+  UpdateState(State::INACTIVE);
   SetInstallableWebAppCheckResult(InstallableWebAppCheckResult::kUnknown);
 }
 
@@ -546,22 +549,14 @@ void AppBannerManager::UpdateState(State state) {
   state_ = state;
 }
 
-void AppBannerManager::DidStartNavigation(content::NavigationHandle* handle) {
-  if (!handle->IsInMainFrame() || handle->IsSameDocument())
+void AppBannerManager::DidFinishNavigation(content::NavigationHandle* handle) {
+  if (!handle->IsInMainFrame() || !handle->HasCommitted() ||
+      handle->IsSameDocument()) {
     return;
-
+  }
   if (state_ != State::COMPLETE && state_ != State::INACTIVE)
     Terminate();
-  UpdateState(State::INACTIVE);
-  load_finished_ = false;
-  has_sufficient_engagement_ = false;
-}
-
-void AppBannerManager::DidFinishNavigation(content::NavigationHandle* handle) {
-  if (handle->IsInMainFrame() && handle->HasCommitted() &&
-      !handle->IsSameDocument()) {
-    ResetCurrentPageData();
-  }
+  ResetCurrentPageData();
 }
 
 void AppBannerManager::DidFinishLoad(

chrome/browser/banners/app_banner_manager.h

@@ -284,7 +284,6 @@ class AppBannerManager : public content::WebContentsObserver,
   virtual void UpdateState(State state);
 
   // content::WebContentsObserver overrides.
-  void DidStartNavigation(content::NavigationHandle* handle) override;
   void DidFinishNavigation(content::NavigationHandle* handle) override;
   void DidFinishLoad(content::RenderFrameHost* render_frame_host,
                      const GURL& validated_url) override;