chrome.hid: enrich model with report IDs

chrome/browser/extensions/api/hid/hid_device_manager.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/api/hid/hid_device_manager.h"
 
 #include <limits>
 #include <vector>
 
 #include "base/lazy_instance.h"
@@ skipping 18 matching lines @@
 }
 
 scoped_ptr<base::ListValue> HidDeviceManager::GetApiDevices(
     uint16_t vendor_id,
     uint16_t product_id) {
   UpdateDevices();
 
   HidService* hid_service = HidService::GetInstance();
   DCHECK(hid_service);
   base::ListValue* api_devices = new base::ListValue();
+

[Ken Rockot(use gerrit already), 2014/06/06 20:04:43]

nit: No need to add vertical whitespace

[jracle (use Gerrit), 2014/06/07 12:56:21]

OK. Though I'm a bit puzzled by this rule, for me code lacks readability when
too many groups of code are found. I also have hard time not starting with a new
line after '{', but this is subject to (big) debate in my team!

On 2014/06/06 20:04:43, Ken Rockot wrote:

   for (ResourceIdToDeviceIdMap::const_iterator device_iter =
            device_ids_.begin();
        device_iter != device_ids_.end();
        ++device_iter) {
     int resource_id = device_iter->first;
     device::HidDeviceId device_id = device_iter->second;
     device::HidDeviceInfo device_info;
+
     if (hid_service->GetDeviceInfo(device_id, &device_info)) {
       if (device_info.vendor_id == vendor_id &&
-          device_info.product_id == product_id &&
-          IsDeviceAccessible(device_info)) {
+          device_info.product_id == product_id) {
         api::hid::HidDeviceInfo api_device_info;
         api_device_info.device_id = resource_id;
         api_device_info.vendor_id = device_info.vendor_id;
         api_device_info.product_id = device_info.product_id;
-        for (std::vector<device::HidUsageAndPage>::const_iterator usage_iter =
-                 device_info.usages.begin();
-             usage_iter != device_info.usages.end();
-             ++usage_iter) {
-          api::hid::HidUsageAndPage* usage_and_page =
-              new api::hid::HidUsageAndPage();
-          usage_and_page->usage_page = (*usage_iter).usage_page;
-          usage_and_page->usage = (*usage_iter).usage;
-          linked_ptr<api::hid::HidUsageAndPage> usage_and_page_ptr(
-              usage_and_page);
-          api_device_info.usages.push_back(usage_and_page_ptr);
+        api_device_info.max_input_report_size =
+            device_info.max_input_report_size;
+        api_device_info.max_output_report_size =
+            device_info.max_output_report_size;
+        api_device_info.max_feature_report_size =
+            device_info.max_feature_report_size;
+
+        for (std::vector<device::HidCollectionInfo>::const_iterator
+                 collections_iter = device_info.collections.begin();
+             collections_iter != device_info.collections.end();
+             ++collections_iter) {
+          device::HidCollectionInfo collection = *collections_iter;
+
+          // Don't expose sensitive data.
+          if (collection.usage.IsSensitive())

[Ken Rockot(use gerrit already), 2014/06/06 20:04:43]

nit: Please include braces around single-line blocks, here and elsewhere.

[jracle (use Gerrit), 2014/06/07 12:56:21]

This I sign 100%, that's safest.. I recall Apple's OpenSSL bug

On 2014/06/06 20:04:43, Ken Rockot wrote:

+            continue;
+
+          api::hid::HidCollectionInfo* api_collection =
+              new api::hid::HidCollectionInfo();
+          api_collection->usage.usage_page = collection.usage.usage_page;
+          api_collection->usage.usage = collection.usage.usage;
+
+          for (std::set<int>::const_iterator report_ids_iter =

[Ken Rockot(use gerrit already), 2014/06/06 20:04:43]

I think you could just:

api_collection->report_ids.resize(collection.report_ids.size());
std::copy(collection.report_ids.begin(), collection.report_ids.end(),
api->collection->report_ids.begin());

Yes?

[jracle (use Gerrit), 2014/06/07 12:56:21]

Oh yes!

On 2014/06/06 20:04:43, Ken Rockot wrote:

+                   collection.report_ids.begin();
+               report_ids_iter != collection.report_ids.end();
+               ++report_ids_iter) {
+            api_collection->report_ids.push_back(*report_ids_iter);
+          }
+          linked_ptr<api::hid::HidCollectionInfo> collection_ptr(
+              api_collection);
+          api_device_info.collections.push_back(collection_ptr);

[Ken Rockot(use gerrit already), 2014/06/06 20:04:43]

Instead of naming the linked_ptr, just do

api_device_info.collections.push_back(make_linked_ptr(api_collection));

[jracle (use Gerrit), 2014/06/07 12:56:21]

gr8, sure. Much better.

On 2014/06/06 20:04:43, Ken Rockot wrote:

         }
-        api_devices->Append(api_device_info.ToValue().release());
+
+        // Expose devices with which user can communicate.
+        if (api_device_info.collections.size() > 0)
+          api_devices->Append(api_device_info.ToValue().release());
       }
     }
   }
+
   return scoped_ptr<base::ListValue>(api_devices);
 }
 
 bool HidDeviceManager::GetDeviceInfo(int resource_id,
                                      device::HidDeviceInfo* device_info) {
   UpdateDevices();
   HidService* hid_service = HidService::GetInstance();
   DCHECK(hid_service);
 
   ResourceIdToDeviceIdMap::const_iterator device_iter =
@@ skipping 29 matching lines @@
       DCHECK_LT(next_resource_id_, std::numeric_limits<int>::max());
       new_id = next_resource_id_++;
     }
     new_resource_ids[device_info.device_id] = new_id;
     new_device_ids[new_id] = device_info.device_id;
   }
   device_ids_.swap(new_device_ids);
   resource_ids_.swap(new_resource_ids);
 }
 
-// static
-// TODO(rockot): Add some tests for this.
-bool HidDeviceManager::IsDeviceAccessible(
-    const device::HidDeviceInfo& device_info) {
-  for (std::vector<device::HidUsageAndPage>::const_iterator iter =
-           device_info.usages.begin();
-      iter != device_info.usages.end(); ++iter) {
-    if (!IsUsageAccessible(*iter)) {
-      return false;
-    }
-  }
-  return true;
-}
-
-// static
-bool HidDeviceManager::IsUsageAccessible(
-    const HidUsageAndPage& usage_and_page) {
-  if (usage_and_page.usage_page == HidUsageAndPage::kPageKeyboard)
-    return false;
-
-  if (usage_and_page.usage_page != HidUsageAndPage::kPageGenericDesktop)
-    return true;
-
-  uint16_t usage = usage_and_page.usage;
-  if (usage == HidUsageAndPage::kGenericDesktopPointer ||
-      usage == HidUsageAndPage::kGenericDesktopMouse ||
-      usage == HidUsageAndPage::kGenericDesktopKeyboard ||
-      usage == HidUsageAndPage::kGenericDesktopKeypad) {
-    return false;
-  }
-
-  if (usage >= HidUsageAndPage::kGenericDesktopSystemControl &&
-      usage <= HidUsageAndPage::kGenericDesktopSystemWarmRestart) {
-    return false;
-  }
-
-  if (usage >= HidUsageAndPage::kGenericDesktopSystemDock &&
-      usage <= HidUsageAndPage::kGenericDesktopSystemDisplaySwap) {
-    return false;
-  }
-
-  return true;
-}
-
 }  // namespace extensions