Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
530193004:
Fix wifi_component build with 10.9+ SDK (Closed)
|
Created:
6 years, 3 months ago by Jiang Jiang Modified:
6 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionFix wifi_component build with 10.9+ SDK
Use forward declaration to build on both 10.6 and 10.9 SDK and
runtime checking to make sure we don't call the wrong API.
BUG=390212
Committed: https://crrev.com/6acb57597d5d18ab9d0e2954993863a8e2848774
Cr-Commit-Position: refs/heads/master@{#293911}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Move forward declare to base and fix style issues #
Total comments: 6
Patch Set 3 : Add missing comment after #endif #
Total comments: 6
Patch Set 4 : Combine 10.9 forward declare section #Patch Set 5 : Update comments #Patch Set 6 : Remove one more "we" #Patch Set 7 : Use correct notification #
Total comments: 2
Patch Set 8 : Remove runtime checking in favor of simpler code #
Total comments: 4
Patch Set 9 : Fix remaining issues #
Messages
Total messages: 50 (15 generated)
jiangj@opera.com changed reviewers: + mef@chromium.org
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
Thanks for doing this! It's less tricky than I thought it was, though they did kind of make a mess of the SDK by breaking the ABI… https://codereview.chromium.org/530193004/diff/1/components/wifi/wifi_service... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/530193004/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_mac.mm:24: #if !defined(MAC_OS_X_VERSION_10_9) || \ Move these to base/mac/sdk_forward_declarations.h. https://codereview.chromium.org/530193004/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_mac.mm:46: -(BOOL)supportsSecurity:(CWSecurity)security; nit: space before ( https://codereview.chromium.org/530193004/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_mac.mm:560: if ([network respondsToSelector:@selector(supportsSecurity:)]) nit: since the else has curly braces, the if body needs them too. https://codereview.chromium.org/530193004/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_mac.mm:597: [network supportsSecurity:kCWSecurityWPA2Enterprise]) nit: braces needed since condition is multi-line, same on line 600
https://codereview.chromium.org/530193004/diff/1/components/wifi/wifi_service... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/530193004/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_mac.mm:24: #if !defined(MAC_OS_X_VERSION_10_9) || \ On 2014/09/04 15:13:30, rsesek wrote: > Move these to base/mac/sdk_forward_declarations.h. Done. https://codereview.chromium.org/530193004/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_mac.mm:46: -(BOOL)supportsSecurity:(CWSecurity)security; On 2014/09/04 15:13:30, rsesek wrote: > nit: space before ( Done. https://codereview.chromium.org/530193004/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_mac.mm:560: if ([network respondsToSelector:@selector(supportsSecurity:)]) On 2014/09/04 15:13:30, rsesek wrote: > nit: since the else has curly braces, the if body needs them too. Done. https://codereview.chromium.org/530193004/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_mac.mm:597: [network supportsSecurity:kCWSecurityWPA2Enterprise]) On 2014/09/04 15:13:30, rsesek wrote: > nit: braces needed since condition is multi-line, same on line 600 Done.
https://codereview.chromium.org/530193004/diff/20001/base/mac/sdk_forward_dec... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/530193004/diff/20001/base/mac/sdk_forward_dec... base/mac/sdk_forward_declarations.h:279: #if !defined(MAC_OS_X_VERSION_10_9) || \ There's already a 10.9 section between lines 208 and 260. https://codereview.chromium.org/530193004/diff/20001/base/mac/sdk_forward_dec... base/mac/sdk_forward_declarations.h:299: @interface CWNetwork (ForwardDeclarations) nit: MavericksSDK for the category, same on 317.
https://codereview.chromium.org/530193004/diff/40001/base/mac/sdk_forward_dec... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/530193004/diff/40001/base/mac/sdk_forward_dec... base/mac/sdk_forward_declarations.h:277: // build with SDKs from 10.6 to 10.9 we need to forward declare both and do nit: I don't think we use 'we' in comments. :) https://codereview.chromium.org/530193004/diff/40001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/530193004/diff/40001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:103: // Converts |CWSecurity| into onc::wifi::k{WPA|WEP}* security constant. Nit: Either change this function to take |CWSecurity| or update the comment. https://codereview.chromium.org/530193004/diff/40001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:407: addObserverForName:CWSSIDDidChangeNotification does it compile on 10.6?
https://codereview.chromium.org/530193004/diff/20001/base/mac/sdk_forward_dec... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/530193004/diff/20001/base/mac/sdk_forward_dec... base/mac/sdk_forward_declarations.h:279: #if !defined(MAC_OS_X_VERSION_10_9) || \ On 2014/09/04 15:24:35, rsesek wrote: > There's already a 10.9 section between lines 208 and 260. Is it OK for me to use #else for the forward declare for 10.6 SDK in that section? https://codereview.chromium.org/530193004/diff/20001/base/mac/sdk_forward_dec... base/mac/sdk_forward_declarations.h:299: @interface CWNetwork (ForwardDeclarations) On 2014/09/04 15:24:35, rsesek wrote: > nit: MavericksSDK for the category, same on 317. The done down in line 317 is actually forward declaring things defined in 10.6 SDK, I suppose I should use SnowLeopardSDK there?
https://codereview.chromium.org/530193004/diff/20001/base/mac/sdk_forward_dec... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/530193004/diff/20001/base/mac/sdk_forward_dec... base/mac/sdk_forward_declarations.h:279: #if !defined(MAC_OS_X_VERSION_10_9) || \ On 2014/09/04 15:32:17, Jiang wrote: > On 2014/09/04 15:24:35, rsesek wrote: > > There's already a 10.9 section between lines 208 and 260. > > Is it OK for me to use #else for the forward declare for 10.6 SDK in that > section? I guess we don't have any other use of #else, so it may be best to keep it here, then. https://codereview.chromium.org/530193004/diff/20001/base/mac/sdk_forward_dec... base/mac/sdk_forward_declarations.h:299: @interface CWNetwork (ForwardDeclarations) On 2014/09/04 15:32:17, Jiang wrote: > On 2014/09/04 15:24:35, rsesek wrote: > > nit: MavericksSDK for the category, same on 317. > > The done down in line 317 is actually forward declaring things defined in 10.6 > SDK, I suppose I should use SnowLeopardSDK there? Yes, sorry, meant to just use the name for the appropriate SDK.
https://codereview.chromium.org/530193004/diff/40001/base/mac/sdk_forward_dec... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/530193004/diff/40001/base/mac/sdk_forward_dec... base/mac/sdk_forward_declarations.h:277: // build with SDKs from 10.6 to 10.9 we need to forward declare both and do On 2014/09/04 15:31:34, mef wrote: > nit: I don't think we use 'we' in comments. :) Done. https://codereview.chromium.org/530193004/diff/40001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/530193004/diff/40001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:103: // Converts |CWSecurity| into onc::wifi::k{WPA|WEP}* security constant. On 2014/09/04 15:31:34, mef wrote: > Nit: Either change this function to take |CWSecurity| or update the comment. Done. https://codereview.chromium.org/530193004/diff/40001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:407: addObserverForName:CWSSIDDidChangeNotification On 2014/09/04 15:31:34, mef wrote: > does it compile on 10.6? Yes, according to the docs it's declared in 10.6 SDK.
LGTM
lgtm
The CQ bit was checked by jiangj@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiangj@opera.com/530193004/100001
On 2014/09/04 17:50:09, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/jiangj@opera.com/530193004/100001 Looks like we still need forward declare for CWSSIDDidChangeNotification.
On 2014/09/04 19:14:26, Jiang wrote: > On 2014/09/04 17:50:09, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-status.appspot.com/cq/jiangj@opera.com/530193004/100001 > > Looks like we still need forward declare for CWSSIDDidChangeNotification. I think that's what I've observed with initial CL. Documentation lies. :(
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/51700) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2014/09/04 19:14:26, Jiang wrote: > On 2014/09/04 17:50:09, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-status.appspot.com/cq/jiangj@opera.com/530193004/100001 > > Looks like we still need forward declare for CWSSIDDidChangeNotification. Is there any easy way to do runtime checking for this CWSSIDDidChangeNotification symbol without using dlopen/dysym? I just checked the CoreWLAN framework on 10.6 and it doesn't have this symbol unfortunately, so we will have to use the one with k prefix instead. (I can use base::mac::IsOSLionOrLater() but I don't know if that's preferred.)
On 2014/09/04 19:20:18, mef wrote: > On 2014/09/04 19:14:26, Jiang wrote: > > On 2014/09/04 17:50:09, I haz the power (commit-bot) wrote: > > > CQ is trying da patch. Follow status at > > > https://chromium-status.appspot.com/cq/jiangj@opera.com/530193004/100001 > > > > Looks like we still need forward declare for CWSSIDDidChangeNotification. > > I think that's what I've observed with initial CL. Documentation lies. :( Updated, please take another look.
https://codereview.chromium.org/530193004/diff/120001/components/wifi/wifi_se... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/530193004/diff/120001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:408: NSBundle* bundle = [NSBundle If anything I'd move this out into separate method, but I'd defer to rsesek in hope for better solution. https://codereview.chromium.org/530193004/diff/120001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:419: symbol = dlsym(library_handle, "kCWSSIDDidChangeNotification"); Is kCWSSIDDidChangeNotification actually causing issues on 10.7+?
On 2014/09/05 17:56:23, mef wrote: > https://codereview.chromium.org/530193004/diff/120001/components/wifi/wifi_se... > File components/wifi/wifi_service_mac.mm (right): > > https://codereview.chromium.org/530193004/diff/120001/components/wifi/wifi_se... > components/wifi/wifi_service_mac.mm:408: NSBundle* bundle = [NSBundle > If anything I'd move this out into separate method, but I'd defer to rsesek in > hope for better solution. > > https://codereview.chromium.org/530193004/diff/120001/components/wifi/wifi_se... > components/wifi/wifi_service_mac.mm:419: symbol = dlsym(library_handle, > "kCWSSIDDidChangeNotification"); > Is kCWSSIDDidChangeNotification actually causing issues on 10.7+? Hmm, looks like it still exists all the way to 10.10, just not declared anymore, so probably forward declare is sufficient, I will update the code.
On 2014/09/05 18:04:54, Jiang wrote: > On 2014/09/05 17:56:23, mef wrote: > > > https://codereview.chromium.org/530193004/diff/120001/components/wifi/wifi_se... > > File components/wifi/wifi_service_mac.mm (right): > > > > > https://codereview.chromium.org/530193004/diff/120001/components/wifi/wifi_se... > > components/wifi/wifi_service_mac.mm:408: NSBundle* bundle = [NSBundle > > If anything I'd move this out into separate method, but I'd defer to rsesek in > > hope for better solution. > > > > > https://codereview.chromium.org/530193004/diff/120001/components/wifi/wifi_se... > > components/wifi/wifi_service_mac.mm:419: symbol = dlsym(library_handle, > > "kCWSSIDDidChangeNotification"); > > Is kCWSSIDDidChangeNotification actually causing issues on 10.7+? > > Hmm, looks like it still exists all the way to 10.10, just not declared anymore, > so probably forward declare is sufficient, I will update the code. Fixed.
LGTM https://codereview.chromium.org/530193004/diff/140001/base/mac/sdk_forward_de... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/530193004/diff/140001/base/mac/sdk_forward_de... base/mac/sdk_forward_declarations.h:285: #else // !MAC_OS_X_VERSION_10_9
I'm glad it is settled to less ugly hack. :) Still LGTM. https://codereview.chromium.org/530193004/diff/140001/components/wifi/wifi_se... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/530193004/diff/140001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:513: if ([network respondsToSelector:@selector(supportsSecurity:)]) { Maybe add comment why do we need respondsToSelector here and below?
https://codereview.chromium.org/530193004/diff/140001/base/mac/sdk_forward_de... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/530193004/diff/140001/base/mac/sdk_forward_de... base/mac/sdk_forward_declarations.h:285: #else On 2014/09/05 18:18:43, rsesek wrote: > // !MAC_OS_X_VERSION_10_9 Done. https://codereview.chromium.org/530193004/diff/140001/components/wifi/wifi_se... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/530193004/diff/140001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:513: if ([network respondsToSelector:@selector(supportsSecurity:)]) { On 2014/09/05 18:22:50, mef wrote: > Maybe add comment why do we need respondsToSelector here and below? Done.
The CQ bit was checked by jiangj@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiangj@opera.com/530193004/140001
The CQ bit was unchecked by jiangj@opera.com
The CQ bit was checked by jiangj@opera.com
The CQ bit was unchecked by jiangj@opera.com
The CQ bit was checked by jiangj@opera.com
The CQ bit was unchecked by jiangj@opera.com
The CQ bit was checked by jiangj@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiangj@opera.com/530193004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by jiangj@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiangj@opera.com/530193004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
On 2014/09/08 16:54:32, I haz the power (commit-bot) wrote: > Exceeded time limit waiting for builds to trigger. Any idea why this keep happening?
On 2014/09/08 17:03:11, Jiang wrote: > On 2014/09/08 16:54:32, I haz the power (commit-bot) wrote: > > Exceeded time limit waiting for builds to trigger. > > Any idea why this keep happening? No :/. Seems like an infra issue. Can you email this CL to chrome-troopers+tryserver@google.com so they're aware?
On 2014/09/08 17:05:13, rsesek wrote: > On 2014/09/08 17:03:11, Jiang wrote: > > On 2014/09/08 16:54:32, I haz the power (commit-bot) wrote: > > > Exceeded time limit waiting for builds to trigger. > > > > Any idea why this keep happening? > > No :/. Seems like an infra issue. Can you email this CL to > mailto:chrome-troopers+tryserver@google.com so they're aware? I asked yesterday but got no reply.
The CQ bit was checked by jiangj@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiangj@opera.com/530193004/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as bac6205d2a0dc31dfd2040fe6f58416d0a7905af
Message was sent while issue was closed.
On 2014/09/09 09:58:18, I haz the power (commit-bot) wrote: > Committed patchset #9 (id:160001) as bac6205d2a0dc31dfd2040fe6f58416d0a7905af For anyone else who finds themselves here, this patch seems to have broken local builds on 10.9 and it is being looked at in crbug.com/390212
Message was sent while issue was closed.
On 2014/09/09 21:41:11, pdr wrote: > On 2014/09/09 09:58:18, I haz the power (commit-bot) wrote: > > Committed patchset #9 (id:160001) as bac6205d2a0dc31dfd2040fe6f58416d0a7905af > > For anyone else who finds themselves here, this patch seems to have broken local > builds on 10.9 and it is being looked at in crbug.com/390212 Thanks for the notice!
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/6acb57597d5d18ab9d0e2954993863a8e2848774 Cr-Commit-Position: refs/heads/master@{#293911} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
