Send WM_DEVICECHANGE message through SystemMonitor

Send WM_DEVICECHANGE message through SystemMonitor

WM_DEVICECHANGE is sent when there's been a change to devices or the computer;
specifically when a USB device is connected or disconnected. This is intended
for use in support of Gamepads for more performant polling and
connect/disconnect testing. Currently only on Windows, though seems reasonable
to add for other platforms in the future.

BUG=79050
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109960

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110014

[sky, 2011-11-11 02:32:59]

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
File base/system_monitor/system_monitor.h (right):

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
base/system_monitor/system_monitor.h:96: virtual void OnDeviceChange() {}
OnDevicesChanged?

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
base/system_monitor/system_monitor.h:97: protected:
newline between 96 and 97

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
base/system_monitor/system_monitor.h:105: void AddObserver(DeviceChangeObserver*
obs);
style guide recommends using different names for this sort of thing.

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
base/system_monitor/system_monitor.h:124: void ProcessDeviceChange();
ProcessDevicesChanged?

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
base/system_monitor/system_monitor.h:142: void NotifyDeviceChange();
NotifyDevicesChanged

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
File base/system_monitor/system_monitor_unittest.cc (right):

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
base/system_monitor/system_monitor_unittest.cc:100: void OnDeviceChange() {
virtual and OVERRIDE

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
base/system_monitor/system_monitor_unittest.cc:105: int changes() { return
changes_; }
const

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
base/system_monitor/system_monitor_unittest.cc:108: int changes_;  // Count of
OnDeviceChange notifications.
DISALLOW_...

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
base/system_monitor/system_monitor_unittest.cc:128: EXPECT_EQ(test[0].changes(),
1);
assertions should have expected first, then actual.

[sky, 2011-11-11 02:34:57]

One point worth mentioning. The reason I suggest DevicesChanged
instead of DeviceChange is that the latter sounds more like the
characteristics of a device have changed. Where I think DevicesChanged
makes it sound like the number of devices have changed.

  -Scott

On Thu, Nov 10, 2011 at 6:32 PM,  <sky@chromium.org> wrote:
>
>
http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
> File base/system_monitor/system_monitor.h (right):
>
>
http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
> base/system_monitor/system_monitor.h:96: virtual void OnDeviceChange()
> {}
> OnDevicesChanged?
>
>
http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
> base/system_monitor/system_monitor.h:97: protected:
> newline between 96 and 97
>
>
http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
> base/system_monitor/system_monitor.h:105: void
> AddObserver(DeviceChangeObserver* obs);
> style guide recommends using different names for this sort of thing.
>
>
http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
> base/system_monitor/system_monitor.h:124: void ProcessDeviceChange();
> ProcessDevicesChanged?
>
>
http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
> base/system_monitor/system_monitor.h:142: void NotifyDeviceChange();
> NotifyDevicesChanged
>
>
http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
> File base/system_monitor/system_monitor_unittest.cc (right):
>
>
http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
> base/system_monitor/system_monitor_unittest.cc:100: void
> OnDeviceChange() {
> virtual and OVERRIDE
>
>
http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
> base/system_monitor/system_monitor_unittest.cc:105: int changes() {
> return changes_; }
> const
>
>
http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
> base/system_monitor/system_monitor_unittest.cc:108: int changes_;  //
> Count of OnDeviceChange notifications.
> DISALLOW_...
>
>
http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
> base/system_monitor/system_monitor_unittest.cc:128:
> EXPECT_EQ(test[0].changes(), 1);
> assertions should have expected first, then actual.
>
> http://codereview.chromium.org/8523021/
>

[Ben Goodger (Google), 2011-11-11 04:30:31]

I don't think NWW is the right place for this. If this is intended to support a
web api, how do you suppose it'll be hooked up in Content Shell?

Seems like RWHVW may be a better location, though for something low level and
not really UI related like this, it makes me wonder if we should have a msg
window somewhere in Chrome that responds to system notifications like this.

[scottmg, 2011-11-11 05:06:08]

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
File base/system_monitor/system_monitor.h (right):

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
base/system_monitor/system_monitor.h:96: virtual void OnDeviceChange() {}
On 2011/11/11 02:32:59, sky wrote:
> OnDevicesChanged?

Done.

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
base/system_monitor/system_monitor.h:97: protected:
On 2011/11/11 02:32:59, sky wrote:
> newline between 96 and 97

Done.

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
base/system_monitor/system_monitor.h:105: void AddObserver(DeviceChangeObserver*
obs);
On 2011/11/11 02:32:59, sky wrote:
> style guide recommends using different names for this sort of thing.

Sorry, couldn't find something that looked relevant. Do you mean they both
shouldn't be AddObserver? i.e. AddPowerObserver and AddDeviceChangeObserver
instead?

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
base/system_monitor/system_monitor.h:124: void ProcessDeviceChange();
On 2011/11/11 02:32:59, sky wrote:
> ProcessDevicesChanged?

Done.

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
base/system_monitor/system_monitor.h:142: void NotifyDeviceChange();
On 2011/11/11 02:32:59, sky wrote:
> NotifyDevicesChanged

Done.

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
File base/system_monitor/system_monitor_unittest.cc (right):

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
base/system_monitor/system_monitor_unittest.cc:100: void OnDeviceChange() {
On 2011/11/11 02:32:59, sky wrote:
> virtual and OVERRIDE

Done.

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
base/system_monitor/system_monitor_unittest.cc:105: int changes() { return
changes_; }
On 2011/11/11 02:32:59, sky wrote:
> const

Done.

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
base/system_monitor/system_monitor_unittest.cc:108: int changes_;  // Count of
OnDeviceChange notifications.
On 2011/11/11 02:32:59, sky wrote:
> DISALLOW_...

Done.

http://codereview.chromium.org/8523021/diff/1/base/system_monitor/system_moni...
base/system_monitor/system_monitor_unittest.cc:128: EXPECT_EQ(test[0].changes(),
1);
On 2011/11/11 02:32:59, sky wrote:
> assertions should have expected first, then actual.

Done.

[scottmg, 2011-11-11 05:13:18]

On 2011/11/11 04:30:31, Ben Goodger (Google) wrote:
> I don't think NWW is the right place for this. If this is intended to support
a
> web api, how do you suppose it'll be hooked up in Content Shell?
> 
> Seems like RWHVW may be a better location, though for something low level and
> not really UI related like this, it makes me wonder if we should have a msg
> window somewhere in Chrome that responds to system notifications like this.

OK thanks, I was following the handling of WM_POWERBROADCAST which seemed
similar. I'll move the hookup to RenderWidgetHostViewWin.

[Ben Goodger (Google), 2011-11-11 16:53:01]

Can we put it in the msg window that Chrome has instead? I think that'd be
even better... since RWHVW is a UI-related class too. I think power
broadcast should move there too.

-Ben

On Thu, Nov 10, 2011 at 9:13 PM, <scottmg@chromium.org> wrote:

> On 2011/11/11 04:30:31, Ben Goodger (Google) wrote:
>
>> I don't think NWW is the right place for this. If this is intended to
>> support
>>
> a
>
>> web api, how do you suppose it'll be hooked up in Content Shell?
>>
>
>  Seems like RWHVW may be a better location, though for something low level
>> and
>> not really UI related like this, it makes me wonder if we should have a
>> msg
>> window somewhere in Chrome that responds to system notifications like
>> this.
>>
>
> OK thanks, I was following the handling of WM_POWERBROADCAST which seemed
> similar. I'll move the hookup to RenderWidgetHostViewWin.
>
>
http://codereview.chromium.**org/8523021/<http://codereview.chromium.org/8523...
>

[scottmg, 2011-11-11 17:06:48]

Sorry, I'm new(-ish) here. Do you mean all the way down to
ProcessSingleton::WndProc? Or is there another more handler-y window?

I'll move power broadcast as a separate change.

On Fri, Nov 11, 2011 at 8:52 AM, Ben Goodger (Google) <ben@chromium.org> wrote:
> Can we put it in the msg window that Chrome has instead? I think that'd be
> even better... since RWHVW is a UI-related class too. I think power
> broadcast should move there too.
> -Ben
>
> On Thu, Nov 10, 2011 at 9:13 PM, <scottmg@chromium.org> wrote:
>>
>> On 2011/11/11 04:30:31, Ben Goodger (Google) wrote:
>>>
>>> I don't think NWW is the right place for this. If this is intended to
>>> support
>>
>> a
>>>
>>> web api, how do you suppose it'll be hooked up in Content Shell?
>>
>>> Seems like RWHVW may be a better location, though for something low level
>>> and
>>> not really UI related like this, it makes me wonder if we should have a
>>> msg
>>> window somewhere in Chrome that responds to system notifications like
>>> this.
>>
>> OK thanks, I was following the handling of WM_POWERBROADCAST which seemed
>> similar. I'll move the hookup to RenderWidgetHostViewWin.
>>
>> http://codereview.chromium.org/8523021/
>
>

[Ben Goodger (Google), 2011-11-11 17:15:17]

I think that may in fact be the window we're thinking of.

-Ben

On Fri, Nov 11, 2011 at 9:06 AM, Scott Graham <scottmg@chromium.org> wrote:

> Sorry, I'm new(-ish) here. Do you mean all the way down to
> ProcessSingleton::WndProc? Or is there another more handler-y window?
>
> I'll move power broadcast as a separate change.
>
> On Fri, Nov 11, 2011 at 8:52 AM, Ben Goodger (Google) <ben@chromium.org>
> wrote:
> > Can we put it in the msg window that Chrome has instead? I think that'd
> be
> > even better... since RWHVW is a UI-related class too. I think power
> > broadcast should move there too.
> > -Ben
> >
> > On Thu, Nov 10, 2011 at 9:13 PM, <scottmg@chromium.org> wrote:
> >>
> >> On 2011/11/11 04:30:31, Ben Goodger (Google) wrote:
> >>>
> >>> I don't think NWW is the right place for this. If this is intended to
> >>> support
> >>
> >> a
> >>>
> >>> web api, how do you suppose it'll be hooked up in Content Shell?
> >>
> >>> Seems like RWHVW may be a better location, though for something low
> level
> >>> and
> >>> not really UI related like this, it makes me wonder if we should have a
> >>> msg
> >>> window somewhere in Chrome that responds to system notifications like
> >>> this.
> >>
> >> OK thanks, I was following the handling of WM_POWERBROADCAST which
> seemed
> >> similar. I'll move the hookup to RenderWidgetHostViewWin.
> >>
> >> http://codereview.chromium.org/8523021/
> >
> >
>

[scottmg, 2011-11-11 17:57:25]

On 2011/11/11 17:15:17, Ben Goodger (Google) wrote:
> I think that may in fact be the window we're thinking of.

After much single-stepping, that top-level ProcessSingleton does not process
messages during the main run loop (it's some sort of hack to pass data between
re-invocations).

The top-level NativeBrowserFrame's WndProc is NativeWidgetWin's, hence I assume,
why POWERBROADCAST was put in there in the first place.

So... are we back to putting it in NWW or RWHVW? I'm a bit confused now. :)

[Ben Goodger (Google), 2011-11-11 18:06:11]

Seems like we should make a new message window then. Seems also like it
should probably live in content.

-Ben

On Fri, Nov 11, 2011 at 9:57 AM, <scottmg@chromium.org> wrote:

> On 2011/11/11 17:15:17, Ben Goodger (Google) wrote:
>
>> I think that may in fact be the window we're thinking of.
>>
>
> After much single-stepping, that top-level ProcessSingleton does not
> process
> messages during the main run loop (it's some sort of hack to pass data
> between
> re-invocations).
>
> The top-level NativeBrowserFrame's WndProc is NativeWidgetWin's, hence I
> assume,
> why POWERBROADCAST was put in there in the first place.
>
> So... are we back to putting it in NWW or RWHVW? I'm a bit confused now. :)
>
>
http://codereview.chromium.**org/8523021/<http://codereview.chromium.org/8523...
>

[willchan no longer on Chromium, 2011-11-11 19:05:27]

base/ and net/ LGTM

On 2011/11/11 18:06:11, Ben Goodger (Google) wrote:
> Seems like we should make a new message window then. Seems also like it
> should probably live in content.
> 
> -Ben
> 
> On Fri, Nov 11, 2011 at 9:57 AM, <mailto:scottmg@chromium.org> wrote:
> 
> > On 2011/11/11 17:15:17, Ben Goodger (Google) wrote:
> >
> >> I think that may in fact be the window we're thinking of.
> >>
> >
> > After much single-stepping, that top-level ProcessSingleton does not
> > process
> > messages during the main run loop (it's some sort of hack to pass data
> > between
> > re-invocations).
> >
> > The top-level NativeBrowserFrame's WndProc is NativeWidgetWin's, hence I
> > assume,
> > why POWERBROADCAST was put in there in the first place.
> >
> > So... are we back to putting it in NWW or RWHVW? I'm a bit confused now. :)
> >
> >
>
http://codereview.chromium.**org/8523021/%3Chttp://codereview.chromium.org/85...>
> >

[scottmg, 2011-11-11 19:36:15]

On 2011/11/11 18:06:11, Ben Goodger (Google) wrote:
> Seems like we should make a new message window then. Seems also like it
> should probably live in content.

Could anyone comment on where this should happen?

My proposal would be a new window class under content/browser/ that gets created
in PreMainMessageLoop of ShellBrowserMainParts and ChromeBrowserMainPartsWin?

(Maybe "browser_system_message_window_win"? ugh)

[Ben Goodger (Google), 2011-11-11 20:04:07]

+jam@

[jam, 2011-11-11 20:32:53]

On 2011/11/11 19:36:15, scottmg wrote:
> On 2011/11/11 18:06:11, Ben Goodger (Google) wrote:
> > Seems like we should make a new message window then. Seems also like it
> > should probably live in content.
> 
> Could anyone comment on where this should happen?
> 
> My proposal would be a new window class under content/browser/

sgtm

> that gets created
> in PreMainMessageLoop of ShellBrowserMainParts and ChromeBrowserMainPartsWin?
> 
> (Maybe "browser_system_message_window_win"? ugh)

better to put it in one place that all embedders get for free, i.e. in
BrowserMainLoop::MainMessageLoopStart  after the
"network_change_notifier_.reset(net::NetworkChangeNotifier::Create());"

[scottmg, 2011-11-11 21:46:54]

+thestig, +jschuh for nearby changes

Hmm, I was wrong about ProcessSingleton.

The reason it's not processing broadcast messages (i.e. WM_DEVICECHANGE) is
because it's a message-only window.
http://msdn.microsoft.com/en-us/library/windows/desktop/ms632599(v=vs.85).asp...

ben, thestig, jschuh: it looks you touched the related
CreateWindow/FindWindowEx calls most recently. Does it seem like a
reasonable idea to make ProcessSingleton's window a normal invisible
top-level window, rather than HWND_MESSAGE? Or better to add a new HWND
elsewhere to handle these messages?

On Fri, Nov 11, 2011 at 12:32 PM, <jam@chromium.org> wrote:

> On 2011/11/11 19:36:15, scottmg wrote:
>
>> On 2011/11/11 18:06:11, Ben Goodger (Google) wrote:
>> > Seems like we should make a new message window then. Seems also like it
>> > should probably live in content.
>>
>
>  Could anyone comment on where this should happen?
>>
>
>  My proposal would be a new window class under content/browser/
>>
>
> sgtm
>
>
>  that gets created
>> in PreMainMessageLoop of ShellBrowserMainParts and
>> ChromeBrowserMainPartsWin?
>>
>
>  (Maybe "browser_system_message_**window_win"? ugh)
>>
>
> better to put it in one place that all embedders get for free, i.e. in
> BrowserMainLoop::**MainMessageLoopStart  after the
> "network_change_notifier_.**reset(net::**NetworkChangeNotifier::Create(**
> ));"
>
>
http://codereview.chromium.**org/8523021/<http://codereview.chromium.org/8523...
>

[Ben Goodger (Google), 2011-11-11 21:51:48]

Probably better to add a new HWND inside content, since it sounds like your
code affects web platform stuff.

-Ben

On Fri, Nov 11, 2011 at 1:46 PM, Scott Graham <scottmg@chromium.org> wrote:

> +thestig, +jschuh for nearby changes
>
> Hmm, I was wrong about ProcessSingleton.
>
> The reason it's not processing broadcast messages (i.e. WM_DEVICECHANGE)
> is because it's a message-only window.
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms632599(v=vs.85).asp...
>
> ben, thestig, jschuh: it looks you touched the related
> CreateWindow/FindWindowEx calls most recently. Does it seem like a
> reasonable idea to make ProcessSingleton's window a normal invisible
> top-level window, rather than HWND_MESSAGE? Or better to add a new HWND
> elsewhere to handle these messages?
>
> On Fri, Nov 11, 2011 at 12:32 PM, <jam@chromium.org> wrote:
>
>> On 2011/11/11 19:36:15, scottmg wrote:
>>
>>> On 2011/11/11 18:06:11, Ben Goodger (Google) wrote:
>>> > Seems like we should make a new message window then. Seems also like it
>>> > should probably live in content.
>>>
>>
>>  Could anyone comment on where this should happen?
>>>
>>
>>  My proposal would be a new window class under content/browser/
>>>
>>
>> sgtm
>>
>>
>>  that gets created
>>> in PreMainMessageLoop of ShellBrowserMainParts and
>>> ChromeBrowserMainPartsWin?
>>>
>>
>>  (Maybe "browser_system_message_**window_win"? ugh)
>>>
>>
>> better to put it in one place that all embedders get for free, i.e. in
>> BrowserMainLoop::**MainMessageLoopStart  after the
>> "network_change_notifier_.**reset(net::**NetworkChangeNotifier::Create(**
>> ));"
>>
>>
http://codereview.chromium.**org/8523021/<http://codereview.chromium.org/8523...
>>
>
>

[scottmg, 2011-11-14 17:45:00]

jam@ or sky@, does this seem OK to you for the content/ changes?

thanks

[jam, 2011-11-14 18:48:13]

content lgtm

http://codereview.chromium.org/8523021/diff/10012/content/browser/system_mess...
File content/browser/system_message_window_win.h (right):

http://codereview.chromium.org/8523021/diff/10012/content/browser/system_mess...
content/browser/system_message_window_win.h:11: #if defined(OS_WIN)
better to include this file only for windows, and then you odn't need this
OS_WIN in the file at all (since it ends in _win after all)

[commit-bot: I haz the power, 2011-11-14 20:47:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/8523021/17001

[commit-bot: I haz the power, 2011-11-14 22:27:08]

Change committed as 109960

[commit-bot: I haz the power, 2011-11-15 00:42:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/8523021/17001

[commit-bot: I haz the power, 2011-11-15 01:59:51]

Change committed as 110014