Fixes bad code in OnViewVisibilityChanged
Problem was when OnViewVisibilityChanged is received the client was
calling back to the server to change the visibility again. This is
totally wrong as the client receiving a notification doesn't need to
tell the server to change the visibility agian. This lead to problems
if a client was rapidly changing the visibility as the second client
would receive the change and then try to change the visibility too.
R=erg@chromium.org, alhaad@chromium.org
Review URL: https://codereview.chromium.org/945273002
diff --git a/mojo/services/view_manager/public/cpp/lib/view.cc b/mojo/services/view_manager/public/cpp/lib/view.cc
index bfbc67a..9ed458c 100644
--- a/mojo/services/view_manager/public/cpp/lib/view.cc
+++ b/mojo/services/view_manager/public/cpp/lib/view.cc
@@ -220,9 +220,7 @@
if (manager_)
static_cast<ViewManagerClientImpl*>(manager_)->SetVisible(id_, value);
- FOR_EACH_OBSERVER(ViewObserver, observers_, OnViewVisibilityChanging(this));
- visible_ = value;
- NotifyViewVisibilityChanged(this);
+ LocalSetVisible(value);
}
void View::SetSharedProperty(const std::string& name,
@@ -531,6 +529,15 @@
FOR_EACH_OBSERVER(ViewObserver, observers_, OnViewDrawnChanged(this));
}
+void View::LocalSetVisible(bool visible) {
+ if (visible_ == visible)
+ return;
+
+ FOR_EACH_OBSERVER(ViewObserver, observers_, OnViewVisibilityChanging(this));
+ visible_ = visible;
+ NotifyViewVisibilityChanged(this);
+}
+
void View::NotifyViewVisibilityChanged(View* target) {
if (!NotifyViewVisibilityChangedDown(target)) {
return; // |this| has been deleted.
diff --git a/mojo/services/view_manager/public/cpp/lib/view_manager_client_impl.cc b/mojo/services/view_manager/public/cpp/lib/view_manager_client_impl.cc
index b988903..9a3c022 100644
--- a/mojo/services/view_manager/public/cpp/lib/view_manager_client_impl.cc
+++ b/mojo/services/view_manager/public/cpp/lib/view_manager_client_impl.cc
@@ -379,7 +379,7 @@
// Deal with this some how.
View* view = GetViewById(view_id);
if (view)
- view->SetVisible(visible);
+ ViewPrivate(view).LocalSetVisible(visible);
}
void ViewManagerClientImpl::OnViewDrawnStateChanged(Id view_id, bool drawn) {
diff --git a/mojo/services/view_manager/public/cpp/lib/view_private.h b/mojo/services/view_manager/public/cpp/lib/view_private.h
index dc37196..0b58280 100644
--- a/mojo/services/view_manager/public/cpp/lib/view_private.h
+++ b/mojo/services/view_manager/public/cpp/lib/view_private.h
@@ -59,6 +59,7 @@
view_->LocalSetBounds(old_bounds, new_bounds);
}
void LocalSetDrawn(bool drawn) { view_->LocalSetDrawn(drawn); }
+ void LocalSetVisible(bool visible) { view_->LocalSetVisible(visible); }
private:
View* view_;
diff --git a/mojo/services/view_manager/public/cpp/view.h b/mojo/services/view_manager/public/cpp/view.h
index a48dbc2..fcb49bc 100644
--- a/mojo/services/view_manager/public/cpp/view.h
+++ b/mojo/services/view_manager/public/cpp/view.h
@@ -159,6 +159,7 @@
void LocalSetViewportMetrics(const ViewportMetrics& old_metrics,
const ViewportMetrics& new_metrics);
void LocalSetDrawn(bool drawn);
+ void LocalSetVisible(bool visible);
// Methods implementing visibility change notifications. See ViewObserver
// for more details.
diff --git a/services/view_manager/view_manager_client_apptest.cc b/services/view_manager/view_manager_client_apptest.cc
index 994bfb0..52c2587 100644
--- a/services/view_manager/view_manager_client_apptest.cc
+++ b/services/view_manager/view_manager_client_apptest.cc
@@ -425,10 +425,7 @@
} // namespace
-// TODO(alhaad): Re-enable this once
-// ViewManagerClientImpl::OnViewVisibilityChanged() does not cause a race
-// condition when called from two clients.
-TEST_F(ViewManagerTest, DISABLED_Visible) {
+TEST_F(ViewManagerTest, Visible) {
View* view1 = window_manager()->CreateView();
view1->SetVisible(true);
window_manager()->GetRoot()->AddChild(view1);
