Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
566243005:
Cast: Allow extension to control wifi options on windows (Closed)
|
Created:
6 years, 3 months ago by hubbe Modified:
6 years, 3 months ago CC:
chromium-reviews, hclam+watch_chromium.org, pwestin+watch_google.com, imcheng+watch_chromium.org, hguihot+watch_chromium.org, jasonroberts+watch_google.com, avayvod+watch_chromium.org, gavinp+memory_chromium.org, cbentzel+watch_chromium.org, miu+watch_chromium.org, feature-media-reviews_chromium.org, erikwright+watch_chromium.org, hubbe+watch_chromium.org, mikhal+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionCast: Allow extension to control wifi options on windows
This allows the cast extension to turn off wifi scan while
streaming is in progress, and also set the "media streaming mode"
further experimentation is need to see if these options are actually
useful.
BUG=410500
Committed: https://crrev.com/693959c4726d968b0396345918a84bc284aea510
Cr-Commit-Position: refs/heads/master@{#296253}
Patch Set 1 #
Total comments: 4
Patch Set 2 : comments addressed #Patch Set 3 : posix fix #Patch Set 4 : posix fixed again #
Total comments: 16
Patch Set 5 : added tests #Patch Set 6 : bugfix #Patch Set 7 : less base #
Total comments: 14
Patch Set 8 : bitified #Patch Set 9 : (most) comments addressed #Patch Set 10 : test improved #Patch Set 11 : posix fix #Patch Set 12 : add missing NET_EXPORT #
Total comments: 18
Patch Set 13 : comments addressed #Patch Set 14 : comments addressed #
Total comments: 4
Patch Set 15 : comments addressed #
Messages
Total messages: 31 (4 generated)
hubbe@chromium.org changed reviewers: + hclam@chromium.org
Please add a base/ review to review the introduction of LifetimeInterface. https://codereview.chromium.org/566243005/diff/1/media/cast/net/cast_transpor... File media/cast/net/cast_transport_sender_impl.cc (right): https://codereview.chromium.org/566243005/diff/1/media/cast/net/cast_transpor... media/cast/net/cast_transport_sender_impl.cc:90: if (options->HasKey("DISABLE_WIFI_SCAN")) { Note that the pacer options are using lowercase name for the key. These should probably be lower case to be consistent. https://codereview.chromium.org/566243005/diff/1/media/cast/net/cast_transpor... File media/cast/net/cast_transport_sender_impl.h (right): https://codereview.chromium.org/566243005/diff/1/media/cast/net/cast_transpor... media/cast/net/cast_transport_sender_impl.h:62: scoped_ptr<base::DictionaryValue> options, Need to document the new options.
https://codereview.chromium.org/566243005/diff/1/media/cast/net/cast_transpor... File media/cast/net/cast_transport_sender_impl.cc (right): https://codereview.chromium.org/566243005/diff/1/media/cast/net/cast_transpor... media/cast/net/cast_transport_sender_impl.cc:90: if (options->HasKey("DISABLE_WIFI_SCAN")) { On 2014/09/15 20:12:29, Alpha wrote: > Note that the pacer options are using lowercase name for the key. These should > probably be lower case to be consistent. Done. https://codereview.chromium.org/566243005/diff/1/media/cast/net/cast_transpor... File media/cast/net/cast_transport_sender_impl.h (right): https://codereview.chromium.org/566243005/diff/1/media/cast/net/cast_transpor... media/cast/net/cast_transport_sender_impl.h:62: scoped_ptr<base::DictionaryValue> options, On 2014/09/15 20:12:29, Alpha wrote: > Need to document the new options. Done.
hubbe@chromium.org changed reviewers: + ajwong@chromium.org, pauljensen@chromium.org
+ pauljensen for net/base/* + ajwong for base/memory/*
1. This fails to build. 2. This needs a test. Perhaps set the innocuous looking disable-scan option temporarily and verify in the test that it actually gets set and reset. https://codereview.chromium.org/566243005/diff/60001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/566243005/diff/60001/net/base/net_util.h#newc... net/base/net_util.h:509: // Set temporary options on all interfaces. Can you clarify what interfaces is? WiFi network interfaces? https://codereview.chromium.org/566243005/diff/60001/net/base/net_util.h#newc... net/base/net_util.h:513: scoped_ptr<base::LifetimeInterface> SetWifiOptions(int options); Shouldn't this be NET_EXPORT? https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc File net/base/net_util_win.cc (right): https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc... net/base/net_util_win.cc:224: namespace { I think moving this up into the anonymous namespace at the top of the file might be cleaner. https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc... net/base/net_util_win.cc:252: }; // namespace https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc... net/base/net_util_win.cc:319: WifiOptionSetter(int options) { indent https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc... net/base/net_util_win.cc:325: const DWORD kMaxClientVersion = 2; Please add a comment about where this 0 and 2 come from and imply. https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc... net/base/net_util_win.cc:338: // Assume at most one connected wifi interface. "at most one"? This function appears to loop through all interfaces. The comment for SetWifiOptions also says all interfaces. https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc... net/base/net_util_win.cc:361: Maybe I'm missing something, but shouldn't there be a destructor here the resets these options back?
1. Should hopefully compile now (trybots are slow, but I will of course make sure it compiles everywhere.) 2. Proper tests are difficult to make since this only operates on wifi network interfaces, and most test machines won't have any of those. However, I added tests that run through as much of the code as possible, and did some more manual testing. https://codereview.chromium.org/566243005/diff/60001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/566243005/diff/60001/net/base/net_util.h#newc... net/base/net_util.h:509: // Set temporary options on all interfaces. On 2014/09/16 15:01:18, pauljensen wrote: > Can you clarify what interfaces is? WiFi network interfaces? Done. https://codereview.chromium.org/566243005/diff/60001/net/base/net_util.h#newc... net/base/net_util.h:513: scoped_ptr<base::LifetimeInterface> SetWifiOptions(int options); On 2014/09/16 15:01:18, pauljensen wrote: > Shouldn't this be NET_EXPORT? Yes, yes it should. (Done) https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc File net/base/net_util_win.cc (right): https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc... net/base/net_util_win.cc:224: namespace { On 2014/09/16 15:01:18, pauljensen wrote: > I think moving this up into the anonymous namespace at the top of the file might > be cleaner. Done. https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc... net/base/net_util_win.cc:252: }; On 2014/09/16 15:01:18, pauljensen wrote: > // namespace No longer relevant. https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc... net/base/net_util_win.cc:319: WifiOptionSetter(int options) { On 2014/09/16 15:01:18, pauljensen wrote: > indent Done (finally configured emacs on my windows box, yay!) https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc... net/base/net_util_win.cc:325: const DWORD kMaxClientVersion = 2; On 2014/09/16 15:01:18, pauljensen wrote: > Please add a comment about where this 0 and 2 come from and imply. Gladly, but I don't know. I copied this from line 252 https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc... net/base/net_util_win.cc:338: // Assume at most one connected wifi interface. On 2014/09/16 15:01:18, pauljensen wrote: > "at most one"? This function appears to loop through all interfaces. The > comment for SetWifiOptions also says all interfaces. Oops, removed. https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc... net/base/net_util_win.cc:361: On 2014/09/16 15:01:18, pauljensen wrote: > Maybe I'm missing something, but shouldn't there be a destructor here the resets > these options back? If I'm reading the documentation correctly, the OS sets these options only as long as some client wants them. This means that when the client handle is closed, the options are reset. I also tested this manually to make sure and added a comment to make it less confusing.
This is an extremely generic interface that effectively creating a void*. I'm not sure why you don't just put this into net_util.h? -Albert On 2014/09/16 21:06:51, hubbe wrote: > 1. Should hopefully compile now (trybots are slow, but I will of course make > sure it compiles everywhere.) > 2. Proper tests are difficult to make since this only operates on wifi network > interfaces, and most test machines won't have any of those. However, I added > tests that run through as much of the code as possible, and did some more manual > testing. > > https://codereview.chromium.org/566243005/diff/60001/net/base/net_util.h > File net/base/net_util.h (right): > > https://codereview.chromium.org/566243005/diff/60001/net/base/net_util.h#newc... > net/base/net_util.h:509: // Set temporary options on all interfaces. > On 2014/09/16 15:01:18, pauljensen wrote: > > Can you clarify what interfaces is? WiFi network interfaces? > > Done. > > https://codereview.chromium.org/566243005/diff/60001/net/base/net_util.h#newc... > net/base/net_util.h:513: scoped_ptr<base::LifetimeInterface> SetWifiOptions(int > options); > On 2014/09/16 15:01:18, pauljensen wrote: > > Shouldn't this be NET_EXPORT? > > Yes, yes it should. (Done) > > https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc > File net/base/net_util_win.cc (right): > > https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc... > net/base/net_util_win.cc:224: namespace { > On 2014/09/16 15:01:18, pauljensen wrote: > > I think moving this up into the anonymous namespace at the top of the file > might > > be cleaner. > > Done. > > https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc... > net/base/net_util_win.cc:252: }; > On 2014/09/16 15:01:18, pauljensen wrote: > > // namespace > > No longer relevant. > > https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc... > net/base/net_util_win.cc:319: WifiOptionSetter(int options) { > On 2014/09/16 15:01:18, pauljensen wrote: > > indent > > Done (finally configured emacs on my windows box, yay!) > > https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc... > net/base/net_util_win.cc:325: const DWORD kMaxClientVersion = 2; > On 2014/09/16 15:01:18, pauljensen wrote: > > Please add a comment about where this 0 and 2 come from and imply. > > Gladly, but I don't know. I copied this from line 252 > > https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc... > net/base/net_util_win.cc:338: // Assume at most one connected wifi interface. > On 2014/09/16 15:01:18, pauljensen wrote: > > "at most one"? This function appears to loop through all interfaces. The > > comment for SetWifiOptions also says all interfaces. > > Oops, removed. > > https://codereview.chromium.org/566243005/diff/60001/net/base/net_util_win.cc... > net/base/net_util_win.cc:361: > On 2014/09/16 15:01:18, pauljensen wrote: > > Maybe I'm missing something, but shouldn't there be a destructor here the > resets > > these options back? > > If I'm reading the documentation correctly, the OS sets these options only as > long as some client wants them. This means that when the client handle is > closed, the options are reset. > > I also tested this manually to make sure and added a comment to make it less > confusing.
As I wrote the code, I noticed that I was re-creating a class that I've written a couple of times before. Here is another one: https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/test/ut... For documentation purposes it might actually be better to create a new base class for each such instance, but re-use is good too. I can go either way on this, but decided to get a second opinion. (Yes, that's you.) I'm not sure why you think it's a void* though. It's more like a closure. (In fact, I could use a closure....) /Hubbe On Tue, Sep 16, 2014 at 2:51 PM, <ajwong@chromium.org> wrote: > This is an extremely generic interface that effectively creating a void*. > > I'm not sure why you don't just put this into net_util.h? > > -Albert > > > On 2014/09/16 21:06:51, hubbe wrote: > >> 1. Should hopefully compile now (trybots are slow, but I will of course >> make >> sure it compiles everywhere.) >> 2. Proper tests are difficult to make since this only operates on wifi >> network >> interfaces, and most test machines won't have any of those. However, I >> added >> tests that run through as much of the code as possible, and did some more >> > manual > >> testing. >> > > https://codereview.chromium.org/566243005/diff/60001/net/base/net_util.h >> File net/base/net_util.h (right): >> > > > https://codereview.chromium.org/566243005/diff/60001/net/ > base/net_util.h#newcode509 > >> net/base/net_util.h:509: // Set temporary options on all interfaces. >> On 2014/09/16 15:01:18, pauljensen wrote: >> > Can you clarify what interfaces is? WiFi network interfaces? >> > > Done. >> > > > https://codereview.chromium.org/566243005/diff/60001/net/ > base/net_util.h#newcode513 > >> net/base/net_util.h:513: scoped_ptr<base::LifetimeInterface> >> > SetWifiOptions(int > >> options); >> On 2014/09/16 15:01:18, pauljensen wrote: >> > Shouldn't this be NET_EXPORT? >> > > Yes, yes it should. (Done) >> > > https://codereview.chromium.org/566243005/diff/60001/net/ >> base/net_util_win.cc >> File net/base/net_util_win.cc (right): >> > > > https://codereview.chromium.org/566243005/diff/60001/net/ > base/net_util_win.cc#newcode224 > >> net/base/net_util_win.cc:224: namespace { >> On 2014/09/16 15:01:18, pauljensen wrote: >> > I think moving this up into the anonymous namespace at the top of the >> file >> might >> > be cleaner. >> > > Done. >> > > > https://codereview.chromium.org/566243005/diff/60001/net/ > base/net_util_win.cc#newcode252 > >> net/base/net_util_win.cc:252: }; >> On 2014/09/16 15:01:18, pauljensen wrote: >> > // namespace >> > > No longer relevant. >> > > > https://codereview.chromium.org/566243005/diff/60001/net/ > base/net_util_win.cc#newcode319 > >> net/base/net_util_win.cc:319: WifiOptionSetter(int options) { >> On 2014/09/16 15:01:18, pauljensen wrote: >> > indent >> > > Done (finally configured emacs on my windows box, yay!) >> > > > https://codereview.chromium.org/566243005/diff/60001/net/ > base/net_util_win.cc#newcode325 > >> net/base/net_util_win.cc:325: const DWORD kMaxClientVersion = 2; >> On 2014/09/16 15:01:18, pauljensen wrote: >> > Please add a comment about where this 0 and 2 come from and imply. >> > > Gladly, but I don't know. I copied this from line 252 >> > > > https://codereview.chromium.org/566243005/diff/60001/net/ > base/net_util_win.cc#newcode338 > >> net/base/net_util_win.cc:338: // Assume at most one connected wifi >> interface. >> On 2014/09/16 15:01:18, pauljensen wrote: >> > "at most one"? This function appears to loop through all interfaces. >> The >> > comment for SetWifiOptions also says all interfaces. >> > > Oops, removed. >> > > > https://codereview.chromium.org/566243005/diff/60001/net/ > base/net_util_win.cc#newcode361 > >> net/base/net_util_win.cc:361: >> On 2014/09/16 15:01:18, pauljensen wrote: >> > Maybe I'm missing something, but shouldn't there be a destructor here >> the >> resets >> > these options back? >> > > If I'm reading the documentation correctly, the OS sets these options >> only as >> long as some client wants them. This means that when the client handle is >> closed, the options are reset. >> > > I also tested this manually to make sure and added a comment to make it >> less >> confusing. >> > > > > https://codereview.chromium.org/566243005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I actually think a new base class makes more sense. Ignore the void* comment. Functionally, you're attempting to inject a vtable for the destructor. By creating one common type though, there's a sort-of implicit statement in the type system that the subtypes are related which is only barely true. I think it makes more sense to leave this as a per module thing and not put it in base. -Albert On Tue, Sep 16, 2014 at 4:22 PM, Fredrik Hubinette <hubbe@google.com> wrote: > As I wrote the code, I noticed that I was re-creating a class that I've > written a couple of times before. > Here is another one: > https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/test/ut... > > For documentation purposes it might actually be better to create a new > base class for each such instance, but re-use is good too. I can go either > way on this, but decided to get a second opinion. (Yes, that's you.) > > I'm not sure why you think it's a void* though. It's more like a closure. > (In fact, I could use a closure....) > > /Hubbe > > On Tue, Sep 16, 2014 at 2:51 PM, <ajwong@chromium.org> wrote: > >> This is an extremely generic interface that effectively creating a void*. >> >> I'm not sure why you don't just put this into net_util.h? >> >> -Albert >> >> >> On 2014/09/16 21:06:51, hubbe wrote: >> >>> 1. Should hopefully compile now (trybots are slow, but I will of course >>> make >>> sure it compiles everywhere.) >>> 2. Proper tests are difficult to make since this only operates on wifi >>> network >>> interfaces, and most test machines won't have any of those. However, I >>> added >>> tests that run through as much of the code as possible, and did some more >>> >> manual >> >>> testing. >>> >> >> https://codereview.chromium.org/566243005/diff/60001/net/base/net_util.h >>> File net/base/net_util.h (right): >>> >> >> >> https://codereview.chromium.org/566243005/diff/60001/net/ >> base/net_util.h#newcode509 >> >>> net/base/net_util.h:509: // Set temporary options on all interfaces. >>> On 2014/09/16 15:01:18, pauljensen wrote: >>> > Can you clarify what interfaces is? WiFi network interfaces? >>> >> >> Done. >>> >> >> >> https://codereview.chromium.org/566243005/diff/60001/net/ >> base/net_util.h#newcode513 >> >>> net/base/net_util.h:513: scoped_ptr<base::LifetimeInterface> >>> >> SetWifiOptions(int >> >>> options); >>> On 2014/09/16 15:01:18, pauljensen wrote: >>> > Shouldn't this be NET_EXPORT? >>> >> >> Yes, yes it should. (Done) >>> >> >> https://codereview.chromium.org/566243005/diff/60001/net/ >>> base/net_util_win.cc >>> File net/base/net_util_win.cc (right): >>> >> >> >> https://codereview.chromium.org/566243005/diff/60001/net/ >> base/net_util_win.cc#newcode224 >> >>> net/base/net_util_win.cc:224: namespace { >>> On 2014/09/16 15:01:18, pauljensen wrote: >>> > I think moving this up into the anonymous namespace at the top of the >>> file >>> might >>> > be cleaner. >>> >> >> Done. >>> >> >> >> https://codereview.chromium.org/566243005/diff/60001/net/ >> base/net_util_win.cc#newcode252 >> >>> net/base/net_util_win.cc:252: }; >>> On 2014/09/16 15:01:18, pauljensen wrote: >>> > // namespace >>> >> >> No longer relevant. >>> >> >> >> https://codereview.chromium.org/566243005/diff/60001/net/ >> base/net_util_win.cc#newcode319 >> >>> net/base/net_util_win.cc:319: WifiOptionSetter(int options) { >>> On 2014/09/16 15:01:18, pauljensen wrote: >>> > indent >>> >> >> Done (finally configured emacs on my windows box, yay!) >>> >> >> >> https://codereview.chromium.org/566243005/diff/60001/net/ >> base/net_util_win.cc#newcode325 >> >>> net/base/net_util_win.cc:325: const DWORD kMaxClientVersion = 2; >>> On 2014/09/16 15:01:18, pauljensen wrote: >>> > Please add a comment about where this 0 and 2 come from and imply. >>> >> >> Gladly, but I don't know. I copied this from line 252 >>> >> >> >> https://codereview.chromium.org/566243005/diff/60001/net/ >> base/net_util_win.cc#newcode338 >> >>> net/base/net_util_win.cc:338: // Assume at most one connected wifi >>> interface. >>> On 2014/09/16 15:01:18, pauljensen wrote: >>> > "at most one"? This function appears to loop through all interfaces. >>> The >>> > comment for SetWifiOptions also says all interfaces. >>> >> >> Oops, removed. >>> >> >> >> https://codereview.chromium.org/566243005/diff/60001/net/ >> base/net_util_win.cc#newcode361 >> >>> net/base/net_util_win.cc:361: >>> On 2014/09/16 15:01:18, pauljensen wrote: >>> > Maybe I'm missing something, but shouldn't there be a destructor here >>> the >>> resets >>> > these options back? >>> >> >> If I'm reading the documentation correctly, the OS sets these options >>> only as >>> long as some client wants them. This means that when the client handle is >>> closed, the options are reset. >>> >> >> I also tested this manually to make sure and added a comment to make it >>> less >>> confusing. >>> >> >> >> >> https://codereview.chromium.org/566243005/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
hubbe@chromium.org changed reviewers: - ajwong@chromium.org
-ajwong pauljensen: ping?
media/cast LGTM.
https://codereview.chromium.org/566243005/diff/120001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/566243005/diff/120001/net/base/net_util.h#new... net/base/net_util.h:511: }; private: DISALLOW_COPY_AND_ASSIGN(ScopedWifiOptions); https://codereview.chromium.org/566243005/diff/120001/net/base/net_util_unitt... File net/base/net_util_unittest.cc (right): https://codereview.chromium.org/566243005/diff/120001/net/base/net_util_unitt... net/base/net_util_unittest.cc:799: WIFI_OPTIONS_MEDIA_STREAMING_MODE); I assume you can verify that these functions are taking effect and cleaning up after themselves with WlanQueryInterface. Can you please add that verification here? https://codereview.chromium.org/566243005/diff/120001/net/base/net_util_win.cc File net/base/net_util_win.cc (right): https://codereview.chromium.org/566243005/diff/120001/net/base/net_util_win.c... net/base/net_util_win.cc:118: ERROR_SUCCESS; Please put this on the previous line. https://codereview.chromium.org/566243005/diff/120001/net/base/net_util_win.c... net/base/net_util_win.cc:127: Remove the extra line. https://codereview.chromium.org/566243005/diff/120001/net/base/net_util_win.c... net/base/net_util_win.cc:368: new WifiOptionSetter(options)); Please put this on the previous line.
https://codereview.chromium.org/566243005/diff/120001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/566243005/diff/120001/net/base/net_util.h#new... net/base/net_util.h:506: }; Since these should be ORed together, I suggest: 1->1 << 0 2->1 << 1 Otherwise I can see someone adding "blah = 3" and wondering why everything fails.
https://codereview.chromium.org/566243005/diff/120001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/566243005/diff/120001/net/base/net_util.h#new... net/base/net_util.h:506: }; On 2014/09/18 14:25:35, pauljensen wrote: > Since these should be ORed together, I suggest: > 1->1 << 0 > 2->1 << 1 > Otherwise I can see someone adding "blah = 3" and wondering why everything > fails. Done. https://codereview.chromium.org/566243005/diff/120001/net/base/net_util.h#new... net/base/net_util.h:511: }; On 2014/09/18 14:16:50, pauljensen wrote: > private: > DISALLOW_COPY_AND_ASSIGN(ScopedWifiOptions); Done. https://codereview.chromium.org/566243005/diff/120001/net/base/net_util_unitt... File net/base/net_util_unittest.cc (right): https://codereview.chromium.org/566243005/diff/120001/net/base/net_util_unitt... net/base/net_util_unittest.cc:799: WIFI_OPTIONS_MEDIA_STREAMING_MODE); On 2014/09/18 14:16:50, pauljensen wrote: > I assume you can verify that these functions are taking effect and cleaning up > after themselves with WlanQueryInterface. Can you please add that verification > here? It's possible, but is it worth it? I'll have to expose WlanQueryInterface from inside net_util_win.h, or duplicate the code here, and then it will only ever do something if the computer you run the test on actually has a wifi interface. (Which I don't think any of our test bots have.) AND, to top it all off, it's possible to configure your window ACLs such that calling SetWifiOptions always fails unless you're administrator.... (Although I'm not sure if anybody anywhere actually does that.) If you still think it's worth it, do you prefer that I expose the wlanapi, or duplicate the code? https://codereview.chromium.org/566243005/diff/120001/net/base/net_util_win.cc File net/base/net_util_win.cc (right): https://codereview.chromium.org/566243005/diff/120001/net/base/net_util_win.c... net/base/net_util_win.cc:118: ERROR_SUCCESS; On 2014/09/18 14:16:50, pauljensen wrote: > Please put this on the previous line. Done. https://codereview.chromium.org/566243005/diff/120001/net/base/net_util_win.c... net/base/net_util_win.cc:127: On 2014/09/18 14:16:50, pauljensen wrote: > Remove the extra line. Done. https://codereview.chromium.org/566243005/diff/120001/net/base/net_util_win.c... net/base/net_util_win.cc:368: new WifiOptionSetter(options)); On 2014/09/18 14:16:50, pauljensen wrote: > Please put this on the previous line. Done.
https://codereview.chromium.org/566243005/diff/120001/net/base/net_util_unitt... File net/base/net_util_unittest.cc (right): https://codereview.chromium.org/566243005/diff/120001/net/base/net_util_unitt... net/base/net_util_unittest.cc:799: WIFI_OPTIONS_MEDIA_STREAMING_MODE); On 2014/09/18 18:21:05, hubbe wrote: > It's possible, but is it worth it? > > I'll have to expose WlanQueryInterface from inside net_util_win.h, or duplicate > the code here, and then it will only ever do something if the computer you run > the test on actually has a wifi interface. > (Which I don't think any of our test bots have.) > > AND, to top it all off, it's possible to configure your window ACLs such that > calling SetWifiOptions always fails unless you're administrator.... (Although > I'm not sure if anybody anywhere actually does that.) > > If you still think it's worth it, do you prefer that I expose the wlanapi, or > duplicate the code? I believe it is worth adding a test when adding a new API and implementation. If there is value in adding the functionality, there is value in ensuring that functionality works now and continues to work. It's unfortunate that most/all of our bots don't have Wifi; which begs the question, how is the Cast extension tested? i.e. how will you know this new API works? Exposing the wlanapi just to tests seems fine.
Ok, tests have been improved to actually check that things are reset. I ran the test on a machine with a wifi interface to make sure they work. https://codereview.chromium.org/566243005/diff/120001/net/base/net_util_unitt... File net/base/net_util_unittest.cc (right): https://codereview.chromium.org/566243005/diff/120001/net/base/net_util_unitt... net/base/net_util_unittest.cc:799: WIFI_OPTIONS_MEDIA_STREAMING_MODE); On 2014/09/18 18:40:20, pauljensen wrote: > On 2014/09/18 18:21:05, hubbe wrote: > > It's possible, but is it worth it? > > > > I'll have to expose WlanQueryInterface from inside net_util_win.h, or > duplicate > > the code here, and then it will only ever do something if the computer you run > > the test on actually has a wifi interface. > > (Which I don't think any of our test bots have.) > > > > AND, to top it all off, it's possible to configure your window ACLs such that > > calling SetWifiOptions always fails unless you're administrator.... (Although > > I'm not sure if anybody anywhere actually does that.) > > > > If you still think it's worth it, do you prefer that I expose the wlanapi, or > > duplicate the code? > I believe it is worth adding a test when adding a new API and implementation. > If there is value in adding the functionality, there is value in ensuring that > functionality works now and continues to work. It's unfortunate that most/all > of our bots don't have Wifi; which begs the question, how is the Cast extension > tested? i.e. how will you know this new API works? > > Exposing the wlanapi just to tests seems fine. Done.
Fails to build. Please get it building and I'll take another look.
On 2014/09/19 15:05:08, pauljensen wrote: > Fails to build. Please get it building and I'll take another look. Now builds, PTAL.
This is looking pretty good. I assume you verified that in your testing that GetWifiOptions() was returning something other than -1? https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_unitt... File net/base/net_util_unittest.cc (right): https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_unitt... net/base/net_util_unittest.cc:796: #if OS_WIN defined(OS_WIN) https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_unitt... net/base/net_util_unittest.cc:882: unnecessary lines https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_unitt... net/base/net_util_unittest.cc:891: } need a new line https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_unitt... net/base/net_util_unittest.cc:892: }; // namespace https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_unitt... net/base/net_util_unittest.cc:903: unnecessary line https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_win.cc File net/base/net_util_win.cc (right): https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_win.c... net/base/net_util_win.cc:158: prefix->PrefixLength); unnecessary change? https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_win.c... net/base/net_util_win.cc:264: // is closed. I couldn't find any documentation for this. Have you seen any? https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_win.h File net/base/net_util_win.h (right): https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_win.h... net/base/net_util_win.h:8: // This file is only used for testing. This is not accurate as this file is used by net_util_win.cc for non-testing purposes. https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_win.h... net/base/net_util_win.h:15: namespace net { this should also be within "namespace internal {"
PTAL https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_unitt... File net/base/net_util_unittest.cc (right): https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_unitt... net/base/net_util_unittest.cc:796: #if OS_WIN On 2014/09/23 14:03:54, pauljensen wrote: > defined(OS_WIN) Done. https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_unitt... net/base/net_util_unittest.cc:882: On 2014/09/23 14:03:55, pauljensen wrote: > unnecessary lines Done. https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_unitt... net/base/net_util_unittest.cc:891: } On 2014/09/23 14:03:54, pauljensen wrote: > need a new line Done. https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_unitt... net/base/net_util_unittest.cc:892: }; On 2014/09/23 14:03:55, pauljensen wrote: > // namespace Done. https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_unitt... net/base/net_util_unittest.cc:903: On 2014/09/23 14:03:54, pauljensen wrote: > unnecessary line Done. https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_win.cc File net/base/net_util_win.cc (right): https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_win.c... net/base/net_util_win.cc:158: prefix->PrefixLength); On 2014/09/23 14:03:55, pauljensen wrote: > unnecessary change? Done. https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_win.c... net/base/net_util_win.cc:264: // is closed. On 2014/09/23 14:03:55, pauljensen wrote: > I couldn't find any documentation for this. Have you seen any? It's very poorly documented, but the documentation for wlan_intf_opcode_background_scan_enabled and wlan_intf_opcode_media_streaming_mode (found here: http://msdn.microsoft.com/en-us/library/windows/desktop/ms706886(v=vs.85).aspx) hints at how it actually works, it just doesn't call it out explicitly. https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_win.h File net/base/net_util_win.h (right): https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_win.h... net/base/net_util_win.h:8: // This file is only used for testing. On 2014/09/23 14:03:55, pauljensen wrote: > This is not accurate as this file is used by net_util_win.cc for non-testing > purposes. Comment updated. https://codereview.chromium.org/566243005/diff/220001/net/base/net_util_win.h... net/base/net_util_win.h:15: namespace net { On 2014/09/23 14:03:55, pauljensen wrote: > this should also be within "namespace internal {" Done
https://codereview.chromium.org/566243005/diff/260001/net/base/net_util_unitt... File net/base/net_util_unittest.cc (right): https://codereview.chromium.org/566243005/diff/260001/net/base/net_util_unitt... net/base/net_util_unittest.cc:35: using namespace net::internal; "using namespace" is strictly forbidden in the style guide. https://codereview.chromium.org/566243005/diff/260001/net/base/net_util_win.cc File net/base/net_util_win.cc (right): https://codereview.chromium.org/566243005/diff/260001/net/base/net_util_win.c... net/base/net_util_win.cc:28: using namespace net::internal; ditto
PTAL https://codereview.chromium.org/566243005/diff/260001/net/base/net_util_unitt... File net/base/net_util_unittest.cc (right): https://codereview.chromium.org/566243005/diff/260001/net/base/net_util_unitt... net/base/net_util_unittest.cc:35: using namespace net::internal; On 2014/09/23 17:59:03, pauljensen wrote: > "using namespace" is strictly forbidden in the style guide. Done. https://codereview.chromium.org/566243005/diff/260001/net/base/net_util_win.cc File net/base/net_util_win.cc (right): https://codereview.chromium.org/566243005/diff/260001/net/base/net_util_win.c... net/base/net_util_win.cc:28: using namespace net::internal; On 2014/09/23 17:59:03, pauljensen wrote: > ditto Done.
lgtm. I assume you verified that in your testing that GetWifiOptions() was returning something other than -1?
On 2014/09/23 18:59:55, pauljensen wrote: > lgtm. I assume you verified that in your testing that GetWifiOptions() was > returning something other than -1? Yes I did.
The CQ bit was checked by hubbe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/566243005/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as b3456d0d7383baac954d7a8cfe8b102c66ec5f38
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/693959c4726d968b0396345918a84bc284aea510 Cr-Commit-Position: refs/heads/master@{#296253} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
