Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
223143006:
Make necessary changes for chromecast build as chromecast has it own implementation of NetworkChang… (Closed)
|
Created:
6 years, 8 months ago by lcwu1 Modified:
6 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, damienv1, kollas, rjkroege Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMake necessary changes for chromecast build as chromecast has it own implementation of NetworkChangeNotifier and doesn't support proxy.
BUG=336640
Patch Set 1 #Patch Set 2 : Modified based on review comments on patch set 1 #Patch Set 3 : Fix a check failure when we don't yet have a NetworkChangeNotifierFactory for chromecast build #
Total comments: 10
Patch Set 4 : Remove the use of CHROMECAST_BUILD macro. Use feature-based gyp variables and C macros to specify c… #
Messages
Total messages: 14 (0 generated)
I would rather see positive handling of the CHROMECAST_BUILD, instead of falling into the default path (which will hit a NOTIMPLEMENTED() or LOG(ERROR)) That is, explicit conditionals indicating that this is intentionally something you don't support.
On 2014/04/07 16:42:03, Ryan Sleevi wrote: > I would rather see positive handling of the CHROMECAST_BUILD, instead of falling > into the default path (which will hit a NOTIMPLEMENTED() or LOG(ERROR)) > > That is, explicit conditionals indicating that this is intentionally something > you don't support. Ryan, Sorry for my delay in response. My workstation died last Friday and the replacement didn't arrive until yesterday. Anyway, I have modified proxy_service.cc and (part of) network_change_notifier.cc as you suggested. However, we still need to keep a negative check in the include section of network_change_notifier.cc as Chromecast will report itself as Linux, but we have our own NetworkChangeNotifier and factory classes (which will be uploaded in a separate CL). Please take a look at patch set 2. Thanks.
https://codereview.chromium.org/223143006/diff/40001/net/base/network_change_... File net/base/network_change_notifier.cc (right): https://codereview.chromium.org/223143006/diff/40001/net/base/network_change_... net/base/network_change_notifier.cc:497: #elif defined(CHROMECAST_BUILD) The comment that Chromecast MUST use its own class factory seems to suggest that there should also be a CHECK(false) in the CHROMECAST_BUILD case, no?
https://codereview.chromium.org/223143006/diff/40001/net/base/network_change_... File net/base/network_change_notifier.cc (right): https://codereview.chromium.org/223143006/diff/40001/net/base/network_change_... net/base/network_change_notifier.cc:19: #elif defined(OS_LINUX) && !defined(OS_CHROMEOS) && !defined(CHROMECAST_BUILD) is it actually problematic to include this header? https://codereview.chromium.org/223143006/diff/40001/net/base/network_change_... net/base/network_change_notifier.cc:483: #if defined(OS_WIN) we could eliminate #ifdefs here by having separate .cc files for each configuration. then we could just select the proper one at link time via GYP instead of at preprocessor time here. https://codereview.chromium.org/223143006/diff/40001/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/223143006/diff/40001/net/proxy/proxy_service.... net/proxy/proxy_service.cc:1365: ProxyConfigService* ProxyService::CreateSystemProxyConfigService( Perhaps a better approach here would be to have a separate .cc file for each port, and then select the right one at link time instead of at preprocessor time.
https://codereview.chromium.org/223143006/diff/40001/net/base/network_change_... File net/base/network_change_notifier.cc (right): https://codereview.chromium.org/223143006/diff/40001/net/base/network_change_... net/base/network_change_notifier.cc:489: // ChromeOS, Android, and Chromecast builds MUST use their own class factory. This comment doesn't seem like it should be updated - you're not handling this on line 488-496, but below. https://codereview.chromium.org/223143006/diff/40001/net/base/network_change_... net/base/network_change_notifier.cc:497: #elif defined(CHROMECAST_BUILD) On 2014/04/14 04:48:00, darin wrote: > The comment that Chromecast MUST use its own class factory seems to suggest that > there should also be a CHECK(false) in the CHROMECAST_BUILD case, no? Apologies that I wasn't clearer, this is what I was trying to suggest earlier with my first review comments - that is, if this is something that shouldn't be called/reached, that you actively assert (eg: CHECK) to make sure it's not hit.
https://codereview.chromium.org/223143006/diff/40001/net/base/network_change_... File net/base/network_change_notifier.cc (right): https://codereview.chromium.org/223143006/diff/40001/net/base/network_change_... net/base/network_change_notifier.cc:19: #elif defined(OS_LINUX) && !defined(OS_CHROMEOS) && !defined(CHROMECAST_BUILD) On 2014/04/14 05:03:29, darin wrote: > is it actually problematic to include this header? No, it doesn't cause any problem if we include the linux version. I will remove the defined here. https://codereview.chromium.org/223143006/diff/40001/net/base/network_change_... net/base/network_change_notifier.cc:483: #if defined(OS_WIN) On 2014/04/14 05:03:29, darin wrote: > we could eliminate #ifdefs here by having separate .cc files for each > configuration. then we could just select the proper one at link time via GYP > instead of at preprocessor time here. Sure, let me try to re-factor the code to do that. https://codereview.chromium.org/223143006/diff/40001/net/base/network_change_... net/base/network_change_notifier.cc:489: // ChromeOS, Android, and Chromecast builds MUST use their own class factory. On 2014/04/14 21:28:43, Ryan Sleevi wrote: > This comment doesn't seem like it should be updated - you're not handling this > on line 488-496, but below. Yes, this comment only makes sense in patch set 2. I forgot to change it after creating patch set 3. I will modify it if refactoring the code to remove #ifdefs proves to be much more difficult. https://codereview.chromium.org/223143006/diff/40001/net/base/network_change_... net/base/network_change_notifier.cc:497: #elif defined(CHROMECAST_BUILD) On 2014/04/14 21:28:43, Ryan Sleevi wrote: > On 2014/04/14 04:48:00, darin wrote: > > The comment that Chromecast MUST use its own class factory seems to suggest > that > > there should also be a CHECK(false) in the CHROMECAST_BUILD case, no? > > Apologies that I wasn't clearer, this is what I was trying to suggest earlier > with my first review comments - that is, if this is something that shouldn't be > called/reached, that you actively assert (eg: CHECK) to make sure it's not hit. Yes, agreed. The reason why I didn't have a CHECK here is because we have not hooked up our NetworkChangeNotifier[Factory] implementation yet (as we are doing staged CL submission). So having a CHECK here will always fire. I will hook up a stub implementation first and add back the CHECK.
In patch set #4, we removed the references to CHROMECAST_BUILD macro and defined a couple of feature-based macros and gyp variables to hook up chromecast's network change notifier factory and its use of direct proxy config service. Please take another look. Thanks,
On 2014/06/24 22:39:12, lcwu1 wrote: > In patch set #4, we removed the references to CHROMECAST_BUILD macro and defined > a couple of feature-based macros and gyp variables to hook up chromecast's > network change notifier factory and its use of direct proxy config service. > > Please take another look. Thanks, This doesn't really seem to get to the suggestion provided in comment #5. In general, we should be trying to use fewer, not more, GYP variables.
In motivations for this CL, it's unclear why you can't use g_network_change_notifier_factory / NetworkChangeNotifier::SetFactory() to create a factory appropriate for ChromeCast's use - https://code.google.com/p/chromium/codesearch#chromium/src/net/base/network_c...
On 2014/06/24 23:16:08, Ryan Sleevi wrote: > In motivations for this CL, it's unclear why you can't use > g_network_change_notifier_factory / NetworkChangeNotifier::SetFactory() to > create a factory appropriate for ChromeCast's use > > - > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/network_c... We are actually using NetworkChangeNotifier::SetFactory to specify our NetworkChangeNotifierFactory implementation. Please see this CL (line 63): https://codereview.chromium.org/223143003/diff/60001/chromecast/shell/browser... However, the problem here is that in network_change_notifier.cc, even if a platform or an implementation already provided a factory, code between line 505 - 527 would still get compiled, and because our platform would report us as LINUX, the compiler would try to compile line 524 and look for the declaration/definition of NetworkChangeNotifierLinux, which we don't have (as we exclude it from the chromecast build in net.gyp). But I guess it might not hurt even if we include network_change_notifier_linux.cc. I will give it a try.
On 2014/06/24 23:42:49, lcwu1 wrote: > On 2014/06/24 23:16:08, Ryan Sleevi wrote: > > In motivations for this CL, it's unclear why you can't use > > g_network_change_notifier_factory / NetworkChangeNotifier::SetFactory() to > > create a factory appropriate for ChromeCast's use > > > > - > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/network_c... > > We are actually using NetworkChangeNotifier::SetFactory to specify our > NetworkChangeNotifierFactory implementation. Please see this CL (line 63): > > https://codereview.chromium.org/223143003/diff/60001/chromecast/shell/browser... > > However, the problem here is that in network_change_notifier.cc, even if a > platform or an implementation already provided a factory, code between line 505 > - 527 would still get compiled, and because our platform would report us as > LINUX, the compiler would try to compile line 524 and look for the > declaration/definition of NetworkChangeNotifierLinux, which we don't have (as we > exclude it from the chromecast build in net.gyp). But I guess it might not hurt > even if we include network_change_notifier_linux.cc. I will give it a try. If it doesn't work, then you can use the path Darin suggested, such that the file exclusion rules contain which implementation of ::Create() gets compiled in. That is, you'd do something like NCN::Create - n_c_n.cc - Checks for the factory - if found, calls it - If not, calls NCN::CreateForPlatform() NCN::CreateForPlatform would live in (n_c_n[_linux/_mac/_win/_chromeos/_android].cc) and return an appropriate one (or NULL, if the implementation MUST use its own factory). Any unsupported platform == compile error.
On 2014/06/24 23:13:31, Ryan Sleevi wrote: > On 2014/06/24 22:39:12, lcwu1 wrote: > > In patch set #4, we removed the references to CHROMECAST_BUILD macro and > defined > > a couple of feature-based macros and gyp variables to hook up chromecast's > > network change notifier factory and its use of direct proxy config service. > > > > Please take another look. Thanks, > > This doesn't really seem to get to the suggestion provided in comment #5. > > In general, we should be trying to use fewer, not more, GYP variables. Regarding the gyp variable and macro introduced for using direct proxy config service, since we don't support proxy config, and we cannot use Linux's implementation (as we don't have glib on our platform), it makes sense for us to simply use ProxyConfigServiceDirect. Based on our early upstream discussion with Darin and a couple of other Chromium folks, creating a new platform #ifdef is strongly discouraged, but it's OK to create feature-based gyp variable and/or #ifdefs (e.g. Take use_direct_config_service for example, maybe some Windows-based platforms might decide to use ProxyConfigServiceDirect instead of ProxyConfigServiceWin and can simply set use_direct_config_service=1 on their platforms). Any other suggestions/ideas on how we can solve this problem differently are welcomed. Thanks.
On 2014/06/24 23:57:44, lcwu1 wrote: > On 2014/06/24 23:13:31, Ryan Sleevi wrote: > > On 2014/06/24 22:39:12, lcwu1 wrote: > > > In patch set #4, we removed the references to CHROMECAST_BUILD macro and > > defined > > > a couple of feature-based macros and gyp variables to hook up chromecast's > > > network change notifier factory and its use of direct proxy config service. > > > > > > Please take another look. Thanks, > > > > This doesn't really seem to get to the suggestion provided in comment #5. > > > > In general, we should be trying to use fewer, not more, GYP variables. > > Regarding the gyp variable and macro introduced for using direct proxy config > service, since we don't support proxy config, and we cannot use Linux's > implementation (as we don't have glib on our platform), it makes sense for us to > simply use ProxyConfigServiceDirect. Based on our early upstream discussion with > Darin and a couple of other Chromium folks, creating a new platform #ifdef is > strongly discouraged, but it's OK to create feature-based gyp variable and/or > #ifdefs (e.g. Take use_direct_config_service for example, maybe some > Windows-based platforms might decide to use ProxyConfigServiceDirect instead of > ProxyConfigServiceWin and can simply set use_direct_config_service=1 on their > platforms). > > Any other suggestions/ideas on how we can solve this problem differently are > welcomed. Thanks. It looks like the use of glib APIs are all guarded by either GCONF or GIO, both of which would not be defined if we set glib = 0. So we should be fine to include and build proxy_config_service_linux.cc. I also tried including network_change_notifier_linux.cc in our build (even though we already provide a factory) and it appears to build/run fine. So this CL is no longer needed. :-) Thanks for your feedback/comments, Ryan. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
