chrome/browser/prefs/browser_prefs.cc - Issue 378823002: Move http_server_properties_manager from chrome/browser/net to net/http.

Unified Diff: chrome/browser/prefs/browser_prefs.cc

Issue 378823002: Move http_server_properties_manager from chrome/browser/net to net/http. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Created 6 years, 5 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« no previous file with comments | « chrome/browser/net/http_server_properties_manager_unittest.cc ('k') | chrome/browser/profiles/profile_impl_io_data.h » ('j') | chrome/browser/profiles/profile_impl_io_data.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: chrome/browser/prefs/browser_prefs.cc

diff --git a/chrome/browser/prefs/browser_prefs.cc b/chrome/browser/prefs/browser_prefs.cc

index 39058b0ea052cb60339a8ed35a2eb34940494b1b..484c5aab6a6b32bdb0fdf5ab2cf7041366dcb53f 100644

--- a/chrome/browser/prefs/browser_prefs.cc

+++ b/chrome/browser/prefs/browser_prefs.cc

@@ -37,7 +37,6 @@

#include "chrome/browser/media/media_stream_devices_controller.h"

#include "chrome/browser/metrics/chrome_metrics_service_client.h"

#include "chrome/browser/metrics/variations/variations_service.h"

-#include "chrome/browser/net/http_server_properties_manager.h"

#include "chrome/browser/net/net_pref_observer.h"

#include "chrome/browser/net/prediction_options.h"

#include "chrome/browser/net/predictor.h"

@@ -96,6 +95,7 @@

#include "components/translate/core/browser/translate_prefs.h"

#include "content/public/browser/render_process_host.h"

#include "extensions/browser/extension_prefs.h"

+#include "net/http/http_server_properties_manager.h"

#if defined(ENABLE_AUTOFILL_DIALOG)

#include "chrome/browser/ui/autofill/autofill_dialog_controller.h"

@@ -222,6 +222,14 @@ const char kBackupPref[] = "backup";

const char kSyncPromoErrorMessage[] = "sync_promo.error_message";

#endif

+// Register |prefs| for properties managed by HttpServerPropertiesManager.

+void HttpServerPropertiesManager_RegisterProfilePrefs(

droger 2014/07/08 13:06:02 Shouldn't this stay in the HttpServerPropertiesMan

mef 2014/07/08 13:32:00 The problem is that user_prefs::PrefRegistrySyncab

battre 2014/07/08 14:16:57 Bernhard, what do you think about this? My feelin

Bernhard Bauer 2014/07/08 16:02:46 We already pass in the pref path for the HttpServe

mef 2014/07/08 16:19:54 Sounds good. Just to make sure that I understand c

Bernhard Bauer 2014/07/08 17:02:45 Yup, pretty much. The only thing I'm not sure abo

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.

mef 2014/07/08 17:11:20 I like ChromeHttpServerPropertiesManager, but I do

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.

mef 2014/07/08 18:04:32 Done.

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.

+ user_prefs::PrefRegistrySyncable* prefs) {

+ prefs->RegisterDictionaryPref(

+ prefs::kHttpServerProperties,

+ user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);

} // namespace

namespace chrome {

@@ -353,8 +361,7 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {

sync_driver::SyncPrefs::RegisterProfilePrefs(registry);

ChromeContentBrowserClient::RegisterProfilePrefs(registry);

ChromeVersionService::RegisterProfilePrefs(registry);

- chrome_browser_net::HttpServerPropertiesManager::RegisterProfilePrefs(

- registry);

+ HttpServerPropertiesManager_RegisterProfilePrefs(registry);

battre 2014/07/08 14:16:57 please consider the request to keep this alphabeti

Bernhard Bauer 2014/07/08 16:02:46 +1

mef 2014/07/08 16:19:54 Will do.

mef 2014/07/08 18:04:32 Done.

chrome_browser_net::Predictor::RegisterProfilePrefs(registry);

chrome_browser_net::RegisterPredictionOptionsProfilePrefs(registry);

chrome_prefs::RegisterProfilePrefs(registry);