Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1175733002:
[Cronet] Set up HttpServerPropertiesManager (Closed)
Description[Cronet] Set up HttpServerPropertiesManager
This CL sets up HttpServerPropertiesManager in the async API.
It also added a test in QuicTest.java to make sure quic pref
data is persisted across shutdowns.
BUG=428937
Committed: https://crrev.com/c56b3bc999401c0a598ebac15f4de3f670dbf7dc
Cr-Commit-Position: refs/heads/master@{#340162}
Patch Set 1 : #
Total comments: 16
Patch Set 2 : Addressed comments #Patch Set 3 : Pass in a HttpServerPropertiesManager directly #
Total comments: 11
Patch Set 4 : Address comments #
Total comments: 10
Patch Set 5 : shutdown #
Total comments: 7
Patch Set 6 : Address Matt's comments #
Total comments: 4
Patch Set 7 : make builder depend on HttpServerProperties, and pass in weak ptr #
Total comments: 2
Patch Set 8 : Pass ownership to storage and keep a raw ptr #
Total comments: 8
Patch Set 9 : Address Matt's comments #Patch Set 10 : Address Findbug error #
Messages
Total messages: 53 (17 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
xunjieli@chromium.org changed reviewers: + pauljensen@chromium.org
Hi Paul, since you are working on some QUIC stuff recently, I am sending this issue to you! Hope you don't mind :D
pauljensen@chromium.org changed reviewers: + mmenke@chromium.org
+mmenke I don't know much about PrefService and what its desired lifetime/threading/ownership model is. Matt, are you more familiar with it and HttpServerPropertiesManager? or should we add a reviewer from base/prefs/OWNERS? https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:210: pref_service_ = factory.Create(registry.get()).Pass(); I don't know much about PrefService. Cronet could potentially be creating several of these running on several different threads. Is that okay? or are they going to be trampling the shared prefs file? Should Cronet have one shared PrefService, like Cronet does for NetworkChangeNotifier? Should PrefService live on the Network threads? Chrome keeps theirs on the UI thread. https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:307: void CronetURLRequestContextAdapter::DestroyOnNetworkThread() { Why is this function needed? Can this be moved to ~CronetURLRequestContextAdapter? https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java (right): https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:92: // after kUpdatePrefsDelayMs in http_server_properties_manager.cc. Yikes, this will pause a full minute. Other tests override this time for testing but this test is in Java and not native... Perhaps for now it's not worth the extra complexity to override it in this case. https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:98: contains = fileContainsString("local_prefs.json", "quic"); This seems prone to failure. Is there a more robust way we could do this? Perhaps use JSONParser? or are you avoiding parsing the file so the test isn't dependent on the format of the prefs file? https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:132: while ((line = reader.readLine()) != null) { Rather than looping, could we just read it all? File file = new File(filename); FileInputStream fileInputStream = new FileInputStream(file); byte[] data = new byte[(int) file.length()]; fileInputStream.read(data); fileInputStream.close(); return new String(data, "UTF-8").contains(content); https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:137: Log.i(TAG, "Pref file line:%s", line); This seems like it might produce an exorbitant amount of logging. https://codereview.chromium.org/1175733002/diff/40001/net/url_request/url_req... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/1175733002/diff/40001/net/url_request/url_req... net/url_request/url_request_context_builder.h:213: const base::WeakPtr<HttpServerProperties>& http_server_properties) { I'm torn as to whether this should be a scoped_ptr or a WeakPtr. URLRequestContext takes a WeakPtr but URLRequestContextStorage takes a scoped_ptr. Using a WeakPtr promotes sharing the HttpServerProperties (which is good for performance) but it forces callers to take ownership (which is added complexity that a lot of URLRequestContextBuilder clients probably don't want to entertain). I'm tempted to go with a scoped_ptr and have URLRequestContextBuilder take ownership, as it does for set_host_resolver() which is another preferably share resource. I guess this depends on how PrefService is supposed to be used (i.e. one versus man instantiations). On a side note, this class desperately needs a useful comment, especially because we encourage its use. It's frankly terrifying that so many of its setters take pointers...but take ownership. Then Build() transfers ownership to the URLRequestContext, so calling Build() twice will have different results (i.e. the first generated context get's the owned item, but the second gets none/default).
https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:210: pref_service_ = factory.Create(registry.get()).Pass(); On 2015/06/15 15:57:42, pauljensen wrote: > I don't know much about PrefService. Cronet could potentially be creating > several of these running on several different threads. Is that okay? or are > they going to be trampling the shared prefs file? Should Cronet have one shared > PrefService, like Cronet does for NetworkChangeNotifier? > Should PrefService live on the Network threads? Chrome keeps theirs on the UI > thread. Each context must have its own cache directory. I'd say passing ownership is the way to go - that's the way all other URLRequestContextBuilder methods currently work.
On 2015/06/15 16:36:27, mmenke wrote: > https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... > File components/cronet/android/cronet_url_request_context_adapter.cc (right): > > https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... > components/cronet/android/cronet_url_request_context_adapter.cc:210: > pref_service_ = factory.Create(registry.get()).Pass(); > On 2015/06/15 15:57:42, pauljensen wrote: > > I don't know much about PrefService. Cronet could potentially be creating > > several of these running on several different threads. Is that okay? or are > > they going to be trampling the shared prefs file? Should Cronet have one > shared > > PrefService, like Cronet does for NetworkChangeNotifier? > > Should PrefService live on the Network threads? Chrome keeps theirs on the UI > > thread. > > Each context must have its own cache directory. I would think we would want one preference store per "storage path", similar to a cache directory. Hmm. I'm confused. The "storage path" contains the cache AFAIK, but the "storage path" is set on the Config which can be used to generate many Contexts. This seems counter-intuitive; shouldn't the "storage path" be set on the Context then? or should we add some check that a single Config isn't used to generate multiple Contexts that might share the same cache directory? > > I'd say passing ownership is the way to go - that's the way all other > URLRequestContextBuilder methods currently work. This agrees with my noted tendency.
On 2015/06/15 17:18:39, pauljensen wrote: > On 2015/06/15 16:36:27, mmenke wrote: > > > https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... > > File components/cronet/android/cronet_url_request_context_adapter.cc (right): > > > > > https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... > > components/cronet/android/cronet_url_request_context_adapter.cc:210: > > pref_service_ = factory.Create(registry.get()).Pass(); > > On 2015/06/15 15:57:42, pauljensen wrote: > > > I don't know much about PrefService. Cronet could potentially be creating > > > several of these running on several different threads. Is that okay? or are > > > they going to be trampling the shared prefs file? Should Cronet have one > > shared > > > PrefService, like Cronet does for NetworkChangeNotifier? > > > Should PrefService live on the Network threads? Chrome keeps theirs on the > UI > > > thread. > > > > Each context must have its own cache directory. > I would think we would want one preference store per "storage path", similar to > a cache directory. > Hmm. I'm confused. The "storage path" contains the cache AFAIK, but the > "storage path" is set on the Config which can be used to generate many Contexts. > This seems counter-intuitive; shouldn't the "storage path" be set on the > Context then? or should we add some check that a single Config isn't used to > generate multiple Contexts that might share the same cache directory? It's intended that one config be used for one context. Config is a separate class because it's used to create a context, and you can't change any of it once you've actually created the context. > > > > > I'd say passing ownership is the way to go - that's the way all other > > URLRequestContextBuilder methods currently work. > This agrees with my noted tendency.
I agree with Paul that there's a chance of misusing the config for multiple contexts, since it isn't clear from the API. I am not sure how we can check that programmatically, but we should definitely take a note of the requirement in UrlRequestContextConfig#setStoragePath that storage path should not be the same for multiple contexts if they are alive at the same time. On Mon, Jun 15, 2015 at 1:23 PM, <mmenke@chromium.org> wrote: > On 2015/06/15 17:18:39, pauljensen wrote: > >> On 2015/06/15 16:36:27, mmenke wrote: >> > >> > > > https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... > >> > File components/cronet/android/cronet_url_request_context_adapter.cc >> > (right): > >> > >> > >> > > > https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... > >> > components/cronet/android/cronet_url_request_context_adapter.cc:210: >> > pref_service_ = factory.Create(registry.get()).Pass(); >> > On 2015/06/15 15:57:42, pauljensen wrote: >> > > I don't know much about PrefService. Cronet could potentially be >> creating >> > > several of these running on several different threads. Is that okay? >> or >> > are > >> > > they going to be trampling the shared prefs file? Should Cronet have >> one >> > shared >> > > PrefService, like Cronet does for NetworkChangeNotifier? >> > > Should PrefService live on the Network threads? Chrome keeps theirs >> on >> > the > >> UI >> > > thread. >> > >> > Each context must have its own cache directory. >> I would think we would want one preference store per "storage path", >> similar >> > to > >> a cache directory. >> Hmm. I'm confused. The "storage path" contains the cache AFAIK, but the >> "storage path" is set on the Config which can be used to generate many >> > Contexts. > >> This seems counter-intuitive; shouldn't the "storage path" be set on the >> Context then? or should we add some check that a single Config isn't used >> to >> generate multiple Contexts that might share the same cache directory? >> > > It's intended that one config be used for one context. Config is a > separate > class because it's used to create a context, and you can't change any of > it once > you've actually created the context. > > > > > >> > I'd say passing ownership is the way to go - that's the way all other >> > URLRequestContextBuilder methods currently work. >> This agrees with my noted tendency. >> > > > > https://codereview.chromium.org/1175733002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:210: pref_service_ = factory.Create(registry.get()).Pass(); On 2015/06/15 15:57:42, pauljensen wrote: > I don't know much about PrefService. Cronet could potentially be creating > several of these running on several different threads. Is that okay? or are > they going to be trampling the shared prefs file? Should Cronet have one shared > PrefService, like Cronet does for NetworkChangeNotifier? > Should PrefService live on the Network threads? Chrome keeps theirs on the UI > thread. Just for the record. Talked to Paul offline, and we decided to add a check for not reusing the same config in a followup CL. https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:307: void CronetURLRequestContextAdapter::DestroyOnNetworkThread() { On 2015/06/15 15:57:42, pauljensen wrote: > Why is this function needed? Can this be moved to > ~CronetURLRequestContextAdapter? Done. Oops. Didn't realize that we can do that. https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java (right): https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:92: // after kUpdatePrefsDelayMs in http_server_properties_manager.cc. On 2015/06/15 15:57:42, pauljensen wrote: > Yikes, this will pause a full minute. Other tests override this time for > testing but this test is in Java and not native... Perhaps for now it's not > worth the extra complexity to override it in this case. Acknowledged. Yea. This is pretty ugly. https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:98: contains = fileContainsString("local_prefs.json", "quic"); On 2015/06/15 15:57:42, pauljensen wrote: > This seems prone to failure. Is there a more robust way we could do this? > Perhaps use JSONParser? or are you avoiding parsing the file so the test isn't > dependent on the format of the prefs file? Yes to the second part. There is no documentation on the format of the json file. When Bence changed the manager for altService, the keys changed, so I think we can't rely on the format. https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:132: while ((line = reader.readLine()) != null) { On 2015/06/15 15:57:42, pauljensen wrote: > Rather than looping, could we just read it all? > > File file = new File(filename); > FileInputStream fileInputStream = new FileInputStream(file); > byte[] data = new byte[(int) file.length()]; > fileInputStream.read(data); > fileInputStream.close(); > return new String(data, "UTF-8").contains(content); Done. That is indeed cleaner. https://codereview.chromium.org/1175733002/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:137: Log.i(TAG, "Pref file line:%s", line); On 2015/06/15 15:57:42, pauljensen wrote: > This seems like it might produce an exorbitant amount of logging. Done. The file turns out to be pretty small. I added it for mainly for debugging, but I guess we can remove it. https://codereview.chromium.org/1175733002/diff/40001/net/url_request/url_req... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/1175733002/diff/40001/net/url_request/url_req... net/url_request/url_request_context_builder.h:213: const base::WeakPtr<HttpServerProperties>& http_server_properties) { On 2015/06/15 15:57:42, pauljensen wrote: > I'm torn as to whether this should be a scoped_ptr or a WeakPtr. > URLRequestContext takes a WeakPtr but URLRequestContextStorage takes a > scoped_ptr. Using a WeakPtr promotes sharing the HttpServerProperties (which is > good for performance) but it forces callers to take ownership (which is added > complexity that a lot of URLRequestContextBuilder clients probably don't want to > entertain). I'm tempted to go with a scoped_ptr and have > URLRequestContextBuilder take ownership, as it does for set_host_resolver() > which is another preferably share resource. I guess this depends on how > PrefService is supposed to be used (i.e. one versus man instantiations). > I also prefer that storage takes ownership of the HttpServerPropertiesManager, but HttpServerPropertiesManager has this requirement that it needs to be explicitly shut down on the pref thread (See HttpServerPropertiesManager::ShutdownOnPrefThread, which the interface class does not have). And In Cronet, there isn't a pref thread, we just use the network thread as the pref thread for scheduling updates. This forces me to use WeakPtr instead to manage the initialization and shutdown of the HttpServerPropertiesManager instance explicitly. Not sure if there's a work around. > On a side note, this class desperately needs a useful comment, especially > because we encourage its use. It's frankly terrifying that so many of its > setters take pointers...but take ownership. Then Build() transfers ownership to > the URLRequestContext, so calling Build() twice will have different results > (i.e. the first generated context get's the owned item, but the second gets > none/default). Agreed.
https://codereview.chromium.org/1175733002/diff/40001/net/url_request/url_req... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/1175733002/diff/40001/net/url_request/url_req... net/url_request/url_request_context_builder.h:213: const base::WeakPtr<HttpServerProperties>& http_server_properties) { On 2015/06/15 18:41:36, xunjieli wrote: > On 2015/06/15 15:57:42, pauljensen wrote: > > I'm torn as to whether this should be a scoped_ptr or a WeakPtr. > > URLRequestContext takes a WeakPtr but URLRequestContextStorage takes a > > scoped_ptr. Using a WeakPtr promotes sharing the HttpServerProperties (which > is > > good for performance) but it forces callers to take ownership (which is added > > complexity that a lot of URLRequestContextBuilder clients probably don't want > to > > entertain). I'm tempted to go with a scoped_ptr and have > > URLRequestContextBuilder take ownership, as it does for set_host_resolver() > > which is another preferably share resource. I guess this depends on how > > PrefService is supposed to be used (i.e. one versus man instantiations). > > > I also prefer that storage takes ownership of the HttpServerPropertiesManager, > but > HttpServerPropertiesManager has this requirement that it needs to be explicitly > shut down on the pref thread > (See HttpServerPropertiesManager::ShutdownOnPrefThread, which the interface > class does not have). And In Cronet, there isn't a pref thread, we just use the > network thread as the pref thread for scheduling updates. This forces me to use > WeakPtr instead to manage the initialization and shutdown of the > HttpServerPropertiesManager instance explicitly. Not sure if there's a work > around. I guess if we make storage to take ownership of the HttpServerPropertiesManager. During shutdown, we can get HttpServerProperties from the URLRequestContext and do a cast to see if it is an instance of HttpServerPropertiesManager? If yes, we will invoke ShutDownOnPrefThread?
On 2015/06/15 18:49:22, xunjieli wrote: > https://codereview.chromium.org/1175733002/diff/40001/net/url_request/url_req... > File net/url_request/url_request_context_builder.h (right): > > https://codereview.chromium.org/1175733002/diff/40001/net/url_request/url_req... > net/url_request/url_request_context_builder.h:213: const > base::WeakPtr<HttpServerProperties>& http_server_properties) { > On 2015/06/15 18:41:36, xunjieli wrote: > > On 2015/06/15 15:57:42, pauljensen wrote: > > > I'm torn as to whether this should be a scoped_ptr or a WeakPtr. > > > URLRequestContext takes a WeakPtr but URLRequestContextStorage takes a > > > scoped_ptr. Using a WeakPtr promotes sharing the HttpServerProperties > (which > > is > > > good for performance) but it forces callers to take ownership (which is > added > > > complexity that a lot of URLRequestContextBuilder clients probably don't > want > > to > > > entertain). I'm tempted to go with a scoped_ptr and have > > > URLRequestContextBuilder take ownership, as it does for set_host_resolver() > > > which is another preferably share resource. I guess this depends on how > > > PrefService is supposed to be used (i.e. one versus man instantiations). > > > > > I also prefer that storage takes ownership of the HttpServerPropertiesManager, > > but > > HttpServerPropertiesManager has this requirement that it needs to be > explicitly > > shut down on the pref thread > > (See HttpServerPropertiesManager::ShutdownOnPrefThread, which the interface > > class does not have). And In Cronet, there isn't a pref thread, we just use > the > > network thread as the pref thread for scheduling updates. This forces me to > use > > WeakPtr instead to manage the initialization and shutdown of the > > HttpServerPropertiesManager instance explicitly. Not sure if there's a work > > around. > I guess if we make storage to take ownership of the HttpServerPropertiesManager. > During shutdown, we can get HttpServerProperties from the URLRequestContext and > do a cast to see if it is an instance of HttpServerPropertiesManager? If yes, we > will invoke ShutDownOnPrefThread? How about changing to allow passing in a HttpServerPropertiesManager? We'll have to make the assumption that the prefs thread is the networking thread for calling ShutDownOnPrefThread in ~BasicURLRequestContext, but I think this should be fine for most simple users like Cronet and most URLRequestContextBuilder clients. BTW, AFAIK we don't have RTTI so I'm not sure how we'd "cast to see if it is an instance of HttpServerPropertiesManager", and this sounds somewhat fragile.
Patchset #3 (id:80001) has been deleted
Thanks for the suggestion! I think it is a reasonable asssumption to make. I made the builder to set a manager on the BasicURLRequestContext directly. PTAL. https://codereview.chromium.org/1175733002/diff/100001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1175733002/diff/100001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:14: #include "base/message_loop/message_loop.h" Added this include because this file uses base::MessageLoop, and the include is missing. Not related to this CL. Happy to revert if you'd like me to keep things separate.
https://codereview.chromium.org/1175733002/diff/100001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/1175733002/diff/100001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:110: // |pref_service_| should outlive |http_server_properties_manager_|. change |http_server_properties_manager_| to refer to the HttpServerPropertiesManager owned by |context_| https://codereview.chromium.org/1175733002/diff/100001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:116: scoped_ptr<net::HttpServerPropertiesManager> http_server_properties_manager_; this doesn't need to be a class member anymore as it's short lived https://codereview.chromium.org/1175733002/diff/100001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1175733002/diff/100001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:170: // thread, shut down the |http_server_properties_manager_| if one is set. I don't really like referring to a particular embedder by name here; sorta strikes me as a layering violation. How about instead just mentioning that it needs to be shutdown and this is the only place BasicURLRequestContext can do so. https://codereview.chromium.org/1175733002/diff/100001/net/url_request/url_re... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/1175733002/diff/100001/net/url_request/url_re... net/url_request/url_request_context_builder.h:25: #include "base/memory/weak_ptr.h" unneeded? https://codereview.chromium.org/1175733002/diff/100001/net/url_request/url_re... net/url_request/url_request_context_builder.h:46: class HttpServerProperties; unneeded?
Patchset #4 (id:120001) has been deleted
Thanks for the review! PTAL. https://codereview.chromium.org/1175733002/diff/100001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/1175733002/diff/100001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:110: // |pref_service_| should outlive |http_server_properties_manager_|. On 2015/07/01 18:42:22, pauljensen wrote: > change |http_server_properties_manager_| to refer to the > HttpServerPropertiesManager owned by |context_| Done. https://codereview.chromium.org/1175733002/diff/100001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:116: scoped_ptr<net::HttpServerPropertiesManager> http_server_properties_manager_; On 2015/07/01 18:42:22, pauljensen wrote: > this doesn't need to be a class member anymore as it's short lived Done. https://codereview.chromium.org/1175733002/diff/100001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1175733002/diff/100001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:170: // thread, shut down the |http_server_properties_manager_| if one is set. On 2015/07/01 18:42:22, pauljensen wrote: > I don't really like referring to a particular embedder by name here; sorta > strikes me as a layering violation. How about instead just mentioning that it > needs to be shutdown and this is the only place BasicURLRequestContext can do > so. Done. https://codereview.chromium.org/1175733002/diff/100001/net/url_request/url_re... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/1175733002/diff/100001/net/url_request/url_re... net/url_request/url_request_context_builder.h:25: #include "base/memory/weak_ptr.h" On 2015/07/01 18:42:22, pauljensen wrote: > unneeded? Done. https://codereview.chromium.org/1175733002/diff/100001/net/url_request/url_re... net/url_request/url_request_context_builder.h:46: class HttpServerProperties; On 2015/07/01 18:42:22, pauljensen wrote: > unneeded? Done.
https://codereview.chromium.org/1175733002/diff/140001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/1175733002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:110: Can we remove this new line? and the one on line 114, 116, 118, 120, 131, 133? Our generally policy is to minimize use of vertical whitespace: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Vertical_Whit... https://codereview.chromium.org/1175733002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:119: scoped_refptr<JsonPrefStore> json_pref_store_; why is this line moved up? https://codereview.chromium.org/1175733002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:126: // Context config is only valid untng context is initialized. untng->until https://codereview.chromium.org/1175733002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java (right): https://codereview.chromium.org/1175733002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:92: // after kUpdatePrefsDelayMs in http_server_properties_manager.cc. Does this mean that if your app is never allowed to live more than 59s that the QUIC server preferences will never be persisted to disk? This seems bad for many Android apps that potentially don't live very long. Can we flush these when the context is shutdown similar to what the context adapter's comment says "tearing down |sdch_owner_| forces |json_pref_store_| to flush pending writes to the disk."? https://codereview.chromium.org/1175733002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:98: contains = fileContainsString("local_prefs.json", "quic"); can we remove |contains| and just use "if (blah) break;"?
https://codereview.chromium.org/1175733002/diff/140001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/1175733002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:110: On 2015/07/06 16:31:46, pauljensen wrote: > Can we remove this new line? and the one on line 114, 116, 118, 120, 131, 133? > Our generally policy is to minimize use of vertical whitespace: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Vertical_Whit... Partially done. I still left a new line before variable with comments. Tried removing those, but it looks too crowded. https://codereview.chromium.org/1175733002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:119: scoped_refptr<JsonPrefStore> json_pref_store_; On 2015/07/06 16:31:46, pauljensen wrote: > why is this line moved up? Before this CL, only sdch_owner uses json_pref_store, so I put them together. With this change, HttpServerPropertiesManager is also using json_pref_store. I moved json_pref_store above the comment so json_pref_store does not appear exclusively associated with sdch_owner. https://codereview.chromium.org/1175733002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:126: // Context config is only valid untng context is initialized. On 2015/07/06 16:31:46, pauljensen wrote: > untng->until Done. https://codereview.chromium.org/1175733002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java (right): https://codereview.chromium.org/1175733002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:92: // after kUpdatePrefsDelayMs in http_server_properties_manager.cc. On 2015/07/06 16:31:46, pauljensen wrote: > Does this mean that if your app is never allowed to live more than 59s that the > QUIC server preferences will never be persisted to disk? This seems bad for > many Android apps that potentially don't live very long. Can we flush these > when the context is shutdown similar to what the context adapter's comment says > "tearing down |sdch_owner_| forces |json_pref_store_| to flush pending writes to > the disk."? In this our case, we can't easily flush the updates during context shutdown(pref_service_->CommitPendingWrites is not enough, since the updates/writes might not have been scheduled). Filed https://code.google.com/p/chromium/issues/detail?id=507389 to change how the scheduling works in http_server_properties_manager.cc. Let me know if you have any suggestion. https://codereview.chromium.org/1175733002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:98: contains = fileContainsString("local_prefs.json", "quic"); On 2015/07/06 16:31:46, pauljensen wrote: > can we remove |contains| and just use "if (blah) break;"? Done.
https://codereview.chromium.org/1175733002/diff/160001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1175733002/diff/160001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:215: base::ThreadTaskRunnerHandle::Get())); You should be using GetFileThread()'s task runner here, since we don't want to do file IO on the network thread. https://codereview.chromium.org/1175733002/diff/160001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1175733002/diff/160001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:353: http_server_properties_manager_.Pass()); Why can't you just do: storage->set_http_server_properties(http_server_properties_manager_.Pass());? The naming is incredibly bad and confusing, but HttpServerPropertiesManager is an HttpServerProperties. https://codereview.chromium.org/1175733002/diff/160001/net/url_request/url_re... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/1175733002/diff/160001/net/url_request/url_re... net/url_request/url_request_context_builder.h:214: // be passed to the URLRequestContext. The "Note..." sentence probably isn't needed - that's implied by the fact it takes a scoped_ptr.
https://codereview.chromium.org/1175733002/diff/160001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1175733002/diff/160001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:215: base::ThreadTaskRunnerHandle::Get())); On 2015/07/07 19:18:29, mmenke wrote: > You should be using GetFileThread()'s task runner here, since we don't want to > do file IO on the network thread. Hmm.. but according to the header file, the third param is supposed to be network_task_runner. https://codereview.chromium.org/1175733002/diff/160001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1175733002/diff/160001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:353: http_server_properties_manager_.Pass()); On 2015/07/07 19:18:29, mmenke wrote: > Why can't you just do: > > storage->set_http_server_properties(http_server_properties_manager_.Pass());? > > The naming is incredibly bad and confusing, but HttpServerPropertiesManager is > an HttpServerProperties. I agree that the naming is very confusing here. (The manager being a subclass of HttpServerProperties). The reason that I did this is because we need to perform shutdown on the manager, see "http_server_properties_manager_->ShutdownOnPrefThread()" on line 169. If we do storage->set_http_server_properties(http_server_properties_manager_.Pass()); then we will have a member variable of the parent class and cannot call ShutdownOnPrefThread on it right? Any suggestion? https://codereview.chromium.org/1175733002/diff/160001/net/url_request/url_re... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/1175733002/diff/160001/net/url_request/url_re... net/url_request/url_request_context_builder.h:214: // be passed to the URLRequestContext. On 2015/07/07 19:18:29, mmenke wrote: > The "Note..." sentence probably isn't needed - that's implied by the fact it > takes a scoped_ptr. Done.
https://codereview.chromium.org/1175733002/diff/160001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1175733002/diff/160001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:215: base::ThreadTaskRunnerHandle::Get())); On 2015/07/07 19:37:34, xunjieli wrote: > On 2015/07/07 19:18:29, mmenke wrote: > > You should be using GetFileThread()'s task runner here, since we don't want to > > do file IO on the network thread. > > Hmm.. but according to the header file, the third param is supposed to be > network_task_runner. Oops...You're right. I was thinking this was the pref service constructor call. https://codereview.chromium.org/1175733002/diff/180001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1175733002/diff/180001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:169: http_server_properties_manager_->ShutdownOnPrefThread(); Isn't the pref thread actually the file thread? If so, we're calling this on the wrong thread. Solution seems to be to have Cronet keep around a pointer to the manager, and take care of that before shutting down the context. Think the order would have to be: * Post task to do that on the pref (file) thread * Wait for that task to complete * Post task to shut down and destroy the context * Stop the network thread (Which implicitly waits for the previous event to complete) * Stop the file thread.
https://codereview.chromium.org/1175733002/diff/180001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1175733002/diff/180001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:169: http_server_properties_manager_->ShutdownOnPrefThread(); On 2015/07/07 20:28:12, mmenke wrote: > Isn't the pref thread actually the file thread? If so, we're calling this on > the wrong thread. > > Solution seems to be to have Cronet keep around a pointer to the manager, and > take care of that before shutting down the context. > > Think the order would have to be: > > * Post task to do that on the pref (file) thread > * Wait for that task to complete > * Post task to shut down and destroy the context > * Stop the network thread (Which implicitly waits for the previous event to > complete) > * Stop the file thread. Why does the pref thread need to be the file thread? It can also be the network thread, right? The pref thread just needs to do scheduling and listen for pref updates. It doesn't do disk i/o. Sorry, I tried reading the code, but I don't really understand the threading requirement. Could you explain a bit more on why the pref thread is actually the file thread?
https://codereview.chromium.org/1175733002/diff/180001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1175733002/diff/180001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:169: http_server_properties_manager_->ShutdownOnPrefThread(); On 2015/07/08 20:07:22, xunjieli wrote: > On 2015/07/07 20:28:12, mmenke wrote: > > Isn't the pref thread actually the file thread? If so, we're calling this on > > the wrong thread. > > > > Solution seems to be to have Cronet keep around a pointer to the manager, and > > take care of that before shutting down the context. > > > > Think the order would have to be: > > > > * Post task to do that on the pref (file) thread > > * Wait for that task to complete > > * Post task to shut down and destroy the context > > * Stop the network thread (Which implicitly waits for the previous event to > > complete) > > * Stop the file thread. > > Why does the pref thread need to be the file thread? It can also be the network > thread, right? The pref thread just needs to do scheduling and listen for pref > updates. It doesn't do disk i/o. Sorry, I tried reading the code, but I don't > really understand the threading requirement. Could you explain a bit more on why > the pref thread is actually the file thread? I'm not sure. The pref code doesn't say much about threading. I had assumed it was the thread we pass into PrefStore's constructor, I may be wrong. Regardless, I'd rather not make URLRequestContextBuilder dependent on HttpServerProperties rather than HttpServerPropertiesManager. Disallowing use of HttpServerPropertiesImpl in the interface seems a bit weird, and unnecessary.
Patchset #7 (id:200001) has been deleted
https://codereview.chromium.org/1175733002/diff/180001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1175733002/diff/180001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:169: http_server_properties_manager_->ShutdownOnPrefThread(); On 2015/07/08 20:21:48, mmenke wrote: > On 2015/07/08 20:07:22, xunjieli wrote: > > On 2015/07/07 20:28:12, mmenke wrote: > > > Isn't the pref thread actually the file thread? If so, we're calling this > on > > > the wrong thread. > > > > > > Solution seems to be to have Cronet keep around a pointer to the manager, > and > > > take care of that before shutting down the context. > > > > > > Think the order would have to be: > > > > > > * Post task to do that on the pref (file) thread > > > * Wait for that task to complete > > > * Post task to shut down and destroy the context > > > * Stop the network thread (Which implicitly waits for the previous event to > > > complete) > > > * Stop the file thread. > > > > Why does the pref thread need to be the file thread? It can also be the > network > > thread, right? The pref thread just needs to do scheduling and listen for pref > > updates. It doesn't do disk i/o. Sorry, I tried reading the code, but I don't > > really understand the threading requirement. Could you explain a bit more on > why > > the pref thread is actually the file thread? > > I'm not sure. The pref code doesn't say much about threading. I had assumed it > was the thread we pass into PrefStore's constructor, I may be wrong. > > Regardless, I'd rather not make URLRequestContextBuilder dependent on > HttpServerProperties rather than HttpServerPropertiesManager. Disallowing use > of HttpServerPropertiesImpl in the interface seems a bit weird, and unnecessary. Done. I made the builder take in a weak ptr of the parent class, and manage the shutdown on cronet side. PTAL.
https://codereview.chromium.org/1175733002/diff/220001/net/url_request/url_re... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/1175733002/diff/220001/net/url_request/url_re... net/url_request/url_request_context_builder.h:216: base::WeakPtr<HttpServerProperties> http_server_properties); This should probably take ownership (And then the called can keep a non-owning pointer). That's how all the other methods work, except the ones that copy their data or take ref pointers. The caller should keep a non-weak pointers (Exposing weak pointers to the outside world in general is bad, and we shouldn't do that - I've filed a bug about HttpServerProperties doing that).
https://codereview.chromium.org/1175733002/diff/220001/net/url_request/url_re... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/1175733002/diff/220001/net/url_request/url_re... net/url_request/url_request_context_builder.h:216: base::WeakPtr<HttpServerProperties> http_server_properties); On 2015/07/09 16:18:03, mmenke wrote: > This should probably take ownership (And then the called can keep a non-owning > pointer). That's how all the other methods work, except the ones that copy > their data or take ref pointers. > > The caller should keep a non-weak pointers (Exposing weak pointers to the > outside world in general is bad, and we shouldn't do that - I've filed a bug > about HttpServerProperties doing that). Done. I see. Thanks for the explanation! I didn't realize that caller can keep a raw pointer.
LGTM, just nits https://codereview.chromium.org/1175733002/diff/240001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1175733002/diff/240001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:210: scoped_refptr<PrefRegistrySimple> registry = new PrefRegistrySimple; optional: Suggest using: scoped_refptr<PrefRegistrySimple> registry(...); syntax instead (Just because that's what has to be done with scoped_ptrs, and I like to delude myself into thinking they're more similar than they really are. https://codereview.chromium.org/1175733002/diff/240001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:217: base::ThreadTaskRunnerHandle::Get()); I'd suggest this: scoped_ptr<net::HttpServerPropertiesManager> http_properties_manager(new ...); http_server_properties_manager->InitializeOnNetworkThread(); http_server_properties_manager_ = http_server_properties_manager.get(); context_builder.SetHttpServerProperties(http_server_properties_manager.get()); Keeping something in a scoped_ptr is a bit more robust if error cases are added or something, and just ownership semantics a bit clearer . https://codereview.chromium.org/1175733002/diff/240001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:217: base::ThreadTaskRunnerHandle::Get()); Should either use GetNetworkTaskRunner() instead of base::ThreadTaskRunnerHandle::Get(), or modify the CronetDataReductionProxy constructor call to use base::ThreadTaskRunnerHandle::Get(). I don't have strong feelings on the subject - GetNetworkTaskRunner() emphasizes it uses the network thread, while ThreadTaskRunnerHandle::Get() emphasizes it uses the current thread. https://codereview.chromium.org/1175733002/diff/240001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java (right): https://codereview.chromium.org/1175733002/diff/240001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:108: // Make another request using a new context but with no quic hints. nit: QUIC
Patchset #9 (id:260001) has been deleted
Thank you very much for the reviews, Matt! https://codereview.chromium.org/1175733002/diff/240001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1175733002/diff/240001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:210: scoped_refptr<PrefRegistrySimple> registry = new PrefRegistrySimple; On 2015/07/09 19:08:10, mmenke wrote: > optional: Suggest using: > scoped_refptr<PrefRegistrySimple> registry(...); > > syntax instead (Just because that's what has to be done with scoped_ptrs, and I > like to delude myself into thinking they're more similar than they really are. Done. https://codereview.chromium.org/1175733002/diff/240001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:217: base::ThreadTaskRunnerHandle::Get()); On 2015/07/09 19:08:10, mmenke wrote: > Should either use GetNetworkTaskRunner() instead of > base::ThreadTaskRunnerHandle::Get(), or modify the CronetDataReductionProxy > constructor call to use base::ThreadTaskRunnerHandle::Get(). I don't have > strong feelings on the subject - GetNetworkTaskRunner() emphasizes it uses the > network thread, while ThreadTaskRunnerHandle::Get() emphasizes it uses the > current thread. Done. https://codereview.chromium.org/1175733002/diff/240001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:217: base::ThreadTaskRunnerHandle::Get()); On 2015/07/09 19:08:10, mmenke wrote: > I'd suggest this: > > scoped_ptr<net::HttpServerPropertiesManager> http_properties_manager(new ...); > http_server_properties_manager->InitializeOnNetworkThread(); > http_server_properties_manager_ = http_server_properties_manager.get(); > context_builder.SetHttpServerProperties(http_server_properties_manager.get()); > > Keeping something in a scoped_ptr is a bit more robust if error cases are added > or something, and just ownership semantics a bit clearer . Done. https://codereview.chromium.org/1175733002/diff/240001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java (right): https://codereview.chromium.org/1175733002/diff/240001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:108: // Make another request using a new context but with no quic hints. On 2015/07/09 19:08:10, mmenke wrote: > nit: QUIC Done.
Hey Helen, what's the status of this CL? Not rushing you, was just looking at this stuff in the context of refactoring context creation elsewhere.
On 2015/07/22 18:16:51, mmenke wrote: > Hey Helen, what's the status of this CL? Not rushing you, was just looking at > this stuff in the context of refactoring context creation elsewhere. I am waiting for Paul to get back to the review. Paul, ping! Could you take a look?
lgtm
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1175733002/#ps280001 (title: "Address Matt's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1175733002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1175733002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pauljensen@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1175733002/#ps300001 (title: "Address Findbug error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1175733002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1175733002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1175733002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1175733002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1175733002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1175733002/300001
Message was sent while issue was closed.
Committed patchset #10 (id:300001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/c56b3bc999401c0a598ebac15f4de3f670dbf7dc Cr-Commit-Position: refs/heads/master@{#340162} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
