Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11088012:
[Win, MediaGallery] Enumerate and handle mtp device attach/detach events. (Closed)
|
Created:
8 years, 2 months ago by kmadhusu Modified:
8 years, 1 month ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Win, MediaGallery] Enumerate and handle media transfer protocol (windows portable) device attach/detach events.
Added code to:
(1) Enumerate attached portable mtp devices.
(2) Handle new mtp device arrival and detach events.
(3) Extract mtp device details such as friendly name, device storage unique id, etc.
BUG=151679
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165126
Patch Set 1 : '' #Patch Set 2 : '' #
Total comments: 10
Patch Set 3 : Addressed review comments #
Total comments: 71
Patch Set 4 : '' #Patch Set 5 : Addressed review comments #
Total comments: 104
Patch Set 6 : Removed StringPrintf() #
Total comments: 34
Patch Set 7 : Addressed comments. #
Total comments: 6
Patch Set 8 : Addressed review comments #
Total comments: 97
Patch Set 9 : Addressed review comments #
Total comments: 60
Patch Set 10 : Addressed review comments #Patch Set 11 : Rebase #
Total comments: 2
Patch Set 12 : Fixed nit. #Patch Set 13 : Rebase #
Total comments: 5
Patch Set 14 : Declared loop variables in the for loop. #
Total comments: 2
Patch Set 15 : Rename storage_iter => storage_map_iter #Messages
Total messages: 43 (0 generated)
https://codereview.chromium.org/11088012/diff/8023/chrome/browser/system_moni... File chrome/browser/system_monitor/media_transfer_protocol_device_observer_win.cc (right): https://codereview.chromium.org/11088012/diff/8023/chrome/browser/system_moni... chrome/browser/system_monitor/media_transfer_protocol_device_observer_win.cc:138: bool SetupConnection(LPWSTR pnp_device_id, nit: SetUp https://codereview.chromium.org/11088012/diff/8023/chrome/browser/system_moni... chrome/browser/system_monitor/media_transfer_protocol_device_observer_win.cc:380: EnumerateStoragesOnBlockingThread, this)); Is there a reason to post this to the thread you're already on? https://codereview.chromium.org/11088012/diff/8023/chrome/browser/system_moni... File chrome/browser/system_monitor/media_transfer_protocol_device_observer_win.h (right): https://codereview.chromium.org/11088012/diff/8023/chrome/browser/system_moni... chrome/browser/system_monitor/media_transfer_protocol_device_observer_win.h:25: namespace mtp { I don't think the mtp namespace is needed https://codereview.chromium.org/11088012/diff/8023/chrome/browser/system_moni... chrome/browser/system_monitor/media_transfer_protocol_device_observer_win.h:29: class MediaTransferProtocolDeviceObserverWin I don't see any particular reason this should be a different class... https://codereview.chromium.org/11088012/diff/8023/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win.cc (right): https://codereview.chromium.org/11088012/diff/8023/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:289: if (message == WM_CREATE) { content/browser/system_message_window_win.cc just does this stuff in the constructor/destructor. Is there a good reason to do it this way instead?
vandebo@: Addressed review comments. PTAL. Thanks. http://codereview.chromium.org/11088012/diff/8023/chrome/browser/system_monit... File chrome/browser/system_monitor/media_transfer_protocol_device_observer_win.cc (right): http://codereview.chromium.org/11088012/diff/8023/chrome/browser/system_monit... chrome/browser/system_monitor/media_transfer_protocol_device_observer_win.cc:138: bool SetupConnection(LPWSTR pnp_device_id, On 2012/10/16 17:42:36, vandebo wrote: > nit: SetUp Done. http://codereview.chromium.org/11088012/diff/8023/chrome/browser/system_monit... chrome/browser/system_monitor/media_transfer_protocol_device_observer_win.cc:380: EnumerateStoragesOnBlockingThread, this)); On 2012/10/16 17:42:36, vandebo wrote: > Is there a reason to post this to the thread you're already on? oops. Fixed. http://codereview.chromium.org/11088012/diff/8023/chrome/browser/system_monit... File chrome/browser/system_monitor/media_transfer_protocol_device_observer_win.h (right): http://codereview.chromium.org/11088012/diff/8023/chrome/browser/system_monit... chrome/browser/system_monitor/media_transfer_protocol_device_observer_win.h:25: namespace mtp { On 2012/10/16 17:42:36, vandebo wrote: > I don't think the mtp namespace is needed Done. http://codereview.chromium.org/11088012/diff/8023/chrome/browser/system_monit... chrome/browser/system_monitor/media_transfer_protocol_device_observer_win.h:29: class MediaTransferProtocolDeviceObserverWin On 2012/10/16 17:42:36, vandebo wrote: > I don't see any particular reason this should be a different class... This class has several portable device specific code. I dont want to add them in RemovableDeviceNotificationsWindowWin class. http://codereview.chromium.org/11088012/diff/8023/chrome/browser/system_monit... File chrome/browser/system_monitor/removable_device_notifications_window_win.cc (right): http://codereview.chromium.org/11088012/diff/8023/chrome/browser/system_monit... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:289: if (message == WM_CREATE) { On 2012/10/16 17:42:36, vandebo wrote: > content/browser/system_message_window_win.cc just does this stuff in the > constructor/destructor. Is there a good reason to do it this way instead? This code is very similar to the SystemMessageWindowWin::DeviceNotifications. SystemMessageWindowWin::DeviceNotifications constructor has access to window handle. In this case, this is the earliest we can get access to the window handle.
(Adding jam@ as reviewer) jam@: Can you please review windows specific code (ScopedComPtr, ScopedComMem, portable device manager code etc.,) in portable_device_watcher_win.cc, removable_device_notifications_window_win.cc, removable_device_notifications_window_win_unittest.cc? Thanks.
I'm too overloaded with reviews in content/, please redirect.
btw I suggest one of pkasting (who's been doing work with the scoped windows objects lately), cpu, ananta
(Adding pkasting@ as reviewer) pkasting@: Can you please review windows specific code (ScopedComPtr, ScopedComMem, portable device manager code etc.,) in portable_device_watcher_win.cc, removable_device_notifications_window_win.cc, removable_device_notifications_window_win_unittest.cc? Thanks.
https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.cc:31: using content::BrowserThread; Please avoid using statements unless they save a great deal of space. We prefer explicit qualification when possible. https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.cc:43: broadcast_hdr->dbch_devicetype != DBT_DEVTYP_DEVICEINTERFACE) { Nit: {} not necessary with one-line conditional bodies (multiple places) Also, I suggest parens around binary subexpressions (multiple places) https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.cc:61: return base::i18n::ToLower(dev_interface->dbcc_name); Is ToLower() really appropriate here? It's locale-specific, so if we actually need lowering of unicode text or the like it's unlikely to be correct. Why do you need to change case at all? https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.cc:71: const_cast<IPortableDeviceManager*>(portable_device_mgr); Don't const-cast. Take a non-const pointer. If this means callers can't be const functions then so be it. https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.cc:78: ZeroMemory(friendly_name.get(), name_len); Don't do this... use WriteInto() with a standard string16. (several places) https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.cc:83: return string16(friendly_name.get()); Nit: No need for explicit string16 conversions (several places) https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.cc:139: DPLOG(ERROR) << "Failed to create an instance of IPortableDeviceValues"; I've never even heard of DPLOG. What the heck is it? If we really need logging we normally use DVLOG(1), but generally logging is discouraged unless you absolutely know you have to have it since it's hard to get log data back from user machines. (many places) https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.cc:168: return true; It seems strange that we check for a bunch of failures above but then continue anyway and return true. Is this correct? Can these calls ever actually fail? https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.cc:187: LPWSTR device_id = const_cast<char16*>(pnp_device_id.c_str()); Again, don't const_cast. In this case MSDN claims Open() takes an LPCWSTR so you shouldn't need to supply a non-const pointer anyway. https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.cc:206: if (IsDeviceObjectId(object_id)) Nit: Or just return IsDeviceObjectId(object_id) ? WPD_DEVICE_SERIAL_NUMBER : WPD_OBJECT_PERSISTENT_UNIQUE_ID; https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.cc:229: const ScopedComPtr<IPortableDeviceValues>& properties_values, Nit: Passing ScopedComPtr by const ref is weird. I suggest passing a const IPortableDeviceValues& instead unless you actually need some functionality of the ScopedComPtr class. (several places) https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.cc:286: string16 unique_id = base::StringPrintf(L"%ls%ls%ls%ls", Nit: I find it clearer to just do: string16 unique_id(chrome::kMtpDeviceStorageIdPrefix + storage_id + chrome::kNonSpaceDelim + device_serial_num); It's slightly less efficient but who cares. You can even inline this into the next statement. https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.cc:363: void PortableDeviceWatcherWin::OnWindowMessage(UINT event_type, LPARAM data) { I can't find anywhere that actually calls this or supplies its address to anyone? https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.cc:386: registrar_.RemoveAll(); Nit: Not necessary, this happens automatically https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.cc:402: if (hr == CO_E_NOTINITIALIZED) Nit: DCHECK_NE(CO_E_NOTINITIALIZED, hr); https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.cc:419: string16 name = GetFriendlyName(pnp_device_id, portable_device_mgr_.get()); Nit: If you change these functions to return a bool, you can condense this to just: string16 name; return GetFriendlyName(pnp_device_id, portable_device_mgr_.get()) || GetDeviceDescription(pnp_device_id, portable_device_mgr_.get()) || GetManufacturerName(pnp_device_id, portable_device_mgr_.get()); The fact that all the functions do the same mucking with getting a device ID and an IPortableDeviceManager* initially suggests that you should factor that code out to have it happen here, and then pass those values in directly. https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.cc:493: for (DWORD index = 0; index < pnp_device_count; ++index) { Nit: {} unnecessary https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.cc:529: continue; Don't handle assertion failure. This block should just be a DCHECK_NE(storage_map_.end(), storage_entry); https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.cc:547: if (on_shutdown_event_.IsSignaled()) { Nit: Shouldn't this be checked before we make any other calls, to save wasted work? https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... File chrome/browser/system_monitor/portable_device_watcher_win.h (right): https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.h:37: content::BrowserThread::DeleteOnUIThread>, Except in extreme cases, refcounting implementations are banned, especially ones with scary annotations like DeleteOnUIThread. You should not need to be refcounted. Your class should be created and destroyed on a single thread. Activity that happens on other threads should either be safed with locks and guaranteed to complete before the main object is destroyed; bound to callbacks that take weak pointers to the main class if the callbacks must be canceled automatically on main class destruction; or be confined to member classes whose instantiations are themselves scoped to those threads. Try contacting willchan@ if you need advice on how to convert your classes to avoid refcounting. Please also fix the other classes in this subsystem that have similar designs, e.g. the volume mount watcher. https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/portable_device_watcher_win.h:102: std::vector<DeviceStorageInfo>* storages); "Storages" is not a word (grammar: storage is a mass noun, not a count noun). Use constructions like "StorageIDs" or "StorageInfoForDevices" or the like instead, in function names, args, and temps. (several places across multiple files) https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... File chrome/browser/system_monitor/removable_device_notifications_window_win.cc (right): https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:138: static HDEVNOTIFY hDeviceNotify; Overriding the WndProc and then looking for specific messages is crufty, and the use of statics here is fragile. Instead, have a class that uses the standard cracking macros to intercept WM_CREATE and WM_CLOSE and can store the HDEVNOTIFY in a member. See OmniboxViewWin for an example of a class which uses these sorts of macros. https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:143: DEV_BROADCAST_DEVICEINTERFACE db = {0}; Nit: Or just: DEV_BROADCAST_DEVICEINTERFACE db = { sizeof(DEV_BROADCAST_DEVICEINTERFACE), DBT_DEVTYP_DEVICEINTERFACE, 0, guidDevInterface }; https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:177: bool result = volume_mount_watcher_->GetDeviceInfo(device_path, Nit: Shorter: return volume_mount_watcher_->GetDeviceInfo(device_path, device_location, unique_id, name, removable) || portable_device_watcher_->GetDeviceInfo(device_path, device_location, unique_id, name, removable); https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... File chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc (right): https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:53: bool* removable) { Nit: One arg per line, indented even (several places) https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:95: if (pnp_device_id == string16(kMtpDeviceWithInvalidInfo)) Nit: No need for explicit string16 construction (many places) https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:103: return std::string(); Nit: Or just: return (storage_object_id == kStorageObjectIdB) ? string16(kStorageUniqueIdB) : string16(); (several places) https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:124: std::vector<string16> GetMtpStorageObjectId(const string16& pnp_device_id) { Nit: GetMtpStorageObjectIds? https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:139: bool GetMtpStorageDetails( This method can't fail, so return void. https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:161: } Nit: Do not define functions inline in class declarations even for classes declared in a .cc file. Separate declarations and definitions. Go ahead and fix the rest of this file while you're here. https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:170: // Override PortableDeviceWatcherWin::InitOnBlockingThread. Nit: Don't comment individual function overrides this way. Instead do this: // PortableDeviceWatcherWin: void InitOnBlockingThread() OVERRIDE; virtual string16 GetDeviceName(const string16& pnp_device_id) OVERRIDE; virtual bool GetStorages(const string16& pnp_device_id, std::vector<DeviceStorageInfo>* storages) OVERRIDE; https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:408: size_t dbcc_name_size = device_id_size + sizeof(char16); Nit: Inline into next statement (2 places) https://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_mon... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:410: scoped_ptr_malloc<DEV_BROADCAST_DEVICEINTERFACE> dev_interface_broadcast( Ugh, why are you using scoped_ptr_malloc<> and ZeroMemory() and memcpy() and the like? Why not just use a standard scoped_ptr<>?
pkasting: Addressed review comments. PTAL. Thanks. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:31: using content::BrowserThread; On 2012/10/19 21:31:12, Peter Kasting wrote: > Please avoid using statements unless they save a great deal of space. We prefer > explicit qualification when possible. Done. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:43: broadcast_hdr->dbch_devicetype != DBT_DEVTYP_DEVICEINTERFACE) { On 2012/10/19 21:31:12, Peter Kasting wrote: > Nit: {} not necessary with one-line conditional bodies (multiple places) > If the condition spans over a line, I thought it is better to have parens. > Also, I suggest parens around binary subexpressions (multiple places) Done. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:61: return base::i18n::ToLower(dev_interface->dbcc_name); On 2012/10/19 21:31:12, Peter Kasting wrote: > Is ToLower() really appropriate here? It's locale-specific, so if we actually > need lowering of unicode text or the like it's unlikely to be correct. Why do > you need to change case at all? When a mtp device is attached, DEV_BROADCAST_DEVICEINTERFACE provides an upper case plug and play id string (E.g: L"\\?\USB#VID_FF&PID_000F#32&2&1#{ABCD-1234-FFFE-1112-9172})"). But IPortableDeviceManager uses lower case pnp device id string. In order to make the string comparisons simpler, I changed this string to a lower case value. Modified code to use StringToLowerASCII() function. I think that is the right way to convert a string16 string. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:71: const_cast<IPortableDeviceManager*>(portable_device_mgr); On 2012/10/19 21:31:12, Peter Kasting wrote: > Don't const-cast. Take a non-const pointer. If this means callers can't be > const functions then so be it. Done. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:78: ZeroMemory(friendly_name.get(), name_len); On 2012/10/19 21:31:12, Peter Kasting wrote: > Don't do this... use WriteInto() with a standard string16. (several places) Done. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:83: return string16(friendly_name.get()); On 2012/10/19 21:31:12, Peter Kasting wrote: > Nit: No need for explicit string16 conversions (several places) Done. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:139: DPLOG(ERROR) << "Failed to create an instance of IPortableDeviceValues"; On 2012/10/19 21:31:12, Peter Kasting wrote: > I've never even heard of DPLOG. What the heck is it? > > If we really need logging we normally use DVLOG(1), but generally logging is > discouraged unless you absolutely know you have to have it since it's hard to > get log data back from user machines. (many places) DPLOG appends the last system error (GetLastError()) to the log message in string form. As you pointed, it's hard to get log data back from the users. Therefore, I removed most of the messages and retained only the important ones. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:168: return true; On 2012/10/19 21:31:12, Peter Kasting wrote: > It seems strange that we check for a bunch of failures above but then continue > anyway and return true. Is this correct? Can these calls ever actually fail? These calls can fail. It is not mandatory to set all these client information. We are just attempting to set all these client details. We can always open the device for communication without providing any of these client details. These failure log messages are not important. So, I am removing these log messages. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:187: LPWSTR device_id = const_cast<char16*>(pnp_device_id.c_str()); On 2012/10/19 21:31:12, Peter Kasting wrote: > Again, don't const_cast. In this case MSDN claims Open() takes an LPCWSTR so > you shouldn't need to supply a non-const pointer anyway. Done. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:206: if (IsDeviceObjectId(object_id)) On 2012/10/19 21:31:12, Peter Kasting wrote: > Nit: Or just > > return IsDeviceObjectId(object_id) ? > WPD_DEVICE_SERIAL_NUMBER : WPD_OBJECT_PERSISTENT_UNIQUE_ID; Done. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:229: const ScopedComPtr<IPortableDeviceValues>& properties_values, On 2012/10/19 21:31:12, Peter Kasting wrote: > Nit: Passing ScopedComPtr by const ref is weird. I suggest passing a const > IPortableDeviceValues& instead unless you actually need some functionality of > the ScopedComPtr class. (several places) When I pass "const IPortableDeviceValues&", I need to do a const_cast before calling IPortableDeviceValues functions. Therefore, modified function to take "IPortableDeviceValues*" directly. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:286: string16 unique_id = base::StringPrintf(L"%ls%ls%ls%ls", On 2012/10/19 21:31:12, Peter Kasting wrote: > Nit: I find it clearer to just do: > > string16 unique_id(chrome::kMtpDeviceStorageIdPrefix + storage_id + > chrome::kNonSpaceDelim + device_serial_num); > > It's slightly less efficient but who cares. > > You can even inline this into the next statement. I would like to leave it as it is. I have added the format of |unique_id| to understand this code. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:363: void PortableDeviceWatcherWin::OnWindowMessage(UINT event_type, LPARAM data) { On 2012/10/19 21:31:12, Peter Kasting wrote: > I can't find anywhere that actually calls this or supplies its address to > anyone? It is called from RemovableDeviceNotificationsWindowWin::OnDeviceChange() function. (Ref: http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni...) http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:386: registrar_.RemoveAll(); On 2012/10/19 21:31:12, Peter Kasting wrote: > Nit: Not necessary, this happens automatically Removed. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:402: if (hr == CO_E_NOTINITIALIZED) On 2012/10/19 21:31:12, Peter Kasting wrote: > Nit: DCHECK_NE(CO_E_NOTINITIALIZED, hr); Done. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:419: string16 name = GetFriendlyName(pnp_device_id, portable_device_mgr_.get()); On 2012/10/19 21:31:12, Peter Kasting wrote: > Nit: If you change these functions to return a bool, you can condense this to > just: > > string16 name; > return GetFriendlyName(pnp_device_id, portable_device_mgr_.get()) || > GetDeviceDescription(pnp_device_id, portable_device_mgr_.get()) || > GetManufacturerName(pnp_device_id, portable_device_mgr_.get()); > > The fact that all the functions do the same mucking with getting a device ID and > an IPortableDeviceManager* initially suggests that you should factor that code > out to have it happen here, and then pass those values in directly. Condensed the code as suggested. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:493: for (DWORD index = 0; index < pnp_device_count; ++index) { On 2012/10/19 21:31:12, Peter Kasting wrote: > Nit: {} unnecessary Done. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:529: continue; On 2012/10/19 21:31:12, Peter Kasting wrote: > Don't handle assertion failure. > > This block should just be a DCHECK_NE(storage_map_.end(), storage_entry); DCHECK_NE doesn't seem to work with iterators. Modified code to use DCHECK(storage_entry != storage_map_.end()) http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:547: if (on_shutdown_event_.IsSignaled()) { On 2012/10/19 21:31:12, Peter Kasting wrote: > Nit: Shouldn't this be checked before we make any other calls, to save wasted > work? Done. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.h (right): http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:37: content::BrowserThread::DeleteOnUIThread>, On 2012/10/19 21:31:12, Peter Kasting wrote: > Except in extreme cases, refcounting implementations are banned, especially ones > with scary annotations like DeleteOnUIThread. > > You should not need to be refcounted. Your class should be created and > destroyed on a single thread. Activity that happens on other threads should > either be safed with locks and guaranteed to complete before the main object is > destroyed; bound to callbacks that take weak pointers to the main class if the > callbacks must be canceled automatically on main class destruction; or be > confined to member classes whose instantiations are themselves scoped to those > threads. > > Try contacting willchan@ if you need advice on how to convert your classes to > avoid refcounting. > > Please also fix the other classes in this subsystem that have similar designs, > e.g. the volume mount watcher. Thanks for the suggestion. Removed refcounting implementations. Added code to support weak pointers. All the functions of this class are accessed on UI thread. I will submit a separate CL to fix volume mount watcher. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:102: std::vector<DeviceStorageInfo>* storages); On 2012/10/19 21:31:12, Peter Kasting wrote: > "Storages" is not a word (grammar: storage is a mass noun, not a count noun). > Use constructions like "StorageIDs" or "StorageInfoForDevices" or the like > instead, in function names, args, and temps. (several places across multiple > files) Renamed GetStorages => GetDeviceStorageInfoList Renamed |storages| => |storage_info_list| http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win.cc (right): http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:138: static HDEVNOTIFY hDeviceNotify; On 2012/10/19 21:31:12, Peter Kasting wrote: > Overriding the WndProc and then looking for specific messages is crufty, and the > use of statics here is fragile. > > Instead, have a class that uses the standard cracking macros to intercept > WM_CREATE and WM_CLOSE and can store the HDEVNOTIFY in a member. See > OmniboxViewWin for an example of a class which uses these sorts of macros. Done. I used SystemMessageWindowWin::DeviceNotifications as a reference and created RemovableDeviceNotificationsWindowWin::DeviceNotifications class to register and unregister portable device notifications. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:143: DEV_BROADCAST_DEVICEINTERFACE db = {0}; On 2012/10/19 21:31:12, Peter Kasting wrote: > Nit: Or just: > > DEV_BROADCAST_DEVICEINTERFACE db = { > sizeof(DEV_BROADCAST_DEVICEINTERFACE), DBT_DEVTYP_DEVICEINTERFACE, 0, > guidDevInterface }; Done. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:177: bool result = volume_mount_watcher_->GetDeviceInfo(device_path, On 2012/10/19 21:31:12, Peter Kasting wrote: > Nit: Shorter: > > return volume_mount_watcher_->GetDeviceInfo(device_path, device_location, > unique_id, name, removable) || > portable_device_watcher_->GetDeviceInfo(device_path, device_location, > unique_id, name, removable); Done. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc (right): http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:53: bool* removable) { On 2012/10/19 21:31:12, Peter Kasting wrote: > Nit: One arg per line, indented even (several places) Done. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:95: if (pnp_device_id == string16(kMtpDeviceWithInvalidInfo)) On 2012/10/19 21:31:12, Peter Kasting wrote: > Nit: No need for explicit string16 construction (many places) Done. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:103: return std::string(); On 2012/10/19 21:31:12, Peter Kasting wrote: > Nit: Or just: > > return (storage_object_id == kStorageObjectIdB) ? > string16(kStorageUniqueIdB) : string16(); > > (several places) Done. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:124: std::vector<string16> GetMtpStorageObjectId(const string16& pnp_device_id) { On 2012/10/19 21:31:12, Peter Kasting wrote: > Nit: GetMtpStorageObjectIds? Done. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:139: bool GetMtpStorageDetails( On 2012/10/19 21:31:12, Peter Kasting wrote: > This method can't fail, so return void. Done. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:161: } On 2012/10/19 21:31:12, Peter Kasting wrote: > Nit: Do not define functions inline in class declarations even for classes > declared in a .cc file. Separate declarations and definitions. > > Go ahead and fix the rest of this file while you're here. Done. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:170: // Override PortableDeviceWatcherWin::InitOnBlockingThread. On 2012/10/19 21:31:12, Peter Kasting wrote: > Nit: Don't comment individual function overrides this way. Instead do this: > > // PortableDeviceWatcherWin: > void InitOnBlockingThread() OVERRIDE; > virtual string16 GetDeviceName(const string16& pnp_device_id) OVERRIDE; > virtual bool GetStorages(const string16& pnp_device_id, > std::vector<DeviceStorageInfo>* storages) OVERRIDE; Done. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:408: size_t dbcc_name_size = device_id_size + sizeof(char16); On 2012/10/19 21:31:12, Peter Kasting wrote: > Nit: Inline into next statement (2 places) I would like to leave this as it is. To me, this is easy to read and understand. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:410: scoped_ptr_malloc<DEV_BROADCAST_DEVICEINTERFACE> dev_interface_broadcast( On 2012/10/19 21:31:12, Peter Kasting wrote: > Ugh, why are you using scoped_ptr_malloc<> and ZeroMemory() and memcpy() and the > like? Why not just use a standard scoped_ptr<>? By default, DEV_BROADCAST_DEVICEINTERFACE dbcc_name field is a single element array of type TCHAR. For unit tests, I want to overwrite that data field with a pnp_device_id (whose length is > 1). So, I allocated sufficient memory using scoped_ptr_malloc<>, initialized using ZeroMemory() and copied the data using memcpy(). Since I am allocating more memory than the basic structure size, I thought it is better to use scoped_ptr_malloc<> rather than doing scoped_ptr<>(new char16[new_size]);
Haven't re-reviewed, just responding to a few comments. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:43: broadcast_hdr->dbch_devicetype != DBT_DEVTYP_DEVICEINTERFACE) { On 2012/10/23 23:44:17, kmadhusu wrote: > On 2012/10/19 21:31:12, Peter Kasting wrote: > > Nit: {} not necessary with one-line conditional bodies (multiple places) > > > If the condition spans over a line, I thought it is better to have parens. It's not banned, but it's not required either. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:61: return base::i18n::ToLower(dev_interface->dbcc_name); On 2012/10/23 23:44:17, kmadhusu wrote: > On 2012/10/19 21:31:12, Peter Kasting wrote: > > Is ToLower() really appropriate here? It's locale-specific, so if we actually > > need lowering of unicode text or the like it's unlikely to be correct. Why do > > you need to change case at all? > > When a mtp device is attached, DEV_BROADCAST_DEVICEINTERFACE provides an upper > case plug and play id string (E.g: > L"\\?\USB#VID_FF&PID_000F#32&2&1#{ABCD-1234-FFFE-1112-9172})"). But > IPortableDeviceManager uses lower case pnp device id string. In order to make > the string comparisons simpler, I changed this string to a lower case value. > > Modified code to use StringToLowerASCII() function. I think that is the right > way to convert a string16 string. It's correct as long as you're guaranteed that the input is ASCII. I couldn't find such a guarantee on MSDN. Are you sure the input will always be ASCII? (Might want a DCHECK(IsStringASCII()) if so, to document this.) http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:286: string16 unique_id = base::StringPrintf(L"%ls%ls%ls%ls", On 2012/10/23 23:44:17, kmadhusu wrote: > On 2012/10/19 21:31:12, Peter Kasting wrote: > > Nit: I find it clearer to just do: > > > > string16 unique_id(chrome::kMtpDeviceStorageIdPrefix + storage_id + > > chrome::kNonSpaceDelim + device_serial_num); > > > > It's slightly less efficient but who cares. > > > > You can even inline this into the next statement. > > I would like to leave it as it is. I have added the format of |unique_id| to > understand this code. Are you sure %ls is correct on non-Windows, then? I thought %ls was "wchar_t*", which a string16 isn't (on non-Windows). The benefit of just appending strings together is that not only is the code shorter but you're safed against incorrect type specifiers. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc (right): http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:408: size_t dbcc_name_size = device_id_size + sizeof(char16); On 2012/10/23 23:44:17, kmadhusu wrote: > On 2012/10/19 21:31:12, Peter Kasting wrote: > > Nit: Inline into next statement (2 places) > > I would like to leave this as it is. To me, this is easy to read and understand. Actually, now that I look closer, this statement is wrong. You're reserving space for a \0 terminator, but the struct already accounts for this by declaring dbcc_name as a char[1]. So you shouldn't actually add another char16 here. http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:410: scoped_ptr_malloc<DEV_BROADCAST_DEVICEINTERFACE> dev_interface_broadcast( On 2012/10/23 23:44:17, kmadhusu wrote: > On 2012/10/19 21:31:12, Peter Kasting wrote: > > Ugh, why are you using scoped_ptr_malloc<> and ZeroMemory() and memcpy() and > the > > like? Why not just use a standard scoped_ptr<>? > > By default, DEV_BROADCAST_DEVICEINTERFACE dbcc_name field is a single element > array of type TCHAR. For unit tests, I want to overwrite that data field with a > pnp_device_id (whose length is > 1). So, I allocated sufficient memory using > scoped_ptr_malloc<>, initialized using ZeroMemory() and copied the data using > memcpy(). Since I am allocating more memory than the basic structure size, I > thought it is better to use scoped_ptr_malloc<> rather than doing > scoped_ptr<>(new char16[new_size]); Yeah, you're fine. I didn't really realize this struct is basically variable-size.
Addressed review comments. Please refer to my inline reply to your comments. Thanks. On 2012/10/24 00:06:32, Peter Kasting wrote: > Haven't re-reviewed, just responding to a few comments. > > http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... > File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): > > http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... > chrome/browser/system_monitor/portable_device_watcher_win.cc:43: > broadcast_hdr->dbch_devicetype != DBT_DEVTYP_DEVICEINTERFACE) { > On 2012/10/23 23:44:17, kmadhusu wrote: > > On 2012/10/19 21:31:12, Peter Kasting wrote: > > > Nit: {} not necessary with one-line conditional bodies (multiple places) > > > > > If the condition spans over a line, I thought it is better to have parens. > > It's not banned, but it's not required either. > > http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... > chrome/browser/system_monitor/portable_device_watcher_win.cc:61: return > base::i18n::ToLower(dev_interface->dbcc_name); > On 2012/10/23 23:44:17, kmadhusu wrote: > > On 2012/10/19 21:31:12, Peter Kasting wrote: > > > Is ToLower() really appropriate here? It's locale-specific, so if we > actually > > > need lowering of unicode text or the like it's unlikely to be correct. Why > do > > > you need to change case at all? > > > > When a mtp device is attached, DEV_BROADCAST_DEVICEINTERFACE provides an upper > > case plug and play id string (E.g: > > L"\\?\USB#VID_FF&PID_000F#32&2&1#{ABCD-1234-FFFE-1112-9172})"). But > > IPortableDeviceManager uses lower case pnp device id string. In order to make > > the string comparisons simpler, I changed this string to a lower case value. > > > > Modified code to use StringToLowerASCII() function. I think that is the right > > way to convert a string16 string. > > It's correct as long as you're guaranteed that the input is ASCII. I couldn't > find such a guarantee on MSDN. Are you sure the input will always be ASCII? > (Might want a DCHECK(IsStringASCII()) if so, to document this.) > Done. Added a DCHECK. > http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... > chrome/browser/system_monitor/portable_device_watcher_win.cc:286: string16 > unique_id = base::StringPrintf(L"%ls%ls%ls%ls", > On 2012/10/23 23:44:17, kmadhusu wrote: > > On 2012/10/19 21:31:12, Peter Kasting wrote: > > > Nit: I find it clearer to just do: > > > > > > string16 unique_id(chrome::kMtpDeviceStorageIdPrefix + storage_id + > > > chrome::kNonSpaceDelim + device_serial_num); > > > > > > It's slightly less efficient but who cares. > > > > > > You can even inline this into the next statement. > > > > I would like to leave it as it is. I have added the format of |unique_id| to > > understand this code. > > Are you sure %ls is correct on non-Windows, then? I thought %ls was "wchar_t*", > which a string16 isn't (on non-Windows). > This code is windows specific. So, "%ls" seems correct here. > The benefit of just appending strings together is that not only is the code > shorter but you're safed against incorrect type specifiers. > > http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... > File > chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc > (right): > > http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... > chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:408: > size_t dbcc_name_size = device_id_size + sizeof(char16); > On 2012/10/23 23:44:17, kmadhusu wrote: > > On 2012/10/19 21:31:12, Peter Kasting wrote: > > > Nit: Inline into next statement (2 places) > > > > I would like to leave this as it is. To me, this is easy to read and > understand. > > Actually, now that I look closer, this statement is wrong. You're reserving > space for a \0 terminator, but the struct already accounts for this by declaring > dbcc_name as a char[1]. So you shouldn't actually add another char16 here. > Removed the extra char16 size. > http://codereview.chromium.org/11088012/diff/15004/chrome/browser/system_moni... > chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:410: > scoped_ptr_malloc<DEV_BROADCAST_DEVICEINTERFACE> dev_interface_broadcast( > On 2012/10/23 23:44:17, kmadhusu wrote: > > On 2012/10/19 21:31:12, Peter Kasting wrote: > > > Ugh, why are you using scoped_ptr_malloc<> and ZeroMemory() and memcpy() and > > the > > > like? Why not just use a standard scoped_ptr<>? > > > > By default, DEV_BROADCAST_DEVICEINTERFACE dbcc_name field is a single element > > array of type TCHAR. For unit tests, I want to overwrite that data field with > a > > pnp_device_id (whose length is > 1). So, I allocated sufficient memory using > > scoped_ptr_malloc<>, initialized using ZeroMemory() and copied the data using > > memcpy(). Since I am allocating more memory than the basic structure size, I > > thought it is better to use scoped_ptr_malloc<> rather than doing > > scoped_ptr<>(new char16[new_size]); > > Yeah, you're fine. I didn't really realize this struct is basically > variable-size.
On 2012/10/24 22:53:01, kmadhusu wrote: > > > I would like to leave it as it is. I have added the format of |unique_id| to > > > understand this code. > > > > Are you sure %ls is correct on non-Windows, then? I thought %ls was > "wchar_t*", > > which a string16 isn't (on non-Windows). > > > > This code is windows specific. So, "%ls" seems correct here. I won't block you from checking it in, but I REALLY REALLY REALLY think this would be less confusing and just flat-out better overall to use simple string concatenation as I suggested originally. You avoid lowering strings to C-style strings only to write them back into a string, you avoid the reader having to figure out what %ls is and then worry about sizing correctness like I just did, the code is shorter, it's safe if someone changes a variable type later... I'm not precisely sure what benefit you see in the StringPrintf() route. Elsewhere in Chrome I've discouraged people from using it when string concatenation would suffice.
On 2012/10/24 23:00:59, Peter Kasting wrote: > On 2012/10/24 22:53:01, kmadhusu wrote: > > > > I would like to leave it as it is. I have added the format of |unique_id| > to > > > > understand this code. > > > > > > Are you sure %ls is correct on non-Windows, then? I thought %ls was > > "wchar_t*", > > > which a string16 isn't (on non-Windows). > > > > > > > This code is windows specific. So, "%ls" seems correct here. > > I won't block you from checking it in, but I REALLY REALLY REALLY think this > would be less confusing and just flat-out better overall to use simple string > concatenation as I suggested originally. You avoid lowering strings to C-style > strings only to write them back into a string, you avoid the reader having to > figure out what %ls is and then worry about sizing correctness like I just did, > the code is shorter, it's safe if someone changes a variable type later... > > I'm not precisely sure what benefit you see in the StringPrintf() route. > Elsewhere in Chrome I've discouraged people from using it when string > concatenation would suffice. You are correct. Stringprintf() is confusing. Other reviewers insisted me to use StringPrintf() in chrome code. That is the only reason I had it here. Now that I understand the consequences, I flattened the code by using basic string concatenation operator.
On 2012/10/25 01:44:26, kmadhusu wrote: > Other reviewers insisted me to use > StringPrintf() in chrome code. That seems very strange. In the future if you hear this I'd be curious what the rationale for this is. I generally find all printf-like functions to be a readability and safety loss, though sometimes they're the best tool for the job.
Was I supposed to review anything other than portable_device_watcher_win.*? Based on the number of comments I'm thinking maybe I should... http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:59: return StringToLowerASCII(string16(dev_interface->dbcc_name)); Nit: Explicit conversion to string16 unnecessary, I think http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:66: string16* name) { Nit: Add DCHECK(device_manager); to make it clear this function expects that. (several functions) http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:79: if (name) Nit: Is this ever NULL? If not, DCHECK it atop the function, and use it directly in place of |friendly_name|. (similar comments apply in several functions) http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:143: Nit: I'd remove all these blank lines http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:175: if (FAILED(hr)) { Nit: If you early-return for SUCCEEDED(hr) instead, you can omit the brackets. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:186: return (object_id == WPD_DEVICE_OBJECT_ID); Nit: Seems like this should just be inlined into the lone caller. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:222: *value = string16(buffer); Nit: Explicit conversion to string16 may not be necessary (although you may have to add a .get() call or something for that to compile) http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:324: ((device_name[0] >= L'A') && (device_name[0] <= L'Z') || Nit: Indent 4, not even http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:329: // Accessed on the blocking thread. Nit: Comment as to why this (and the other functions that run via the task runner) runs via the task runner, i.e. which call(s) may be slow and why. You may also want to say "accessed via the task runner" in these function comments instead of "blocking thread" since "blocking thread" is slightly unclear. (several functions) http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:334: GetBlockingPool()->RunsTasksOnCurrentThread()); Nit: Breaking after -> seems slightly better than :: (several functions) http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:336: if (!(GetFriendlyName(pnp_device_id, portable_device_manager, &name) || Nit: Can this just be: GetFriendlyName(...) || GetDeviceDescription(...) || GetManufacturerName(...); return name; Because if none of these set |name| on failure, we don't need to explicitly return anything different... http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:369: for (size_t index = 0; index < storage_obj_ids.size(); ++index) { Nit: Consider using a const_iterator instead http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:388: // Gets the device details (name, number of storages, etc.,) on the blocking Nit: storages http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:410: device_details.location = pnp_device_id; Nit: Seems like we could set these directly (instead of using temps) if the intermediate return points returned a DeviceDetails(). Although maybe this should return a bool or something instead, see below. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:417: EnumerateAttachedStoragesOnBlockingThread() { Nit: Indent 4. Also: storages http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:439: ZeroMemory(pnp_device_ids.get(), pnp_device_count); Doesn't operator new zero this memory for you? http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:445: device_info_list.push_back(GetDeviceInfoOnBlockingThread( If GetDeviceInfoOnBlockingThread fails, it returns an empty device details, which we happily shove on this list. Maybe we should be checking the return code from that function. If it took a DeviceDetails* and returned a bool, it could freely write into the supplied DeviceDetails (avoiding temps, as I suggested in that function) and return false on failure, at which point we could discard the details here. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:454: HandleDeviceAttachedEventOnBlockingThread(const string16& pnp_device_id) { Nit: Indent 4 http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:458: base::win::ScopedComPtr<IPortableDeviceManager> portable_device_mgr; Nit: This whole block that gets the portable device manager is duplicated from above, maybe should be its own function. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:468: portable_device_mgr->RefreshDeviceList(); Do we need to do this call in the function above too? http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:475: // for registering device media file system. Nit: device -> the device http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:483: PortableDeviceWatcherWin::PortableDeviceWatcherWin() Nit: Please add divider comments in this file to separate the file-scoped functions from the PortableDeviceWatcherWin functions. See omnibox code for sample divider use. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:496: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); Nit: This DCHECK goes atop this function. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:502: DCHECK(media_task_runner_.get()); Nit: Do we need this DCHECK? Is there some reason a reader might think the call could have failed? http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:512: NOTIMPLEMENTED(); Nit: Is this TODO? http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:521: switch (event_type) { Nit: Simpler: if (IsPortableDeviceStructure(data)) { string16 device_id(GetPnpDeviceId(data)); if (event_type == DBT_DEVICEARRIVAL) HandleDeviceAttachEvent(device_id) else if (event_type == DBT_DEVICEMOVECOMPLETE) HandleDeviceDetachEvent(device_id); } You can replace the final if with a DCHECK if these are the only two cases in which this function is called. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:575: for (size_t index = 0; index < storage_info_list.size(); ++index) { Nit: For looping over any typedefed type, use a [const_]iterator, which will still work if you change the typedef to a non-random-access type. (several functions) http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:593: void PortableDeviceWatcherWin::OnHandleDeviceAttachEvent( Nit: Maybe this should be named OnDidHandleDeviceAttachEvent() http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:615: // Device can have several data storage_info_list. Therefore, add the Nit: This first sentence seems to be missing some words http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:621: base::SystemMonitor::RemovableStorageInfo storage_info(storage_id, Nit: Or just inline this into the next statement http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.h (right): http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:28: namespace chrome { Should this stuff really be in a namespace? I thought we didn't use a chrome namespace for things in chrome/ in general. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:39: // E.g.: s10001 Nit: Slightly clearer if you write this example inline, like "Temporary identifier, e.g. "s10001", that uniquely identifies..." or maybe place the example at the end of the sentence like "...on the device, e.g. "s10001"." (2 places) http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:44: std::string unique_id; Nit: It's confusing to have the first string commented as "uniquely identifies" and then call this string "unique_id". Consider naming these "temporary_id" and "permanent_id" or some other clearer naming scheme. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:51: std::vector<DeviceStorageInfo> storage_info_list; Tiny nit: Does it make sense to put this after the name and location instead of before? http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:63: // Must be called after the browser blocking pool is ready for use. Nit: Document who is supposed to call this. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:68: // |removable|. Nit: Say whether these are all left untouched on failure. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:88: typedef std::vector<DeviceStorageInfo> StorageInfoList; Nit: This should be public, and you should use it in other files that want a vector<DeviceStorageInfo> (e.g. the unittest), as well as in DeviceDetails. Also, it shouldn't use the word "List" if it's not a list; if for example you named the struct "DeviceStorageObject" you could call this "StorageObjects". Consider also having a vector<DeviceDetails> typedef, either public or private depending on who would use it. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:97: std::vector<DeviceDetails> device_details_list); Nit: const ref http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:101: virtual void OnHandleDeviceAttachEvent(DeviceDetails device_details); Nit: const ref http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:111: // Stores attached mtp device storage details. Nit: Don't say "stores" on data members. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:112: MtpStorageMap storage_map_; Tiny nit: Should this go after the device_map_? http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:118: // this thread. Nit: How about: // The task runner used to execute tasks that may take a long time and thus should not be performed on the UI thread. Then note in the class-level comments that the cleass is created, destroyed, and operates on the UI thread, except for long-running tasks it spins off to a task runner. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:121: // Used to notify PortableDeviceWatcherWin about the shutdown sequence. Used by whom? And "the shutdown sequence" is vague. Example of better comment: "Set by FooClass when it receives an XYZ_EVENT notification. PortableDeviceWatcherWin is expected to monitor this flag's state in BarFunc() and respond by deleting itself." But looking at the .cc file, I'm confused why you use this at all. A CancellationFlag is for communicating safely between threads. It looks to me like this thread is set and tested all on the UI thread. You can just use a normal bool in that case. But better would be to not use a bool at all and instead get whoever creates/owns this object to delete it on or before "app termination". Since all your blocking pool tasks are issued with weak pointers, this should be safe to do at any time. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:125: // Used to listen for application termination message. Nit: First sentence is unnecessary. Second sentence should refer to an actual notification (or more than one) and note precisely what will happen when it's received, if this information (including notification name) has not been previously listed in another comment. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:129: // Used for creating callbacks. Nit: How about just: // Used by |media_task_runner_| to create cancelable callbacks. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:267: // |unique_id| format: StorageSerial:|storage_id|:|device_serial_num| Nit: Remove this comment, it just duplicates the code. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:269: UTF8ToUTF16(chrome::kNonSpaceDelim) + device_serial_num); First, use ASCIIToUTF16() when you know the input is ASCII. But even more, don't declare things like ':' and '(' and such as named constants. This makes the code less readable, not more. Just inline "L':'" here, and do similarly everywhere you have kLeftParen and the like. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:616: UTF8ToUTF16(kRightParen)); See comments above regarding nuking these constants
Minor stuff... http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:160: bool Setup(const string16& pnp_device_id, nit: SetUp http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:184: bool IsDeviceObjectId(const string16& object_id) { Used once (in a simple function), inline. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:261: bool GetDeviceStorageUniqueId(const string16& device_serial_num, nit: MakeDeviceStorageDeviceId or ConstructDeviceStorageDeviceId - this method isn't getting anything, it's just putting the pieces together. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:354: std::vector<string16> storage_obj_ids; Unless there's a reason to do this before getting the serial number, move this to just above the for loop on line 365. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:368: if (!GetObjectUniqueId(device.get(), storage_obj_ids[index], If you like, you could do: if (!GetObjectUniqueId(...) || !GetDeviceStorageUniqueId(...) { continue; } http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:401: if (IsVolumeMountedPortableDevice(device_name)) Move above GetDeviceStorageInfoListOnBlockingThread http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:462: return device_details; return PortabelDeviceWatcherWin::DeviceDetails() and remove line 453 http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:473: std::string root_path("\\\\"); nit: const http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:502: bool PortableDeviceWatcherWin::GetDeviceInfo(const FilePath& device_path, Remove this method since it's not needed now and isn't part of any interface. Add a TODO in emovable_device_notifications_window_win.cc saying that we should add this when it's needed because of X or Y... http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:618: base::SystemMonitor::RemovableStorageInfo storage_info(storage_id, nit: combine line 618 and 621 http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.h (right): http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:100: virtual void HandleDeviceAttachEvent(const string16& pnp_device_id); The difference between these methods isn't clear from the name, but it should be. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win.cc (right): http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:36: // Register window handle to receive portable device notifications details. These helper routines make the class more complicated than it needs to be, just inline them into the constructor and destructor. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win.h (right): http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.h:77: class PortableDeviceNotifications; Should go closer to private: http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc (right): http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:145: std::string root_path("\\\\"); nit: const
pkasting@, vandebo@: Addressed review comments. PTAL. Thanks. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:59: return StringToLowerASCII(string16(dev_interface->dbcc_name)); On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Explicit conversion to string16 unnecessary, I think If I don't have string16, compiler complaints that it cannot convert parameter 1 from void to const std::basic_string. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:66: string16* name) { On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Add DCHECK(device_manager); to make it clear this function expects that. > (several functions) Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:79: if (name) On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Is this ever NULL? If not, DCHECK it atop the function, and use it > directly in place of |friendly_name|. (similar comments apply in several > functions) As of now, this function is always called with a valid output param. I thought, I can use this function just to query if a name is available or not. But I guess, I don't need that function format now. As you suggested, added DCHECK and used the name param directly. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:143: On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: I'd remove all these blank lines Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:175: if (FAILED(hr)) { On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: If you early-return for SUCCEEDED(hr) instead, you can omit the brackets. Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:186: return (object_id == WPD_DEVICE_OBJECT_ID); On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Seems like this should just be inlined into the lone caller. Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:222: *value = string16(buffer); On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Explicit conversion to string16 may not be necessary (although you may have > to add a .get() call or something for that to compile) Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:324: ((device_name[0] >= L'A') && (device_name[0] <= L'Z') || On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Indent 4, not even Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:329: // Accessed on the blocking thread. On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Comment as to why this (and the other functions that run via the task > runner) runs via the task runner, i.e. which call(s) may be slow and why. > > You may also want to say "accessed via the task runner" in these function > comments instead of "blocking thread" since "blocking thread" is slightly > unclear. > > (several functions) Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:334: GetBlockingPool()->RunsTasksOnCurrentThread()); On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Breaking after -> seems slightly better than :: (several functions) Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:336: if (!(GetFriendlyName(pnp_device_id, portable_device_manager, &name) || On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Can this just be: > > GetFriendlyName(...) || GetDeviceDescription(...) || GetManufacturerName(...); > return name; > > Because if none of these set |name| on failure, we don't need to explicitly > return anything different... Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:369: for (size_t index = 0; index < storage_obj_ids.size(); ++index) { On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Consider using a const_iterator instead Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:388: // Gets the device details (name, number of storages, etc.,) on the blocking On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: storages Fixed. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:410: device_details.location = pnp_device_id; On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Seems like we could set these directly (instead of using temps) if the > intermediate return points returned a DeviceDetails(). > > Although maybe this should return a bool or something instead, see below. Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:417: EnumerateAttachedStoragesOnBlockingThread() { On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Indent 4. Also: storages Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:439: ZeroMemory(pnp_device_ids.get(), pnp_device_count); On 2012/10/25 05:23:24, Peter Kasting wrote: > Doesn't operator new zero this memory for you? No. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:445: device_info_list.push_back(GetDeviceInfoOnBlockingThread( On 2012/10/25 05:23:24, Peter Kasting wrote: > If GetDeviceInfoOnBlockingThread fails, it returns an empty device details, > which we happily shove on this list. Maybe we should be checking the return > code from that function. If it took a DeviceDetails* and returned a bool, it > could freely write into the supplied DeviceDetails (avoiding temps, as I > suggested in that function) and return false on failure, at which point we could > discard the details here. Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:454: HandleDeviceAttachedEventOnBlockingThread(const string16& pnp_device_id) { On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Indent 4 Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:458: base::win::ScopedComPtr<IPortableDeviceManager> portable_device_mgr; On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: This whole block that gets the portable device manager is duplicated from > above, maybe should be its own function. Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:468: portable_device_mgr->RefreshDeviceList(); On 2012/10/25 05:23:24, Peter Kasting wrote: > Do we need to do this call in the function above too? No. It is not required there. When a new device is attached, sometimes portable device manager is not aware of the new device. Just to be on the safer side, I added this call here. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:475: // for registering device media file system. On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: device -> the device Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:483: PortableDeviceWatcherWin::PortableDeviceWatcherWin() On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Please add divider comments in this file to separate the file-scoped > functions from the PortableDeviceWatcherWin functions. See omnibox code for > sample divider use. Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:496: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: This DCHECK goes atop this function. Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:502: DCHECK(media_task_runner_.get()); On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Do we need this DCHECK? Is there some reason a reader might think the call > could have failed? Removed. We can skip this check. If it failed for some reason, then we are in much trouble. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:512: NOTIMPLEMENTED(); On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Is this TODO? As of now, there is no UI to trigger this function. I just added it as a place holder for future use. vandebo@ said, I can add this function when required. Therefore, removed the function declaration and definition. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:521: switch (event_type) { On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Simpler: > > if (IsPortableDeviceStructure(data)) { > string16 device_id(GetPnpDeviceId(data)); > if (event_type == DBT_DEVICEARRIVAL) > HandleDeviceAttachEvent(device_id) > else if (event_type == DBT_DEVICEMOVECOMPLETE) > HandleDeviceDetachEvent(device_id); > } > > You can replace the final if with a DCHECK if these are the only two cases in > which this function is called. Added the following code: if (!IsPortableDeviceStructure(data)) return; string16 device_id = GetPnpDeviceId(data); if (event_type == DBT_DEVICEARRIVAL) HandleDeviceAttachEvent(device_id); else if (event_type == DBT_DEVICEREMOVECOMPLETE) HandleDeviceDetachEvent(device_id); We can receive other device changed events. So, I didn't add any DCHECK at the end. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:575: for (size_t index = 0; index < storage_info_list.size(); ++index) { On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: For looping over any typedefed type, use a [const_]iterator, which will > still work if you change the typedef to a non-random-access type. (several > functions) Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:593: void PortableDeviceWatcherWin::OnHandleDeviceAttachEvent( On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Maybe this should be named OnDidHandleDeviceAttachEvent() Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:615: // Device can have several data storage_info_list. Therefore, add the On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: This first sentence seems to be missing some words Fixed. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:621: base::SystemMonitor::RemovableStorageInfo storage_info(storage_id, On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Or just inline this into the next statement Done http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.h (right): http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:28: namespace chrome { On 2012/10/25 05:23:24, Peter Kasting wrote: > Should this stuff really be in a namespace? I thought we didn't use a chrome > namespace for things in chrome/ in general. I thought the other way around. Since this file is in chrome/, I added them in chrome namespace. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:39: // E.g.: s10001 On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Slightly clearer if you write this example inline, like "Temporary > identifier, e.g. "s10001", that uniquely identifies..." or maybe place the > example at the end of the sentence like "...on the device, e.g. "s10001"." (2 > places) Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:44: std::string unique_id; On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: It's confusing to have the first string commented as "uniquely identifies" > and then call this string "unique_id". > > Consider naming these "temporary_id" and "permanent_id" or some other clearer > naming scheme. Rephrased the comments. Renamed storage_object_id => object_temporary_id Renamed unique_id => object_persistent_id http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:51: std::vector<DeviceStorageInfo> storage_info_list; On 2012/10/25 05:23:24, Peter Kasting wrote: > Tiny nit: Does it make sense to put this after the name and location instead of > before? Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:63: // Must be called after the browser blocking pool is ready for use. On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Document who is supposed to call this. Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:68: // |removable|. On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Say whether these are all left untouched on failure. Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:88: typedef std::vector<DeviceStorageInfo> StorageInfoList; On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: This should be public, and you should use it in other files that want a > vector<DeviceStorageInfo> (e.g. the unittest), as well as in DeviceDetails. > Also, it shouldn't use the word "List" if it's not a list; if for example you > named the struct "DeviceStorageObject" you could call this "StorageObjects". > > Consider also having a vector<DeviceDetails> typedef, either public or private > depending on who would use it. Renamed DeviceStorageInfo => DeviceStorageObject Renamed StorageInfoList => StorageObjects Moved typedef to public section. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:97: std::vector<DeviceDetails> device_details_list); On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: const ref Changed to const ptr. I couldn't pass const ref to std::vector using PostTaskAndReplyWithResult. It is complaining about the the return type ambiguity.. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:101: virtual void OnHandleDeviceAttachEvent(DeviceDetails device_details); On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: const ref Changed to const ptr. I couldn't pass const ref to std::vector using PostTaskAndReplyWithResult. It is complaining about the the return type ambiguity. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:111: // Stores attached mtp device storage details. On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Don't say "stores" on data members. Fixed. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:112: MtpStorageMap storage_map_; On 2012/10/25 05:23:24, Peter Kasting wrote: > Tiny nit: Should this go after the device_map_? Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:118: // this thread. On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: How about: > > // The task runner used to execute tasks that may take a long time and thus > should not be performed on the UI thread. > > Then note in the class-level comments that the cleass is created, destroyed, and > operates on the UI thread, except for long-running tasks it spins off to a task > runner. Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:121: // Used to notify PortableDeviceWatcherWin about the shutdown sequence. On 2012/10/25 05:23:24, Peter Kasting wrote: > Used by whom? And "the shutdown sequence" is vague. > > Example of better comment: "Set by FooClass when it receives an XYZ_EVENT > notification. PortableDeviceWatcherWin is expected to monitor this flag's state > in BarFunc() and respond by deleting itself." > > But looking at the .cc file, I'm confused why you use this at all. A > CancellationFlag is for communicating safely between threads. It looks to me > like this thread is set and tested all on the UI thread. You can just use a > normal bool in that case. But better would be to not use a bool at all and > instead get whoever creates/owns this object to delete it on or before "app > termination". Since all your blocking pool tasks are issued with weak pointers, > this should be safe to do at any time. Rephrased the comment. Removed cancellationFlag and used bool variable. By using a bool variable, I can stop posting tasks on the blocking thread after the application termination message is received. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:125: // Used to listen for application termination message. On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: First sentence is unnecessary. Second sentence should refer to an actual > notification (or more than one) and note precisely what will happen when it's > received, if this information (including notification name) has not been > previously listed in another comment. Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:129: // Used for creating callbacks. On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: How about just: > > // Used by |media_task_runner_| to create cancelable callbacks. Done. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:160: bool Setup(const string16& pnp_device_id, On 2012/10/25 19:24:47, vandebo wrote: > nit: SetUp Done. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:184: bool IsDeviceObjectId(const string16& object_id) { On 2012/10/25 19:24:47, vandebo wrote: > Used once (in a simple function), inline. Done. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:261: bool GetDeviceStorageUniqueId(const string16& device_serial_num, On 2012/10/25 19:24:47, vandebo wrote: > nit: MakeDeviceStorageDeviceId or ConstructDeviceStorageDeviceId - this method > isn't getting anything, it's just putting the pieces together. Renamed GetDeviceStorageUniqueId => ConstructDeviceStorageUniqueId. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:267: // |unique_id| format: StorageSerial:|storage_id|:|device_serial_num| On 2012/10/25 05:23:24, Peter Kasting wrote: > Nit: Remove this comment, it just duplicates the code. Done. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:269: UTF8ToUTF16(chrome::kNonSpaceDelim) + device_serial_num); On 2012/10/25 05:23:24, Peter Kasting wrote: > First, use ASCIIToUTF16() when you know the input is ASCII. > > But even more, don't declare things like ':' and '(' and such as named > constants. This makes the code less readable, not more. Just inline "L':'" > here, and do similarly everywhere you have kLeftParen and the like. Removed UTF..() function and used L':' directly. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:354: std::vector<string16> storage_obj_ids; On 2012/10/25 19:24:47, vandebo wrote: > Unless there's a reason to do this before getting the serial number, move this > to just above the for loop on line 365. Done. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:368: if (!GetObjectUniqueId(device.get(), storage_obj_ids[index], On 2012/10/25 19:24:47, vandebo wrote: > If you like, you could do: > > if (!GetObjectUniqueId(...) || !GetDeviceStorageUniqueId(...) { > continue; > } I would like to leave it as it is. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:401: if (IsVolumeMountedPortableDevice(device_name)) On 2012/10/25 19:24:47, vandebo wrote: > Move above GetDeviceStorageInfoListOnBlockingThread Done. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:462: return device_details; On 2012/10/25 19:24:47, vandebo wrote: > return PortabelDeviceWatcherWin::DeviceDetails() and remove line 453 Modified the function to return void. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:473: std::string root_path("\\\\"); On 2012/10/25 19:24:47, vandebo wrote: > nit: const Done. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:502: bool PortableDeviceWatcherWin::GetDeviceInfo(const FilePath& device_path, On 2012/10/25 19:24:47, vandebo wrote: > Remove this method since it's not needed now and isn't part of any interface. > Add a TODO in emovable_device_notifications_window_win.cc saying that we should > add this when it's needed because of X or Y... Done. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:616: UTF8ToUTF16(kRightParen)); On 2012/10/25 05:23:24, Peter Kasting wrote: > See comments above regarding nuking these constants Done. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:618: base::SystemMonitor::RemovableStorageInfo storage_info(storage_id, On 2012/10/25 19:24:47, vandebo wrote: > nit: combine line 618 and 621 Done. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.h (right): http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:100: virtual void HandleDeviceAttachEvent(const string16& pnp_device_id); On 2012/10/25 19:24:47, vandebo wrote: > The difference between these methods isn't clear from the name, but it should > be. Renamed OnHandleDeviceAttachEvent => OnDidHandleDeviceAttachEvent. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win.cc (right): http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:36: // Register window handle to receive portable device notifications details. On 2012/10/25 19:24:47, vandebo wrote: > These helper routines make the class more complicated than it needs to be, just > inline them into the constructor and destructor. Done. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win.h (right): http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.h:77: class PortableDeviceNotifications; On 2012/10/25 19:24:47, vandebo wrote: > Should go closer to private: Done. http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc (right): http://codereview.chromium.org/11088012/diff/47010/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:145: std::string root_path("\\\\"); On 2012/10/25 19:24:47, vandebo wrote: > nit: const Done.
http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:59: return StringToLowerASCII(string16(dev_interface->dbcc_name)); On 2012/10/26 02:01:24, kmadhusu wrote: > On 2012/10/25 05:23:24, Peter Kasting wrote: > > Nit: Explicit conversion to string16 unnecessary, I think > > If I don't have string16, compiler complaints that it cannot convert parameter 1 > from void to const std::basic_string. From void? That doesn't make sense, you can't convert void to a string anyway even with an explicit call. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:439: ZeroMemory(pnp_device_ids.get(), pnp_device_count); On 2012/10/26 02:01:24, kmadhusu wrote: > On 2012/10/25 05:23:24, Peter Kasting wrote: > > Doesn't operator new zero this memory for you? > > No. Oh hey, while I'm here, the style guide says to use the C++ types (in this case wchar_t*) in place of Windows typedefed types like LPWSTR. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:468: portable_device_mgr->RefreshDeviceList(); On 2012/10/26 02:01:24, kmadhusu wrote: > On 2012/10/25 05:23:24, Peter Kasting wrote: > > Do we need to do this call in the function above too? > > No. It is not required there. When a new device is attached, sometimes portable > device manager is not aware of the new device. Just to be on the safer side, I > added this call here. Please comment about why this is here, then, especially if you can give specific circumstances which cause this lack of awareness to happen. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.h (right): http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:28: namespace chrome { On 2012/10/26 02:01:24, kmadhusu wrote: > On 2012/10/25 05:23:24, Peter Kasting wrote: > > Should this stuff really be in a namespace? I thought we didn't use a chrome > > namespace for things in chrome/ in general. > > I thought the other way around. Since this file is in chrome/, I added them in > chrome namespace. Almost no code in chrome uses a chrome namespace. Please check with brettw on the rules for using the chrome namespace. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:97: std::vector<DeviceDetails> device_details_list); On 2012/10/26 02:01:24, kmadhusu wrote: > On 2012/10/25 05:23:24, Peter Kasting wrote: > > Nit: const ref > > Changed to const ptr. I couldn't pass const ref to std::vector using > PostTaskAndReplyWithResult. It is complaining about the the return type > ambiguity.. Check with willchan, akalin, or some other knowledgeable person about this. I haven't used PostTaskAndReplyWithResult() enough offhand to know what the solution is here but it seems like we ought to be able to make this work. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:121: // Used to notify PortableDeviceWatcherWin about the shutdown sequence. On 2012/10/26 02:01:24, kmadhusu wrote: > By using > a bool variable, I can stop posting tasks on the blocking thread after the > application termination message is received. But why does this class exist at all after app termination? Shouldn't the correct way to stop posting tasks -- as well as immediately cancel all outstanding ones -- be to delete this object entirely? I thought I'd asked this in another comment but I didn't see an answer.
I think we're trying to use the chrome namespace more, so I'd use it if the code around you is.
http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.h (right): http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:121: // Used to notify PortableDeviceWatcherWin about the shutdown sequence. On 2012/10/26 02:14:49, Peter Kasting wrote: > On 2012/10/26 02:01:24, kmadhusu wrote: > > By using > > a bool variable, I can stop posting tasks on the blocking thread after the > > application termination message is received. > > But why does this class exist at all after app termination? > > Shouldn't the correct way to stop posting tasks -- as well as immediately cancel > all outstanding ones -- be to delete this object entirely? I thought I'd asked > this in another comment but I didn't see an answer. I'm with Peter here. On shutdown, the owner of this class will be destroyed and delete this class. At that point, no new tasks will be posted and any outstanding tasks will safely exit because of the WeakPointer. http://codereview.chromium.org/11088012/diff/53002/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/53002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:248: string16 unique_id(chrome::kMtpDeviceStorageIdPrefix + storage_id + L':' + If there's only one format of device ids for this type, then there's no need for a prefix. http://codereview.chromium.org/11088012/diff/53002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:518: base::Unretained(device_details_vector)), nit: Unretained not needed http://codereview.chromium.org/11088012/diff/53002/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_constants.h (left): http://codereview.chromium.org/11088012/diff/53002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_constants.h:18: extern const char kLeftParen[]; Can you do a cleanup cl to just remove these?
pkasting@, vandebo@: Addressed review comments. PTAL. Thanks. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:59: return StringToLowerASCII(string16(dev_interface->dbcc_name)); On 2012/10/26 02:14:49, Peter Kasting wrote: > On 2012/10/26 02:01:24, kmadhusu wrote: > > On 2012/10/25 05:23:24, Peter Kasting wrote: > > > Nit: Explicit conversion to string16 unnecessary, I think > > > > If I don't have string16, compiler complaints that it cannot convert parameter > 1 > > from void to const std::basic_string. > > From void? That doesn't make sense, you can't convert void to a string anyway > even with an explicit call. StringToLowerASCII template will not work with w_char* objects. It can only work with string objects. So, I have to construct a string16 object to use StringToLowerASCII. IsStringASCII function was implicitly converting the device_id to std::wstring. Therefore, I constructed a local variable and used it in both function calls. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:439: ZeroMemory(pnp_device_ids.get(), pnp_device_count); On 2012/10/26 02:14:49, Peter Kasting wrote: > On 2012/10/26 02:01:24, kmadhusu wrote: > > On 2012/10/25 05:23:24, Peter Kasting wrote: > > > Doesn't operator new zero this memory for you? > > > > No. > > Oh hey, while I'm here, the style guide says to use the C++ types (in this case > wchar_t*) in place of Windows typedefed types like LPWSTR. Done. Used char16* instead of LPWSTR. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:468: portable_device_mgr->RefreshDeviceList(); On 2012/10/26 02:14:49, Peter Kasting wrote: > On 2012/10/26 02:01:24, kmadhusu wrote: > > On 2012/10/25 05:23:24, Peter Kasting wrote: > > > Do we need to do this call in the function above too? > > > > No. It is not required there. When a new device is attached, sometimes > portable > > device manager is not aware of the new device. Just to be on the safer side, I > > added this call here. > > Please comment about why this is here, then, especially if you can give specific > circumstances which cause this lack of awareness to happen. Done. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.h (right): http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:28: namespace chrome { On 2012/10/26 02:14:49, Peter Kasting wrote: > On 2012/10/26 02:01:24, kmadhusu wrote: > > On 2012/10/25 05:23:24, Peter Kasting wrote: > > > Should this stuff really be in a namespace? I thought we didn't use a > chrome > > > namespace for things in chrome/ in general. > > > > I thought the other way around. Since this file is in chrome/, I added them in > > chrome namespace. > > Almost no code in chrome uses a chrome namespace. Please check with brettw on > the rules for using the chrome namespace. As per brettw@ comment, I am leaving this as it is. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:97: std::vector<DeviceDetails> device_details_list); On 2012/10/26 02:14:49, Peter Kasting wrote: > On 2012/10/26 02:01:24, kmadhusu wrote: > > On 2012/10/25 05:23:24, Peter Kasting wrote: > > > Nit: const ref > > > > Changed to const ptr. I couldn't pass const ref to std::vector using > > PostTaskAndReplyWithResult. It is complaining about the the return type > > ambiguity.. > > Check with willchan, akalin, or some other knowledgeable person about this. I > haven't used PostTaskAndReplyWithResult() enough offhand to know what the > solution is here but it seems like we ought to be able to make this work. Contacted willchan@ and akalin@. Looks like it is a bug in our implementation. I guess they are working on a fix. In the mean time, I will use PostTaskAndReply function. http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:121: // Used to notify PortableDeviceWatcherWin about the shutdown sequence. On 2012/10/26 20:32:10, vandebo wrote: > On 2012/10/26 02:14:49, Peter Kasting wrote: > > On 2012/10/26 02:01:24, kmadhusu wrote: > > > By using > > > a bool variable, I can stop posting tasks on the blocking thread after the > > > application termination message is received. > > > > But why does this class exist at all after app termination? > > > > Shouldn't the correct way to stop posting tasks -- as well as immediately > cancel > > all outstanding ones -- be to delete this object entirely? I thought I'd > asked > > this in another comment but I didn't see an answer. > > I'm with Peter here. On shutdown, the owner of this class will be destroyed and > delete this class. At that point, no new tasks will be posted and any > outstanding tasks will safely exit because of the WeakPointer. Done. This class no longer listens for application termination message. When the application terminates, RemovableDeviceNotificationsWindowWin will destruct this object. http://codereview.chromium.org/11088012/diff/53002/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/53002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:248: string16 unique_id(chrome::kMtpDeviceStorageIdPrefix + storage_id + L':' + On 2012/10/26 20:32:10, vandebo wrote: > If there's only one format of device ids for this type, then there's no need for > a prefix. Done. http://codereview.chromium.org/11088012/diff/53002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:518: base::Unretained(device_details_vector)), On 2012/10/26 20:32:10, vandebo wrote: > nit: Unretained not needed Removed. http://codereview.chromium.org/11088012/diff/53002/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_constants.h (left): http://codereview.chromium.org/11088012/diff/53002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_constants.h:18: extern const char kLeftParen[]; On 2012/10/26 20:32:10, vandebo wrote: > Can you do a cleanup cl to just remove these? sure.
http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.h (right): http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:28: namespace chrome { On 2012/10/26 22:23:26, kmadhusu wrote: > On 2012/10/26 02:14:49, Peter Kasting wrote: > > On 2012/10/26 02:01:24, kmadhusu wrote: > > > On 2012/10/25 05:23:24, Peter Kasting wrote: > > > > Should this stuff really be in a namespace? I thought we didn't use a > > chrome > > > > namespace for things in chrome/ in general. > > > > > > I thought the other way around. Since this file is in chrome/, I added them > in > > > chrome namespace. > > > > Almost no code in chrome uses a chrome namespace. Please check with brettw on > > the rules for using the chrome namespace. > > As per brettw@ comment, I am leaving this as it is. I assume you're saying all the other code in system_monitor/ uses this namespace? If not, don't do it here either.
On 2012/10/26 22:35:34, Peter Kasting wrote: > http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... > File chrome/browser/system_monitor/portable_device_watcher_win.h (right): > > http://codereview.chromium.org/11088012/diff/42001/chrome/browser/system_moni... > chrome/browser/system_monitor/portable_device_watcher_win.h:28: namespace chrome > { > On 2012/10/26 22:23:26, kmadhusu wrote: > > On 2012/10/26 02:14:49, Peter Kasting wrote: > > > On 2012/10/26 02:01:24, kmadhusu wrote: > > > > On 2012/10/25 05:23:24, Peter Kasting wrote: > > > > > Should this stuff really be in a namespace? I thought we didn't use a > > > chrome > > > > > namespace for things in chrome/ in general. > > > > > > > > I thought the other way around. Since this file is in chrome/, I added > them > > in > > > > chrome namespace. > > > > > > Almost no code in chrome uses a chrome namespace. Please check with brettw > on > > > the rules for using the chrome namespace. > > > > As per brettw@ comment, I am leaving this as it is. > > I assume you're saying all the other code in system_monitor/ uses this > namespace? If not, don't do it here either. Yes.
This is getting pretty close. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:75: return SUCCEEDED(hr) ? !name->empty() : false; Nit: Simpler: return SUCCEEDED(hr) && !name->empty(); (several functions) http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:198: *value = static_cast<const wchar_t*>(buffer); Nit: wchar_t -> char16, since the original type and the receiving object both use char16 http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:299: ((device_name[0] >= L'A') && (device_name[0] <= L'Z') || Nit: Should be indented 4. Also, add parens: right now you have "a && b || (c && d)", presumably you want "(a && b) || (c && d)" http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:320: // runner. On success, returns true and populates |storage_objects| with device Nit: You added comments to a number of functions saying "this task may take a long time to complete, so access via task runner" or similar. That's not what I meant to happen -- it's already obvious that's the case since you're directly DCHECKing that you're being run in the blocking pool. Instead, I was looking for comments on the particular Windows API calls that take a long time. For example: // This function calls XYZApi(), which checks for attached file systems. If any of these are on network drives, Windows may take an arbitrarily long time to respond. Additionally, the call to ABCApi(), while not an arbitrary long call, can take up to 250 ms to return. We can't block the UI thread in either of these cases, so we segregate this functionality and run it via the task runner. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:341: for (std::vector<string16>::const_iterator id_iter = storage_obj_ids.begin(); Nit: You are inconsistent across this file about how you name your iterators: xyz_iter, iter, or it. Be consistent. The rest of Chrome code tends to use "i" or "it". I personally prefer the former. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:350: PortableDeviceWatcherWin::DeviceStorageObject storage_object; Nit: Use aggregate initialization form or add a constructor for this struct. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:359: // Access the device and gets the device details (name, storage info, etc.,). Nit: "Access" -> "Accesses", "etc.," -> "etc." http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:395: // Either there is no portable device support (it must be a XP with Windows Nit: "it must be a XP with Windows Media Player Version < 10" -> "i.e. Windows XP with old versions of Media Player" http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:461: const std::string root_path("\\\\"); Nit: Frankly I think this should just be inlined into the next statement. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:470: /////////////////////////////////////////////////////////////////////////////// Nit: More common is the following: ... } // namespace // PortableDeviceWatcherWin --------------------------------------------------- PortableDeviceWatcherWin::PortableDeviceWatcherWin() ... http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:521: it != device_details_vector->end(); ++it) { Nit: {} unnecessary http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:545: HandleDeviceAttachedEvent() can fail and leave |device_details| empty. This function needs to detect this and no-op, either by checking for an empty struct here or by checking a return code from the other function (which might be clearer). http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:548: const string16& location = device_details->location; Nit: If any of these refs doesn't end up saving lines overall, I'd remove it and just deref things directly where used http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:567: L')'); Nit: Indent 4 http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:570: location); Nit: Arg must be aligned with first arg above http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:587: StorageObjects storage_objects = device_entry->second; Nit: Does the loop below modify or invalidate what |device_entry| points to? If not, use a const reference or refer to device_entry->second explicitly below. If so, comment here what precisely makes a copy necessary. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.h (right): http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:31: // running tasks it spins off to a media task runner. Nit: media task runner -> SequencedTaskRunner http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:34: // Struct to store device storage information. Nit: This comment doesn't add anything, remove. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:36: // Storage object temporary identifier, e.g.: "s10001". Tiny nit: ':' not necessary after "e.g." (2 places) http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:44: // Vector of media transfer protocol(mtp) device storage objects. Nit: Comment doesn't add anything, remove. (Same as with next typedef.) I would also eliminate the blank line between the struct and the typedef, which makes it look less weird that you do struct, typedef, second struct. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:60: typedef std::vector<DeviceDetails> DeviceDetailsVector; Nit: How about just "Devices" as the type name? (and the variable names, where this is used) http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win.cc (right): http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:33: /////////////////////////////////////////////////////////////////////////// Nit: As in other file, I suggest: ... } // namespace // RemovableDeviceNotificationsWindowWin::PortableDeviceNotifications --------- class RemovableDeviceNotificationsWindowWin::PortableDeviceNotifications { ... I don't think the descriptive comment you had adds much so I'd omit it. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:49: PortableDeviceNotifications(HWND hwnd) { Nit: Indent 4 http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:54: DEV_BROADCAST_DEVICEINTERFACE db = {sizeof(DEV_BROADCAST_DEVICEINTERFACE), Nit: I suggest: DEV_BROADCAST_DEVICEINTERFACE db = { sizeof(DEV_BROADCAST_DEVICEINTERFACE), DBT_DEVTYP_DEVICEINTERFACE, 0, dev_interface_guid }; This makes it easier for a reader to compare this with a declaration of the struct in question to see what members get what values. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:62: ~PortableDeviceNotifications() { Nit: Indent 4 http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc (right): http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:39: const char16 kMtpDeviceWithMultipleStorage[] = Nit: Multiple storage what? Objects? (several places) http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:83: std::vector<FilePath> GetTestAttachedDevices() { Nit: Consider adding "typedef std::vector<FilePath> AttachedDevices;" and using pervasively http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:124: std::vector<string16> GetMtpStorageObjectIds(const string16& pnp_device_id) { Nit: Likewise, consider a "typedef std::vector<string16> ObjectIDs;" for this http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:127: Nit: I'd remove this blank line http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:145: const std::string root_path("\\\\"); Nit: Inline into next line (then remove {}) http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:165: PortableDeviceWatcherWin::DeviceStorageObject storage; Nit: Use aggregate initialization form or add a constructor for this struct. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:166: const string16& object_id = *id_iter; Nit: Could inline this below http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:167: storage.object_persistent_id = GetMtpStorageUniqueId(pnp_device_id, Nit: I'd break after '=' instead of ',' http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:179: ////////////////////////////////////////////////////////////////////// Nit: See comments on dividers in other files (multiple classes) http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:186: // Override PortableDeviceWatcherWin:: Nit: Just say "PortableDeviceWatcherWin:" (with one colon) (several places) http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:206: device_details.storage_objects = GetDeviceStorageObjects(pnp_device_id); Nit: Use aggregate initialization form or add a constructor for this struct. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:246: pre_attach_devices_ = pre_attach_devices; Nit: This definition should be inlined in the class declaration. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:279: void InitWithTestDataAndAttachedDevices(); Nit: Consider combining these into "void InitWithTestData(bool pre_attach_devices)" http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:287: TestRemovableDeviceNotificationsWindowWin( Nit: Indent 4 for class member function names wrapped like this (several places) http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:325: virtual void SetUp() OVERRIDE; Nit: Add "// testing::Test:" above http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:326: virtual void TearDown(); Nit: OVERRIDE http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:396: for (size_t i = 0; i < initial_devices.size(); i++) Nit: Especially if you add the typedef for this type, use a const_iterator here http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:400: volume_mount_watcher_.get(), new TestPortableDeviceWatcherWin)); Nit: Extra space http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:482: for (size_t index = 0; index < storage_object_ids.size(); ++index) { Nit: Again, especially if you add the typedef for this, use a const_iterator here (2 places) http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:488: int cardinality = 1; Nit: Shorter: int cardinality = (name.empty() || unique_id.empty()) ? 0 : 1; You could then also inline this into the next statement. (2 places) http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:503: const string16& pnp_device_id) { This function looks just like DoMtpDeviceAttachedTest() except for the name of the function called in EXPECT_CALL() and the event type in InjectDeviceChange(). Convert to a single function which takes arguments. The easiest way to solve this is to take a simple "bool attached" arg and use that to switch the behavior (with a conditional for the first issue and a ternary operator for the second). http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:669: // Attach a mtp device. Nit: These comment add nothing to the code. (several places)
Removing myself, it seems like this is not ready for owners review yet.
pkasting@: Addressed review comments. PTAL. Thanks. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:75: return SUCCEEDED(hr) ? !name->empty() : false; On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Simpler: > > return SUCCEEDED(hr) && !name->empty(); > > (several functions) Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:198: *value = static_cast<const wchar_t*>(buffer); On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: wchar_t -> char16, since the original type and the receiving object both > use char16 Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:299: ((device_name[0] >= L'A') && (device_name[0] <= L'Z') || On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Should be indented 4. I thought we align the conditions. I am not sure about your comment. Line #299 already has 4 spaces. > > Also, add parens: right now you have "a && b || (c && d)", presumably you want > "(a && b) || (c && d)" Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:320: // runner. On success, returns true and populates |storage_objects| with device On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: You added comments to a number of functions saying "this task may take a > long time to complete, so access via task runner" or similar. That's not what I > meant to happen -- it's already obvious that's the case since you're directly > DCHECKing that you're being run in the blocking pool. > > Instead, I was looking for comments on the particular Windows API calls that > take a long time. For example: > > // This function calls XYZApi(), which checks for attached file systems. If any > of these are on network drives, Windows may take an arbitrarily long time to > respond. Additionally, the call to ABCApi(), while not an arbitrary long call, > can take up to 250 ms to return. We can't block the UI thread in either of > these cases, so we segregate this functionality and run it via the task runner. I misunderstood your earlier comment. How about this? "Media devices may respond slowly to the windows portable device API request. These requests may take an arbitrarily long time to complete. Therefore, segregate this functionality and run it via sequenced task runner instead of blocking the UI thread". Once we finalize the text, I will update all the function comments. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:341: for (std::vector<string16>::const_iterator id_iter = storage_obj_ids.begin(); On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: You are inconsistent across this file about how you name your iterators: > xyz_iter, iter, or it. Be consistent. > > The rest of Chrome code tends to use "i" or "it". I personally prefer the > former. "i" or "it" does not provide any information about the type of the iterator. I have to look at the declaration to find that information. I am going to follow "xyz_iter" pattern in this file. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:350: PortableDeviceWatcherWin::DeviceStorageObject storage_object; On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Use aggregate initialization form or add a constructor for this struct. Added a constructor. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:359: // Access the device and gets the device details (name, storage info, etc.,). On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: "Access" -> "Accesses", "etc.," -> "etc." Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:395: // Either there is no portable device support (it must be a XP with Windows On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: "it must be a XP with Windows Media Player Version < 10" -> "i.e. Windows > XP with old versions of Media Player" Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:461: const std::string root_path("\\\\"); On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Frankly I think this should just be inlined into the next statement. Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:470: /////////////////////////////////////////////////////////////////////////////// On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: More common is the following: > > ... > } // namespace > > > // PortableDeviceWatcherWin --------------------------------------------------- > > PortableDeviceWatcherWin::PortableDeviceWatcherWin() > ... Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:521: it != device_details_vector->end(); ++it) { On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: {} unnecessary If the condition spans over a line, I am adding {}. Just to be consistent, I am leaving this as it is. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:545: On 2012/10/26 23:46:23, Peter Kasting wrote: > HandleDeviceAttachedEvent() can fail and leave |device_details| empty. This > function needs to detect this and no-op, either by checking for an empty struct > here or by checking a return code from the other function (which might be > clearer). Modified code to check the return value of HandleDeviceAttachEvent(). http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:548: const string16& location = device_details->location; On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: If any of these refs doesn't end up saving lines overall, I'd remove it and > just deref things directly where used I would like to leave it as it is. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:567: L')'); On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Indent 4 Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:570: location); On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Arg must be aligned with first arg above Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:587: StorageObjects storage_objects = device_entry->second; On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Does the loop below modify or invalidate what |device_entry| points to? If > not, use a const reference or refer to device_entry->second explicitly below. > If so, comment here what precisely makes a copy necessary. Loop does not modify |device_entry|->second value. Instead of copying, created a reference variable to point to |device_entry|->second value. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.h (right): http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:31: // running tasks it spins off to a media task runner. On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: media task runner -> SequencedTaskRunner Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:34: // Struct to store device storage information. On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: This comment doesn't add anything, remove. Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:36: // Storage object temporary identifier, e.g.: "s10001". On 2012/10/26 23:46:23, Peter Kasting wrote: > Tiny nit: ':' not necessary after "e.g." (2 places) Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:44: // Vector of media transfer protocol(mtp) device storage objects. On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Comment doesn't add anything, remove. (Same as with next typedef.) > > I would also eliminate the blank line between the struct and the typedef, which > makes it look less weird that you do struct, typedef, second struct. Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:60: typedef std::vector<DeviceDetails> DeviceDetailsVector; On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: How about just "Devices" as the type name? (and the variable names, where > this is used) Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win.cc (right): http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:33: /////////////////////////////////////////////////////////////////////////// On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: As in other file, I suggest: > > ... > } // namespace > > > // RemovableDeviceNotificationsWindowWin::PortableDeviceNotifications --------- > > class RemovableDeviceNotificationsWindowWin::PortableDeviceNotifications { > ... > > I don't think the descriptive comment you had adds much so I'd omit it. Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:49: PortableDeviceNotifications(HWND hwnd) { On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Indent 4 Done. Some reviewer said if the function name spans over a line, we don't need to indent 4 spaces. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:54: DEV_BROADCAST_DEVICEINTERFACE db = {sizeof(DEV_BROADCAST_DEVICEINTERFACE), On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: I suggest: > > DEV_BROADCAST_DEVICEINTERFACE db = { > sizeof(DEV_BROADCAST_DEVICEINTERFACE), > DBT_DEVTYP_DEVICEINTERFACE, > 0, > dev_interface_guid > }; > > This makes it easier for a reader to compare this with a declaration of the > struct in question to see what members get what values. Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:62: ~PortableDeviceNotifications() { On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Indent 4 Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc (right): http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:39: const char16 kMtpDeviceWithMultipleStorage[] = On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Multiple storage what? Objects? (several places) Renamed kMtpDeviceWithMultipleStorage => kMtpDeviceWithMultipleStorageObjects. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:83: std::vector<FilePath> GetTestAttachedDevices() { On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Consider adding "typedef std::vector<FilePath> AttachedDevices;" and using > pervasively Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:124: std::vector<string16> GetMtpStorageObjectIds(const string16& pnp_device_id) { On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Likewise, consider a "typedef std::vector<string16> ObjectIDs;" for this Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:127: On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: I'd remove this blank line Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:145: const std::string root_path("\\\\"); On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Inline into next line (then remove {}) Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:165: PortableDeviceWatcherWin::DeviceStorageObject storage; On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Use aggregate initialization form or add a constructor for this struct. Added a constructor. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:166: const string16& object_id = *id_iter; On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Could inline this below Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:167: storage.object_persistent_id = GetMtpStorageUniqueId(pnp_device_id, On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: I'd break after '=' instead of ',' Inlined this code. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:179: ////////////////////////////////////////////////////////////////////// On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: See comments on dividers in other files (multiple classes) Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:186: // Override PortableDeviceWatcherWin:: On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Just say "PortableDeviceWatcherWin:" (with one colon) (several places) Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:206: device_details.storage_objects = GetDeviceStorageObjects(pnp_device_id); On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Use aggregate initialization form or add a constructor for this struct. Used aggregate initialization. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:246: pre_attach_devices_ = pre_attach_devices; On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: This definition should be inlined in the class declaration. Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:279: void InitWithTestDataAndAttachedDevices(); On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Consider combining these into "void InitWithTestData(bool > pre_attach_devices)" Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:287: TestRemovableDeviceNotificationsWindowWin( On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Indent 4 for class member function names wrapped like this (several places) Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:325: virtual void SetUp() OVERRIDE; On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Add "// testing::Test:" above Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:326: virtual void TearDown(); On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: OVERRIDE Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:396: for (size_t i = 0; i < initial_devices.size(); i++) On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Especially if you add the typedef for this type, use a const_iterator here Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:400: volume_mount_watcher_.get(), new TestPortableDeviceWatcherWin)); On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Extra space Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:482: for (size_t index = 0; index < storage_object_ids.size(); ++index) { On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Again, especially if you add the typedef for this, use a const_iterator > here (2 places) Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:488: int cardinality = 1; On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: Shorter: > > int cardinality = (name.empty() || unique_id.empty()) ? 0 : 1; > > You could then also inline this into the next statement. (2 places) Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:503: const string16& pnp_device_id) { On 2012/10/26 23:46:23, Peter Kasting wrote: > This function looks just like DoMtpDeviceAttachedTest() except for the name of > the function called in EXPECT_CALL() and the event type in InjectDeviceChange(). > Convert to a single function which takes arguments. The easiest way to solve > this is to take a simple "bool attached" arg and use that to switch the behavior > (with a conditional for the first issue and a ternary operator for the second). Done. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:669: // Attach a mtp device. On 2012/10/26 23:46:23, Peter Kasting wrote: > Nit: These comment add nothing to the code. (several places) Done.
http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:299: ((device_name[0] >= L'A') && (device_name[0] <= L'Z') || On 2012/10/28 22:57:16, kmadhusu wrote: > On 2012/10/26 23:46:23, Peter Kasting wrote: > > Nit: Should be indented 4. > > I thought we align the conditions. I am not sure about your comment. Line #299 > already has 4 spaces. Huh, somehow in the "diff against prior CL" view it appears to have a different number of spaces. Weird. Ignore that then. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:320: // runner. On success, returns true and populates |storage_objects| with device On 2012/10/28 22:57:16, kmadhusu wrote: > On 2012/10/26 23:46:23, Peter Kasting wrote: > > Nit: You added comments to a number of functions saying "this task may take a > > long time to complete, so access via task runner" or similar. That's not what > I > > meant to happen -- it's already obvious that's the case since you're directly > > DCHECKing that you're being run in the blocking pool. > > > > Instead, I was looking for comments on the particular Windows API calls that > > take a long time. For example: > > > > // This function calls XYZApi(), which checks for attached file systems. If > any > > of these are on network drives, Windows may take an arbitrarily long time to > > respond. Additionally, the call to ABCApi(), while not an arbitrary long > call, > > can take up to 250 ms to return. We can't block the UI thread in either of > > these cases, so we segregate this functionality and run it via the task > runner. > > I misunderstood your earlier comment. > > How about this? > "Media devices may respond slowly to the windows portable device API request. > These requests may take an arbitrarily long time to complete. Therefore, > segregate this functionality and run it via sequenced task runner instead of > blocking the UI thread". > > Once we finalize the text, I will update all the function comments. I don't want a block of text that's pasted into several function comments. That's worse than no comment at all. The idea is to be specific about precisely which call(s) can take a long time, ideally with supporting documentation like MSDN articles that note the blocking/responsiveness characteristics of certain APIs. This way someone refactoring in the future can understand how to change the code if they move/add/delete API calls. The sort of comment you're suggesting here is vague. It basically just says "things might be slow", which is already obvious from the way you wrote the code.
LGTM with conversion to PostTaskAndReplyWithResult http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:247: string16 unique_id(storage_id + L':' + device_serial_num); Nit: I suggest inlining this below to avoid the temp or the need for explicit string16 construction http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:248: Nit: Blank line unnecessary http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:372: Nit: No blank line after precondition DCHECKs (since you don't have one in the majority of the functions in this file) (several places) http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:431: if (!GetDeviceInfoOnBlockingThread(portable_device_mgr, Nit: Or just: if (GetDeviceInfoOnBlockingThread( portable_device_mgr, pnp_device_ids[index], &device_details)) devices->push_back(device_details); http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:450: bool* result) { You should be able to avoid this outparam and return bool directly by using PostTaskAndReplyWithResult in the caller. See for example how SetTemporaryGlobalOverrideQuotaOnDBThread works in http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/quota/quota_manager.cc . http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:475: Nit: Consider adding one more blank line here to make the separation more obvious http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:497: pool->GetNamedSequenceToken(kMediaTaskRunnerName); Nit: The declaration here is so verbose I'd just inline this into the next line http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.h (right): http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:28: // to base::SystemMonitor about the attached/detached Mtp devices. This is a Nit: Mtp -> MTP (or "media transfer protocol (MTP)") http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:44: // e.g. "StorageSerial:<SID-{10001,D,31080448}>:<123456789>" Nit: Trailing period http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:79: MtpStorageMap; Nit: Style guide does not rule on this, but I think capitalizing all letters of an abbreviation (MTP) in types/names/etc. is clearer then InitialCaps style http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:95: // Handles the detach event of the device specified by |pnp_device_id|. Nit: Extra space http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_constants.h (right): http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_constants.h:23: extern const char kRightParen[]; Just make sure you remember to remove these four constants in a followup if you're not going to do it here http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win.cc (right): http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:28: // RemovableDeviceNotificationsWindowWin::PortableDeviceNotifications --------- Why make this a separate class instead of simply placing its code into RemovableDeviceNotificationsWindowWin::Init() and ~RemovableDeviceNotificationsWindowWin()? It just seems to add verbosity and indirection. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:29: class RemovableDeviceNotificationsWindowWin::PortableDeviceNotifications { Nit: Add blank line. Consider also adding a blank line above the divider to make the separation more discernable (2 places) http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:31: // Register window handle to receive portable device notifications details. Nit: Register window handle -> Registers |hwnd| http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:64: // RemovableDeviceNotificationsWindowWin implementaion ----------------------- Nit: remove "implementation" http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win.h (right): http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.h:48: RemovableDeviceNotificationsWindowWin( Nit: Add comments about why we take these objects as args instead of creating them inside the constructor. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc (right): http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:105: kStorageUniqueIdB : std::string(); Nit: Indent 4 (2 places) http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:162: storage_object_ids.begin(); Nit: Indent 1 more (for loop indenting is weird) http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:172: // TestPortableDeviceWatcherWin ----------------------------------------------- Nit: Consider a blank line above these dividers (4 places) http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:180: // Override PortableDeviceWatcherWin: Nit: Remove "override" (2 places) http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:199: (pnp_device_id != kMtpDeviceWithInvalidInfo) ? kMtpDeviceFriendlyName : Nit: Break after ?, not : (2 places) http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:204: bool result(true); Nit: Don't use constructor-style initialization for POD types http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:254: std::vector<FilePath>(); Nit: AttachedDevices() http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:257: // TestRemovableDeviceNotificationsWindowWin ------------ Nit: More dashes http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:296: UINT event_type, DWORD data) { Nit: One arg per line http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:318: void DoMassStorageDeviceAttachedTest(const std::vector<int>& device_indices); Nit: Consider a vector<int> typedef http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:323: // false. Nit: How about: Injects a device attach or detach change (depeneding on the value of |attached|) and tests that the appropriate handler is called. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:324: void DoMtpDeviceTest(const string16& pnp_device_id, bool attached); Nit: Consider renaming |attached| to |test_attach|
pkasting@: Addressed review comments. PTAL. Thanks. http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/64002/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:320: // runner. On success, returns true and populates |storage_objects| with device On 2012/10/29 20:41:33, Peter Kasting wrote: > On 2012/10/28 22:57:16, kmadhusu wrote: > > On 2012/10/26 23:46:23, Peter Kasting wrote: > > > Nit: You added comments to a number of functions saying "this task may take > a > > > long time to complete, so access via task runner" or similar. That's not > what > > I > > > meant to happen -- it's already obvious that's the case since you're > directly > > > DCHECKing that you're being run in the blocking pool. > > > > > > Instead, I was looking for comments on the particular Windows API calls that > > > take a long time. For example: > > > > > > // This function calls XYZApi(), which checks for attached file systems. If > > any > > > of these are on network drives, Windows may take an arbitrarily long time to > > > respond. Additionally, the call to ABCApi(), while not an arbitrary long > > call, > > > can take up to 250 ms to return. We can't block the UI thread in either of > > > these cases, so we segregate this functionality and run it via the task > > runner. > > > > I misunderstood your earlier comment. > > > > How about this? > > "Media devices may respond slowly to the windows portable device API request. > > These requests may take an arbitrarily long time to complete. Therefore, > > segregate this functionality and run it via sequenced task runner instead of > > blocking the UI thread". > > > > Once we finalize the text, I will update all the function comments. > > I don't want a block of text that's pasted into several function comments. > That's worse than no comment at all. > > The idea is to be specific about precisely which call(s) can take a long time, > ideally with supporting documentation like MSDN articles that note the > blocking/responsiveness characteristics of certain APIs. This way someone > refactoring in the future can understand how to change the code if they > move/add/delete API calls. > > The sort of comment you're suggesting here is vague. It basically just says > "things might be slow", which is already obvious from the way you wrote the > code. Removed the redundant (on why I am running these tasks on an blocking thread instead of using UI thread) comment from all the function headers. While I was testing with a very old Kodak camera, I noticed that it takes > 100ms to get the device properties. Whereas, if I use a Nexus 7, it is pretty fast. So, the time taken to complete these requests depends on the device capability. I didn't find any MSDN article to backup my statement. That is the reason, I cannot provide any numerical values regarding the delay. I think anyone who is reading this code, can understand that we are communicating with the device to get certain information. Therefore, it is better to do these tasks on a blocking thread instead of an UI thread. Added a file level comment regarding this. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:247: string16 unique_id(storage_id + L':' + device_serial_num); On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: I suggest inlining this below to avoid the temp or the need for explicit > string16 construction Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:248: On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: Blank line unnecessary Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:372: On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: No blank line after precondition DCHECKs (since you don't have one in the > majority of the functions in this file) (several places) Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:431: if (!GetDeviceInfoOnBlockingThread(portable_device_mgr, On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: Or just: > > if (GetDeviceInfoOnBlockingThread( > portable_device_mgr, pnp_device_ids[index], &device_details)) > devices->push_back(device_details); Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:450: bool* result) { On 2012/10/29 21:57:19, Peter Kasting wrote: > You should be able to avoid this outparam and return bool directly by using > PostTaskAndReplyWithResult in the caller. See for example how > SetTemporaryGlobalOverrideQuotaOnDBThread works in > http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/quota/quota_manager.cc > . Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:475: On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: Consider adding one more blank line here to make the separation more > obvious Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:497: pool->GetNamedSequenceToken(kMediaTaskRunnerName); On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: The declaration here is so verbose I'd just inline this into the next line Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.h (right): http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:28: // to base::SystemMonitor about the attached/detached Mtp devices. This is a On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: Mtp -> MTP (or "media transfer protocol (MTP)") Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:44: // e.g. "StorageSerial:<SID-{10001,D,31080448}>:<123456789>" On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: Trailing period Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:79: MtpStorageMap; On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: Style guide does not rule on this, but I think capitalizing all letters of > an abbreviation (MTP) in types/names/etc. is clearer then InitialCaps style Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.h:95: // Handles the detach event of the device specified by |pnp_device_id|. On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: Extra space Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_constants.h (right): http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_constants.h:23: extern const char kRightParen[]; On 2012/10/29 21:57:19, Peter Kasting wrote: > Just make sure you remember to remove these four constants in a followup if > you're not going to do it here Done. In this CL, I removed all instances of kLeftParen, kRightParen, kNonSpaceDelim and kSpaceDelim in windows specific code. Will remove them from linux specific code soon. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win.cc (right): http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:28: // RemovableDeviceNotificationsWindowWin::PortableDeviceNotifications --------- On 2012/10/29 21:57:19, Peter Kasting wrote: > Why make this a separate class instead of simply placing its code into > RemovableDeviceNotificationsWindowWin::Init() and > ~RemovableDeviceNotificationsWindowWin()? It just seems to add verbosity and > indirection. Done. Removed this helper class. Made code changes to register and unregister device notifications in PortableDeviceWatcherWin::Init() and ~PortableDeviceWatcherWin destructor respectively. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:29: class RemovableDeviceNotificationsWindowWin::PortableDeviceNotifications { On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: Add blank line. Consider also adding a blank line above the divider to > make the separation more discernable (2 places) -NA- http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:31: // Register window handle to receive portable device notifications details. On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: Register window handle -> Registers |hwnd| Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:64: // RemovableDeviceNotificationsWindowWin implementaion ----------------------- On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: remove "implementation" Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win.h (right): http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.h:48: RemovableDeviceNotificationsWindowWin( On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: Add comments about why we take these objects as args instead of creating > them inside the constructor. Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc (right): http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:105: kStorageUniqueIdB : std::string(); On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: Indent 4 (2 places) Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:162: storage_object_ids.begin(); On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: Indent 1 more (for loop indenting is weird) Fixed. Moved the iterator declaration outside the loop. PortableDeviceWatcherWin::StorageObjectIDs::const_iterator it = storage_object_ids.begin(); for (; it != storage_object_ids.end(); ++it) { } http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:172: // TestPortableDeviceWatcherWin ----------------------------------------------- On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: Consider a blank line above these dividers (4 places) Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:180: // Override PortableDeviceWatcherWin: On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: Remove "override" (2 places) Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:199: (pnp_device_id != kMtpDeviceWithInvalidInfo) ? kMtpDeviceFriendlyName : On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: Break after ?, not : (2 places) Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:204: bool result(true); On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: Don't use constructor-style initialization for POD types Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:254: std::vector<FilePath>(); On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: AttachedDevices() Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:257: // TestRemovableDeviceNotificationsWindowWin ------------ On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: More dashes Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:296: UINT event_type, DWORD data) { On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: One arg per line Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:318: void DoMassStorageDeviceAttachedTest(const std::vector<int>& device_indices); On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: Consider a vector<int> typedef Done. typedef std::vector<int> DeviceIndices; http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:323: // false. On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: How about: > > Injects a device attach or detach change (depeneding on the value of |attached|) > and tests that the appropriate handler is called. Done. http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:324: void DoMtpDeviceTest(const string16& pnp_device_id, bool attached); On 2012/10/29 21:57:19, Peter Kasting wrote: > Nit: Consider renaming |attached| to |test_attach| Done.
LGTM http://codereview.chromium.org/11088012/diff/72006/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/72006/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:6: // complete. Those tasks should be run on an blocking thread instead of an nit: an->the
vandebo@: Fixed nit. Thanks. http://codereview.chromium.org/11088012/diff/72006/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/72006/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:6: // complete. Those tasks should be run on an blocking thread instead of an On 2012/10/30 21:54:24, vandebo wrote: > nit: an->the Done.
thestig@: Could you press an owner's stamp for chrome/ changes? Thanks.
lgtm with a couple nits: http://codereview.chromium.org/11088012/diff/79014/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/79014/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:599: StorageObjects::const_iterator storage_object_iter = storage_objects.begin(); Put this in the for loop, since it's not needed outside of that scope. http://codereview.chromium.org/11088012/diff/79014/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win.cc (left): http://codereview.chromium.org/11088012/diff/79014/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:11: #include "base/file_path.h" FilePath is used here though. Undelete?
thestig@: Addressed review comments. Thanks. http://codereview.chromium.org/11088012/diff/79014/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/79014/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:599: StorageObjects::const_iterator storage_object_iter = storage_objects.begin(); On 2012/10/30 22:46:03, Lei Zhang wrote: > Put this in the for loop, since it's not needed outside of that scope. If I have it within the for loop, it crosses the 80 column limit and the indentation looks weird. So I declared outside. http://codereview.chromium.org/11088012/diff/79014/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win.cc (left): http://codereview.chromium.org/11088012/diff/79014/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win.cc:11: #include "base/file_path.h" On 2012/10/30 22:46:03, Lei Zhang wrote: > FilePath is used here though. Undelete? I am just forwarding the file path reference param. I am not de-referencing those param values. This header file is not required here.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11088012/79014
Looks like thestig found another instance of my comment below http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc (right): http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:162: storage_object_ids.begin(); On 2012/10/30 01:29:24, kmadhusu wrote: > On 2012/10/29 21:57:19, Peter Kasting wrote: > > Nit: Indent 1 more (for loop indenting is weird) > Fixed. Moved the iterator declaration outside the loop. > > PortableDeviceWatcherWin::StorageObjectIDs::const_iterator it = > storage_object_ids.begin(); > for (; it != storage_object_ids.end(); ++it) { > } No, do not do that. Never declare loop variables outside the loop unless they will be needed outside the loop.
http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc (right): http://codereview.chromium.org/11088012/diff/70004/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:162: storage_object_ids.begin(); On 2012/10/30 23:17:52, Peter Kasting wrote: > On 2012/10/30 01:29:24, kmadhusu wrote: > > On 2012/10/29 21:57:19, Peter Kasting wrote: > > > Nit: Indent 1 more (for loop indenting is weird) > > Fixed. Moved the iterator declaration outside the loop. > > > > PortableDeviceWatcherWin::StorageObjectIDs::const_iterator it = > > storage_object_ids.begin(); > > for (; it != storage_object_ids.end(); ++it) { > > } > > No, do not do that. Never declare loop variables outside the loop unless they > will be needed outside the loop. Fixed. http://codereview.chromium.org/11088012/diff/79014/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/79014/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:599: StorageObjects::const_iterator storage_object_iter = storage_objects.begin(); On 2012/10/30 22:46:03, Lei Zhang wrote: > Put this in the for loop, since it's not needed outside of that scope. Fixed.
Sure, LGTM http://codereview.chromium.org/11088012/diff/81008/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/81008/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:602: MTPStorageMap::iterator storage_iter = storage_map_.find(storage_id); Nit: Might want to name this |storage_map_iter| to reduce confusion
http://codereview.chromium.org/11088012/diff/81008/chrome/browser/system_moni... File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): http://codereview.chromium.org/11088012/diff/81008/chrome/browser/system_moni... chrome/browser/system_monitor/portable_device_watcher_win.cc:602: MTPStorageMap::iterator storage_iter = storage_map_.find(storage_id); On 2012/10/30 23:58:32, Peter Kasting wrote: > Nit: Might want to name this |storage_map_iter| to reduce confusion Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11088012/72016
Retried try job too often for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11088012/72016
Change committed as 165126 |
