[SystemInfo API] Finish TODOs in SystemInfoProvider

chrome/browser/extensions/api/system_info_display/display_info_provider_unittest.cc

Index: chrome/browser/extensions/api/system_info_display/display_info_provider_unittest.cc
diff --git a/chrome/browser/extensions/api/system_info_display/display_info_provider_unittest.cc b/chrome/browser/extensions/api/system_info_display/display_info_provider_unittest.cc
index 46d50625b635b1fd7570d3cd4c77464485cb3e49..d0e2bd67dca2fa65b86e930afcb397561ad80041 100644
--- a/chrome/browser/extensions/api/system_info_display/display_info_provider_unittest.cc
+++ b/chrome/browser/extensions/api/system_info_display/display_info_provider_unittest.cc
@@ -40,7 +40,7 @@ const struct TestDisplayInfo kTestingDisplayInfoData[] = {
 
 class TestDisplayInfoProvider : public DisplayInfoProvider {
  public:
-  virtual bool QueryInfo(DisplayInfo* info) OVERRIDE;
+  virtual bool QueryInfo() OVERRIDE;
 
  private:
   virtual ~TestDisplayInfoProvider();
@@ -48,10 +48,8 @@ class TestDisplayInfoProvider : public DisplayInfoProvider {
 
 TestDisplayInfoProvider::~TestDisplayInfoProvider() {}
 
-bool TestDisplayInfoProvider::QueryInfo(DisplayInfo* info) {
-  if (info == NULL)
-    return false;
-  info->clear();
+bool TestDisplayInfoProvider::QueryInfo() {
+  info_.clear();
   for (size_t i = 0; i < arraysize(kTestingDisplayInfoData); ++i) {
     linked_ptr<DisplayUnitInfo> unit(new DisplayUnitInfo());
     unit->id = kTestingDisplayInfoData[i].id;
@@ -69,7 +67,7 @@ bool TestDisplayInfoProvider::QueryInfo(DisplayInfo* info) {
     unit->work_area.top = kTestingDisplayInfoData[i].work_area.top;
     unit->work_area.width = kTestingDisplayInfoData[i].work_area.width;
     unit->work_area.height = kTestingDisplayInfoData[i].work_area.height;
-    info->push_back(unit);
+    info_.push_back(unit);
   }
   return true;
 }
@@ -87,12 +85,12 @@ DisplayInfoProviderTest::DisplayInfoProviderTest()
 }
 
 TEST_F(DisplayInfoProviderTest, QueryDisplayInfo) {
-  scoped_ptr<DisplayInfo> display_info(new DisplayInfo());
-  EXPECT_TRUE(display_info_provider_->QueryInfo(display_info.get()));
-  EXPECT_EQ(arraysize(kTestingDisplayInfoData), display_info->size());
+  EXPECT_TRUE(display_info_provider_->QueryInfo());
+  const DisplayInfo& display_info = display_info_provider_->display_info();

[Jeffrey Yasskin, 2013/07/08 19:09:49]

FWIW, this doesn't test anything that runs in the production system except the
type definitions of DisplayInfo and api::system_info_display::DisplayUnitInfo.
You've overridden QueryInfo in the test fixture, and that's all you're calling
here, so no production code runs.

You didn't make this worse in this CL, but I wanted to point it out so that Greg
doesn't think these APIs are ready to escape experimental before it's fixed.

[Haojian Wu, 2013/07/09 13:45:04]

Your suggestion reminds me that the unit_tests of CpuInfoProvider,
MemoryInfoProvider, DisplayInfoProvider
are the same, just overriding the QueryInfo. We need to improve the way of
testing SystemInfo API, especially StorageInfo api. 

About the unit_tests here, as you said, Are they unnecessary? Should we remove
them?

[Jeffrey Yasskin, 2013/07/09 18:19:59]

Yeah, we should remove them and replace them (likely in a future CL) with
something that actually tests the production code. It's possible that these
classes are too thin to be worth testing in a unit test, or that it's extremely
hard to mock the underlying system results, in which case you should just use
the apitest to make sure that the functions exist all the way down and return
results that match what more direct calls return.

By the way, this is why I don't like putting virtual methods in the class I'm
trying to test
(https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension...),
and especially not overriding them inside the test. It's way too easy to
accidentally wind up testing nothing.

I prefer to "design for testing" by designing pure virtual interfaces (or
classes with some pure virtual functions and other non-virtual helper
functions), and then passing mock or fake instances of those classes to the
constructor of the class I'm trying to test. It's still possible to wind up
testing a very thin function with a mock instance under it, but then it's much
more obvious when looking at the production code that this class or function
isn't doing anything.

+  EXPECT_EQ(arraysize(kTestingDisplayInfoData), display_info.size());
   for (size_t i = 0; i < arraysize(kTestingDisplayInfoData); ++i) {
     const linked_ptr<api::system_info_display::DisplayUnitInfo> unit =
-        (*display_info.get())[i];
+        display_info[i];
     EXPECT_EQ(kTestingDisplayInfoData[i].id, unit->id);
     EXPECT_EQ(kTestingDisplayInfoData[i].name, unit->name);
     EXPECT_DOUBLE_EQ(kTestingDisplayInfoData[i].dpi_x, unit->dpi_x);