Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
378823002:
Move http_server_properties_manager from chrome/browser/net to net/http. (Closed)
DescriptionMove http_server_properties_manager from chrome/browser/net to net/http.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282319
Patch Set 1 #
Total comments: 26
Patch Set 2 : Address review comments. #
Total comments: 40
Patch Set 3 : Address review comments, fix gn build. #
Total comments: 6
Patch Set 4 : Use RunLoop and address comments. #Patch Set 5 : Rebased to r282022 #Patch Set 6 : Fix compilation error. #
Total comments: 14
Patch Set 7 : Fix renames in comments. #Messages
Total messages: 35 (0 generated)
Hi guys, please take a look. See a net-dev thread on discussion why it is moving to net/http instead of components/. thanks, -m
Just to clarify my request: droger - everything. OWNER review: mmenke - chrome/browser/profiles/profile_impl_io_data.* battre - chrome/browser/prefs/browser_prefs.cc FYI: rtenneti, rch thanks, -m
https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... chrome/browser/prefs/browser_prefs.cc:226: void HttpServerPropertiesManager_RegisterProfilePrefs( Shouldn't this stay in the HttpServerPropertiesManager class? https://codereview.chromium.org/378823002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/378823002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_impl_io_data.cc:318: prefs::kHttpServerProperties, It seems that the kHttpServerProperties constant is only used in http_server_properties_manager. Could we move it to that file?
thanks! https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... chrome/browser/prefs/browser_prefs.cc:226: void HttpServerPropertiesManager_RegisterProfilePrefs( On 2014/07/08 13:06:02, droger wrote: > Shouldn't this stay in the HttpServerPropertiesManager class? The problem is that user_prefs::PrefRegistrySyncable is in //components/pref_registry, and //net cannot depend on it. https://codereview.chromium.org/378823002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/378823002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_impl_io_data.cc:318: prefs::kHttpServerProperties, On 2014/07/08 13:06:02, droger wrote: > It seems that the kHttpServerProperties constant is only used in > http_server_properties_manager. Could we move it to that file? Good question. We could move it to http_server_properties_manager.h as it is also used in chrome/browser/prefs/browser_prefs.cc, but I'm open for better suggestions.
+bauerb https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... chrome/browser/prefs/browser_prefs.cc:226: void HttpServerPropertiesManager_RegisterProfilePrefs( On 2014/07/08 13:32:00, mef wrote: > On 2014/07/08 13:06:02, droger wrote: > > Shouldn't this stay in the HttpServerPropertiesManager class? > The problem is that user_prefs::PrefRegistrySyncable is in > //components/pref_registry, and //net cannot depend on it. Bernhard, what do you think about this? My feeling is that ownership and registration of preferences should not be split. OTOH, //net should not depend on //components/pref_registry. I can see two approaches. 1) Leave the preference management in //chrome/browser. 2) Provide a wrapper interface in //base/prefs that is implemented by //components/pref_registry and can be pushed into //net. I don't like either of the two apporaches. WDYT? Do you have a better idea? https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... chrome/browser/prefs/browser_prefs.cc:364: HttpServerPropertiesManager_RegisterProfilePrefs(registry); please consider the request to keep this alphabetized.
A couple quick comments https://codereview.chromium.org/378823002/diff/1/net/http/http_server_propert... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/378823002/diff/1/net/http/http_server_propert... net/http/http_server_properties_manager.h:40: // It must be constructed on the UI thread, to set up |ui_method_factory_| and ui_method_factory_? https://codereview.chromium.org/378823002/diff/1/net/http/http_server_propert... net/http/http_server_properties_manager.h:47: // destruction order of Profile and ProfileIOData guarantees that if this net/ shouldn't know about Profile, ProfileIOData, or the UI thread (There are a lot of uses of IO thread to mean the network thread in net/, so I guess we should stick with that). https://codereview.chromium.org/378823002/diff/1/net/http/http_server_propert... net/http/http_server_properties_manager.h:59: const char* path, I don't think it's clear that path is the preference path, as opposed to a file path of a preference file. https://codereview.chromium.org/378823002/diff/1/net/http/http_server_propert... net/http/http_server_properties_manager.h:61: scoped_refptr<base::SingleThreadTaskRunner> io_task_runner); net/ shouldn't know (or care) what a UI thread is. Suggest calling it the profile_task_runner. Hrm...We're also requiring it be the current thread. Could even just grab the current message loop proxy.
https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... chrome/browser/prefs/browser_prefs.cc:226: void HttpServerPropertiesManager_RegisterProfilePrefs( On 2014/07/08 14:16:57, battre wrote: > On 2014/07/08 13:32:00, mef wrote: > > On 2014/07/08 13:06:02, droger wrote: > > > Shouldn't this stay in the HttpServerPropertiesManager class? > > The problem is that user_prefs::PrefRegistrySyncable is in > > //components/pref_registry, and //net cannot depend on it. > > Bernhard, what do you think about this? > > My feeling is that ownership and registration of preferences should not be > split. OTOH, //net should not depend on //components/pref_registry. > > I can see two approaches. > 1) Leave the preference management in //chrome/browser. > 2) Provide a wrapper interface in //base/prefs that is implemented by > //components/pref_registry and can be pushed into //net. > > I don't like either of the two apporaches. WDYT? Do you have a better idea? We already pass in the pref path for the HttpServerPropertiesManager to use when creating it (currently in ProfileIOData). How about creating a method somewhere in chrome/browser/net that will register the pref, and extracting the code that creates the HttpServerPropertiesManager into another method in that same file in chrome/browser/net? The idea is that chrome/browser/net (due to being in chrome/, the top-level component), is the embedder of lower-level components like net/ and components/pref_registry, so it can inject their dependencies. https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... chrome/browser/prefs/browser_prefs.cc:364: HttpServerPropertiesManager_RegisterProfilePrefs(registry); On 2014/07/08 14:16:57, battre wrote: > please consider the request to keep this alphabetized. +1 https://codereview.chromium.org/378823002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/378823002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_impl_io_data.cc:318: prefs::kHttpServerProperties, On 2014/07/08 13:32:00, mef wrote: > On 2014/07/08 13:06:02, droger wrote: > > It seems that the kHttpServerProperties constant is only used in > > http_server_properties_manager. Could we move it to that file? > Good question. We could move it to http_server_properties_manager.h as it is > also used in chrome/browser/prefs/browser_prefs.cc, but I'm open for better > suggestions. Do you mean moving the declaration, or where it's referenced? We declare all prefs that are used in chrome/ in chrome/common/pref_names.{h,cc}.
Thanks, couple of follow-up questions... https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... chrome/browser/prefs/browser_prefs.cc:226: void HttpServerPropertiesManager_RegisterProfilePrefs( On 2014/07/08 16:02:46, Bernhard Bauer wrote: > On 2014/07/08 14:16:57, battre wrote: > > On 2014/07/08 13:32:00, mef wrote: > > > On 2014/07/08 13:06:02, droger wrote: > > > > Shouldn't this stay in the HttpServerPropertiesManager class? > > > The problem is that user_prefs::PrefRegistrySyncable is in > > > //components/pref_registry, and //net cannot depend on it. > > > > Bernhard, what do you think about this? > > > > My feeling is that ownership and registration of preferences should not be > > split. OTOH, //net should not depend on //components/pref_registry. > > > > I can see two approaches. > > 1) Leave the preference management in //chrome/browser. > > 2) Provide a wrapper interface in //base/prefs that is implemented by > > //components/pref_registry and can be pushed into //net. > > > > I don't like either of the two apporaches. WDYT? Do you have a better idea? > > We already pass in the pref path for the HttpServerPropertiesManager to use when > creating it (currently in ProfileIOData). How about creating a method somewhere > in chrome/browser/net that will register the pref, and extracting the code that > creates the HttpServerPropertiesManager into another method in that same file in > chrome/browser/net? The idea is that chrome/browser/net (due to being in > chrome/, the top-level component), is the embedder of lower-level components > like net/ and components/pref_registry, so it can inject their dependencies. Sounds good. Just to make sure that I understand correctly, you are suggesting to create a class in c/b/n similar to https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net... which will have static methods to instantiate and to register the HttpServerPropertiesManager. Would something like HttpServerPropertiesManagerPrefs be a good name for that? https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... chrome/browser/prefs/browser_prefs.cc:364: HttpServerPropertiesManager_RegisterProfilePrefs(registry); On 2014/07/08 16:02:46, Bernhard Bauer wrote: > On 2014/07/08 14:16:57, battre wrote: > > please consider the request to keep this alphabetized. > > +1 Will do. https://codereview.chromium.org/378823002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/378823002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_impl_io_data.cc:318: prefs::kHttpServerProperties, On 2014/07/08 16:02:47, Bernhard Bauer wrote: > On 2014/07/08 13:32:00, mef wrote: > > On 2014/07/08 13:06:02, droger wrote: > > > It seems that the kHttpServerProperties constant is only used in > > > http_server_properties_manager. Could we move it to that file? > > Good question. We could move it to http_server_properties_manager.h as it is > > also used in chrome/browser/prefs/browser_prefs.cc, but I'm open for better > > suggestions. > > Do you mean moving the declaration, or where it's referenced? We declare all > prefs that are used in chrome/ in chrome/common/pref_names.{h,cc}. We were discussing moving both declaration and definition into //net/http/http_server_properties_manager.{cc|h}, but it sounds like they should stay in c/c/pref_names?
https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... chrome/browser/prefs/browser_prefs.cc:226: void HttpServerPropertiesManager_RegisterProfilePrefs( On 2014/07/08 16:19:54, mef wrote: > On 2014/07/08 16:02:46, Bernhard Bauer wrote: > > On 2014/07/08 14:16:57, battre wrote: > > > On 2014/07/08 13:32:00, mef wrote: > > > > On 2014/07/08 13:06:02, droger wrote: > > > > > Shouldn't this stay in the HttpServerPropertiesManager class? > > > > The problem is that user_prefs::PrefRegistrySyncable is in > > > > //components/pref_registry, and //net cannot depend on it. > > > > > > Bernhard, what do you think about this? > > > > > > My feeling is that ownership and registration of preferences should not be > > > split. OTOH, //net should not depend on //components/pref_registry. > > > > > > I can see two approaches. > > > 1) Leave the preference management in //chrome/browser. > > > 2) Provide a wrapper interface in //base/prefs that is implemented by > > > //components/pref_registry and can be pushed into //net. > > > > > > I don't like either of the two apporaches. WDYT? Do you have a better idea? > > > > We already pass in the pref path for the HttpServerPropertiesManager to use > when > > creating it (currently in ProfileIOData). How about creating a method > somewhere > > in chrome/browser/net that will register the pref, and extracting the code > that > > creates the HttpServerPropertiesManager into another method in that same file > in > > chrome/browser/net? The idea is that chrome/browser/net (due to being in > > chrome/, the top-level component), is the embedder of lower-level components > > like net/ and components/pref_registry, so it can inject their dependencies. > > Sounds good. Just to make sure that I understand correctly, you are suggesting > to create a class in c/b/n similar to > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net... > which will have static methods to instantiate and to register the > HttpServerPropertiesManager. Would something like > HttpServerPropertiesManagerPrefs be a good name for that? Yup, pretty much. The only thing I'm not sure about is the naming. If it also creates the HttpServerPropertiesManager, I'd not want to call it ...Prefs; it has a bigger role, so maybe ...Context, or -- dare I say it -- ...Factory? :) Or, another pattern would be to call it ChromeHttpServerPropertiesManager, signifying that it's about the chrome/ embedding of the component. https://codereview.chromium.org/378823002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/378823002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_impl_io_data.cc:318: prefs::kHttpServerProperties, On 2014/07/08 16:19:54, mef wrote: > On 2014/07/08 16:02:47, Bernhard Bauer wrote: > > On 2014/07/08 13:32:00, mef wrote: > > > On 2014/07/08 13:06:02, droger wrote: > > > > It seems that the kHttpServerProperties constant is only used in > > > > http_server_properties_manager. Could we move it to that file? > > > Good question. We could move it to http_server_properties_manager.h as it is > > > also used in chrome/browser/prefs/browser_prefs.cc, but I'm open for better > > > suggestions. > > > > Do you mean moving the declaration, or where it's referenced? We declare all > > prefs that are used in chrome/ in chrome/common/pref_names.{h,cc}. > > We were discussing moving both declaration and definition into > //net/http/http_server_properties_manager.{cc|h}, but it sounds like they should > stay in c/c/pref_names? Yeah, exactly. We would like all pref names used in chrome to be in a single place.
thanks! https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... chrome/browser/prefs/browser_prefs.cc:226: void HttpServerPropertiesManager_RegisterProfilePrefs( On 2014/07/08 17:02:45, Bernhard Bauer wrote: > On 2014/07/08 16:19:54, mef wrote: > > On 2014/07/08 16:02:46, Bernhard Bauer wrote: > > > On 2014/07/08 14:16:57, battre wrote: > > > > On 2014/07/08 13:32:00, mef wrote: > > > > > On 2014/07/08 13:06:02, droger wrote: > > > > > > Shouldn't this stay in the HttpServerPropertiesManager class? > > > > > The problem is that user_prefs::PrefRegistrySyncable is in > > > > > //components/pref_registry, and //net cannot depend on it. > > > > > > > > Bernhard, what do you think about this? > > > > > > > > My feeling is that ownership and registration of preferences should not be > > > > split. OTOH, //net should not depend on //components/pref_registry. > > > > > > > > I can see two approaches. > > > > 1) Leave the preference management in //chrome/browser. > > > > 2) Provide a wrapper interface in //base/prefs that is implemented by > > > > //components/pref_registry and can be pushed into //net. > > > > > > > > I don't like either of the two apporaches. WDYT? Do you have a better > idea? > > > > > > We already pass in the pref path for the HttpServerPropertiesManager to use > > when > > > creating it (currently in ProfileIOData). How about creating a method > > somewhere > > > in chrome/browser/net that will register the pref, and extracting the code > > that > > > creates the HttpServerPropertiesManager into another method in that same > file > > in > > > chrome/browser/net? The idea is that chrome/browser/net (due to being in > > > chrome/, the top-level component), is the embedder of lower-level components > > > like net/ and components/pref_registry, so it can inject their dependencies. > > > > Sounds good. Just to make sure that I understand correctly, you are suggesting > > to create a class in c/b/n similar to > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net... > > which will have static methods to instantiate and to register the > > HttpServerPropertiesManager. Would something like > > HttpServerPropertiesManagerPrefs be a good name for that? > > Yup, pretty much. > > The only thing I'm not sure about is the naming. If it also creates the > HttpServerPropertiesManager, I'd not want to call it ...Prefs; it has a bigger > role, so maybe ...Context, or -- dare I say it -- ...Factory? :) > > Or, another pattern would be to call it ChromeHttpServerPropertiesManager, > signifying that it's about the chrome/ embedding of the component. I like ChromeHttpServerPropertiesManager, but I don't think it inherits from from HttpServerPropertiesManager, so HttpServerPropertiesManagerFactory sounds more correct.
Thanks, PTAL! https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... chrome/browser/prefs/browser_prefs.cc:226: void HttpServerPropertiesManager_RegisterProfilePrefs( On 2014/07/08 17:11:20, mef wrote: > On 2014/07/08 17:02:45, Bernhard Bauer wrote: > > On 2014/07/08 16:19:54, mef wrote: > > > On 2014/07/08 16:02:46, Bernhard Bauer wrote: > > > > On 2014/07/08 14:16:57, battre wrote: > > > > > On 2014/07/08 13:32:00, mef wrote: > > > > > > On 2014/07/08 13:06:02, droger wrote: > > > > > > > Shouldn't this stay in the HttpServerPropertiesManager class? > > > > > > The problem is that user_prefs::PrefRegistrySyncable is in > > > > > > //components/pref_registry, and //net cannot depend on it. > > > > > > > > > > Bernhard, what do you think about this? > > > > > > > > > > My feeling is that ownership and registration of preferences should not > be > > > > > split. OTOH, //net should not depend on //components/pref_registry. > > > > > > > > > > I can see two approaches. > > > > > 1) Leave the preference management in //chrome/browser. > > > > > 2) Provide a wrapper interface in //base/prefs that is implemented by > > > > > //components/pref_registry and can be pushed into //net. > > > > > > > > > > I don't like either of the two apporaches. WDYT? Do you have a better > > idea? > > > > > > > > We already pass in the pref path for the HttpServerPropertiesManager to > use > > > when > > > > creating it (currently in ProfileIOData). How about creating a method > > > somewhere > > > > in chrome/browser/net that will register the pref, and extracting the code > > > that > > > > creates the HttpServerPropertiesManager into another method in that same > > file > > > in > > > > chrome/browser/net? The idea is that chrome/browser/net (due to being in > > > > chrome/, the top-level component), is the embedder of lower-level > components > > > > like net/ and components/pref_registry, so it can inject their > dependencies. > > > > > > Sounds good. Just to make sure that I understand correctly, you are > suggesting > > > to create a class in c/b/n similar to > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net... > > > which will have static methods to instantiate and to register the > > > HttpServerPropertiesManager. Would something like > > > HttpServerPropertiesManagerPrefs be a good name for that? > > > > Yup, pretty much. > > > > The only thing I'm not sure about is the naming. If it also creates the > > HttpServerPropertiesManager, I'd not want to call it ...Prefs; it has a bigger > > role, so maybe ...Context, or -- dare I say it -- ...Factory? :) > > > > Or, another pattern would be to call it ChromeHttpServerPropertiesManager, > > signifying that it's about the chrome/ embedding of the component. > > I like ChromeHttpServerPropertiesManager, but I don't think it inherits from > from HttpServerPropertiesManager, so HttpServerPropertiesManagerFactory sounds > more correct. Done. https://codereview.chromium.org/378823002/diff/1/chrome/browser/prefs/browser... chrome/browser/prefs/browser_prefs.cc:364: HttpServerPropertiesManager_RegisterProfilePrefs(registry); On 2014/07/08 16:19:54, mef wrote: > On 2014/07/08 16:02:46, Bernhard Bauer wrote: > > On 2014/07/08 14:16:57, battre wrote: > > > please consider the request to keep this alphabetized. > > > > +1 > Will do. Done. https://codereview.chromium.org/378823002/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/378823002/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_impl_io_data.cc:318: prefs::kHttpServerProperties, On 2014/07/08 17:02:45, Bernhard Bauer wrote: > On 2014/07/08 16:19:54, mef wrote: > > On 2014/07/08 16:02:47, Bernhard Bauer wrote: > > > On 2014/07/08 13:32:00, mef wrote: > > > > On 2014/07/08 13:06:02, droger wrote: > > > > > It seems that the kHttpServerProperties constant is only used in > > > > > http_server_properties_manager. Could we move it to that file? > > > > Good question. We could move it to http_server_properties_manager.h as it > is > > > > also used in chrome/browser/prefs/browser_prefs.cc, but I'm open for > better > > > > suggestions. > > > > > > Do you mean moving the declaration, or where it's referenced? We declare all > > > prefs that are used in chrome/ in chrome/common/pref_names.{h,cc}. > > > > We were discussing moving both declaration and definition into > > //net/http/http_server_properties_manager.{cc|h}, but it sounds like they > should > > stay in c/c/pref_names? > > Yeah, exactly. We would like all pref names used in chrome to be in a single > place. Done. https://codereview.chromium.org/378823002/diff/1/net/http/http_server_propert... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/378823002/diff/1/net/http/http_server_propert... net/http/http_server_properties_manager.h:40: // It must be constructed on the UI thread, to set up |ui_method_factory_| and On 2014/07/08 15:05:48, mmenke wrote: > ui_method_factory_? Done. https://codereview.chromium.org/378823002/diff/1/net/http/http_server_propert... net/http/http_server_properties_manager.h:47: // destruction order of Profile and ProfileIOData guarantees that if this On 2014/07/08 15:05:48, mmenke wrote: > net/ shouldn't know about Profile, ProfileIOData, or the UI thread (There are a > lot of uses of IO thread to mean the network thread in net/, so I guess we > should stick with that). Done. https://codereview.chromium.org/378823002/diff/1/net/http/http_server_propert... net/http/http_server_properties_manager.h:59: const char* path, On 2014/07/08 15:05:48, mmenke wrote: > I don't think it's clear that path is the preference path, as opposed to a file > path of a preference file. Done. https://codereview.chromium.org/378823002/diff/1/net/http/http_server_propert... net/http/http_server_properties_manager.h:61: scoped_refptr<base::SingleThreadTaskRunner> io_task_runner); On 2014/07/08 15:05:48, mmenke wrote: > net/ shouldn't know (or care) what a UI thread is. Suggest calling it the > profile_task_runner. > > Hrm...We're also requiring it be the current thread. Could even just grab the > current message loop proxy. Done. As we don't want this class to know about profile I've called it pref_xyz.
Adding sleevi since he's expressed an interest in this space.
Adding sleevi since he's expressed an interest in this space.
So, it's no secret I have no love lost for HSPM, and have been trying to bribe RCH to tackle some of the oddities. I've noted a few things here for future, but I think some of the changes - in particular, related to MessageLoop & task runners, which you're already updating here - should better reflect our desired outcome (and expressions within //net) - though it does impose a bit more work for this CL. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.cc:62: : pref_task_runner_(base::MessageLoop::current()->message_loop_proxy()), use ThreadTaskRunnerHandle::Get() (see Chromium-dev thread). Downcast it to a SequencedTaskRunner. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.cc:161: DCHECK(io_task_runner_->BelongsToCurrentThread()); All of the BelongsTo will be replaced with "RunsTasksOnCurrentThread" BelongsTo is something I also want to remove as a deprecated anachronism, but it exists for the MessageLoopProxy migration. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:24: class SingleThreadTaskRunner; Do you really need an STTR? A SequencedTaskRunner seems sufficient, in that you aren't binding to any thread-local storage. The actual need to bind to a particular thread should be minimal. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:36: // changes are received from, and the IO thread, which owns it (e.g. in the nit: usually called network task runner. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:37: // ProfileIOData) and it persists the changes from network stack that if a ProfileIOData = abstraction leak https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:50: // before the actual update starts, and grab a WeakPtr. 1) extra space in "before the" 2) This destruction order is no longer guaranteed in //net. It needs to be spelled out as a requirement, not as an assumption https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:64: // from |pref_service_|. This last sentence seems to unnecessarily describe implementation detail. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:68: void ShutdownOnPrefThread(); And what happens if they don't? https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:83: virtual base::WeakPtr<HttpServerProperties> GetWeakPtr() OVERRIDE; Hrm. Long-term, I think we will want to remove this method as a WeakPtr smell, but I'm not going to fight for it now. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:207: void UpdatePrefsOnPref(base::ListValue* spdy_server_list, nit: naming could be clearer (OnPref -> OnPrefThread) https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:216: // UI thread out of date comment https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:219: const scoped_refptr<base::SingleThreadTaskRunner> pref_task_runner_; STR seems fine here. Only requirement is it's the same task runner that notifications for prefs come in. No special threading requirements here, just sequencing. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... File net/http/http_server_properties_manager_unittest.cc (right): https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager_unittest.cc:281: base::MessageLoop::current()->RunUntilIdle(); These tests should be using RunLoop
https://codereview.chromium.org/378823002/diff/20001/chrome/browser/net/http_... File chrome/browser/net/http_server_properties_manager_factory.h (right): https://codereview.chromium.org/378823002/diff/20001/chrome/browser/net/http_... chrome/browser/net/http_server_properties_manager_factory.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Update copyright? https://codereview.chromium.org/378823002/diff/20001/chrome/browser/net/http_... chrome/browser/net/http_server_properties_manager_factory.h:9: #include <vector> Nit: empty line between system and regular includes. https://codereview.chromium.org/378823002/diff/20001/chrome/browser/net/http_... chrome/browser/net/http_server_properties_manager_factory.h:34: return new net::HttpServerPropertiesManager( I'm sorry that this will mean that you'll have to create another file, but I think these methods are too big to be inlined. https://codereview.chromium.org/378823002/diff/20001/chrome/browser/net/http_... chrome/browser/net/http_server_properties_manager_factory.h:40: // Register |prefs| for properties managed by HttpServerPropertiesManager. |registry|? https://codereview.chromium.org/378823002/diff/20001/chrome/browser/net/http_... chrome/browser/net/http_server_properties_manager_factory.h:45: } private: DISALLOW_IMPLICIT_CONSTRUCTORS? https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:68: void ShutdownOnPrefThread(); On 2014/07/08 18:19:26, Ryan Sleevi wrote: > And what happens if they don't? A DCHECK will trigger when this object (with its UI-thread-living members) is destroyed on another thread. Is there a better way to describe such a contract of a class in a comment?
https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:68: void ShutdownOnPrefThread(); On 2014/07/08 21:47:21, Bernhard Bauer wrote: > On 2014/07/08 18:19:26, Ryan Sleevi wrote: > > And what happens if they don't? > > A DCHECK will trigger when this object (with its UI-thread-living members) is > destroyed on another thread. Is there a better way to describe such a contract > of a class in a comment? Well, this is more of a "long term design" issue, and not a refactoring that I think is essential to moving this (but one I do think is essential for code health), but an 'ideal' world is that the HSPM lives on a single thread - the Pref thread - and that there's no need for an explicit Shutdown() state, as C++ provides that (via the DTOR). Since HSP has to live on the IO thread, it would be a separate class, and the HSPM and the HSP(Prefs) could communicate via internal, implementation-detail methods. rch@ has a much better idea of this vision. My goal here with this comment is to do our best, in the time until things are fixed, to make sure there's no security issues and we catch developers as quickly as possible. DCHECK() is probably fine, or any other sanity/safety things. a CHECK might even be more appropriate, depending - I'm not familiar enough to know.
On 2014/07/08 22:54:36, Ryan Sleevi wrote: > https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... > File net/http/http_server_properties_manager.h (right): > > https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... > net/http/http_server_properties_manager.h:68: void ShutdownOnPrefThread(); > On 2014/07/08 21:47:21, Bernhard Bauer wrote: > > On 2014/07/08 18:19:26, Ryan Sleevi wrote: > > > And what happens if they don't? > > > > A DCHECK will trigger when this object (with its UI-thread-living members) is > > destroyed on another thread. Is there a better way to describe such a contract > > of a class in a comment? > > Well, this is more of a "long term design" issue, and not a refactoring that I > think is essential to moving this (but one I do think is essential for code > health), but an 'ideal' world is that the HSPM lives on a single thread - the > Pref thread - and that there's no need for an explicit Shutdown() state, as C++ > provides that (via the DTOR). > > Since HSP has to live on the IO thread, it would be a separate class, and the > HSPM and the HSP(Prefs) could communicate via internal, implementation-detail > methods. > > rch@ has a much better idea of this vision. I started working on such a CL here: https://codereview.chromium.org/375083002 Basically it breaks HSPM into 2 classes. * one which lives only on the IO thread and knows nothing about the UI thread * one which lives only on the UI thread and owns the first object. But it's incomplete. I started last week and then got distracted.
On 2014/07/08 23:00:52, Ryan Hamilton wrote: > On 2014/07/08 22:54:36, Ryan Sleevi wrote: > > > https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... > > File net/http/http_server_properties_manager.h (right): > > > > > https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... > > net/http/http_server_properties_manager.h:68: void ShutdownOnPrefThread(); > > On 2014/07/08 21:47:21, Bernhard Bauer wrote: > > > On 2014/07/08 18:19:26, Ryan Sleevi wrote: > > > > And what happens if they don't? > > > > > > A DCHECK will trigger when this object (with its UI-thread-living members) > is > > > destroyed on another thread. Is there a better way to describe such a > contract > > > of a class in a comment? > > > > Well, this is more of a "long term design" issue, and not a refactoring that I > > think is essential to moving this (but one I do think is essential for code > > health), but an 'ideal' world is that the HSPM lives on a single thread - the > > Pref thread - and that there's no need for an explicit Shutdown() state, as > C++ > > provides that (via the DTOR). > > > > Since HSP has to live on the IO thread, it would be a separate class, and the > > HSPM and the HSP(Prefs) could communicate via internal, implementation-detail > > methods. > > > > rch@ has a much better idea of this vision. > > I started working on such a CL here: https://codereview.chromium.org/375083002 > > Basically it breaks HSPM into 2 classes. > * one which lives only on the IO thread and knows nothing about the UI thread > * one which lives only on the UI thread and owns the first object. > > But it's incomplete. I started last week and then got distracted. Oops, sorry to step on your toes. Does it make sense for me to continue with this CL or could you add the move to //net to yours?
Thanks, PTAL. https://codereview.chromium.org/378823002/diff/20001/chrome/browser/net/http_... File chrome/browser/net/http_server_properties_manager_factory.h (right): https://codereview.chromium.org/378823002/diff/20001/chrome/browser/net/http_... chrome/browser/net/http_server_properties_manager_factory.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/07/08 21:47:21, Bernhard Bauer wrote: > Update copyright? Done. https://codereview.chromium.org/378823002/diff/20001/chrome/browser/net/http_... chrome/browser/net/http_server_properties_manager_factory.h:9: #include <vector> On 2014/07/08 21:47:20, Bernhard Bauer wrote: > Nit: empty line between system and regular includes. Done. https://codereview.chromium.org/378823002/diff/20001/chrome/browser/net/http_... chrome/browser/net/http_server_properties_manager_factory.h:34: return new net::HttpServerPropertiesManager( On 2014/07/08 21:47:21, Bernhard Bauer wrote: > I'm sorry that this will mean that you'll have to create another file, but I > think these methods are too big to be inlined. Done. https://codereview.chromium.org/378823002/diff/20001/chrome/browser/net/http_... chrome/browser/net/http_server_properties_manager_factory.h:40: // Register |prefs| for properties managed by HttpServerPropertiesManager. On 2014/07/08 21:47:20, Bernhard Bauer wrote: > |registry|? Done. I think we are registering prefs in |registry|, are we not? https://codereview.chromium.org/378823002/diff/20001/chrome/browser/net/http_... chrome/browser/net/http_server_properties_manager_factory.h:45: } On 2014/07/08 21:47:21, Bernhard Bauer wrote: > private: > DISALLOW_IMPLICIT_CONSTRUCTORS? Done. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.cc:62: : pref_task_runner_(base::MessageLoop::current()->message_loop_proxy()), On 2014/07/08 18:19:26, Ryan Sleevi wrote: > use ThreadTaskRunnerHandle::Get() (see Chromium-dev thread). Downcast it to a > SequencedTaskRunner. Done. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.cc:161: DCHECK(io_task_runner_->BelongsToCurrentThread()); On 2014/07/08 18:19:26, Ryan Sleevi wrote: > All of the BelongsTo will be replaced with "RunsTasksOnCurrentThread" > > BelongsTo is something I also want to remove as a deprecated anachronism, but it > exists for the MessageLoopProxy migration. Done. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:24: class SingleThreadTaskRunner; On 2014/07/08 18:19:26, Ryan Sleevi wrote: > Do you really need an STTR? > > A SequencedTaskRunner seems sufficient, in that you aren't binding to any > thread-local storage. The actual need to bind to a particular thread should be > minimal. Done. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:37: // ProfileIOData) and it persists the changes from network stack that if a On 2014/07/08 18:19:26, Ryan Sleevi wrote: > ProfileIOData = abstraction leak Done. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:50: // before the actual update starts, and grab a WeakPtr. On 2014/07/08 18:19:26, Ryan Sleevi wrote: > 1) extra space in "before the" > > 2) This destruction order is no longer guaranteed in //net. It needs to be > spelled out as a requirement, not as an assumption Done. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:64: // from |pref_service_|. On 2014/07/08 18:19:26, Ryan Sleevi wrote: > This last sentence seems to unnecessarily describe implementation detail. Done. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:68: void ShutdownOnPrefThread(); On 2014/07/08 22:54:35, Ryan Sleevi wrote: > On 2014/07/08 21:47:21, Bernhard Bauer wrote: > > On 2014/07/08 18:19:26, Ryan Sleevi wrote: > > > And what happens if they don't? > > > > A DCHECK will trigger when this object (with its UI-thread-living members) is > > destroyed on another thread. Is there a better way to describe such a contract > > of a class in a comment? > > Well, this is more of a "long term design" issue, and not a refactoring that I > think is essential to moving this (but one I do think is essential for code > health), but an 'ideal' world is that the HSPM lives on a single thread - the > Pref thread - and that there's no need for an explicit Shutdown() state, as C++ > provides that (via the DTOR). > > Since HSP has to live on the IO thread, it would be a separate class, and the > HSPM and the HSP(Prefs) could communicate via internal, implementation-detail > methods. > > rch@ has a much better idea of this vision. > > My goal here with this comment is to do our best, in the time until things are > fixed, to make sure there's no security issues and we catch developers as > quickly as possible. DCHECK() is probably fine, or any other sanity/safety > things. a CHECK might even be more appropriate, depending - I'm not familiar > enough to know. Ack. I'm fine with abandoning this CL in favor or rch's changes if they go to net. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:83: virtual base::WeakPtr<HttpServerProperties> GetWeakPtr() OVERRIDE; On 2014/07/08 18:19:26, Ryan Sleevi wrote: > Hrm. Long-term, I think we will want to remove this method as a WeakPtr smell, > but I'm not going to fight for it now. Roger that. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:207: void UpdatePrefsOnPref(base::ListValue* spdy_server_list, On 2014/07/08 18:19:26, Ryan Sleevi wrote: > nit: naming could be clearer (OnPref -> OnPrefThread) Done. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:216: // UI thread On 2014/07/08 18:19:26, Ryan Sleevi wrote: > out of date comment Done. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager.h:219: const scoped_refptr<base::SingleThreadTaskRunner> pref_task_runner_; On 2014/07/08 18:19:26, Ryan Sleevi wrote: > STR seems fine here. Only requirement is it's the same task runner that > notifications for prefs come in. No special threading requirements here, just > sequencing. Done. https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... File net/http/http_server_properties_manager_unittest.cc (right): https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager_unittest.cc:281: base::MessageLoop::current()->RunUntilIdle(); On 2014/07/08 18:19:27, Ryan Sleevi wrote: > These tests should be using RunLoop Could you elaborate? Switching to RunLoop causes DCHECK(!run_called_); What am I missing?
lgtm https://codereview.chromium.org/378823002/diff/20001/chrome/browser/net/http_... File chrome/browser/net/http_server_properties_manager_factory.h (right): https://codereview.chromium.org/378823002/diff/20001/chrome/browser/net/http_... chrome/browser/net/http_server_properties_manager_factory.h:40: // Register |prefs| for properties managed by HttpServerPropertiesManager. On 2014/07/09 12:12:04, mef wrote: > On 2014/07/08 21:47:20, Bernhard Bauer wrote: > > |registry|? > > Done. I think we are registering prefs in |registry|, are we not? Yeah, this way it makes more sense. Thanks! https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... File net/http/http_server_properties_manager_unittest.cc (right): https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager_unittest.cc:281: base::MessageLoop::current()->RunUntilIdle(); On 2014/07/09 12:12:04, mef wrote: > On 2014/07/08 18:19:27, Ryan Sleevi wrote: > > These tests should be using RunLoop > Could you elaborate? > Switching to RunLoop causes DCHECK(!run_called_); > What am I missing? RunLoop is the prefered interface for running a message loop, as it will take care of nested message loops, and allow sending the quit signal before it's even started. A given run loop can only be run once, but you can just allocate one on the stack, then run it: base::RunLoop().RunUntilIdle();
lgtm https://codereview.chromium.org/378823002/diff/40001/net/http/http_server_pro... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/378823002/diff/40001/net/http/http_server_pro... net/http/http_server_properties_manager.cc:714: } // namespace chrome_browser_net Fix the comment. https://codereview.chromium.org/378823002/diff/40001/net/http/http_server_pro... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/378823002/diff/40001/net/http/http_server_pro... net/http/http_server_properties_manager.h:256: } // namespace chrome_browser_net Fix the comment. https://codereview.chromium.org/378823002/diff/40001/net/http/http_server_pro... File net/http/http_server_properties_manager_unittest.cc (right): https://codereview.chromium.org/378823002/diff/40001/net/http/http_server_pro... net/http/http_server_properties_manager_unittest.cc:484: } // namespace chrome_browser_net Fix the comment.
Thanks! https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... File net/http/http_server_properties_manager_unittest.cc (right): https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... net/http/http_server_properties_manager_unittest.cc:281: base::MessageLoop::current()->RunUntilIdle(); On 2014/07/09 12:24:43, Bernhard Bauer wrote: > On 2014/07/09 12:12:04, mef wrote: > > On 2014/07/08 18:19:27, Ryan Sleevi wrote: > > > These tests should be using RunLoop > > Could you elaborate? > > Switching to RunLoop causes DCHECK(!run_called_); > > What am I missing? > > RunLoop is the prefered interface for running a message loop, as it will take > care of nested message loops, and allow sending the quit signal before it's even > started. A given run loop can only be run once, but you can just allocate one on > the stack, then run it: > > base::RunLoop().RunUntilIdle(); Done. https://codereview.chromium.org/378823002/diff/40001/net/http/http_server_pro... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/378823002/diff/40001/net/http/http_server_pro... net/http/http_server_properties_manager.cc:714: } // namespace chrome_browser_net On 2014/07/09 12:26:08, droger wrote: > Fix the comment. Done. https://codereview.chromium.org/378823002/diff/40001/net/http/http_server_pro... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/378823002/diff/40001/net/http/http_server_pro... net/http/http_server_properties_manager.h:256: } // namespace chrome_browser_net On 2014/07/09 12:26:08, droger wrote: > Fix the comment. Done. https://codereview.chromium.org/378823002/diff/40001/net/http/http_server_pro... File net/http/http_server_properties_manager_unittest.cc (right): https://codereview.chromium.org/378823002/diff/40001/net/http/http_server_pro... net/http/http_server_properties_manager_unittest.cc:484: } // namespace chrome_browser_net On 2014/07/09 12:26:08, droger wrote: > Fix the comment. Done.
I'm going to go ahead and defer to rch and rsleevi on this one - they both are much more familiar with the code than I am.
On 2014/07/09 14:26:28, mmenke wrote: > I'm going to go ahead and defer to rch and rsleevi on this one - they both are > much more familiar with the code than I am. Oh, but they don't own chrome/browser/profiles/profile_impl_io_data.*.
On 2014/07/09 14:28:35, mef wrote: > On 2014/07/09 14:26:28, mmenke wrote: > > I'm going to go ahead and defer to rch and rsleevi on this one - they both are > > much more familiar with the code than I am. > > Oh, but they don't own chrome/browser/profiles/profile_impl_io_data.*. Good point. profiles/ LGTM. I defer to them on the other changes (Happy to review them, too, if you want, just seems like they're doing a great job, and know the old state of affairs much better than I do).
On 2014/07/09 09:10:14, mef wrote: > On 2014/07/08 23:00:52, Ryan Hamilton wrote: > > On 2014/07/08 22:54:36, Ryan Sleevi wrote: > > > > > > https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... > > > File net/http/http_server_properties_manager.h (right): > > > > > > > > > https://codereview.chromium.org/378823002/diff/20001/net/http/http_server_pro... > > > net/http/http_server_properties_manager.h:68: void ShutdownOnPrefThread(); > > > On 2014/07/08 21:47:21, Bernhard Bauer wrote: > > > > On 2014/07/08 18:19:26, Ryan Sleevi wrote: > > > > > And what happens if they don't? > > > > > > > > A DCHECK will trigger when this object (with its UI-thread-living members) > > is > > > > destroyed on another thread. Is there a better way to describe such a > > contract > > > > of a class in a comment? > > > > > > Well, this is more of a "long term design" issue, and not a refactoring that > I > > > think is essential to moving this (but one I do think is essential for code > > > health), but an 'ideal' world is that the HSPM lives on a single thread - > the > > > Pref thread - and that there's no need for an explicit Shutdown() state, as > > C++ > > > provides that (via the DTOR). > > > > > > Since HSP has to live on the IO thread, it would be a separate class, and > the > > > HSPM and the HSP(Prefs) could communicate via internal, > implementation-detail > > > methods. > > > > > > rch@ has a much better idea of this vision. > > > > I started working on such a CL here: https://codereview.chromium.org/375083002 > > > > Basically it breaks HSPM into 2 classes. > > * one which lives only on the IO thread and knows nothing about the UI thread > > * one which lives only on the UI thread and owns the first object. > > > > But it's incomplete. I started last week and then got distracted. > > Oops, sorry to step on your toes. Does it make sense for me to continue with > this CL or could you add the move to //net to yours? Yes, please continue with your CL. I can do my refactor after yours lands.
On 2014/07/09 14:26:28, mmenke wrote: > I'm going to go ahead and defer to rch and rsleevi on this one - they both are > much more familiar with the code than I am. In turn, I'll defer to rsleevi. If he's happy, I'm happy.
On 2014/07/09 15:35:22, Ryan Hamilton wrote: > On 2014/07/09 14:26:28, mmenke wrote: > > I'm going to go ahead and defer to rch and rsleevi on this one - they both are > > much more familiar with the code than I am. > > In turn, I'll defer to rsleevi. If he's happy, I'm happy. Sounds good. I shall make rsleevi happy.
I focused on the net/ components. The meat of the comments I'll save for rch@, for which I'm comfortably excited. For this, I mostly focused on nits related to your renaming. Still a number of references to UI to clean up. I have a slight preference towards "PrefsThread" over "Pref" thread, if only because when I expand the abbreviation, "Preferences Thread" seems more accurate than "Preference Thread", but that's neither here nor there, and I defer to your judgement. https://codereview.chromium.org/378823002/diff/70033/net/http/http_server_pro... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/378823002/diff/70033/net/http/http_server_pro... net/http/http_server_properties_manager.cc:303: // The preferences can only be read on the UI thread. s/UI/pref/ https://codereview.chromium.org/378823002/diff/70033/net/http/http_server_pro... net/http/http_server_properties_manager.cc:572: // Update the preferences on the UI thread. s/UI/pref/ https://codereview.chromium.org/378823002/diff/70033/net/http/http_server_pro... net/http/http_server_properties_manager.cc:585: // UpdatePrefsOnUI. s/UpdatePrefsOnUI/UpdatePrefsOnPrefThread/ https://codereview.chromium.org/378823002/diff/70033/net/http/http_server_pro... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/378823002/diff/70033/net/http/http_server_pro... net/http/http_server_properties_manager.h:176: // preferences. It gets the data on UI thread and calls s/UI/Pref/ https://codereview.chromium.org/378823002/diff/70033/net/http/http_server_pro... net/http/http_server_properties_manager.h:201: // posts a task (UpdatePrefsOnUI) to update the preferences UI thread. s/UpdatePrefsOnUI/UpdatePrefsOnPrefThread/ s/UI/pref/ https://codereview.chromium.org/378823002/diff/70033/net/http/http_server_pro... net/http/http_server_properties_manager.h:204: // Same as above, but fires an optional |completion| callback on the UI thread s/UI/pref/ https://codereview.chromium.org/378823002/diff/70033/net/http/http_server_pro... File net/http/http_server_properties_manager_unittest.cc (right): https://codereview.chromium.org/378823002/diff/70033/net/http/http_server_pro... net/http/http_server_properties_manager_unittest.cc:52: void UpdateCacheFromPrefsOnUIConcrete() { *cough* https://codereview.chromium.org/378823002/diff/70033/net/http/http_server_pro... net/http/http_server_properties_manager_unittest.cc:396: http_server_props_manager_->Clear(base::MessageLoop::QuitClosure()); You should get the QuitClosure from the RunLoop, not the MessageLoop This means constructing the RunLoop before setting up this callback. That's OK.
lgtm
On 2014/07/09 19:30:08, Ryan Sleevi wrote: > lgtm Thanks, I'll fix missed renames and land it tomorrow. I'll start on moving ssl_config_service_manager following the same pattern afterwards.
Thanks! I've kept the name |PrefThread| instead of |PrefsThread| for consistency with member variables that start with |pref_|. https://codereview.chromium.org/378823002/diff/70033/net/http/http_server_pro... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/378823002/diff/70033/net/http/http_server_pro... net/http/http_server_properties_manager.cc:303: // The preferences can only be read on the UI thread. On 2014/07/09 19:29:58, Ryan Sleevi wrote: > s/UI/pref/ Done. https://codereview.chromium.org/378823002/diff/70033/net/http/http_server_pro... net/http/http_server_properties_manager.cc:572: // Update the preferences on the UI thread. On 2014/07/09 19:29:57, Ryan Sleevi wrote: > s/UI/pref/ Done. https://codereview.chromium.org/378823002/diff/70033/net/http/http_server_pro... net/http/http_server_properties_manager.cc:585: // UpdatePrefsOnUI. On 2014/07/09 19:29:57, Ryan Sleevi wrote: > s/UpdatePrefsOnUI/UpdatePrefsOnPrefThread/ Done. https://codereview.chromium.org/378823002/diff/70033/net/http/http_server_pro... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/378823002/diff/70033/net/http/http_server_pro... net/http/http_server_properties_manager.h:176: // preferences. It gets the data on UI thread and calls On 2014/07/09 19:29:58, Ryan Sleevi wrote: > s/UI/Pref/ Done. https://codereview.chromium.org/378823002/diff/70033/net/http/http_server_pro... net/http/http_server_properties_manager.h:201: // posts a task (UpdatePrefsOnUI) to update the preferences UI thread. On 2014/07/09 19:29:58, Ryan Sleevi wrote: > s/UpdatePrefsOnUI/UpdatePrefsOnPrefThread/ > s/UI/pref/ Done. https://codereview.chromium.org/378823002/diff/70033/net/http/http_server_pro... net/http/http_server_properties_manager.h:204: // Same as above, but fires an optional |completion| callback on the UI thread On 2014/07/09 19:29:58, Ryan Sleevi wrote: > s/UI/pref/ Done.
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/378823002/100001
Message was sent while issue was closed.
Change committed as 282319 |
