Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
231063002:
Add notification for media changed, and notify volume mount watcher when it occurs. (Closed)
|
Created:
6 years, 8 months ago by Kevin Bailey Modified:
6 years, 7 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd notification for media changed, and notify volume mount watcher when it occurs.
BUG=154632
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266914
Patch Set 1 #Patch Set 2 : Linting. #
Total comments: 15
Patch Set 3 : Code review #Patch Set 4 : Simplified a function #
Total comments: 18
Patch Set 5 : Move method #Patch Set 6 : Other code review #
Total comments: 7
Patch Set 7 : Name and constant change #
Messages
Total messages: 28 (0 generated)
Sorry for the delay https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... File components/storage_monitor/storage_monitor_win.cc (right): https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:47: #define WM_USER_MEDIACHANGED (WM_USER+1) Put this on before line 18 https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:49: void StorageMonitorWin::ChangeNotifyRegister() { Could also just put this code in the Init method, presumably with a comment saying that it registers for disk change events. https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:55: ULONG result = SHChangeNotifyRegister(window_, SHCNE_DISKEVENTS, result isn't used, inline the if https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:155: void StorageMonitorWin::OnMediaChanged(WPARAM wparam, LPARAM lparam) { Move to after line 183 https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... File components/storage_monitor/storage_monitor_win.h (right): https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... components/storage_monitor/storage_monitor_win.h:34: void ChangeNotifyRegister(); Make private. RegisterForDeviceChangeNotifications() ? https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... File components/storage_monitor/volume_mount_watcher_win.cc (right): https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... components/storage_monitor/volume_mount_watcher_win.cc:480: } SHNOTIFYSTRUCT; Can you figure out a better name for this. https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... components/storage_monitor/volume_mount_watcher_win.cc:483: SHNOTIFYSTRUCT* shns = reinterpret_cast<SHNOTIFYSTRUCT*>(wparam); Seem to only use dwItem1, should we just cast is to DWORD* ?
Sorry for the late reply. Was on vacation and cleaning inbox. Responded to everything. I'll start a try job. https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... File components/storage_monitor/storage_monitor_win.cc (right): https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:47: #define WM_USER_MEDIACHANGED (WM_USER+1) On 2014/04/11 22:24:26, vandebo wrote: > Put this on before line 18 Done. https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:49: void StorageMonitorWin::ChangeNotifyRegister() { It's a little messy, and would add entirely new calls and a local variable. I'm happy to include it if you like. I could also move it down, to preserve the class definition order. https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:55: ULONG result = SHChangeNotifyRegister(window_, SHCNE_DISKEVENTS, On 2014/04/11 22:24:26, vandebo wrote: > result isn't used, inline the if Done. https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:155: void StorageMonitorWin::OnMediaChanged(WPARAM wparam, LPARAM lparam) { On 2014/04/11 22:24:26, vandebo wrote: > Move to after line 183 Done. https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... File components/storage_monitor/storage_monitor_win.h (right): https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... components/storage_monitor/storage_monitor_win.h:34: void ChangeNotifyRegister(); On 2014/04/11 22:24:26, vandebo wrote: > Make private. > > RegisterForDeviceChangeNotifications() ? Done, and proposed better name. https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... File components/storage_monitor/volume_mount_watcher_win.cc (right): https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... components/storage_monitor/volume_mount_watcher_win.cc:480: } SHNOTIFYSTRUCT; Gone. https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... components/storage_monitor/volume_mount_watcher_win.cc:483: SHNOTIFYSTRUCT* shns = reinterpret_cast<SHNOTIFYSTRUCT*>(wparam); Ya, got rid of it entirely, and simplified the function.
+rvargas for any Windows gotchas https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... File components/storage_monitor/storage_monitor_win.cc (right): https://codereview.chromium.org/231063002/diff/20001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:49: void StorageMonitorWin::ChangeNotifyRegister() { On 2014/04/22 15:43:03, Kevin Bailey wrote: > It's a little messy, and would add entirely new calls and a local variable. I'm > happy to include it if you like. I think inlining it would be fine, but leaving it a seperate function is also fine. https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... File components/storage_monitor/storage_monitor_win.cc (right): https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:49: void StorageMonitorWin::MediaChangeNotificationRegister() { Move to line 169
win_rel try bot succeeded. https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... File components/storage_monitor/storage_monitor_win.cc (right): https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:49: void StorageMonitorWin::MediaChangeNotificationRegister() { On 2014/04/22 18:07:42, vandebo wrote: > Move to line 169 Done.
https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... File components/storage_monitor/storage_monitor_win.cc (right): https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:18: #define WM_USER_MEDIACHANGED (WM_USER+1) This is numerically the same as the wake up message used by base::MesssagePumpWin. Better to use a different value to avoid any possible confusion. nit: spaces around + https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:38: StorageMonitorWin::~StorageMonitorWin() { SHChangeNotifyDeregister ? https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:50: LPITEMIDLIST ppidl; nit: even for Windows code, variables should follow chromium's naming style, not Windows naming style. (and below) https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:51: if (SHGetSpecialFolderLocation(window_, CSIDL_DESKTOP, &ppidl) == NOERROR) { CSIDL_DRIVES ? https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:51: if (SHGetSpecialFolderLocation(window_, CSIDL_DESKTOP, &ppidl) == NOERROR) { The first argument is reserved. https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:55: if (!SHChangeNotifyRegister(window_, SHCNE_DISKEVENTS, The second argument (fSources) is one of SHCNRF_* values. SHCNE_DISKEVENTS is actually a mask of other fEvents. https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:56: SHCNE_MEDIAINSERTED|SHCNE_MEDIAREMOVED, nit: spaces around |
Most addressed, one q. https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... File components/storage_monitor/storage_monitor_win.cc (right): https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:18: #define WM_USER_MEDIACHANGED (WM_USER+1) On 2014/04/22 21:09:44, rvargas wrote: > This is numerically the same as the wake up message used by > base::MesssagePumpWin. Better to use a different value to avoid any possible > confusion. I was under the impression that it is the intent of the WM_USER range to overlap, that it is the WM_APP range that must coordinate within the app. Not true? > nit: spaces around + Done. https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:38: StorageMonitorWin::~StorageMonitorWin() { On 2014/04/22 21:09:44, rvargas wrote: > SHChangeNotifyDeregister ? Done. https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:50: LPITEMIDLIST ppidl; On 2014/04/22 21:09:44, rvargas wrote: > nit: even for Windows code, variables should follow chromium's naming style, not > Windows naming style. (and below) Done. https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:51: if (SHGetSpecialFolderLocation(window_, CSIDL_DESKTOP, &ppidl) == NOERROR) { On 2014/04/22 21:09:44, rvargas wrote: > CSIDL_DRIVES ? Done. https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:51: if (SHGetSpecialFolderLocation(window_, CSIDL_DESKTOP, &ppidl) == NOERROR) { On 2014/04/22 21:09:44, rvargas wrote: > The first argument is reserved. I assume this means, pass 0. https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:55: if (!SHChangeNotifyRegister(window_, SHCNE_DISKEVENTS, On 2014/04/22 21:09:44, rvargas wrote: > The second argument (fSources) is one of SHCNRF_* values. SHCNE_DISKEVENTS is > actually a mask of other fEvents. Ok, I hope ShellLevel is correct. It seems to work (as do most values, it appears.) https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:56: SHCNE_MEDIAINSERTED|SHCNE_MEDIAREMOVED, On 2014/04/22 21:09:44, rvargas wrote: > nit: spaces around | Done.
https://codereview.chromium.org/231063002/diff/90001/components/storage_monit... File components/storage_monitor/storage_monitor_win.h (right): https://codereview.chromium.org/231063002/diff/90001/components/storage_monit... components/storage_monitor/storage_monitor_win.h:77: ULONG registration_handle_; This could have a more descriptive name
https://codereview.chromium.org/231063002/diff/90001/components/storage_monit... File components/storage_monitor/storage_monitor_win.h (right): https://codereview.chromium.org/231063002/diff/90001/components/storage_monit... components/storage_monitor/storage_monitor_win.h:77: ULONG registration_handle_; On 2014/04/23 16:34:35, vandebo wrote: > This could have a more descriptive name |notification_registration_handle_| ? I use "handle" because different versions of the SDK call it different things, one of which is a handle. Would it help to say more in the comment ? "This is returned by ...Register and must be passed to ...Deregister."
https://codereview.chromium.org/231063002/diff/90001/components/storage_monit... File components/storage_monitor/storage_monitor_win.h (right): https://codereview.chromium.org/231063002/diff/90001/components/storage_monit... components/storage_monitor/storage_monitor_win.h:77: ULONG registration_handle_; On 2014/04/23 18:06:14, Kevin Bailey wrote: > On 2014/04/23 16:34:35, vandebo wrote: > > This could have a more descriptive name > > |notification_registration_handle_| ? I use "handle" because different versions > of the SDK call it different things, one of which is a handle. > > Would it help to say more in the comment ? > "This is returned by ...Register and must be passed to ...Deregister." There's lots of things that are registered... shell_change_notify_handle_ ?
https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... File components/storage_monitor/storage_monitor_win.cc (right): https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:18: #define WM_USER_MEDIACHANGED (WM_USER+1) On 2014/04/23 16:02:23, Kevin Bailey wrote: > On 2014/04/22 21:09:44, rvargas wrote: > > This is numerically the same as the wake up message used by > > base::MesssagePumpWin. Better to use a different value to avoid any possible > > confusion. > > I was under the impression that it is the intent of the WM_USER range to > overlap, that it is the WM_APP range that must coordinate within the app. Not > true? > > > nit: spaces around + > > Done. There is no bug in using the same value, as long as the message doesn't end up processed by the wrong window/wndproc. It's just a matter of avoiding any chance of developer confusion. I looked briefly for the code that called Init() to see how the thread is handled but couldn't find it so I'm just _guessing_ this is fine. If this code ends up relying on a base thread (of type UI) we'll have issues. https://codereview.chromium.org/231063002/diff/90001/components/storage_monit... File components/storage_monitor/storage_monitor_win.cc (right): https://codereview.chromium.org/231063002/diff/90001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:160: if (SHGetSpecialFolderLocation(0, CSIDL_DRIVES, &id_list) == NOERROR) { nit: use NULL for the handle. https://codereview.chromium.org/231063002/diff/90001/components/storage_monit... File components/storage_monitor/storage_monitor_win.h (right): https://codereview.chromium.org/231063002/diff/90001/components/storage_monit... components/storage_monitor/storage_monitor_win.h:77: ULONG registration_handle_; On 2014/04/23 18:09:27, vandebo wrote: > On 2014/04/23 18:06:14, Kevin Bailey wrote: > > On 2014/04/23 16:34:35, vandebo wrote: > > > This could have a more descriptive name > > > > |notification_registration_handle_| ? I use "handle" because different > versions > > of the SDK call it different things, one of which is a handle. > > > > Would it help to say more in the comment ? > > "This is returned by ...Register and must be passed to ...Deregister." > > There's lots of things that are registered... shell_change_notify_handle_ ? Given that this is not really a handle, shell_change_notify_id_ seems better.
Should all be addressed. https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... File components/storage_monitor/storage_monitor_win.cc (right): https://codereview.chromium.org/231063002/diff/60001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:18: #define WM_USER_MEDIACHANGED (WM_USER+1) Agreed, but now I have little idea who it's colliding with, or who in the future it might collide with. It might help avoid collisions if there was a small header file (or maybe IDL, since Python appears to use them too), perhaps in /base, with the list of all used window messages. https://codereview.chromium.org/231063002/diff/90001/components/storage_monit... File components/storage_monitor/storage_monitor_win.cc (right): https://codereview.chromium.org/231063002/diff/90001/components/storage_monit... components/storage_monitor/storage_monitor_win.cc:160: if (SHGetSpecialFolderLocation(0, CSIDL_DRIVES, &id_list) == NOERROR) { On 2014/04/23 18:45:15, rvargas wrote: > nit: use NULL for the handle. Done. https://codereview.chromium.org/231063002/diff/90001/components/storage_monit... File components/storage_monitor/storage_monitor_win.h (right): https://codereview.chromium.org/231063002/diff/90001/components/storage_monit... components/storage_monitor/storage_monitor_win.h:77: ULONG registration_handle_; On 2014/04/23 18:45:15, rvargas wrote: > On 2014/04/23 18:09:27, vandebo wrote: > > On 2014/04/23 18:06:14, Kevin Bailey wrote: > > > On 2014/04/23 16:34:35, vandebo wrote: > > > > This could have a more descriptive name > > > > > > |notification_registration_handle_| ? I use "handle" because different > > versions > > > of the SDK call it different things, one of which is a handle. > > > > > > Would it help to say more in the comment ? > > > "This is returned by ...Register and must be passed to ...Deregister." > > > > There's lots of things that are registered... shell_change_notify_handle_ ? > > Given that this is not really a handle, shell_change_notify_id_ seems better. Done.
LGTM
lgtm
The CQ bit was checked by krb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/krb@chromium.org/231063002/110001
The CQ bit was unchecked by commit-bot@chromium.org
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
The CQ bit was checked by vandebo@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/krb@chromium.org/231063002/110001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on android_dbg_triggered_tests
The CQ bit was checked by vandebo@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/krb@chromium.org/231063002/110001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on android_dbg_triggered_tests
The CQ bit was checked by krb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/krb@chromium.org/231063002/110001
Message was sent while issue was closed.
Change committed as 266914 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
