UIDevTools: Show View's visible info from its metadata

Since View's metadata already includes 'Visible' information now, we can
get rid of direct query about visibility information from View elements.
Also updated the tests to reflect these changes.

BUG=954675

Change-Id: I82a1bb1459984d54c8a71b53832e45338e295e6d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1585070Reviewed-by: Leonard Grey <lgrey@chromium.org>
Commit-Queue: Wei Li <weili@chromium.org>
Cr-Commit-Position: refs/heads/master@{#654508}

components/ui_devtools/css_agent.cc

@@ -51,28 +51,29 @@ std::unique_ptr<CSS::CSSProperty> BuildCSSProperty(const std::string& name,
 }
 
 std::unique_ptr<Array<CSS::CSSProperty>> BuildCSSPropertyArray(
-    const gfx::Rect& bounds,
-    const bool visible) {
+    const gfx::Rect& bounds) {
   auto cssProperties = Array<CSS::CSSProperty>::create();
   cssProperties->addItem(BuildCSSProperty(kHeight, bounds.height()));
   cssProperties->addItem(BuildCSSProperty(kWidth, bounds.width()));
   cssProperties->addItem(BuildCSSProperty(kX, bounds.x()));
   cssProperties->addItem(BuildCSSProperty(kY, bounds.y()));
-  cssProperties->addItem(BuildCSSProperty(kVisibility, visible));
   return cssProperties;
 }
 
 std::unique_ptr<CSS::CSSStyle> BuildCSSStyle(UIElement* ui_element) {
   gfx::Rect bounds;
-  bool visible;
   ui_element->GetBounds(&bounds);
-  ui_element->GetVisible(&visible);
-
   std::unique_ptr<Array<CSS::CSSProperty>> css_properties(
-      BuildCSSPropertyArray(bounds, visible));
+      BuildCSSPropertyArray(bounds));
+
+  if (ui_element->type() != VIEW) {
+    bool visible;
+    ui_element->GetVisible(&visible);
+    css_properties->addItem(BuildCSSProperty(kVisibility, visible));
+  }
+
   const std::vector<std::pair<std::string, std::string>> properties(
       ui_element->GetCustomProperties());
-
   for (const auto& it : properties)
     css_properties->addItem(BuildCSSProperty(it.first, it.second));
 
@@ -203,7 +204,8 @@ bool CSSAgent::GetPropertiesForUIElement(UIElement* ui_element,
                                          bool* visible) {
   if (ui_element) {
     ui_element->GetBounds(bounds);
-    ui_element->GetVisible(visible);
+    if (ui_element->type() != VIEW)
+      ui_element->GetVisible(visible);
     return true;
   }
   return false;

components/ui_devtools/views/view_element.cc

@@ -84,11 +84,13 @@ void ViewElement::SetBounds(const gfx::Rect& bounds) {
 }
 
 void ViewElement::GetVisible(bool* visible) const {
-  *visible = view_->visible();
+  // Visibility information should be directly retrieved from View's metadata,
+  // no need for this function any more.
+  NOTREACHED();
 }
 
 void ViewElement::SetVisible(bool visible) {
-  view_->SetVisible(visible);
+  // Intentional No-op.
 }
 
 bool ViewElement::SetPropertiesFromString(const std::string& text) {

components/ui_devtools/views/view_element_unittest.cc

@@ -9,6 +9,43 @@
 #include "components/ui_devtools/ui_devtools_unittest_utils.h"
 #include "ui/views/test/views_test_base.h"
 
+namespace {
+
+size_t GetPropertyIndex(ui_devtools::ViewElement* element,
+                        const std::string& property_name) {
+  auto props = element->GetCustomProperties();
+  for (size_t index = 0; index < props.size(); ++index) {
+    if (props[index].first == property_name)
+      return index;
+  }
+  DCHECK(false) << "Property " << property_name << " can not be found.";
+  return 0;
+}
+
+void TestBooleanCustomPropertySetting(ui_devtools::ViewElement* element,
+                                      const std::string& property_name,
+                                      bool init_value) {
+  size_t index = GetPropertyIndex(element, property_name);
+  std::string old_value(init_value ? "true" : "false");
+  auto props = element->GetCustomProperties();
+  EXPECT_EQ(props[index].second, old_value);
+
+  // Check the property can be set accordingly.
+  std::string new_value(init_value ? "false" : "true");
+  std::string separator(":");
+  element->SetPropertiesFromString(property_name + separator + new_value);
+  props = element->GetCustomProperties();
+  EXPECT_EQ(props[index].first, property_name);
+  EXPECT_EQ(props[index].second, new_value);
+
+  element->SetPropertiesFromString(property_name + separator + old_value);
+  props = element->GetCustomProperties();
+  EXPECT_EQ(props[index].first, property_name);
+  EXPECT_EQ(props[index].second, old_value);
+}
+
+}  // namespace
+
 namespace ui_devtools {
 
 using ::testing::_;
@@ -79,65 +116,29 @@ TEST_F(ViewElementTest, SettingsBoundsOnElementSetsOnView) {
   EXPECT_EQ(view()->bounds(), gfx::Rect(1, 2, 3, 4));
 }
 
-TEST_F(ViewElementTest, SettingVisibleOnElementSetsOnView) {
-  DCHECK(view()->visible());
-
-  element()->SetVisible(false);
-  EXPECT_FALSE(view()->visible());
-
-  element()->SetVisible(true);
-  EXPECT_TRUE(view()->visible());
-}
-
 TEST_F(ViewElementTest, SetPropertiesFromString) {
-  static const char* kTestProperty = "Enabled";
-
-  auto props = element()->GetCustomProperties();
-  size_t index;
-  for (index = 0; index < props.size(); ++index) {
-    if (props[index].first == kTestProperty) {
-      EXPECT_EQ(props[index].second, "true");
-      break;
-    }
-  }
-
-  // Check the property can be set accordingly.
-  element()->SetPropertiesFromString("Enabled:false");
-  props = element()->GetCustomProperties();
-  EXPECT_EQ(props[index].first, kTestProperty);
-  EXPECT_EQ(props[index].second, "false");
-
-  element()->SetPropertiesFromString("Enabled:true");
-  props = element()->GetCustomProperties();
-  EXPECT_EQ(props[index].first, kTestProperty);
-  EXPECT_EQ(props[index].second, "true");
+  static const char* kEnabledProperty = "Enabled";
+  TestBooleanCustomPropertySetting(element(), kEnabledProperty, true);
 
   // Test setting a non-existent property has no effect.
   element()->SetPropertiesFromString("Enable:false");
-  props = element()->GetCustomProperties();
-  EXPECT_EQ(props[index].first, kTestProperty);
+  auto props = element()->GetCustomProperties();
+  size_t index = GetPropertyIndex(element(), kEnabledProperty);
+  EXPECT_EQ(props[index].first, kEnabledProperty);
   EXPECT_EQ(props[index].second, "true");
 
   // Test setting empty string for property value has no effect.
   element()->SetPropertiesFromString("Enabled:");
   props = element()->GetCustomProperties();
-  EXPECT_EQ(props[index].first, kTestProperty);
+  EXPECT_EQ(props[index].first, kEnabledProperty);
   EXPECT_EQ(props[index].second, "true");
 
   // Ensure setting pure whitespace doesn't crash.
   ASSERT_NO_FATAL_FAILURE(element()->SetPropertiesFromString("   \n  "));
 }
 
-TEST_F(ViewElementTest, GetVisible) {
-  bool visible;
-
-  view()->SetVisible(false);
-  element()->GetVisible(&visible);
-  EXPECT_FALSE(visible);
-
-  view()->SetVisible(true);
-  element()->GetVisible(&visible);
-  EXPECT_TRUE(visible);
+TEST_F(ViewElementTest, SettingVisibilityOnView) {
+  TestBooleanCustomPropertySetting(element(), "Visible", true);
 }
 
 TEST_F(ViewElementTest, GetBounds) {
@@ -167,6 +168,19 @@ TEST_F(ViewElementTest, GetCustomProperties) {
   EXPECT_EQ(props.back().second, "This is the tooltip");
 }
 
+TEST_F(ViewElementTest, CheckCustomProperties) {
+  auto props = element()->GetCustomProperties();
+  DCHECK_GT(props.size(), 1U);
+
+  // Check visibility information is passed in.
+  bool is_visible_set = false;
+  for (size_t i = 0; i < props.size() - 1; ++i) {
+    if (props[i].first == "Visible")
+      is_visible_set = true;
+  }
+  EXPECT_TRUE(is_visible_set);
+}
+
 TEST_F(ViewElementTest, GetNodeWindowAndScreenBounds) {
   // For this to be meaningful, the view must be in
   // a widget.