Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
508473002:
Componentize component_updater: Move URLRequestPrepackagedInterceptor from content/ to net/ (Closed)
|
Created:
6 years, 4 months ago by tommycli Modified:
6 years, 3 months ago Reviewers:
Yoyo Zhou, willchan no longer on Chromium, Ryan Sleevi, Lei Zhang, wtc, Sorin Jianu, pauljensen, blundell CC:
chromium-reviews, chromium-apps-reviews_chromium.org, darin-cc_chromium.org, jam, cbentzel+watch_chromium.org, extensions-reviews_chromium.org, waffles Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionComponentize component_updater: Move URLRequestPrepackagedInterceptor from content/ to net/.
This patch breaks the URLRequestPrepackagedInterceptor dependencies on content/.
We want to use URLRequestPrepackagedInterceptor in unit tests that will be in components/. These tests cannot depend on content/.
BUG=371463
TBR=wtc@chromium.org
Committed: https://crrev.com/eaae5d98d9adb42a70c5529dbb6c70e95e848701
Cr-Commit-Position: refs/heads/master@{#293861}
Patch Set 1 #Patch Set 2 : move to net #Patch Set 3 : self review #
Total comments: 22
Patch Set 4 : #Patch Set 5 : rebase onto patch that moves tests into components/ #Patch Set 6 : rename to test_url_request_interceptor #Patch Set 7 : run git cl format #
Total comments: 2
Patch Set 8 : Oops. Patchsets 5-7 include some junk because I didn't set the parent branch correctly. #Patch Set 9 : limit git cl format to only portions relevant to this patch #Patch Set 10 : #Patch Set 11 : rebase onto origin/master #
Total comments: 2
Patch Set 12 : #
Total comments: 4
Patch Set 13 : address rsleevi comments #Patch Set 14 : #Patch Set 15 : merge #
Total comments: 21
Patch Set 16 : #Patch Set 17 : #Patch Set 18 : rebase #Patch Set 19 : rebase. #Patch Set 20 : add to net/BUILD.gn #Patch Set 21 : fix typo #Messages
Total messages: 56 (4 generated)
tommycli@chromium.org changed reviewers: + blundell@chromium.org, sorin@chromium.org, wtc@chromium.org
wtc, sorin, blundell: PTAL This is required to move component_updater tests to components/ without introducing content/ dependencies.
blundell@chromium.org changed reviewers: + rsleevi@chromium.org, willchan@chromium.org
+rsleevi@, willchan@ Ryan and/or Will might have feedback here as well.
willchan@chromium.org changed reviewers: + pauljensen@chromium.org
+pauljensen I'm going to defer to Paul on this, but my inclination is to NACK this from net/. Here are the problems I see with it, and I defer to Paul as to whether he agrees, and if so, how to deal with it: * Interceptors are deprecated, we should be killing them off * This moves an existing interceptor from content/test to net/, which makes it more visible and removes the "test" part of it, which may increase its usage (which goes against our deprecation process for it). I don't necessarily expect you to fix the interceptor usage now. That's a gradual deprecation process. But I somewhat object to legitimizing this Interceptor by moving it into net/.
Review comments on patch set 3: https://codereview.chromium.org/508473002/diff/40001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/508473002/diff/40001/content/content_tests.gy... content/content_tests.gypi:134: 'test/net/url_request_slow_download_job.h', I assume the rest of the test/net/url_request_* are not a problem for componentization? https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... File net/url_request/url_request_prepackaged_interceptor.h (right): https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... net/url_request/url_request_prepackaged_interceptor.h:25: class URLRequestPrepackagedInterceptor { I think chrome/browser/net is a better home for this class because all the files using this class are under chrome/browser.
On 2014/08/26 18:06:23, willchan wrote: > +pauljensen > > I'm going to defer to Paul on this, but my inclination is to NACK this from > net/. Here are the problems I see with it, and I defer to Paul as to whether he > agrees, and if so, how to deal with it: > > * Interceptors are deprecated, we should be killing them off Yes, but no one's actively working on this, are they? > * This moves an existing interceptor from content/test to net/, which makes it > more visible and removes the "test" part of it, which may increase its usage > (which goes against our deprecation process for it). That was my scan from reading this CL this morning, and I'm glad you commented on it and beat me to it. The biggest issue is that this should only be used for test code, but I suspect if you're wanting to use it from iOS, it's no longer being used for test code. That's the reason to NACK it. If it was in //net/test, I think I'd be less worried. > > I don't necessarily expect you to fix the interceptor usage now. That's a > gradual deprecation process. But I somewhat object to legitimizing this > Interceptor by moving it into net/. https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... File net/url_request/url_request_prepackaged_interceptor.h (right): https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... net/url_request/url_request_prepackaged_interceptor.h:25: class URLRequestPrepackagedInterceptor { On 2014/08/26 18:15:59, wtc wrote: > > I think chrome/browser/net is a better home for this class because all the files > using this class are under chrome/browser. Except that they wish to use it from iOS
wtc/willchan, addressed your comments below: https://codereview.chromium.org/508473002/diff/40001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/508473002/diff/40001/content/content_tests.gy... content/content_tests.gypi:134: 'test/net/url_request_slow_download_job.h', On 2014/08/26 18:15:59, wtc wrote: > > I assume the rest of the test/net/url_request_* are not a problem for > componentization? Not a problem, as far as i can tell. https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... File net/url_request/url_request_prepackaged_interceptor.h (right): https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... net/url_request/url_request_prepackaged_interceptor.h:25: class URLRequestPrepackagedInterceptor { On 2014/08/26 18:15:59, wtc wrote: > > I think chrome/browser/net is a better home for this class because all the files > using this class are under chrome/browser. I can't do that, as components/ cannot depend on chrome/. I could offer to make a new component under /components/url_request, but that's probably overkill for just one cc/h file.
I just glanced at this. This "interceptor" is just a URLRequestFilter user, not a URLRequest::Interceptor. URLRequest::Interceptor is the deprecated and almost-dead interface that I'd never approve another use of. URLRequestFilter is AFAIK well supported, though strictly for testing only. So on this grounds I wouldn't shoot this down. I do agree with Will on a couple other points though: 1. moving to a directory without test in the file or directory name is misleading 2. moving to net/ may not be the best, perhaps components/
rsleevi: To clarify, this is ONLY for test code that runs under iOS. These files are only in the net_test_support target. I have no objection to putting it under net/test. I was just following the example of test_url_fetcher_factory and url_request_test_util. On 2014/08/26 18:21:09, Ryan Sleevi wrote: > On 2014/08/26 18:06:23, willchan wrote: > > +pauljensen > > > > I'm going to defer to Paul on this, but my inclination is to NACK this from > > net/. Here are the problems I see with it, and I defer to Paul as to whether > he > > agrees, and if so, how to deal with it: > > > > * Interceptors are deprecated, we should be killing them off > > Yes, but no one's actively working on this, are they? > > > * This moves an existing interceptor from content/test to net/, which makes it > > more visible and removes the "test" part of it, which may increase its usage > > (which goes against our deprecation process for it). > > That was my scan from reading this CL this morning, and I'm glad you commented > on it and beat me to it. The biggest issue is that this should only be used for > test code, but I suspect if you're wanting to use it from iOS, it's no longer > being used for test code. That's the reason to NACK it. > > If it was in //net/test, I think I'd be less worried. > > > > > I don't necessarily expect you to fix the interceptor usage now. That's a > > gradual deprecation process. But I somewhat object to legitimizing this > > Interceptor by moving it into net/. > > https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... > File net/url_request/url_request_prepackaged_interceptor.h (right): > > https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... > net/url_request/url_request_prepackaged_interceptor.h:25: class > URLRequestPrepackagedInterceptor { > On 2014/08/26 18:15:59, wtc wrote: > > > > I think chrome/browser/net is a better home for this class because all the > files > > using this class are under chrome/browser. > > Except that they wish to use it from iOS
On 2014/08/26 18:23:50, tommycli wrote: > rsleevi: > > To clarify, this is ONLY for test code that runs under iOS. These files are only > in the net_test_support target. > > I have no objection to putting it under net/test. I was just following the > example of test_url_fetcher_factory and url_request_test_util. Right, which both have Test in their name, hence start the spidey-sense tingling. Will, Paul - my feeling is incorporating a "Test" - either into the class name or via the file name - is probably fine within //net. Thoughts?
On 2014/08/26 18:26:24, Ryan Sleevi wrote: > On 2014/08/26 18:23:50, tommycli wrote: > > rsleevi: > > > > To clarify, this is ONLY for test code that runs under iOS. These files are > only > > in the net_test_support target. > > > > I have no objection to putting it under net/test. I was just following the > > example of test_url_fetcher_factory and url_request_test_util. > > Right, which both have Test in their name, hence start the spidey-sense > tingling. > > Will, Paul - my feeling is incorporating a "Test" - either into the class name > or via the file name - is probably fine within //net. Thoughts? Oh, and I will add I would have found this useful in past tests too if it was available to //net.
https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... line 43 shows a class inheriting from net::URLRequestInterceptor. On Tue, Aug 26, 2014 at 11:22 AM, <pauljensen@chromium.org> wrote: > I just glanced at this. This "interceptor" is just a URLRequestFilter > user, not > a URLRequest::Interceptor. URLRequest::Interceptor is the deprecated and > almost-dead interface that I'd never approve another use of. > URLRequestFilter > is AFAIK well supported, though strictly for testing only. So on this > grounds I > wouldn't shoot this down. > > I do agree with Will on a couple other points though: > 1. moving to a directory without test in the file or directory name is > misleading > 2. moving to net/ may not be the best, perhaps components/ > > https://codereview.chromium.org/508473002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/26 18:32:01, willchan wrote: > https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... > line 43 shows a class inheriting from net::URLRequestInterceptor. URLRequestInterceptor not URLRequest::Interceptor.
I can only hope the URLRequest::Interceptor deprecation moves faster so we don't have this FooBar and Foo::Bar naming confusion too long :) Lol. OK, I withdraw my complaint about the URLRequest::Interceptor, and as long as the test naming is fine, I'm cool with this. On Tue, Aug 26, 2014 at 11:35 AM, <pauljensen@chromium.org> wrote: > On 2014/08/26 18:32:01, willchan wrote: > > https://codereview.chromium.org/508473002/diff/40001/net/ > url_request/url_request_prepackaged_interceptor.cc > >> line 43 shows a class inheriting from net::URLRequestInterceptor. >> > > URLRequestInterceptor not URLRequest::Interceptor. > > https://codereview.chromium.org/508473002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
willchan: nice... net/ owners: I'm cool with whatever you guys want to call it. Maybe simply TestURLRequestInterceptor? The Prepackaged thing wasn't too educational for me anyways... It really just serves static files at specified URLs. On 2014/08/26 18:41:36, willchan wrote: > I can only hope the URLRequest::Interceptor deprecation moves faster so we > don't have this FooBar and Foo::Bar naming confusion too long :) Lol. > > OK, I withdraw my complaint about the URLRequest::Interceptor, and as long > as the test naming is fine, I'm cool with this. > > > On Tue, Aug 26, 2014 at 11:35 AM, <mailto:pauljensen@chromium.org> wrote: > > > On 2014/08/26 18:32:01, willchan wrote: > > > > https://codereview.chromium.org/508473002/diff/40001/net/ > > url_request/url_request_prepackaged_interceptor.cc > > > >> line 43 shows a class inheriting from net::URLRequestInterceptor. > >> > > > > URLRequestInterceptor not URLRequest::Interceptor. > > > > https://codereview.chromium.org/508473002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2014/08/26 18:44:32, tommycli wrote: > willchan: nice... > > net/ owners: I'm cool with whatever you guys want to call it. Maybe simply > TestURLRequestInterceptor? The Prepackaged thing wasn't too educational for me > anyways... TestURLRequestInterceptor is already taken :) I've already combined two classes that do this kind of thing into one. It may be worth combining this third class that does this with the others.
On 2014/08/26 18:48:34, pauljensen wrote: > TestURLRequestInterceptor is already taken :) Oh actually that's in a separate namespace, but my comment about combining these still stands.
On 2014/08/26 18:49:45, pauljensen wrote: > On 2014/08/26 18:48:34, pauljensen wrote: > > TestURLRequestInterceptor is already taken :) > Oh actually that's in a separate namespace, but my comment about combining these > still stands. Oh wait, when combining them I used the one being moved in this CL...haha, please ignore me.
On 2014/08/26 18:49:45, pauljensen wrote: > On 2014/08/26 18:48:34, pauljensen wrote: > > TestURLRequestInterceptor is already taken :) > Oh actually that's in a separate namespace, but my comment about combining these > still stands. Yeah, I think it makes sense to combine them. Probably in a separate CL since this one is more focused on getting the componentization stuff done. Any thoughts on what we should name this class in the meantime?
On 2014/08/26 18:52:41, pauljensen wrote: > On 2014/08/26 18:49:45, pauljensen wrote: > > On 2014/08/26 18:48:34, pauljensen wrote: > > > TestURLRequestInterceptor is already taken :) > > Oh actually that's in a separate namespace, but my comment about combining > these > > still stands. > Oh wait, when combining them I used the one being moved in this CL...haha, > please ignore me. Oh okay. The one other TestURLRequestInterceptor is in an anonymous namespace. If you'd like, we can make this one the canonical TestURLRequestInterceptor to rule the other ones.
Thank you Tommy. I am asking a couple of question to understand the direction this code is going. https://codereview.chromium.org/508473002/diff/40001/chrome/browser/component... File chrome/browser/component_updater/test/component_updater_service_unittest.cc (right): https://codereview.chromium.org/508473002/diff/40001/chrome/browser/component... chrome/browser/component_updater/test/component_updater_service_unittest.cc:21: #include "content/public/browser/browser_thread.h" Are we doing something about the browser thread? https://codereview.chromium.org/508473002/diff/40001/chrome/browser/component... chrome/browser/component_updater/test/component_updater_service_unittest.cc:22: #include "content/public/browser/resource_controller.h" Tommy, what have we decided to do about the resource controllers? I don't recall precisely. https://codereview.chromium.org/508473002/diff/40001/chrome/browser/component... File chrome/browser/component_updater/test/component_updater_service_unittest.h (right): https://codereview.chromium.org/508473002/diff/40001/chrome/browser/component... chrome/browser/component_updater/test/component_updater_service_unittest.h:17: #include "content/public/test/test_browser_thread_bundle.h" is there a plan to move the thread bundle too? https://codereview.chromium.org/508473002/diff/40001/chrome/browser/component... File chrome/browser/component_updater/test/crx_downloader_unittest.cc (right): https://codereview.chromium.org/508473002/diff/40001/chrome/browser/component... chrome/browser/component_updater/test/crx_downloader_unittest.cc:116: NULL /* background_task_runner */)); Consider not using the inline /* */. https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... File net/url_request/url_request_prepackaged_interceptor.cc (right): https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... net/url_request/url_request_prepackaged_interceptor.cc:73: CHECK(io_task_runner_->RunsTasksOnCurrentThread()); I wonder if these CHECKS were correct to begin with. Can you please ask the net owners? See http://www.chromium.org/developers/coding-style https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... File net/url_request/url_request_prepackaged_interceptor.h (right): https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... net/url_request/url_request_prepackaged_interceptor.h:32: const scoped_refptr<base::TaskRunner>& io_task_runner, Any reason we are passing ref counted pointers by reference? See http://www.chromium.org/developers/smart-pointer-guidelines https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... net/url_request/url_request_prepackaged_interceptor.h:54: const scoped_refptr<base::TaskRunner> io_task_runner_; I wonder what is the meaning of the "const" object here. Why invariant are we trying to enforce?
sorin: Responded to your comments. Thanks. https://codereview.chromium.org/508473002/diff/40001/chrome/browser/component... File chrome/browser/component_updater/test/component_updater_service_unittest.cc (right): https://codereview.chromium.org/508473002/diff/40001/chrome/browser/component... chrome/browser/component_updater/test/component_updater_service_unittest.cc:21: #include "content/public/browser/browser_thread.h" On 2014/08/26 20:32:51, Sorin Jianu wrote: > Are we doing something about the browser thread? Hi. The CUS unit-test is going to stay put for now. It would need to be split into two separate tests to be moved. See discussion in https://codereview.chromium.org/493953002/ https://codereview.chromium.org/508473002/diff/40001/chrome/browser/component... chrome/browser/component_updater/test/component_updater_service_unittest.cc:22: #include "content/public/browser/resource_controller.h" On 2014/08/26 20:32:51, Sorin Jianu wrote: > Tommy, what have we decided to do about the resource controllers? I don't recall > precisely. We decided that the ResourceThrottle is going to stay in content/. ComponentUpdaterResourceThrottle is going to stay in chrome/. iOS will have to use a different throttling mechanism. https://codereview.chromium.org/508473002/diff/40001/chrome/browser/component... File chrome/browser/component_updater/test/component_updater_service_unittest.h (right): https://codereview.chromium.org/508473002/diff/40001/chrome/browser/component... chrome/browser/component_updater/test/component_updater_service_unittest.h:17: #include "content/public/test/test_browser_thread_bundle.h" On 2014/08/26 20:32:52, Sorin Jianu wrote: > is there a plan to move the thread bundle too? If and when we move the CUS unittest, then yes. waffles mentioned that this unittest was probably going to be rewritten anyways, so I'm leaving it alone for now. https://codereview.chromium.org/508473002/diff/40001/chrome/browser/component... File chrome/browser/component_updater/test/crx_downloader_unittest.cc (right): https://codereview.chromium.org/508473002/diff/40001/chrome/browser/component... chrome/browser/component_updater/test/crx_downloader_unittest.cc:116: NULL /* background_task_runner */)); On 2014/08/26 20:32:52, Sorin Jianu wrote: > Consider not using the inline /* */. Done. https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... File net/url_request/url_request_prepackaged_interceptor.cc (right): https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... net/url_request/url_request_prepackaged_interceptor.cc:73: CHECK(io_task_runner_->RunsTasksOnCurrentThread()); On 2014/08/26 20:32:52, Sorin Jianu wrote: > I wonder if these CHECKS were correct to begin with. Can you please ask the net > owners? See http://www.chromium.org/developers/coding-style I personally think they should be DCHECKs. I changed them. https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... File net/url_request/url_request_prepackaged_interceptor.h (right): https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... net/url_request/url_request_prepackaged_interceptor.h:32: const scoped_refptr<base::TaskRunner>& io_task_runner, On 2014/08/26 20:32:52, Sorin Jianu wrote: > Any reason we are passing ref counted pointers by reference? See > http://www.chromium.org/developers/smart-pointer-guidelines Quote from that page: The one exception is functions can takes a const scoped_refptr& as a parameter which may avoid extra refcount churn. https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... net/url_request/url_request_prepackaged_interceptor.h:54: const scoped_refptr<base::TaskRunner> io_task_runner_; On 2014/08/26 20:32:52, Sorin Jianu wrote: > I wonder what is the meaning of the "const" object here. Why invariant are we > trying to enforce? I thought: After we initialize its value, it cannot be reassigned to point to something else?
net/ OWNERS: PTAL. I renamed it to TestURLRequestInterceptor to fit in with the naming guidelines under net/url_request. https://codereview.chromium.org/508473002/diff/120001/net/url_request/test_ur... File net/url_request/test_url_request_interceptor.cc (right): https://codereview.chromium.org/508473002/diff/120001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.cc:72: DCHECK(io_task_runner_->RunsTasksOnCurrentThread()); This used to be a CHECK. Changed to DCHECK. This correct? https://codereview.chromium.org/508473002/diff/120001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.cc:97: DCHECK(io_task_runner_->RunsTasksOnCurrentThread()); And here.
lgtm Thank you Tommy! https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... File net/url_request/url_request_prepackaged_interceptor.h (right): https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... net/url_request/url_request_prepackaged_interceptor.h:32: const scoped_refptr<base::TaskRunner>& io_task_runner, On 2014/08/26 21:06:58, tommycli wrote: > On 2014/08/26 20:32:52, Sorin Jianu wrote: > > Any reason we are passing ref counted pointers by reference? See > > http://www.chromium.org/developers/smart-pointer-guidelines > > Quote from that page: > > The one exception is functions can takes a const scoped_refptr& as a parameter > which may avoid extra refcount churn. ok, sg, using CS shows that passing scoped_refptr by ref is more common too. https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... net/url_request/url_request_prepackaged_interceptor.h:54: const scoped_refptr<base::TaskRunner> io_task_runner_; On 2014/08/26 21:06:58, tommycli wrote: > On 2014/08/26 20:32:52, Sorin Jianu wrote: > > I wonder what is the meaning of the "const" object here. Why invariant are we > > trying to enforce? > > I thought: After we initialize its value, it cannot be reassigned to point to > something else? I think this is a strange idiom. It is hard to count by CS (not enough context to filter the declarations of class members) but it seems to me, empirically, that people declare more non-const members than const members. You are right to say that it prevents future assignments to the smart pointer member, but the idiom is odd in the sense that that it gives the impression that the programmer intended to forbid calling non-const functions on the object that the smart pointer wraps. Unlike class members of builtin types or unmanaged user-define types, which can be easily and accidentally modified by the members of the object, it requires effort to change a smart pointer. I assume that's why people don't bother with the const declaration here. I would not use a const declaration here but that is only a personal preference.
sorin: Thanks. I changed the pointer to non-const. Makes sense. https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... File net/url_request/url_request_prepackaged_interceptor.h (right): https://codereview.chromium.org/508473002/diff/40001/net/url_request/url_requ... net/url_request/url_request_prepackaged_interceptor.h:54: const scoped_refptr<base::TaskRunner> io_task_runner_; On 2014/08/26 23:31:49, Sorin Jianu wrote: > On 2014/08/26 21:06:58, tommycli wrote: > > On 2014/08/26 20:32:52, Sorin Jianu wrote: > > > I wonder what is the meaning of the "const" object here. Why invariant are > we > > > trying to enforce? > > > > I thought: After we initialize its value, it cannot be reassigned to point to > > something else? > > I think this is a strange idiom. It is hard to count by CS (not enough context > to filter the declarations of class members) but it seems to me, empirically, > that people declare more non-const members than const members. > > You are right to say that it prevents future assignments to the smart pointer > member, but the idiom is odd in the sense that that it gives the impression that > the programmer intended to forbid calling non-const functions on the object that > the smart pointer wraps. > > Unlike class members of builtin types or unmanaged user-define types, which can > be easily and accidentally modified by the members of the object, it requires > effort to change a smart pointer. I assume that's why people don't bother with > the const declaration here. > > I would not use a const declaration here but that is only a personal preference. Oh I see. You're saying it gives the false impression that the object itself is const. Okay. Sounds good. I changed the pointer to non-const.
LGTM https://codereview.chromium.org/508473002/diff/200001/net/url_request/test_ur... File net/url_request/test_url_request_interceptor.cc (right): https://codereview.chromium.org/508473002/diff/200001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.cc:20: class URLRequestPrepackagedJob : public net::URLRequestFileJob { Should this "Prepackaged" also go away?
blundell: thanks https://codereview.chromium.org/508473002/diff/200001/net/url_request/test_ur... File net/url_request/test_url_request_interceptor.cc (right): https://codereview.chromium.org/508473002/diff/200001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.cc:20: class URLRequestPrepackagedJob : public net::URLRequestFileJob { On 2014/08/27 13:02:27, blundell wrote: > Should this "Prepackaged" also go away? Done.
rsleevi OR willchan: May I have approval for the TestURLRequestInterceptor move into net/url_request? Thanks!!
I'll defer to Will, as he had the most original concerns (I was and am fine w/ this being test only) One small naming nit, however. https://codereview.chromium.org/508473002/diff/220001/net/url_request/test_ur... File net/url_request/test_url_request_interceptor.h (right): https://codereview.chromium.org/508473002/diff/220001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.h:24: // occurs while the TestURLRequestInterceptor is alive. Document more about the threading Namely, that this object can be created on any thread (unlike most objects in //net, which MUST be created on the network task runner unless they explicitly say) https://codereview.chromium.org/508473002/diff/220001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.h:32: const scoped_refptr<base::TaskRunner>& io_task_runner, 1) naming wise, this is traditionally called "network_task_runner"
Wait, I defer to you and Paul :) My concerns revolved around my incorrect understanding that this was a URLRequest::Delegate subclass. I have no objections and haven't done a close review, so I defer :) On Thu, Aug 28, 2014 at 10:39 AM, <rsleevi@chromium.org> wrote: > I'll defer to Will, as he had the most original concerns (I was and am > fine w/ > this being test only) > > One small naming nit, however. > > > https://codereview.chromium.org/508473002/diff/220001/net/ > url_request/test_url_request_interceptor.h > File net/url_request/test_url_request_interceptor.h (right): > > https://codereview.chromium.org/508473002/diff/220001/net/ > url_request/test_url_request_interceptor.h#newcode24 > net/url_request/test_url_request_interceptor.h:24: // occurs while the > TestURLRequestInterceptor is alive. > Document more about the threading > > Namely, that this object can be created on any thread (unlike most > objects in //net, which MUST be created on the network task runner > unless they explicitly say) > > https://codereview.chromium.org/508473002/diff/220001/net/ > url_request/test_url_request_interceptor.h#newcode32 > net/url_request/test_url_request_interceptor.h:32: const > > scoped_refptr<base::TaskRunner>& io_task_runner, > 1) naming wise, this is traditionally called "network_task_runner" > > https://codereview.chromium.org/508473002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Everyone wants to pass the buck :) So, at the risk of being a pain, I find the need to pass a network task runner a bit odd - as mentioned, normally we assume all //net objects are ON the network task runner, and if anyone wants to use them from other threads, they have to do the wrapper themselves. When this code lived in //content, it was OK to use browser threads and handle this marshalling (which it implemented, via the Delegate). Now that in lives in //net, I kind of feel like it may need to follow a bit of the //net-y patterns, which doesn't quite help you, in that you'd need something above //net to handle marshalling. That is, what we'd normally put in //net would be the Delegate (the "exclusively lives and dies on the IO thread" portion), and the bits that say "Let me create this on the UI thread" live in //content (or //components)
On 2014/08/28 17:47:00, Ryan Sleevi wrote: > Everyone wants to pass the buck :) > > So, at the risk of being a pain, I find the need to pass a network task runner a > bit odd - as mentioned, normally we assume all //net objects are ON the network > task runner, and if anyone wants to use them from other threads, they have to do > the wrapper themselves. > > When this code lived in //content, it was OK to use browser threads and handle > this marshalling (which it implemented, via the Delegate). > > Now that in lives in //net, I kind of feel like it may need to follow a bit of > the //net-y patterns, which doesn't quite help you, in that you'd need something > above //net to handle marshalling. That is, what we'd normally put in //net > would be the Delegate (the "exclusively lives and dies on the IO thread" > portion), and the bits that say "Let me create this on the UI thread" live in > //content (or //components) Accidentally sent. I don't feel THAT strongly on this (consider it more like a 70% feeling it'd be better that way), but I was hoping to push that to Paul or Will and see if they yelled, as it might just otherwise be my change aversion/do things the way they're done feeling, rather than a good design remark :)
rsleevi: addressed your comments below https://codereview.chromium.org/508473002/diff/220001/net/url_request/test_ur... File net/url_request/test_url_request_interceptor.h (right): https://codereview.chromium.org/508473002/diff/220001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.h:24: // occurs while the TestURLRequestInterceptor is alive. On 2014/08/28 17:39:53, Ryan Sleevi wrote: > Document more about the threading > > Namely, that this object can be created on any thread (unlike most objects in > //net, which MUST be created on the network task runner unless they explicitly > say) Done. https://codereview.chromium.org/508473002/diff/220001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.h:32: const scoped_refptr<base::TaskRunner>& io_task_runner, On 2014/08/28 17:39:53, Ryan Sleevi wrote: > 1) naming wise, this is traditionally called "network_task_runner" Done.
On 2014/08/28 17:47:59, Ryan Sleevi wrote: > On 2014/08/28 17:47:00, Ryan Sleevi wrote: > > Everyone wants to pass the buck :) > > > > So, at the risk of being a pain, I find the need to pass a network task runner > a > > bit odd - as mentioned, normally we assume all //net objects are ON the > network > > task runner, and if anyone wants to use them from other threads, they have to > do > > the wrapper themselves. > > > > When this code lived in //content, it was OK to use browser threads and handle > > this marshalling (which it implemented, via the Delegate). > > > > Now that in lives in //net, I kind of feel like it may need to follow a bit of > > the //net-y patterns, which doesn't quite help you, in that you'd need > something > > above //net to handle marshalling. That is, what we'd normally put in //net > > would be the Delegate (the "exclusively lives and dies on the IO thread" > > portion), and the bits that say "Let me create this on the UI thread" live in > > //content (or //components) > > Accidentally sent. I don't feel THAT strongly on this (consider it more like a > 70% feeling it'd be better that way), but I was hoping to push that to Paul or > Will and see if they yelled, as it might just otherwise be my change aversion/do > things the way they're done feeling, rather than a good design remark :) rsleevi: I agree with you that the design is not optimal. Splitting it up into net/ and components/ would make sense from a purity standpoint, but I don't think anyone would want to use the Delegate class by itself. Since you're looking at this issue, do you have any feedback on this problem? https://codereview.chromium.org/514473002/diff/40001/components/component_upd... See blundell's question and my reply on Line 108.
wtc@chromium.org changed reviewers: - wtc@chromium.org
rsleevi: ping
Paul for one question below. https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... File net/url_request/test_url_request_interceptor.cc (right): https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.cc:37: }; Document? To be quite honest, I'm not sure why this is needed (to force 200, even for a missing file?) https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.cc:41: class TestURLRequestInterceptor::Delegate : public net::URLRequestInterceptor { Briefly, document. https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.cc:62: hostname); Paul should review this, as it seems there's some asymmetry here in the API calls. https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.cc:73: // is just used for tests. // Note: Actual loading of the file still occurs on |worker_task_runner_| https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.cc:75: EXPECT_TRUE(base::PathExists(path)); This EXPECT_TRUE is (largely) meaningless and won't propogate to any tests unless they're doing ASSERT_NO_FATAL_FAILURE, which isn't documented anywhere as part of the contract for this API. https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.cc:155: TestURLRequestInterceptor::~TestURLRequestInterceptor() { BUG: This does not guarantee that |delegate_| will be safely cleaned up on the network_task_runner, which means that if you create this object and delete it without pumping the network task runner, you'll crash. You'll also run into issues with line 166 and 179. The solution is to DeleteSoon() the delegate from the network_task_runner, which will then guarantee FIFO ordering for Register/SetResponse/SetResponseIgnoreQuery calls, and then immediately delete the object. https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... File net/url_request/test_url_request_interceptor.h (right): https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.h:27: // instantiation on any thread for convenient usage on test threads. 1) Don't use pronouns in comments ("We" allow) 2) Everything from "This is" to the end should be deleted (which conveniently solves #1) https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.h:30: // Registers an interceptor for urls using |scheme| and |hostname|. Urls s/urls/URLs/ s/Urls/URLs/ https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.h:36: const scoped_refptr<base::TaskRunner>& worker_task_runner); Document both |network_task_runner| and |worker_task_runner| // |network_task_runner| is the task runner used for network activity // (e.g. where URL requests are processed). // |worker_task_runner| will be used to read the files specified by // either SetResponse() or SetResponseIgnoreQuery() asynchronously. It // must be a task runner allowed to perform disk IO. https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.h:44: // Identical to SetResponse except that query parameters are ignored on s/SetResponse/SetResponse,/
https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... File net/url_request/test_url_request_interceptor.cc (right): https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.cc:62: hostname); On 2014/09/03 00:23:31, Ryan Sleevi wrote: > Paul should review this, as it seems there's some asymmetry here in the API > calls. This is WAI; seems fine.
rsleevi: Sorry for the delay. I just got back from travel. I addressed your comments below. https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... File net/url_request/test_url_request_interceptor.cc (right): https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.cc:37: }; On 2014/09/03 00:23:31, Ryan Sleevi wrote: > Document? > > To be quite honest, I'm not sure why this is needed (to force 200, even for a > missing file?) I looked into this. net::URLRequestFileJob always gives a -1 status code. So this is required for the component updater to work correctly in tests. I would also be okay with making net::URLRequestFileJob return a 200 or a 404 depending on missing or existing... though should we handle permissions on the file too? https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.cc:41: class TestURLRequestInterceptor::Delegate : public net::URLRequestInterceptor { On 2014/09/03 00:23:31, Ryan Sleevi wrote: > Briefly, document. Done. https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.cc:62: hostname); On 2014/09/03 11:52:51, pauljensen wrote: > On 2014/09/03 00:23:31, Ryan Sleevi wrote: > > Paul should review this, as it seems there's some asymmetry here in the API > > calls. > > This is WAI; seems fine. Done. https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.cc:73: // is just used for tests. On 2014/09/03 00:23:31, Ryan Sleevi wrote: > // Note: Actual loading of the file still occurs on |worker_task_runner_| I removed the ScopedAllowIO without any apparent bad effects. https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.cc:75: EXPECT_TRUE(base::PathExists(path)); On 2014/09/03 00:23:31, Ryan Sleevi wrote: > This EXPECT_TRUE is (largely) meaningless and won't propogate to any tests > unless they're doing ASSERT_NO_FATAL_FAILURE, which isn't documented anywhere as > part of the contract for this API. Done. https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.cc:155: TestURLRequestInterceptor::~TestURLRequestInterceptor() { On 2014/09/03 00:23:31, Ryan Sleevi wrote: > BUG: This does not guarantee that |delegate_| will be safely cleaned up on the > network_task_runner, which means that if you create this object and delete it > without pumping the network task runner, you'll crash. You'll also run into > issues with line 166 and 179. > > The solution is to DeleteSoon() the delegate from the network_task_runner, which > will then guarantee FIFO ordering for > Register/SetResponse/SetResponseIgnoreQuery calls, and then immediately delete > the object. I thought |delegate_| was simply never deleted here, but instead deleted by net::URLRequestFilter::RemoveHostnameHandler when the Delagate::Unregister method was called... https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... File net/url_request/test_url_request_interceptor.h (right): https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.h:27: // instantiation on any thread for convenient usage on test threads. On 2014/09/03 00:23:31, Ryan Sleevi wrote: > 1) Don't use pronouns in comments ("We" allow) > 2) Everything from "This is" to the end should be deleted (which conveniently > solves #1) Done. https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.h:30: // Registers an interceptor for urls using |scheme| and |hostname|. Urls On 2014/09/03 00:23:31, Ryan Sleevi wrote: > s/urls/URLs/ > s/Urls/URLs/ Done. https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.h:36: const scoped_refptr<base::TaskRunner>& worker_task_runner); On 2014/09/03 00:23:31, Ryan Sleevi wrote: > Document both |network_task_runner| and |worker_task_runner| > > // |network_task_runner| is the task runner used for network activity > // (e.g. where URL requests are processed). > // |worker_task_runner| will be used to read the files specified by > // either SetResponse() or SetResponseIgnoreQuery() asynchronously. It > // must be a task runner allowed to perform disk IO. Done. https://codereview.chromium.org/508473002/diff/280001/net/url_request/test_ur... net/url_request/test_url_request_interceptor.h:44: // Identical to SetResponse except that query parameters are ignored on On 2014/09/03 00:23:31, Ryan Sleevi wrote: > s/SetResponse/SetResponse,/ Done.
rsleevi: friendly ping, thanks!
lgtm
tommycli@chromium.org changed reviewers: + thestig@chromium.org
rsleevi: Thanks! thestig: Can I get a review for: chrome/browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc chrome/browser/extensions/api/management/management_browsertest.cc chrome/browser/extensions/extension_disabled_ui_browsertest.cc ?
thestig@chromium.org changed reviewers: + yoz@chromium.org
I defer to yoz, a c/b/extensions OWNER.
On 2014/09/08 20:03:13, Lei Zhang wrote: > I defer to yoz, a c/b/extensions OWNER. owners LGTM for extensions/ assuming the other reviewers have LG'ed it
tommycli@chromium.org changed reviewers: + wtc@chromium.org
wtc/willchan: May I have a stamp for content/test/net ? (No one else on the thread owns these)
The CQ bit was checked by tommycli@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommycli@chromium.org/508473002/400001
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as f5ae2a1166b84fd0bea27ac1ed0ea8669ae2d477
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/eaae5d98d9adb42a70c5529dbb6c70e95e848701 Cr-Commit-Position: refs/heads/master@{#293861}
Message was sent while issue was closed.
Note that both wtc and I were out last week (and I'm going on leave soon). TBR is not appropriate for when reviewers are on vacation, especially when there are 11 people in the OWNERS file. Please find another reviewer next time rather than TBR'ing. Also, rsleevi@ should really be in that OWNERS file. LGTM.
Message was sent while issue was closed.
On 2014/09/15 23:37:09, willchan OOO until 03-22-15 wrote: > Note that both wtc and I were out last week (and I'm going on leave soon). TBR > is not appropriate for when reviewers are on vacation, especially when there are > 11 people in the OWNERS file. Please find another reviewer next time rather than > TBR'ing. > > Also, rsleevi@ should really be in that OWNERS file. > > LGTM. willchan: Sorry about that will. I figured that since it was purely a deletion and rsleevi approved, TBR was acceptable. I'll find a another reviewer next time. |
