Implement querying CPU time on Windows for systemInfo.cpu API

chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc

Index: chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc
diff --git a/chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc b/chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc
index 7a9e086cb03bbfcc4786c00908242fefc30f12ca..0490b54e8d3ea439cec8544ad2c8c4ebbcb96121 100644
--- a/chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc
+++ b/chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc
@@ -4,11 +4,70 @@
 
 #include "chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h"
 
+#include <windows.h>
+#include <winternl.h>
+
+#include "base/sys_info.h"
+
 namespace extensions {
 
+namespace {
+
+const wchar_t kNtdll[] = L"ntdll.dll";
+const char kNtQuerySystemInformationName[] = "NtQuerySystemInformation";
+
+// See MSDN about NtQuerySystemInformation definition.
+typedef DWORD (WINAPI *NtQuerySystemInformationPF)(DWORD system_info_class,
+                                                   PVOID system_info,
+                                                   ULONG system_info_length,
+                                                   PULONG return_length);
+
+}  // namespace
+
 bool CpuInfoProvider::QueryCpuTimePerProcessor(std::vector<CpuTime>* times) {
-  // TODO(hongbo): use NtQuerySystemInformation to query the cpu time.
-  return false;
+  if (!times)

[alexeypa (please no reviews), 2012/09/12 15:41:55]

Actually, you don't need to test |times| for being NULL here at all. AV while
trying to dereference |times| is a better  indication that something is wrong.
The return value is too easy to ignore.

[Hongbo Min, 2012/09/13 13:16:59]

What does AV stand for? Although it is too easy, it guarantees that the
QueryCpuTimePerProcessor works as normal in any case,even a null point is
passed. It is just for the API's completeness:)

[alexeypa (please no reviews), 2012/09/13 16:45:48]

Access violation.
Probing a pointer parameter is a good idea if the API crosses trust boundary
(user -> kernel) or it is a public API. In this case however, no trust
boundaries are crossed. Both the caller and the callee live in the same binary.
If a caller passes a bogus pointer it will be immediately seen as a crash, and
so it will be fixed. Returning an error does not guarantee anything, since it
can be easily ignored.

There is a different way to put it. Without the check if a caller passes an
invalid pointer (either NULL or not-NULL invalid pointer) the function will
generate access violation. With the check the function return false if the
pointer is NULL and crash if the pointer is invalid but not NULL. So you have an
additional variation in behavior that needs to be tested.

+    return false;
+
+  std::vector<CpuTime> results;
+
+  HMODULE ntdll = GetModuleHandle(kNtdll);
+  DCHECK(ntdll != NULL);

[alexeypa (please no reviews), 2012/09/12 15:41:55]

DCHECK -> CHECK

[Hongbo Min, 2012/09/13 13:16:59]

Done.

+  NtQuerySystemInformationPF NtQuerySystemInformation =
+      reinterpret_cast<NtQuerySystemInformationPF>(
+          ::GetProcAddress(ntdll, kNtQuerySystemInformationName));
+
+  if (!NtQuerySystemInformation)

[alexeypa (please no reviews), 2012/09/12 15:41:55]

This also can be CHECK(NtQuerySystemInformation != NULL).

[Hongbo Min, 2012/09/13 13:16:59]

Done.

+    return false;
+
+  int num_of_processors = base::SysInfo::NumberOfProcessors();
+  scoped_array<SYSTEM_PROCESSOR_PERFORMANCE_INFORMATION> processor_info(
+      new SYSTEM_PROCESSOR_PERFORMANCE_INFORMATION[num_of_processors]);
+
+  ULONG bytes = 0;

[alexeypa (please no reviews), 2012/09/12 15:41:55]

You can merge this line with line #48.

[Hongbo Min, 2012/09/13 13:16:59]

Done.

+  ULONG returned_bytes = 0;
+  bytes = sizeof(SYSTEM_PROCESSOR_PERFORMANCE_INFORMATION) * num_of_processors;
+  if (NtQuerySystemInformation(SystemProcessorPerformanceInformation,

[alexeypa (please no reviews), 2012/09/12 15:41:55]

Use NT_SUCCESS macro to test whether NtQuerySystemInformation succeeded.

[Hongbo Min, 2012/09/13 13:16:59]

Done.

+                               processor_info.get(),
+                               bytes,
+                               &returned_bytes) != 0)
+    return false;
+
+  int returned_num_of_processors =
+      returned_bytes / sizeof(SYSTEM_PROCESSOR_PERFORMANCE_INFORMATION);
+  DCHECK(returned_num_of_processors == num_of_processors);

[alexeypa (please no reviews), 2012/09/12 15:41:55]

Remove this DCHECK. It is legal (though unlikely) situation when
NtQuerySystemInformation will return different number of processors than
requested. 

[Hongbo Min, 2012/09/13 13:16:59]

Removed the DCHECK stmt, but still return false if their values are different to
avoid the inconsistency if the consumer also use SysInfo::NumberOfProcessors to
get the number of processors.

+  if (returned_num_of_processors != num_of_processors)
+    return false;
+
+  for (int i = 0; i < returned_num_of_processors; ++i) {

[alexeypa (please no reviews), 2012/09/12 15:41:55]

Since you know the number of elements you are going to put to the vector,
preallocate them with std::vector::reserve() call before the loop.

[Hongbo Min, 2012/09/13 13:16:59]

Done.

+    CpuTime time;
+    time.kernel = processor_info[i].KernelTime.QuadPart;
+    time.user = processor_info[i].UserTime.QuadPart;
+    time.idle = processor_info[i].IdleTime.QuadPart;
+    results.push_back(time);
+  }
+
+  results.swap(*times);
+  return true;
 }
 
 }  // namespace extensions