Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2647323010:
[Chromecast] Add proxy server support to chromecast (Closed)
|
Created:
3 years, 11 months ago by almasrymina Modified:
3 years, 10 months ago CC:
chromium-reviews, alokp+watch_chromium.org, lcwu+watch_chromium.org, cbentzel+watch_chromium.org, halliwell+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromecast] Add proxy server support to chromecast
Proxy server support can be useful for testing purposes. This enables cast_shell to support proxies via the "proxy-server" and "proxy-bypass-list" options stored in pref_service. These options behave exactly as chrome's --proxy-server and --proxy-bypass-list.
Coming in another CL is a setup API that allows updating these proxy settings.
BUG= internal b/34689274
TEST= manual testing with local proxy server
Change-Id: I1284afe626ccfdcafd07cd891b38662fbd46a7a3
Review-Url: https://codereview.chromium.org/2647323010
Cr-Commit-Position: refs/heads/master@{#448896}
Committed: https://chromium.googlesource.com/chromium/src/+/dfb5b36388a651bf0651f6c564563911fc927c7f
Patch Set 1 #Patch Set 2 : Cleanup #
Total comments: 3
Patch Set 3 : Address comments. #Patch Set 4 : Fixing target ref #Patch Set 5 : Add PrefProxyConfigTracker #Patch Set 6 : Update connectivity checker to make it compile on desktop #
Total comments: 8
Patch Set 7 : Addressing comments. #Patch Set 8 : Update #Patch Set 9 : Changing for internal changes #Patch Set 10 : Update code to fix internal unittests failing #Patch Set 11 : Fix target ref. #Patch Set 12 : Fix target ref #Patch Set 13 : Fix unittest failure #
Total comments: 8
Patch Set 14 : Address comments #Patch Set 15 : fix deps #
Total comments: 10
Patch Set 16 : Address cleanup comments #Patch Set 17 : Fixing test break. #Messages
Total messages: 81 (42 generated)
Description was changed from ========== [Chromecast] Add proxy server support to chromecast Proxy servers can be useful for testing purposes. This enables cast_shell to read --proxy-server and --proxy-bypass-list options. BUG= internal b/34689274 TEST= manual testing with local proxy server Change-Id: I1284afe626ccfdcafd07cd891b38662fbd46a7a3 ========== to ========== [Chromecast] Add proxy server support to chromecast Proxy server support can be useful for testing purposes. This enables cast_shell to support proxies via the "proxy-server" and "proxy-bypass-list" options stored in pref_service. These options behave exactly as chrome's --proxy-server and --proxy-bypass-list. Coming in another CL is a setup API that allows updating these proxy settings. BUG= internal b/34689274 TEST= manual testing with local proxy server Change-Id: I1284afe626ccfdcafd07cd891b38662fbd46a7a3 ==========
almasrymina@chromium.org changed reviewers: + gfhuang@chromium.org, halliwell@chromium.org, wzhong@chromium.org
https://codereview.chromium.org/2647323010/diff/20001/chromecast/browser/cast... File chromecast/browser/cast_browser_process.cc (right): https://codereview.chromium.org/2647323010/diff/20001/chromecast/browser/cast... chromecast/browser/cast_browser_process.cc:101: std::string proxy_server = pref_service_->GetString("proxy-server"); I'm not too sure about this part. Essentially I use the SetPrefService method to initialize the proxy_config param, since the proxy config needs to be read from the pref service. https://codereview.chromium.org/2647323010/diff/20001/chromecast/browser/cast... chromecast/browser/cast_browser_process.cc:101: std::string proxy_server = pref_service_->GetString("proxy-server"); the string constants are defined in chromecast/internal/base/base_prefs.h, which I cannot include here because of dependency restrictions. Please advise.
https://codereview.chromium.org/2647323010/diff/20001/chromecast/browser/cast... File chromecast/browser/cast_browser_process.cc (right): https://codereview.chromium.org/2647323010/diff/20001/chromecast/browser/cast... chromecast/browser/cast_browser_process.cc:101: std::string proxy_server = pref_service_->GetString("proxy-server"); On 2017/01/25 02:52:56, almasrymina wrote: > I'm not too sure about this part. Essentially I use the SetPrefService method to > initialize the proxy_config param, since the proxy config needs to be read from > the pref service. In ChromecastSevice, you might load preference and update proxy_config by shell::CastBrowserProcess::GetInstance()->proxy_config()
On 2017/01/25 03:03:45, wzhong wrote: > https://codereview.chromium.org/2647323010/diff/20001/chromecast/browser/cast... > File chromecast/browser/cast_browser_process.cc (right): > > https://codereview.chromium.org/2647323010/diff/20001/chromecast/browser/cast... > chromecast/browser/cast_browser_process.cc:101: std::string proxy_server = > pref_service_->GetString("proxy-server"); > On 2017/01/25 02:52:56, almasrymina wrote: > > I'm not too sure about this part. Essentially I use the SetPrefService method > to > > initialize the proxy_config param, since the proxy config needs to be read > from > > the pref service. > > In ChromecastSevice, you might load preference and update proxy_config by > shell::CastBrowserProcess::GetInstance()->proxy_config() I am a little bit concerns of prefs/setup APIs from security perspective. why not simple flags assuming we have controls in test environment
On 2017/01/25 03:09:22, gfhuang wrote: > On 2017/01/25 03:03:45, wzhong wrote: > > > https://codereview.chromium.org/2647323010/diff/20001/chromecast/browser/cast... > > File chromecast/browser/cast_browser_process.cc (right): > > > > > https://codereview.chromium.org/2647323010/diff/20001/chromecast/browser/cast... > > chromecast/browser/cast_browser_process.cc:101: std::string proxy_server = > > pref_service_->GetString("proxy-server"); > > On 2017/01/25 02:52:56, almasrymina wrote: > > > I'm not too sure about this part. Essentially I use the SetPrefService > method > > to > > > initialize the proxy_config param, since the proxy config needs to be read > > from > > > the pref service. > > > > In ChromecastSevice, you might load preference and update proxy_config by > > shell::CastBrowserProcess::GetInstance()->proxy_config() > > I am a little bit concerns of prefs/setup APIs from security perspective. > why not simple flags assuming we have controls in test environment We need the proxy configuration to be persistent as the device receives new OTA's, so we thought of prefs for this. What alternative do you suggest?
On 2017/01/25 03:11:09, almasrymina wrote: > On 2017/01/25 03:09:22, gfhuang wrote: > > On 2017/01/25 03:03:45, wzhong wrote: > > > > > > https://codereview.chromium.org/2647323010/diff/20001/chromecast/browser/cast... > > > File chromecast/browser/cast_browser_process.cc (right): > > > > > > > > > https://codereview.chromium.org/2647323010/diff/20001/chromecast/browser/cast... > > > chromecast/browser/cast_browser_process.cc:101: std::string proxy_server = > > > pref_service_->GetString("proxy-server"); > > > On 2017/01/25 02:52:56, almasrymina wrote: > > > > I'm not too sure about this part. Essentially I use the SetPrefService > > method > > > to > > > > initialize the proxy_config param, since the proxy config needs to be read > > > from > > > > the pref service. > > > > > > In ChromecastSevice, you might load preference and update proxy_config by > > > shell::CastBrowserProcess::GetInstance()->proxy_config() > > > > I am a little bit concerns of prefs/setup APIs from security perspective. > > why not simple flags assuming we have controls in test environment > > We need the proxy configuration to be persistent as the device receives new > OTA's, so we thought of prefs for this. What alternative do you suggest? I don't think persistent over OTA is a requirement. (otherwise how about persistent over FDR requirement?) on the other hand, pref service is fine. it's the setup API should be cautious from attacker.
On 2017/01/25 03:13:30, gfhuang wrote: > On 2017/01/25 03:11:09, almasrymina wrote: > > On 2017/01/25 03:09:22, gfhuang wrote: > > > On 2017/01/25 03:03:45, wzhong wrote: > > > > > > > > > > https://codereview.chromium.org/2647323010/diff/20001/chromecast/browser/cast... > > > > File chromecast/browser/cast_browser_process.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2647323010/diff/20001/chromecast/browser/cast... > > > > chromecast/browser/cast_browser_process.cc:101: std::string proxy_server = > > > > pref_service_->GetString("proxy-server"); > > > > On 2017/01/25 02:52:56, almasrymina wrote: > > > > > I'm not too sure about this part. Essentially I use the SetPrefService > > > method > > > > to > > > > > initialize the proxy_config param, since the proxy config needs to be > read > > > > from > > > > > the pref service. > > > > > > > > In ChromecastSevice, you might load preference and update proxy_config by > > > > shell::CastBrowserProcess::GetInstance()->proxy_config() > > > > > > I am a little bit concerns of prefs/setup APIs from security perspective. > > > why not simple flags assuming we have controls in test environment > > > > We need the proxy configuration to be persistent as the device receives new > > OTA's, so we thought of prefs for this. What alternative do you suggest? > > I don't think persistent over OTA is a requirement. > (otherwise how about persistent over FDR requirement?) > > on the other hand, pref service is fine. it's the setup API should be cautious > from attacker. Tomer mentioned that hundreds of devices will be needed. Those devices will be beta-channel build in order to receive update. Flags are not feasible. Persistent over OTA is just like over reboot. We should limit API to only non-stable channel.
On 2017/01/25 03:32:44, wzhong wrote: > On 2017/01/25 03:13:30, gfhuang wrote: > > On 2017/01/25 03:11:09, almasrymina wrote: > > > On 2017/01/25 03:09:22, gfhuang wrote: > > > > On 2017/01/25 03:03:45, wzhong wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2647323010/diff/20001/chromecast/browser/cast... > > > > > File chromecast/browser/cast_browser_process.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2647323010/diff/20001/chromecast/browser/cast... > > > > > chromecast/browser/cast_browser_process.cc:101: std::string proxy_server > = > > > > > pref_service_->GetString("proxy-server"); > > > > > On 2017/01/25 02:52:56, almasrymina wrote: > > > > > > I'm not too sure about this part. Essentially I use the SetPrefService > > > > method > > > > > to > > > > > > initialize the proxy_config param, since the proxy config needs to be > > read > > > > > from > > > > > > the pref service. > > > > > > > > > > In ChromecastSevice, you might load preference and update proxy_config > by > > > > > shell::CastBrowserProcess::GetInstance()->proxy_config() > > > > > > > > I am a little bit concerns of prefs/setup APIs from security perspective. > > > > why not simple flags assuming we have controls in test environment > > > > > > We need the proxy configuration to be persistent as the device receives new > > > OTA's, so we thought of prefs for this. What alternative do you suggest? > > > > I don't think persistent over OTA is a requirement. > > (otherwise how about persistent over FDR requirement?) > > > > on the other hand, pref service is fine. it's the setup API should be cautious > > from attacker. > > Tomer mentioned that hundreds of devices will be needed. Those devices will be > beta-channel build in order to receive update. Flags are not feasible. > Persistent over OTA is just like over reboot. > > We should limit API to only non-stable channel. In that case, the limitation is this is won't work for OOBE related testing. and the prefered way is through DCS (with same limitation) But I am ok as-is.
For OOBE, the HTTP API can be called over hotspot before setting up.
On 2017/01/25 03:44:30, wzhong wrote: > For OOBE, the HTTP API can be called over hotspot before setting up. Besides the command-line switch, this feature should also be controllable/settable on the server side, which will make it easier to manage the test devices.
On 2017/01/25 17:46:06, lcwu1 wrote: > On 2017/01/25 03:44:30, wzhong wrote: > > For OOBE, the HTTP API can be called over hotspot before setting up. > > Besides the command-line switch, this feature should also be > controllable/settable on the server side, which will make it easier to manage > the test devices. Server-side configuration will be too cumbersome. Same developer might want to try mocking different locations. Another downside of server-side configuration is the config is not available until devices is online which introduces race condition when cast shell needs to propagate the setting to utility processes. Persisting in preference (and load it before launching utility process) is much simpler in that sense.
On 2017/01/25 17:51:06, wzhong wrote: > On 2017/01/25 17:46:06, lcwu1 wrote: > > On 2017/01/25 03:44:30, wzhong wrote: > > > For OOBE, the HTTP API can be called over hotspot before setting up. > > > > Besides the command-line switch, this feature should also be > > controllable/settable on the server side, which will make it easier to manage > > the test devices. > > Server-side configuration will be too cumbersome. Same developer might want to > try mocking different locations. > > Another downside of server-side configuration is the config is not available > until devices is online which introduces race condition when cast shell needs to > propagate the setting to utility processes. Persisting in preference (and load > it before launching utility process) is much simpler in that sense. Let's move the discussion to the internal bug.
On 2017/01/25 03:03:45, wzhong wrote: > https://codereview.chromium.org/2647323010/diff/20001/chromecast/browser/cast... > File chromecast/browser/cast_browser_process.cc (right): > > https://codereview.chromium.org/2647323010/diff/20001/chromecast/browser/cast... > chromecast/browser/cast_browser_process.cc:101: std::string proxy_server = > pref_service_->GetString("proxy-server"); > On 2017/01/25 02:52:56, almasrymina wrote: > > I'm not too sure about this part. Essentially I use the SetPrefService method > to > > initialize the proxy_config param, since the proxy config needs to be read > from > > the pref service. > > In ChromecastSevice, you might load preference and update proxy_config by > shell::CastBrowserProcess::GetInstance()->proxy_config() I can't seem to do that, given the initialization order of CastBrowserMainParts::PreMainMessageLoopRun. I think this might be a little better too since it initializes the proxy_config as soon as the pref_service is available which is the soonest possible place.
The CQ bit was checked by almasrymina@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CLs for remote refs other than refs/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
The CQ bit was checked by almasrymina@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
It doesn't seem right loading prefs in browser process. Can you try ProxyService::ResetConfigService() when pref is loaded or proxy settings are changed? That should enable live update of proxy settings without reboot.
Actually there is a PrefProxyConfigTracker class that tracks proxy config change based on prefs, which is perfectly what we need.
On 2017/01/26 15:26:13, wzhong wrote: > Actually there is a PrefProxyConfigTracker class that tracks proxy config change > based on prefs, which is perfectly what we need. I'm afraid PrefProxyConfigTracker seems to be not possible. Following the documentation on the class, the dchecks of thread_checker.calledOnValidThread are failing. I've spent time trying to see what's wrong and if I can give a fix but it's taking too long. Any suggestion what to do here?
You didn't say which calledOnValidThread is failing. The checker is to make sure the APIs are called on the same thread the object was created.
The CQ bit was checked by almasrymina@chromium.org to run a CQ dry run
We got over the PrefProxyConfigTracker problem and I've updated the CL with this implementation. PTAL.
The CQ bit was unchecked by almasrymina@chromium.org
https://codereview.chromium.org/2647323010/diff/100001/chromecast/browser/cas... File chromecast/browser/cast_browser_process.cc (right): https://codereview.chromium.org/2647323010/diff/100001/chromecast/browser/cas... chromecast/browser/cast_browser_process.cc:7: #include <string> Needed? Or lint complains? https://codereview.chromium.org/2647323010/diff/100001/chromecast/browser/cas... chromecast/browser/cast_browser_process.cc:23: #include "components/proxy_config/proxy_config_pref_names.h" Do we need so many header file for adding just a couple of lines at 105? https://codereview.chromium.org/2647323010/diff/100001/chromecast/browser/cas... File chromecast/browser/cast_browser_process.h (right): https://codereview.chromium.org/2647323010/diff/100001/chromecast/browser/cas... chromecast/browser/cast_browser_process.h:101: std::unique_ptr<PrefProxyConfigTracker> pref_proxy_config_tracker_; This seems to need to be after pref_service_. See comments in line 102. https://codereview.chromium.org/2647323010/diff/100001/chromecast/browser/url... File chromecast/browser/url_request_context_factory.cc (right): https://codereview.chromium.org/2647323010/diff/100001/chromecast/browser/url... chromecast/browser/url_request_context_factory.cc:36: #include "net/proxy/proxy_config_service_fixed.h" Needed?
I had to update the implementation a bit to fix internal unittests failing. Also addressed comments. PTAL. https://codereview.chromium.org/2647323010/diff/100001/chromecast/browser/cas... File chromecast/browser/cast_browser_process.cc (right): https://codereview.chromium.org/2647323010/diff/100001/chromecast/browser/cas... chromecast/browser/cast_browser_process.cc:7: #include <string> On 2017/01/27 23:37:17, wzhong wrote: > Needed? Or lint complains? Done. https://codereview.chromium.org/2647323010/diff/100001/chromecast/browser/cas... chromecast/browser/cast_browser_process.cc:23: #include "components/proxy_config/proxy_config_pref_names.h" On 2017/01/27 23:37:17, wzhong wrote: > Do we need so many header file for adding just a couple of lines at 105? Done. https://codereview.chromium.org/2647323010/diff/100001/chromecast/browser/cas... File chromecast/browser/cast_browser_process.h (right): https://codereview.chromium.org/2647323010/diff/100001/chromecast/browser/cas... chromecast/browser/cast_browser_process.h:101: std::unique_ptr<PrefProxyConfigTracker> pref_proxy_config_tracker_; On 2017/01/27 23:37:17, wzhong wrote: > This seems to need to be after pref_service_. See comments in line 102. Done. https://codereview.chromium.org/2647323010/diff/100001/chromecast/browser/url... File chromecast/browser/url_request_context_factory.cc (right): https://codereview.chromium.org/2647323010/diff/100001/chromecast/browser/url... chromecast/browser/url_request_context_factory.cc:36: #include "net/proxy/proxy_config_service_fixed.h" On 2017/01/27 23:37:17, wzhong wrote: > Needed? Done.
The CQ bit was checked by almasrymina@chromium.org to run a CQ dry run
The CQ bit was unchecked by almasrymina@chromium.org
The CQ bit was checked by almasrymina@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
It's not straightforward to have ConnectivityChecker use the proxy objects passed in from cast_browser_main_parts.cc. The reason is that the proxy objects need to be initialized on the IO thread, while the constructor of ConnectivityChecker runs on the UI thread. That means that ConnectivityChecker needs to take a dependency on cast_browser_process to query the singleton on the UI thread. In my CL ConnectivityChecker has a dependency on PrefService which I think is less severe than depending on cast_browser_process. Also, not sure I want to move all these things around so much since this is needed before FC.
The new test failure here is quite problematic too. it's a DCHECK is in the proxy_service_ destructor. proxy_service_ cannot be destroyed on the UI thread, since it's used on the IO thread. I'm not sure what could be done here... need to investigate.
Seems like using pref_proxy_config_tracker for this is really problematic. The pref_proxy_config_tracker_ instance needs to be created and destroyed on the UI thread, but the proxy_config_service it generates, needs to be created and destroyed on the IO thread. Only option I see here is to create the pref_proxy_config_tracker objects on the UI thread via a method on CastBrowserProcess. Then CastBrowserProcess can delete the pref_proxy_config_trackers when it's destroyed. The pref_proxy_config_tracker instances can be used on the UI thread and destroyed on the UI thread. It's also very problematic that pref_proxy_config_tracker->DetatchFromPrefService needs to be called on the UI thread before the proxy_config_service is destoryed on the IO thread... not sure at all how to sync that. But even if we could, this would mean that everywhere we need a proxy_service we'll have to take a dependency on CastBrowserProcess. Need to reconsider why we're using pref_proxy_config_tracker at all. It's causing a huge headache for me given our pref_service runs on the UI thread.
Ah, I guess I might be able to use: io_task_runner_->DeleteSoon(FROM_HERE, proxy_server_.release()); in the destructor of CastBrowserProcess, which would fix this issue similar to what we do in connectivity_checker. That should work around this problem I think.
The CQ bit was checked by almasrymina@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by almasrymina@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by almasrymina@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2647323010/diff/240001/chromecast/browser/cas... File chromecast/browser/cast_browser_process.cc (right): https://codereview.chromium.org/2647323010/diff/240001/chromecast/browser/cas... chromecast/browser/cast_browser_process.cc:106: pref_proxy_config_tracker_impl_ = I'd rather you have a separate function SetPrefProxyConfigTrackImpl(). https://codereview.chromium.org/2647323010/diff/240001/chromecast/browser/cas... File chromecast/browser/cast_browser_process.h (right): https://codereview.chromium.org/2647323010/diff/240001/chromecast/browser/cas... chromecast/browser/cast_browser_process.h:15: class PrefProxyConfigTrackerImpl; Order https://codereview.chromium.org/2647323010/diff/240001/chromecast/browser/cas... chromecast/browser/cast_browser_process.h:123: std::unique_ptr<PrefProxyConfigTrackerImpl> pref_proxy_config_tracker_impl_; Move to between 105 and 106. Check comment in line 99. https://codereview.chromium.org/2647323010/diff/240001/chromecast/net/connect... File chromecast/net/connectivity_checker_impl.h (right): https://codereview.chromium.org/2647323010/diff/240001/chromecast/net/connect... chromecast/net/connectivity_checker_impl.h:44: void DetachFromPrefService() override; Can you add a Getter pref_proxy_config_tracker() so it is more readable that the proxy config tracker is detaching?
I actually think that whether connectivity_checker needs to go through proxy settings is debatable. Similarly, proxy
Another thought: can we pass const scoped_refptr<net::URLRequestContextGetter>& request_context around to connectivity_checker and to cast_receiver?
The CQ bit was checked by almasrymina@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/07 16:30:31, wzhong wrote: > Another thought: can we pass const scoped_refptr<net::URLRequestContextGetter>& > request_context around to connectivity_checker and to cast_receiver? Changed to that and the problematic unittests pass. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Great! Much simpler. https://codereview.chromium.org/2647323010/diff/280001/chromecast/net/connect... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/2647323010/diff/280001/chromecast/net/connect... chromecast/net/connectivity_checker_impl.cc:83: url_request_context_ = url_request_context_getter_->GetURLRequestContext(); Nit: can url_request_context_ initialized in constructor? Do we still need url_request_context_getter_?
almasrymina@chromium.org changed reviewers: + stevenjb@chromium.org
Steven, we're using your proxy_config component to add proxy support for chromecast. PTAL.
owner lgtm
just nits https://codereview.chromium.org/2647323010/diff/280001/chromecast/browser/cas... File chromecast/browser/cast_browser_process.cc (right): https://codereview.chromium.org/2647323010/diff/280001/chromecast/browser/cas... chromecast/browser/cast_browser_process.cc:21: #include "net/proxy/proxy_service.h" unused? think you can revert this whole file now? https://codereview.chromium.org/2647323010/diff/280001/chromecast/browser/url... File chromecast/browser/url_request_context_factory.cc (right): https://codereview.chromium.org/2647323010/diff/280001/chromecast/browser/url... chromecast/browser/url_request_context_factory.cc:7: #include <algorithm> not needed? https://codereview.chromium.org/2647323010/diff/280001/chromecast/browser/url... File chromecast/browser/url_request_context_factory.h (right): https://codereview.chromium.org/2647323010/diff/280001/chromecast/browser/url... chromecast/browser/url_request_context_factory.h:129: std::unique_ptr<PrefProxyConfigTracker> pref_proxy_config_tracker_impl_; should include <memory> https://codereview.chromium.org/2647323010/diff/280001/chromecast/net/connect... File chromecast/net/connectivity_checker_impl.h (right): https://codereview.chromium.org/2647323010/diff/280001/chromecast/net/connect... chromecast/net/connectivity_checker_impl.h:38: net::URLRequestContextGetter* url_request_context_getter); forward declare this class?
https://codereview.chromium.org/2647323010/diff/240001/chromecast/browser/cas... File chromecast/browser/cast_browser_process.cc (right): https://codereview.chromium.org/2647323010/diff/240001/chromecast/browser/cas... chromecast/browser/cast_browser_process.cc:106: pref_proxy_config_tracker_impl_ = On 2017/02/07 15:56:02, wzhong wrote: > I'd rather you have a separate function SetPrefProxyConfigTrackImpl(). Removed with new approach. https://codereview.chromium.org/2647323010/diff/240001/chromecast/browser/cas... File chromecast/browser/cast_browser_process.h (right): https://codereview.chromium.org/2647323010/diff/240001/chromecast/browser/cas... chromecast/browser/cast_browser_process.h:15: class PrefProxyConfigTrackerImpl; On 2017/02/07 15:56:03, wzhong wrote: > Order Removed. https://codereview.chromium.org/2647323010/diff/240001/chromecast/browser/cas... chromecast/browser/cast_browser_process.h:123: std::unique_ptr<PrefProxyConfigTrackerImpl> pref_proxy_config_tracker_impl_; On 2017/02/07 15:56:03, wzhong wrote: > Move to between 105 and 106. Check comment in line 99. Removed with new approach. https://codereview.chromium.org/2647323010/diff/240001/chromecast/net/connect... File chromecast/net/connectivity_checker_impl.h (right): https://codereview.chromium.org/2647323010/diff/240001/chromecast/net/connect... chromecast/net/connectivity_checker_impl.h:44: void DetachFromPrefService() override; On 2017/02/07 15:56:03, wzhong wrote: > Can you add a Getter pref_proxy_config_tracker() so it is more readable that the > proxy config tracker is detaching? Removed with new approach. https://codereview.chromium.org/2647323010/diff/280001/chromecast/browser/cas... File chromecast/browser/cast_browser_process.cc (right): https://codereview.chromium.org/2647323010/diff/280001/chromecast/browser/cas... chromecast/browser/cast_browser_process.cc:21: #include "net/proxy/proxy_service.h" On 2017/02/08 00:07:06, halliwell wrote: > unused? think you can revert this whole file now? Done. https://codereview.chromium.org/2647323010/diff/280001/chromecast/browser/url... File chromecast/browser/url_request_context_factory.cc (right): https://codereview.chromium.org/2647323010/diff/280001/chromecast/browser/url... chromecast/browser/url_request_context_factory.cc:7: #include <algorithm> On 2017/02/08 00:07:06, halliwell wrote: > not needed? Done. https://codereview.chromium.org/2647323010/diff/280001/chromecast/browser/url... File chromecast/browser/url_request_context_factory.h (right): https://codereview.chromium.org/2647323010/diff/280001/chromecast/browser/url... chromecast/browser/url_request_context_factory.h:129: std::unique_ptr<PrefProxyConfigTracker> pref_proxy_config_tracker_impl_; On 2017/02/08 00:07:06, halliwell wrote: > should include <memory> Done. https://codereview.chromium.org/2647323010/diff/280001/chromecast/net/connect... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/2647323010/diff/280001/chromecast/net/connect... chromecast/net/connectivity_checker_impl.cc:83: url_request_context_ = url_request_context_getter_->GetURLRequestContext(); On 2017/02/07 22:00:06, wzhong wrote: > Nit: can url_request_context_ initialized in constructor? > > Do we still need url_request_context_getter_? > Done. https://codereview.chromium.org/2647323010/diff/280001/chromecast/net/connect... File chromecast/net/connectivity_checker_impl.h (right): https://codereview.chromium.org/2647323010/diff/280001/chromecast/net/connect... chromecast/net/connectivity_checker_impl.h:38: net::URLRequestContextGetter* url_request_context_getter); On 2017/02/08 00:07:06, halliwell wrote: > forward declare this class? Done.
On 2017/02/08 00:48:04, almasrymina wrote: > https://codereview.chromium.org/2647323010/diff/240001/chromecast/browser/cas... > File chromecast/browser/cast_browser_process.cc (right): > > https://codereview.chromium.org/2647323010/diff/240001/chromecast/browser/cas... > chromecast/browser/cast_browser_process.cc:106: pref_proxy_config_tracker_impl_ > = > On 2017/02/07 15:56:02, wzhong wrote: > > I'd rather you have a separate function SetPrefProxyConfigTrackImpl(). > > Removed with new approach. > > https://codereview.chromium.org/2647323010/diff/240001/chromecast/browser/cas... > File chromecast/browser/cast_browser_process.h (right): > > https://codereview.chromium.org/2647323010/diff/240001/chromecast/browser/cas... > chromecast/browser/cast_browser_process.h:15: class PrefProxyConfigTrackerImpl; > On 2017/02/07 15:56:03, wzhong wrote: > > Order > > Removed. > > https://codereview.chromium.org/2647323010/diff/240001/chromecast/browser/cas... > chromecast/browser/cast_browser_process.h:123: > std::unique_ptr<PrefProxyConfigTrackerImpl> pref_proxy_config_tracker_impl_; > On 2017/02/07 15:56:03, wzhong wrote: > > Move to between 105 and 106. Check comment in line 99. > > Removed with new approach. > > https://codereview.chromium.org/2647323010/diff/240001/chromecast/net/connect... > File chromecast/net/connectivity_checker_impl.h (right): > > https://codereview.chromium.org/2647323010/diff/240001/chromecast/net/connect... > chromecast/net/connectivity_checker_impl.h:44: void DetachFromPrefService() > override; > On 2017/02/07 15:56:03, wzhong wrote: > > Can you add a Getter pref_proxy_config_tracker() so it is more readable that > the > > proxy config tracker is detaching? > > Removed with new approach. > > https://codereview.chromium.org/2647323010/diff/280001/chromecast/browser/cas... > File chromecast/browser/cast_browser_process.cc (right): > > https://codereview.chromium.org/2647323010/diff/280001/chromecast/browser/cas... > chromecast/browser/cast_browser_process.cc:21: #include > "net/proxy/proxy_service.h" > On 2017/02/08 00:07:06, halliwell wrote: > > unused? think you can revert this whole file now? > > Done. > > https://codereview.chromium.org/2647323010/diff/280001/chromecast/browser/url... > File chromecast/browser/url_request_context_factory.cc (right): > > https://codereview.chromium.org/2647323010/diff/280001/chromecast/browser/url... > chromecast/browser/url_request_context_factory.cc:7: #include <algorithm> > On 2017/02/08 00:07:06, halliwell wrote: > > not needed? > > Done. > > https://codereview.chromium.org/2647323010/diff/280001/chromecast/browser/url... > File chromecast/browser/url_request_context_factory.h (right): > > https://codereview.chromium.org/2647323010/diff/280001/chromecast/browser/url... > chromecast/browser/url_request_context_factory.h:129: > std::unique_ptr<PrefProxyConfigTracker> pref_proxy_config_tracker_impl_; > On 2017/02/08 00:07:06, halliwell wrote: > > should include <memory> > > Done. > > https://codereview.chromium.org/2647323010/diff/280001/chromecast/net/connect... > File chromecast/net/connectivity_checker_impl.cc (right): > > https://codereview.chromium.org/2647323010/diff/280001/chromecast/net/connect... > chromecast/net/connectivity_checker_impl.cc:83: url_request_context_ = > url_request_context_getter_->GetURLRequestContext(); > On 2017/02/07 22:00:06, wzhong wrote: > > Nit: can url_request_context_ initialized in constructor? > > > > Do we still need url_request_context_getter_? > > > > Done. > > https://codereview.chromium.org/2647323010/diff/280001/chromecast/net/connect... > File chromecast/net/connectivity_checker_impl.h (right): > > https://codereview.chromium.org/2647323010/diff/280001/chromecast/net/connect... > chromecast/net/connectivity_checker_impl.h:38: net::URLRequestContextGetter* > url_request_context_getter); > On 2017/02/08 00:07:06, halliwell wrote: > > forward declare this class? > > Done. lgtm
The CQ bit was checked by almasrymina@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wzhong@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2647323010/#ps300001 (title: "Address cleanup comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by almasrymina@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wzhong@chromium.org, halliwell@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2647323010/#ps320001 (title: "Fixing test break.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1486519951296370,
"parent_rev": "e692b9e03d389ce076f3a57e27e47dfe8db9f4e6", "commit_rev":
"dfb5b36388a651bf0651f6c564563911fc927c7f"}
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Add proxy server support to chromecast Proxy server support can be useful for testing purposes. This enables cast_shell to support proxies via the "proxy-server" and "proxy-bypass-list" options stored in pref_service. These options behave exactly as chrome's --proxy-server and --proxy-bypass-list. Coming in another CL is a setup API that allows updating these proxy settings. BUG= internal b/34689274 TEST= manual testing with local proxy server Change-Id: I1284afe626ccfdcafd07cd891b38662fbd46a7a3 ========== to ========== [Chromecast] Add proxy server support to chromecast Proxy server support can be useful for testing purposes. This enables cast_shell to support proxies via the "proxy-server" and "proxy-bypass-list" options stored in pref_service. These options behave exactly as chrome's --proxy-server and --proxy-bypass-list. Coming in another CL is a setup API that allows updating these proxy settings. BUG= internal b/34689274 TEST= manual testing with local proxy server Change-Id: I1284afe626ccfdcafd07cd891b38662fbd46a7a3 Review-Url: https://codereview.chromium.org/2647323010 Cr-Commit-Position: refs/heads/master@{#448896} Committed: https://chromium.googlesource.com/chromium/src/+/dfb5b36388a651bf0651f6c56456... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/dfb5b36388a651bf0651f6c56456... |
