RC: Pass the previous state to the OnLoadingState change callback

Without this the observers who only care about the Loaded <=> Loading
transitions don't know if a transition to Loading is coming from a
Loaded state or not (some logic could be added in the observer directly
but this seems cleaner).

Change-Id: I077314c2afc800f14dfddd3b65fa4530feacb559
Reviewed-on: https://chromium-review.googlesource.com/1096306Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566715}

chrome/browser/resource_coordinator/local_site_characteristics_webcontents_observer.cc

@@ -145,7 +145,8 @@ void LocalSiteCharacteristicsWebContentsObserver::OnAudioStateChanged(
 
 void LocalSiteCharacteristicsWebContentsObserver::OnLoadingStateChange(
     content::WebContents* contents,
-    LoadingState loading_state) {
+    LoadingState old_loading_state,
+    LoadingState new_loading_state) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (web_contents() != contents)
     return;
@@ -153,10 +154,11 @@ void LocalSiteCharacteristicsWebContentsObserver::OnLoadingStateChange(
   if (!writer_)
     return;
 
-  if (loading_state == TabLoadTracker::LoadingState::LOADED) {
+  if (new_loading_state == TabLoadTracker::LoadingState::LOADED) {
     writer_->NotifySiteLoaded();
   } else {
-    writer_->NotifySiteUnloaded();
+    if (old_loading_state == TabLoadTracker::LoadingState::LOADED)
+      writer_->NotifySiteUnloaded();
   }
 }
 

chrome/browser/resource_coordinator/local_site_characteristics_webcontents_observer.h

@@ -49,7 +49,8 @@ class LocalSiteCharacteristicsWebContentsObserver
 
   // TabLoadTracker::Observer:
   void OnLoadingStateChange(content::WebContents* web_contents,
-                            LoadingState loading_state) override;
+                            LoadingState old_loading_state,
+                            LoadingState new_loading_state) override;
 
   // PageSignalObserver:
   void OnNonPersistentNotificationCreated(

chrome/browser/resource_coordinator/local_site_characteristics_webcontents_observer_unittest.cc

@@ -275,19 +275,27 @@ TEST_F(LocalSiteCharacteristicsWebContentsObserverTest, LoadEvent) {
 
   EXPECT_CALL(*mock_writer, NotifySiteLoaded());
   observer()->OnLoadingStateChange(web_contents(),
+                                   TabLoadTracker::LoadingState::LOADING,
                                    TabLoadTracker::LoadingState::LOADED);
   ::testing::Mock::VerifyAndClear(mock_writer);
 
   EXPECT_CALL(*mock_writer, NotifySiteUnloaded());
   observer()->OnLoadingStateChange(web_contents(),
+                                   TabLoadTracker::LoadingState::LOADED,
                                    TabLoadTracker::LoadingState::LOADING);
   ::testing::Mock::VerifyAndClear(mock_writer);
 
-  EXPECT_CALL(*mock_writer, NotifySiteUnloaded());
   observer()->OnLoadingStateChange(web_contents(),
+                                   TabLoadTracker::LoadingState::LOADING,
                                    TabLoadTracker::LoadingState::UNLOADED);
   ::testing::Mock::VerifyAndClear(mock_writer);
 
+  // Ensure that a transition from UNLOADED to LOADING doesn't cause any call to
+  // NotifySiteUnloaded.
+  observer()->OnLoadingStateChange(web_contents(),
+                                   TabLoadTracker::LoadingState::LOADING,
+                                   TabLoadTracker::LoadingState::UNLOADED);
+
   EXPECT_CALL(*mock_writer, OnDestroy());
 }
 

chrome/browser/resource_coordinator/performance_measurement_manager.cc

@@ -27,8 +27,9 @@ void PerformanceMeasurementManager::OnStartTracking(
 
 void PerformanceMeasurementManager::OnLoadingStateChange(
     content::WebContents* web_contents,
-    LoadingState loading_state) {
-  if (loading_state == TabLoadTracker::LOADED)
+    LoadingState old_loading_state,
+    LoadingState new_loading_state) {
+  if (new_loading_state == TabLoadTracker::LOADED)
     render_process_probe_->StartSingleGather();
 }
 

chrome/browser/resource_coordinator/performance_measurement_manager.h

@@ -30,7 +30,8 @@ class PerformanceMeasurementManager : public TabLoadTracker::Observer {
   void OnStartTracking(content::WebContents* web_contents,
                        LoadingState loading_state) override;
   void OnLoadingStateChange(content::WebContents* web_contents,
-                            LoadingState loading_state) override;
+                            LoadingState old_loading_state,
+                            LoadingState new_loading_state) override;
 
  private:
   ScopedObserver<TabLoadTracker, PerformanceMeasurementManager>

chrome/browser/resource_coordinator/tab_load_tracker.cc

@@ -219,7 +219,8 @@ void TabLoadTracker::TransitionState(TabMap::iterator it,
   }
 #endif
 
-  --state_counts_[it->second.loading_state];
+  LoadingState previous_state = it->second.loading_state;
+  --state_counts_[previous_state];
   it->second.loading_state = loading_state;
   ++state_counts_[loading_state];
 
@@ -233,7 +234,7 @@ void TabLoadTracker::TransitionState(TabMap::iterator it,
   content::WebContents* web_contents = it->first;
 
   for (Observer& observer : observers_)
-    observer.OnLoadingStateChange(web_contents, loading_state);
+    observer.OnLoadingStateChange(web_contents, previous_state, loading_state);
 }
 
 TabLoadTracker::Observer::Observer() {}

chrome/browser/resource_coordinator/tab_load_tracker.h

@@ -199,7 +199,8 @@ class TabLoadTracker::Observer {
 
   // Called for every loading state change observed on a |web_contents|.
   virtual void OnLoadingStateChange(content::WebContents* web_contents,
-                                    LoadingState loading_state) {}
+                                    LoadingState old_loading_state,
+                                    LoadingState new_loading_state) {}
 
   // Called when a |web_contents| is no longer being tracked.
   virtual void OnStopTracking(content::WebContents* web_contents,

chrome/browser/resource_coordinator/tab_load_tracker_test_support.cc

@@ -55,11 +55,12 @@ class WaitForLoadingStateHelper : public TabLoadTracker::Observer {
 
  protected:
   void OnLoadingStateChange(content::WebContents* web_contents,
-                            LoadingState loading_state) override {
+                            LoadingState old_loading_state,
+                            LoadingState new_loading_state) override {
     if (waiting_for_no_longer_tracked_)
       return;
     if (waiting_for_contents_ == web_contents &&
-        waiting_for_state_ == loading_state) {
+        waiting_for_state_ == new_loading_state) {
       wait_successful_ = true;
       run_loop_quit_closure_.Run();
     }

chrome/browser/resource_coordinator/tab_load_tracker_unittest.cc

@@ -55,7 +55,8 @@ class LenientMockObserver : public TabLoadTracker::Observer {
 
   // TabLoadTracker::Observer implementation:
   MOCK_METHOD2(OnStartTracking, void(content::WebContents*, LoadingState));
-  MOCK_METHOD2(OnLoadingStateChange, void(content::WebContents*, LoadingState));
+  MOCK_METHOD3(OnLoadingStateChange,
+               void(content::WebContents*, LoadingState, LoadingState));
   MOCK_METHOD2(OnStopTracking, void(content::WebContents*, LoadingState));
 
  private:
@@ -218,7 +219,8 @@ void TabLoadTrackerTest::StateTransitionsTest(bool enable_pai) {
 
   // Finish the loading for contents2.
   EXPECT_CALL(observer(),
-              OnLoadingStateChange(contents2(), TestTabLoadTracker::LOADED));
+              OnLoadingStateChange(contents2(), TestTabLoadTracker::LOADING,
+                                   TestTabLoadTracker::LOADED));
   tester2->TestSetIsLoading(false);
   if (enable_pai) {
     // The state transition should only occur *after* the PAI signal when that
@@ -231,7 +233,8 @@ void TabLoadTrackerTest::StateTransitionsTest(bool enable_pai) {
 
   // Start the loading for contents1.
   EXPECT_CALL(observer(),
-              OnLoadingStateChange(contents1(), TestTabLoadTracker::LOADING));
+              OnLoadingStateChange(contents1(), TestTabLoadTracker::UNLOADED,
+                                   TestTabLoadTracker::LOADING));
   tester1->NavigateAndCommit(GURL("http://baz.com"));
   EXPECT_TAB_COUNTS(3, 0, 1, 2);
   testing::Mock::VerifyAndClearExpectations(&observer());
@@ -239,7 +242,8 @@ void TabLoadTrackerTest::StateTransitionsTest(bool enable_pai) {
   // Stop the loading with an error. The tab should go back to a LOADED
   // state.
   EXPECT_CALL(observer(),
-              OnLoadingStateChange(contents1(), TestTabLoadTracker::LOADED));
+              OnLoadingStateChange(contents1(), TestTabLoadTracker::LOADING,
+                                   TestTabLoadTracker::LOADED));
   tester1->TestDidFailLoadWithError(GURL("http://baz.com"), 500,
                                     base::UTF8ToUTF16("server error"));
   ExpectTabCounts(3, 0, 0, 3);

chrome/browser/resource_coordinator/tab_manager.cc

@@ -588,10 +588,11 @@ void TabManager::OnStartTracking(content::WebContents* web_contents,
 }
 
 void TabManager::OnLoadingStateChange(content::WebContents* web_contents,
-                                      LoadingState loading_state) {
-  GetWebContentsData(web_contents)->SetTabLoadingState(loading_state);
+                                      LoadingState old_loading_state,
+                                      LoadingState new_loading_state) {
+  GetWebContentsData(web_contents)->SetTabLoadingState(new_loading_state);
 
-  if (loading_state == TabLoadTracker::LOADED) {
+  if (new_loading_state == TabLoadTracker::LOADED) {
     bool was_in_background_tab_opening_session =
         IsInBackgroundTabOpeningSession();
 

chrome/browser/resource_coordinator/tab_manager.h

@@ -40,7 +40,7 @@ class TabStripModel;
 namespace content {
 class NavigationHandle;
 class WebContents;
-}
+}  // namespace content
 
 namespace resource_coordinator {
 
@@ -318,7 +318,8 @@ class TabManager : public LifecycleUnitObserver,
   void OnStartTracking(content::WebContents* web_contents,
                        LoadingState loading_state) override;
   void OnLoadingStateChange(content::WebContents* web_contents,
-                            LoadingState loading_state) override;
+                            LoadingState old_loading_state,
+                            LoadingState new_loading_state) override;
   void OnStopTracking(content::WebContents* web_contents,
                       LoadingState loading_state) override;
 

chrome/browser/resource_coordinator/tab_memory_metrics_reporter.cc

@@ -51,8 +51,9 @@ void TabMemoryMetricsReporter::OnStartTracking(
 
 void TabMemoryMetricsReporter::OnLoadingStateChange(
     content::WebContents* web_contents,
-    TabLoadTracker::LoadingState loading_state) {
-  if (loading_state != TabLoadTracker::LoadingState::LOADED)
+    TabLoadTracker::LoadingState old_loading_state,
+    LoadingState new_loading_state) {
+  if (new_loading_state != TabLoadTracker::LoadingState::LOADED)
     return;
 
   MonitorWebContents(web_contents);

chrome/browser/resource_coordinator/tab_memory_metrics_reporter.h

@@ -32,7 +32,8 @@ class TabMemoryMetricsReporter : public TabLoadTracker::Observer {
   void OnStartTracking(content::WebContents* web_contents,
                        LoadingState loading_state) override;
   void OnLoadingStateChange(content::WebContents* web_contents,
-                            LoadingState loading_state) override;
+                            LoadingState old_loading_state,
+                            LoadingState new_loading_state) override;
   void OnStopTracking(content::WebContents* web_contents,
                       LoadingState loading_state) override;
 

chrome/browser/resource_coordinator/tab_memory_metrics_reporter_unittest.cc

@@ -139,25 +139,29 @@ TEST_F(TabMemoryMetricsReporterTest, StartTrackingWithLoaded) {
 }
 
 TEST_F(TabMemoryMetricsReporterTest, OnLoadingStateChangeWithUnloaded) {
-  observer().OnLoadingStateChange(contents1(), TabLoadTracker::UNLOADED);
+  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADING,
+                                  TabLoadTracker::UNLOADED);
   EXPECT_FALSE(observer().update_timer_for_testing().IsRunning());
   EXPECT_FALSE(observer().TopMonitoredContent());
 }
 
 TEST_F(TabMemoryMetricsReporterTest, OnLoadingStateChangeWithLoading) {
-  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADING);
+  observer().OnLoadingStateChange(contents1(), TabLoadTracker::UNLOADED,
+                                  TabLoadTracker::LOADING);
   EXPECT_FALSE(observer().update_timer_for_testing().IsRunning());
   EXPECT_FALSE(observer().TopMonitoredContent());
 }
 
 TEST_F(TabMemoryMetricsReporterTest, OnLoadingStateChangeWithLoaded) {
-  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADED);
+  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADING,
+                                  TabLoadTracker::LOADED);
   EXPECT_TRUE(observer().update_timer_for_testing().IsRunning());
   EXPECT_EQ(contents1(), observer().TopMonitoredContent());
 }
 
 TEST_F(TabMemoryMetricsReporterTest, OnStopTracking) {
-  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADED);
+  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADING,
+                                  TabLoadTracker::LOADED);
   EXPECT_TRUE(observer().update_timer_for_testing().IsRunning());
   EXPECT_EQ(contents1(), observer().TopMonitoredContent());
   observer().OnStopTracking(contents1(), TabLoadTracker::UNLOADED);
@@ -167,11 +171,14 @@ TEST_F(TabMemoryMetricsReporterTest, OnStopTracking) {
 
 TEST_F(TabMemoryMetricsReporterTest, TrackingThreeWithLoaded) {
   ScopedSetTickClockForTesting scoped_set_tick_clock_for_testing_(tick_clock());
-  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADED);
+  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADING,
+                                  TabLoadTracker::LOADED);
   task_runner()->AdvanceMockTickClock(base::TimeDelta::FromMinutes(1));
-  observer().OnLoadingStateChange(contents2(), TabLoadTracker::LOADED);
+  observer().OnLoadingStateChange(contents2(), TabLoadTracker::LOADING,
+                                  TabLoadTracker::LOADED);
   task_runner()->AdvanceMockTickClock(base::TimeDelta::FromMinutes(1));
-  observer().OnLoadingStateChange(contents3(), TabLoadTracker::LOADED);
+  observer().OnLoadingStateChange(contents3(), TabLoadTracker::LOADING,
+                                  TabLoadTracker::LOADED);
 
   EXPECT_TRUE(observer().update_timer_for_testing().IsRunning());
   EXPECT_EQ(contents1(), observer().TopMonitoredContent());
@@ -191,7 +198,8 @@ TEST_F(TabMemoryMetricsReporterTest, TrackingThreeWithLoaded) {
 
 TEST_F(TabMemoryMetricsReporterTest, EmitMemoryDumpAfterOneMinute) {
   ScopedSetTickClockForTesting scoped_set_tick_clock_for_testing_(tick_clock());
-  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADED);
+  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADING,
+                                  TabLoadTracker::LOADED);
   task_runner()->FastForwardBy(base::TimeDelta::FromMinutes(1));
   EXPECT_EQ(1U, observer().emit_count());
   EXPECT_TRUE(observer().update_timer_for_testing().IsRunning());
@@ -201,7 +209,8 @@ TEST_F(TabMemoryMetricsReporterTest, EmitMemoryDumpAfterOneMinute) {
 
 TEST_F(TabMemoryMetricsReporterTest, EmitMemoryDumpAfterFiveMinutes) {
   ScopedSetTickClockForTesting scoped_set_tick_clock_for_testing_(tick_clock());
-  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADED);
+  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADING,
+                                  TabLoadTracker::LOADED);
   task_runner()->FastForwardBy(base::TimeDelta::FromMinutes(5));
   EXPECT_EQ(2U, observer().emit_count());
   EXPECT_TRUE(observer().update_timer_for_testing().IsRunning());
@@ -211,7 +220,8 @@ TEST_F(TabMemoryMetricsReporterTest, EmitMemoryDumpAfterFiveMinutes) {
 
 TEST_F(TabMemoryMetricsReporterTest, EmitMemoryDumpAfterTenMinutes) {
   ScopedSetTickClockForTesting scoped_set_tick_clock_for_testing_(tick_clock());
-  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADED);
+  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADING,
+                                  TabLoadTracker::LOADED);
   task_runner()->FastForwardBy(base::TimeDelta::FromMinutes(10));
   EXPECT_EQ(3U, observer().emit_count());
   EXPECT_TRUE(observer().update_timer_for_testing().IsRunning());
@@ -221,7 +231,8 @@ TEST_F(TabMemoryMetricsReporterTest, EmitMemoryDumpAfterTenMinutes) {
 
 TEST_F(TabMemoryMetricsReporterTest, EmitMemoryDumpAfterFifteenMinutes) {
   ScopedSetTickClockForTesting scoped_set_tick_clock_for_testing_(tick_clock());
-  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADED);
+  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADING,
+                                  TabLoadTracker::LOADED);
   task_runner()->FastForwardBy(base::TimeDelta::FromMinutes(15));
   EXPECT_EQ(4U, observer().emit_count());
   EXPECT_FALSE(observer().update_timer_for_testing().IsRunning());
@@ -230,7 +241,8 @@ TEST_F(TabMemoryMetricsReporterTest, EmitMemoryDumpAfterFifteenMinutes) {
 
 TEST_F(TabMemoryMetricsReporterTest, EmitMemoryDumpSkipFiveMinutes) {
   ScopedSetTickClockForTesting scoped_set_tick_clock_for_testing_(tick_clock());
-  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADED);
+  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADING,
+                                  TabLoadTracker::LOADED);
   task_runner()->AdvanceMockTickClock(base::TimeDelta::FromMinutes(5));
   task_runner()->FastForwardBy(base::TimeDelta::FromMinutes(1));
   EXPECT_EQ(1U, observer().emit_count());
@@ -241,7 +253,8 @@ TEST_F(TabMemoryMetricsReporterTest, EmitMemoryDumpSkipFiveMinutes) {
 
 TEST_F(TabMemoryMetricsReporterTest, EmitMemoryDumpSkipTenMinutes) {
   ScopedSetTickClockForTesting scoped_set_tick_clock_for_testing_(tick_clock());
-  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADED);
+  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADING,
+                                  TabLoadTracker::LOADED);
   task_runner()->AdvanceMockTickClock(base::TimeDelta::FromMinutes(10));
   task_runner()->FastForwardBy(base::TimeDelta::FromMinutes(1));
   EXPECT_EQ(1U, observer().emit_count());
@@ -252,7 +265,8 @@ TEST_F(TabMemoryMetricsReporterTest, EmitMemoryDumpSkipTenMinutes) {
 
 TEST_F(TabMemoryMetricsReporterTest, EmitMemoryDumpSkipFifteenMinutes) {
   ScopedSetTickClockForTesting scoped_set_tick_clock_for_testing_(tick_clock());
-  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADED);
+  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADING,
+                                  TabLoadTracker::LOADED);
   task_runner()->AdvanceMockTickClock(base::TimeDelta::FromMinutes(15));
   task_runner()->FastForwardBy(base::TimeDelta::FromMinutes(1));
   EXPECT_EQ(1U, observer().emit_count());
@@ -262,21 +276,24 @@ TEST_F(TabMemoryMetricsReporterTest, EmitMemoryDumpSkipFifteenMinutes) {
 
 TEST_F(TabMemoryMetricsReporterTest, SecondContentComeAfter9_5Minutes) {
   ScopedSetTickClockForTesting scoped_set_tick_clock_for_testing_(tick_clock());
-  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADED);
+  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADING,
+                                  TabLoadTracker::LOADED);
   task_runner()->FastForwardBy(base::TimeDelta::FromMinutes(9) +
                                base::TimeDelta::FromSeconds(30));
   EXPECT_EQ(2U, observer().emit_count());
   EXPECT_EQ(30, observer().NextEmitTimeFromNow().InSeconds());
   EXPECT_EQ(contents1(), observer().TopMonitoredContent());
 
-  observer().OnLoadingStateChange(contents2(), TabLoadTracker::LOADED);
+  observer().OnLoadingStateChange(contents2(), TabLoadTracker::LOADING,
+                                  TabLoadTracker::LOADED);
   EXPECT_EQ(contents1(), observer().TopMonitoredContent());
   EXPECT_EQ(30, observer().NextEmitTimeFromNow().InSeconds());
 }
 
 TEST_F(TabMemoryMetricsReporterTest, EmitMemoryDumpForDiscardedContent) {
   ScopedSetTickClockForTesting scoped_set_tick_clock_for_testing_(tick_clock());
-  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADED);
+  observer().OnLoadingStateChange(contents1(), TabLoadTracker::LOADING,
+                                  TabLoadTracker::LOADED);
   task_runner()->FastForwardBy(base::TimeDelta::FromMinutes(1));
   EXPECT_EQ(1U, observer().emit_count());
   observer().DiscardContent(contents1());

chrome/browser/sessions/tab_loader.cc

@@ -222,12 +222,13 @@ void TabLoader::StartLoading(const std::vector<RestoredTab>& tabs) {
 }
 
 void TabLoader::OnLoadingStateChange(WebContents* contents,
-                                     LoadingState loading_state) {
+                                     LoadingState old_loading_state,
+                                     LoadingState new_loading_state) {
   ReentrancyHelper lifetime_helper(this);
 
   // Calls into this can come from observers that are still running even if
   // |is_loading_enabled_| is false.
-  switch (loading_state) {
+  switch (new_loading_state) {
     // It could be that a tab starts loading from outside of our control. In
     // this case we can consider it as having started to load, and the load
     // start doesn't need to be initiated by us.

chrome/browser/sessions/tab_loader.h

@@ -105,7 +105,8 @@ class TabLoader : public base::RefCounted<TabLoader>,
 
   // resource_coordinator::TabLoadTracker::Observer implementation:
   void OnLoadingStateChange(content::WebContents* contents,
-                            LoadingState loading_state) override;
+                            LoadingState old_loading_state,
+                            LoadingState new_loading_state) override;
   void OnStopTracking(content::WebContents* contents,
                       LoadingState loading_state) override;