Flush the pending change in pref

- Add the callback invoked after the observers.
- Flush the pending change in pref.

This will let WebView consistently see the persisted metrics after
restart.

Bug: 1076549
Change-Id: I9ab7a228bfd0431ead9545c37a07905c1a41ec8e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2226067
Commit-Queue: Tao Bai <michaelbai@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#776150}

android_webview/browser/aw_browser_process.cc

@@ -5,6 +5,7 @@
 #include "android_webview/browser/aw_browser_process.h"
 
 #include "android_webview/browser/aw_browser_context.h"
+#include "android_webview/browser/lifecycle/aw_contents_lifecycle_notifier.h"
 #include "android_webview/browser/metrics/visibility_metrics_logger.h"
 #include "base/base_paths_posix.h"
 #include "base/path_service.h"
@@ -41,6 +42,9 @@ AwBrowserProcess::AwBrowserProcess(
     AwFeatureListCreator* aw_feature_list_creator) {
   g_aw_browser_process = this;
   aw_feature_list_creator_ = aw_feature_list_creator;
+  aw_contents_lifecycle_notifier_ =
+      std::make_unique<AwContentsLifecycleNotifier>(base::BindRepeating(
+          &AwBrowserProcess::OnLoseForeground, base::Unretained(this)));
 }
 
 AwBrowserProcess::~AwBrowserProcess() {
@@ -71,6 +75,11 @@ void AwBrowserProcess::CreateLocalState() {
   DCHECK(local_state_);
 }
 
+void AwBrowserProcess::OnLoseForeground() {
+  if (local_state_)
+    local_state_->CommitPendingWrite();
+}
+
 AwBrowserPolicyConnector* AwBrowserProcess::browser_policy_connector() {
   if (!browser_policy_connector_)
     CreateBrowserPolicyConnector();

android_webview/browser/aw_browser_process.h

@@ -9,6 +9,7 @@
 
 #include "android_webview/browser/aw_browser_context.h"
 #include "android_webview/browser/aw_feature_list_creator.h"
+#include "android_webview/browser/lifecycle/aw_contents_lifecycle_notifier.h"
 #include "android_webview/browser/safe_browsing/aw_safe_browsing_ui_manager.h"
 #include "android_webview/browser/safe_browsing/aw_safe_browsing_whitelist_manager.h"
 #include "base/feature_list.h"
@@ -30,6 +31,7 @@ extern const char kAuthServerWhitelist[];
 
 }  // namespace prefs
 
+class AwContentsLifecycleNotifier;
 class VisibilityMetricsLogger;
 
 class AwBrowserProcess {
@@ -75,6 +77,8 @@ class AwBrowserProcess {
 
   void OnAuthPrefsChanged();
 
+  void OnLoseForeground();
+
   // If non-null, this object holds a pref store that will be taken by
   // AwBrowserProcess to create the |local_state_|.
   // The AwFeatureListCreator is owned by AwMainDelegate.
@@ -103,6 +107,7 @@ class AwBrowserProcess {
       safe_browsing_whitelist_manager_;
 
   std::unique_ptr<VisibilityMetricsLogger> visibility_metrics_logger_;
+  std::unique_ptr<AwContentsLifecycleNotifier> aw_contents_lifecycle_notifier_;
 
   DISALLOW_COPY_AND_ASSIGN(AwBrowserProcess);
 };

android_webview/browser/lifecycle/aw_contents_lifecycle_notifier.cc

@@ -16,12 +16,12 @@ namespace android_webview {
 
 namespace {
 
-AwContentsLifecycleNotifier::AwContentsState CalcuateState(
-    bool is_atached_to_window,
+AwContentsLifecycleNotifier::AwContentsState CalculateState(
+    bool is_attached_to_window,
     bool is_window_visible) {
   // Can't assume the sequence of Attached, Detached, Visible, Invisible event
   // because the app could changed it; Calculate the state here.
-  if (is_atached_to_window) {
+  if (is_attached_to_window) {
     return is_window_visible
                ? AwContentsLifecycleNotifier::AwContentsState::kForeground
                : AwContentsLifecycleNotifier::AwContentsState::kBackground;
@@ -29,6 +29,8 @@ AwContentsLifecycleNotifier::AwContentsState CalcuateState(
   return AwContentsLifecycleNotifier::AwContentsState::kDetached;
 }
 
+AwContentsLifecycleNotifier* g_instance = nullptr;
+
 }  // namespace
 
 AwContentsLifecycleNotifier::AwContentsData::AwContentsData() = default;
@@ -40,8 +42,23 @@ AwContentsLifecycleNotifier::AwContentsData::~AwContentsData() = default;
 
 // static
 AwContentsLifecycleNotifier& AwContentsLifecycleNotifier::GetInstance() {
-  static base::NoDestructor<AwContentsLifecycleNotifier> instance;
-  return *instance;
+  DCHECK(g_instance);
+  g_instance->EnsureOnValidSequence();
+  return *g_instance;
+}
+
+AwContentsLifecycleNotifier::AwContentsLifecycleNotifier(
+    OnLoseForegroundCallback on_lose_foreground_callback)
+    : on_lose_foreground_callback_(std::move(on_lose_foreground_callback)) {
+  EnsureOnValidSequence();
+  DCHECK(!g_instance);
+  g_instance = this;
+}
+
+AwContentsLifecycleNotifier::~AwContentsLifecycleNotifier() {
+  EnsureOnValidSequence();
+  DCHECK(g_instance);
+  g_instance = nullptr;
 }
 
 void AwContentsLifecycleNotifier::OnWebViewCreated(
@@ -133,10 +150,6 @@ std::vector<const AwContents*> AwContentsLifecycleNotifier::GetAllAwContents()
   return result;
 }
 
-AwContentsLifecycleNotifier::AwContentsLifecycleNotifier() = default;
-
-AwContentsLifecycleNotifier::~AwContentsLifecycleNotifier() = default;
-
 size_t AwContentsLifecycleNotifier::ToIndex(AwContentsState state) const {
   size_t index = static_cast<size_t>(state);
   DCHECK(index < base::size(state_count_));
@@ -146,7 +159,7 @@ size_t AwContentsLifecycleNotifier::ToIndex(AwContentsState state) const {
 void AwContentsLifecycleNotifier::OnAwContentsStateChanged(
     AwContentsLifecycleNotifier::AwContentsData* data) {
   AwContentsLifecycleNotifier::AwContentsState state =
-      CalcuateState(data->attached_to_window, data->window_visible);
+      CalculateState(data->attached_to_window, data->window_visible);
   if (data->aw_content_state == state)
     return;
   state_count_[ToIndex(data->aw_content_state)]--;
@@ -167,10 +180,16 @@ void AwContentsLifecycleNotifier::UpdateAppState() {
   else
     state = WebViewAppStateObserver::State::kDestroyed;
   if (state != app_state_) {
+    bool previous_in_foreground =
+        app_state_ == WebViewAppStateObserver::State::kForeground;
+
     app_state_ = state;
     for (auto& observer : observers_) {
       observer.OnAppStateChanged(app_state_);
     }
+    if (previous_in_foreground && on_lose_foreground_callback_) {
+      on_lose_foreground_callback_.Run();
+    }
   }
 }
 

android_webview/browser/lifecycle/aw_contents_lifecycle_notifier.h

@@ -9,9 +9,12 @@
 
 #include "android_webview/browser/lifecycle/webview_app_state_observer.h"
 #include "base/android/jni_android.h"
+#include "base/callback.h"
+#include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/no_destructor.h"
 #include "base/observer_list.h"
+#include "base/sequence_checker.h"
 
 namespace android_webview {
 
@@ -19,6 +22,8 @@ class AwContents;
 
 class AwContentsLifecycleNotifier {
  public:
+  using OnLoseForegroundCallback = base::RepeatingClosure;
+
   enum class AwContentsState {
     // AwContents isn't attached to a window.
     kDetached,
@@ -30,6 +35,12 @@ class AwContentsLifecycleNotifier {
 
   static AwContentsLifecycleNotifier& GetInstance();
 
+  // The |onLoseForegroundCallback| will be invoked after all observers when app
+  // lose foreground.
+  explicit AwContentsLifecycleNotifier(
+      OnLoseForegroundCallback on_lose_foreground_callback);
+  virtual ~AwContentsLifecycleNotifier();
+
   void OnWebViewCreated(const AwContents* aw_contents);
   void OnWebViewDestroyed(const AwContents* aw_contents);
   void OnWebViewAttachedToWindow(const AwContents* aw_contents);
@@ -60,11 +71,11 @@ class AwContentsLifecycleNotifier {
     DISALLOW_COPY(AwContentsData);
   };
 
-  friend base::NoDestructor<AwContentsLifecycleNotifier>;
   friend class TestAwContentsLifecycleNotifier;
 
-  AwContentsLifecycleNotifier();
-  virtual ~AwContentsLifecycleNotifier();
+  void EnsureOnValidSequence() const {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  }
 
   size_t ToIndex(AwContentsState state) const;
   void OnAwContentsStateChanged(
@@ -87,9 +98,14 @@ class AwContentsLifecycleNotifier {
   bool has_aw_contents_ever_created_ = false;
 
   base::ObserverList<WebViewAppStateObserver>::Unchecked observers_;
+
+  OnLoseForegroundCallback on_lose_foreground_callback_;
+
   WebViewAppStateObserver::State app_state_ =
       WebViewAppStateObserver::State::kDestroyed;
 
+  SEQUENCE_CHECKER(sequence_checker_);
+
   DISALLOW_COPY_AND_ASSIGN(AwContentsLifecycleNotifier);
 };
 

android_webview/browser/lifecycle/aw_contents_lifecycle_notifier_unittest.cc

@@ -26,9 +26,28 @@ class TestWebViewAppObserver : public WebViewAppStateObserver {
   WebViewAppStateObserver::State state_;
 };
 
+class TestOnLoseForegroundCallback {
+ public:
+  explicit TestOnLoseForegroundCallback(const TestWebViewAppObserver* other)
+      : other_(other) {}
+
+  ~TestOnLoseForegroundCallback() = default;
+
+  void OnLoseForeground() {
+    ASSERT_NE(other_->state(), WebViewAppStateObserver::State::kForeground);
+    called_ = true;
+  }
+  bool called() const { return called_; }
+
+ private:
+  bool called_ = false;
+  const TestWebViewAppObserver* other_;
+};
+
 class TestAwContentsLifecycleNotifier : public AwContentsLifecycleNotifier {
  public:
-  TestAwContentsLifecycleNotifier() = default;
+  explicit TestAwContentsLifecycleNotifier(OnLoseForegroundCallback callback)
+      : AwContentsLifecycleNotifier(callback) {}
   ~TestAwContentsLifecycleNotifier() override = default;
 
   size_t GetAwContentsStateCount(AwContentsState state) const {
@@ -72,11 +91,20 @@ class AwContentsLifecycleNotifierTest : public testing::Test {
               background_count);
   }
 
+  const TestWebViewAppObserver* observer() const { return observer_.get(); }
+  const TestOnLoseForegroundCallback* callback() const {
+    return callback_.get();
+  }
+
  protected:
   // testing::Test.
   void SetUp() override {
     observer_ = std::make_unique<TestWebViewAppObserver>();
-    notifier_ = std::make_unique<TestAwContentsLifecycleNotifier>();
+    callback_ = std::make_unique<TestOnLoseForegroundCallback>(observer_.get());
+    notifier_ = std::make_unique<TestAwContentsLifecycleNotifier>(
+        base::BindRepeating(&TestOnLoseForegroundCallback::OnLoseForeground,
+                            base::Unretained(callback_.get())));
+
     notifier_->AddObserver(observer_.get());
   }
 
@@ -85,6 +113,7 @@ class AwContentsLifecycleNotifierTest : public testing::Test {
  private:
   content::BrowserTaskEnvironment task_environment_;
   std::unique_ptr<TestWebViewAppObserver> observer_;
+  std::unique_ptr<TestOnLoseForegroundCallback> callback_;
   std::unique_ptr<TestAwContentsLifecycleNotifier> notifier_;
 };
 
@@ -282,4 +311,13 @@ TEST_F(AwContentsLifecycleNotifierTest, GetAllAwContents) {
   ASSERT_TRUE(all_aw_contents.empty());
 }
 
+TEST_F(AwContentsLifecycleNotifierTest, LoseForegroundCallback) {
+  const AwContents* fake_aw_contents = reinterpret_cast<const AwContents*>(1);
+  notifier()->OnWebViewCreated(fake_aw_contents);
+  notifier()->OnWebViewAttachedToWindow(fake_aw_contents);
+  notifier()->OnWebViewWindowBeVisible(fake_aw_contents);
+  notifier()->OnWebViewWindowBeInvisible(fake_aw_contents);
+  EXPECT_TRUE(callback()->called());
+}
+
 }  // namespace android_webview