Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1133883002:
[Cronet] Enable persistence mode for Sdch (Closed)
Description[Cronet] Enable persistence mode for Sdch
This CL adds persistence mode for Sdch metadata in Cronet
for the new async API. This CL also added
unit tests to make sure persistence work.
BUG=414885
Committed: https://crrev.com/99f199f2eedd9b6caed00cbd3526d238fcf94518
Cr-Commit-Position: refs/heads/master@{#332044}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address comments #
Total comments: 10
Patch Set 3 : Used SdchObserver #Patch Set 4 : Revert unneeded changes #Patch Set 5 : Missing include #
Total comments: 5
Patch Set 6 : Rebased #Patch Set 7 : Addressed Misha's comments #Patch Set 8 : Updated doc #Patch Set 9 : Rebased #
Total comments: 10
Patch Set 10 : Addressed comments #
Total comments: 16
Patch Set 11 : Addressed Misha's comments #
Total comments: 17
Patch Set 12 : Address Matt's comments #Patch Set 13 : Remove pref file support from legacy API #
Total comments: 4
Patch Set 14 : Address Misha's comments #
Total comments: 7
Patch Set 15 : Address comments #Patch Set 16 : Removed semicolon after override #
Total comments: 19
Patch Set 17 : Address comments #Patch Set 18 : Removed globals #
Total comments: 2
Patch Set 19 : Remove dictUrl check #
Total comments: 10
Patch Set 20 : Address Misha's comments #Messages
Total messages: 49 (7 generated)
xunjieli@chromium.org changed reviewers: + mef@chromium.org, mmenke@chromium.org
Hi Misha and Matt, this is still somewhat a draft CL to get some initial feedback. Matt: could you take a look at cronet_url_request_context_adapter.cc? Misha: could you take a look at the new Java test? Thanks a lot!
https://codereview.chromium.org/1133883002/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java (right): https://codereview.chromium.org/1133883002/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:152: // FIXME: confirm with Elly where there's a less flaky way. FYI. Elly said she might be able to put in a CL to augment SdchObserver, so we can watch for dictionary add events.
https://codereview.chromium.org/1133883002/diff/1/components/cronet/android/c... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1133883002/diff/1/components/cronet/android/c... components/cronet/android/cronet_url_request_context_adapter.cc:110: base::WriteFile(filepath, "{}", 2); Is this really needed? I don't see any documentation on the subject. Also, I don't see any other consumers doing this.
https://codereview.chromium.org/1133883002/diff/1/components/cronet/android/c... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1133883002/diff/1/components/cronet/android/c... components/cronet/android/cronet_url_request_context_adapter.cc:165: // Only set up pref file if http disk cache is enabled. I think we should set pref file if storage_path is specified, regardless of http cache settings. https://codereview.chromium.org/1133883002/diff/1/components/cronet/android/c... components/cronet/android/cronet_url_request_context_adapter.cc:178: network_json_store_->ReadPrefsAsync(nullptr); It seems that prefstore should just fallback to empty prefs on non-existent file: https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/json_pr... https://codereview.chromium.org/1133883002/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java (right): https://codereview.chromium.org/1133883002/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:259: in = new FileInputStream(prefFile); It seems that canonical way of reading file into string is to use new BufferedReader(new FileReader(prefFile)): https://code.google.com/p/chromium/codesearch#chromium/src/net/android/javate...
Thanks for the valuable feedback! I am going to work on the testing (Elly has uploaded a CL which adds new signal to the observer, and I'll see how I can make use of it). Will let you know once this CL is ready for review again. https://codereview.chromium.org/1133883002/diff/1/components/cronet/android/c... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1133883002/diff/1/components/cronet/android/c... components/cronet/android/cronet_url_request_context_adapter.cc:110: base::WriteFile(filepath, "{}", 2); On 2015/05/11 17:16:19, mmenke wrote: > Is this really needed? I don't see any documentation on the subject. Also, I > don't see any other consumers doing this. Done. Thanks! I saw an error from json pref store complaining about no file found, so I added this part. It turns out that error isn't critical, and a new file is created anyway. I will remove this part. https://codereview.chromium.org/1133883002/diff/1/components/cronet/android/c... components/cronet/android/cronet_url_request_context_adapter.cc:165: // Only set up pref file if http disk cache is enabled. On 2015/05/12 16:39:17, mef wrote: > I think we should set pref file if storage_path is specified, regardless of http > cache settings. Done. https://codereview.chromium.org/1133883002/diff/1/components/cronet/android/c... components/cronet/android/cronet_url_request_context_adapter.cc:178: network_json_store_->ReadPrefsAsync(nullptr); On 2015/05/12 16:39:17, mef wrote: > It seems that prefstore should just fallback to empty prefs on non-existent > file: > https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/json_pr... Done. Yes, you and Matt are both right about this part. Somehow I thought the error was critical when I saw the print statement in the logs. https://codereview.chromium.org/1133883002/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java (right): https://codereview.chromium.org/1133883002/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:259: in = new FileInputStream(prefFile); On 2015/05/12 16:39:17, mef wrote: > It seems that canonical way of reading file into string is to use new > BufferedReader(new FileReader(prefFile)): > https://code.google.com/p/chromium/codesearch#chromium/src/net/android/javate... Done. That's much cleaner! thanks.
https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:165: file_thread_->StartWithOptions( nit: Thread::Start() seems to do the same thing. https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:168: network_json_store_ = new JsonPrefStore( nit: should it be called |json_pref_store_|? https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.h:100: scoped_ptr<net::SdchOwner> sdch_owner_; Should we add a comment that sdch_owner_ must be destroyed before pref store? https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java (right): https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:142: assertTrue(prefFileContains("local_prefs.json", dictUrl)); Need to delete prefs file at some point before or after the test.
https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.h:100: scoped_ptr<net::SdchOwner> sdch_owner_; On 2015/05/13 15:15:22, mef wrote: > Should we add a comment that sdch_owner_ must be destroyed before pref store? PrefStore is refcounted (shudder), so that's actually not the case. We may need to destroy both of them before the file thread, though I'm not sure of that.
https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.h:100: scoped_ptr<net::SdchOwner> sdch_owner_; On 2015/05/13 15:34:41, mmenke wrote: > On 2015/05/13 15:15:22, mef wrote: > > Should we add a comment that sdch_owner_ must be destroyed before pref store? > > PrefStore is refcounted (shudder), so that's actually not the case. Umm, but as long as we hold the ref to PrefStore while destroying the sdch owner it should be the case, right? > > We may need to destroy both of them before the file thread, though I'm not sure > of that. The PrefStore takes task_runner, which is refcounted and can outlive the thread, but I guess if file thread is destroyed, then that could miss some latest writes to prefstore before it is destroyed.
On 2015/05/13 15:50:09, mef wrote: > https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... > File components/cronet/android/cronet_url_request_context_adapter.h (right): > > https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... > components/cronet/android/cronet_url_request_context_adapter.h:100: > scoped_ptr<net::SdchOwner> sdch_owner_; > On 2015/05/13 15:34:41, mmenke wrote: > > On 2015/05/13 15:15:22, mef wrote: > > > Should we add a comment that sdch_owner_ must be destroyed before pref > store? > > > > PrefStore is refcounted (shudder), so that's actually not the case. > Umm, but as long as we hold the ref to PrefStore while destroying the sdch owner > it should be the case, right? Ahh...I was thinking the SdchOwner owned a reference to the PrefStore, but it doesn't. Htm....Using a refcounted variable without owning a reference may be worse than using reference counting in the first place. > > > > We may need to destroy both of them before the file thread, though I'm not > sure > > of that. > The PrefStore takes task_runner, which is refcounted and can outlive the thread, > but I guess if file thread is destroyed, then that could miss some latest writes > to prefstore before it is destroyed. Yea, I was thinking of making sure it saved everything on destruction, as opposed to thread-safety.
I patched in Elly's CL(https://codereview.chromium.org/1133763003/) and added utility methods to register a SdchObserver for the unit tests. PTAL. https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:165: file_thread_->StartWithOptions( On 2015/05/13 15:15:22, mef wrote: > nit: Thread::Start() seems to do the same thing. Done. https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:168: network_json_store_ = new JsonPrefStore( On 2015/05/13 15:15:22, mef wrote: > nit: should it be called |json_pref_store_|? Done. https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.h:100: scoped_ptr<net::SdchOwner> sdch_owner_; On 2015/05/13 15:50:09, mef wrote: > On 2015/05/13 15:34:41, mmenke wrote: > > On 2015/05/13 15:15:22, mef wrote: > > > Should we add a comment that sdch_owner_ must be destroyed before pref > store? > > > > PrefStore is refcounted (shudder), so that's actually not the case. > Umm, but as long as we hold the ref to PrefStore while destroying the sdch owner > it should be the case, right? > > > > We may need to destroy both of them before the file thread, though I'm not > sure > > of that. > The PrefStore takes task_runner, which is refcounted and can outlive the thread, > but I guess if file thread is destroyed, then that could miss some latest writes > to prefstore before it is destroyed. > Done. Added comment about the order.
I missed one comment. https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java (right): https://codereview.chromium.org/1133883002/diff/20001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:142: assertTrue(prefFileContains("local_prefs.json", dictUrl)); On 2015/05/13 15:15:22, mef wrote: > Need to delete prefs file at some point before or after the test. Acknowledged. prepareTestStorage is called in Activity#onCreate to make sure we clear storage each time when we launch a new activity (which is the case for every unit test, I believe)
+ellyjones as I have a couple of questions about observers. https://codereview.chromium.org/1133883002/diff/80001/components/cronet/andro... File components/cronet/android/test/cronet_test_jni.cc (right): https://codereview.chromium.org/1133883002/diff/80001/components/cronet/andro... components/cronet/android/test/cronet_test_jni.cc:23: {"SdchTestUtil", cronet::RegisterSdchTestUtil}, nit: alphabetically S goes after Q. https://codereview.chromium.org/1133883002/diff/80001/components/cronet/andro... File components/cronet/android/test/sdch_test_util.cc (right): https://codereview.chromium.org/1133883002/diff/80001/components/cronet/andro... components/cronet/android/test/sdch_test_util.cc:64: g_sdch_observer = nullptr; do we intentionally leak |g_sdch_observer| here? https://codereview.chromium.org/1133883002/diff/80001/net/base/sdch_observer.h File net/base/sdch_observer.h (right): https://codereview.chromium.org/1133883002/diff/80001/net/base/sdch_observer.... net/base/sdch_observer.h:26: virtual void OnDictionaryAdded(SdchManager* manager, I'm just curious why manager is not const here and elsewhere? I observer allowed to modify it?
Thanks! I added support for the legacy API as well, and update legacy API test. PTAL. https://codereview.chromium.org/1133883002/diff/80001/components/cronet/andro... File components/cronet/android/test/cronet_test_jni.cc (right): https://codereview.chromium.org/1133883002/diff/80001/components/cronet/andro... components/cronet/android/test/cronet_test_jni.cc:23: {"SdchTestUtil", cronet::RegisterSdchTestUtil}, On 2015/05/13 17:57:13, mef wrote: > nit: alphabetically S goes after Q. Done. https://codereview.chromium.org/1133883002/diff/80001/components/cronet/andro... File components/cronet/android/test/sdch_test_util.cc (right): https://codereview.chromium.org/1133883002/diff/80001/components/cronet/andro... components/cronet/android/test/sdch_test_util.cc:64: g_sdch_observer = nullptr; On 2015/05/13 17:57:13, mef wrote: > do we intentionally leak |g_sdch_observer| here? yes, the reason I did this is because we are required to remove the observer that we registered, otherwise sdch_manager will complain during shutdown. So making the observer as a global seems the easy way to doing it, although it might not be the best way. I am open to suggestions.
On 2015/05/13 17:57:13, mef wrote: > +ellyjones as I have a couple of questions about observers. > > https://codereview.chromium.org/1133883002/diff/80001/components/cronet/andro... > File components/cronet/android/test/cronet_test_jni.cc (right): > > https://codereview.chromium.org/1133883002/diff/80001/components/cronet/andro... > components/cronet/android/test/cronet_test_jni.cc:23: {"SdchTestUtil", > cronet::RegisterSdchTestUtil}, > nit: alphabetically S goes after Q. > > https://codereview.chromium.org/1133883002/diff/80001/components/cronet/andro... > File components/cronet/android/test/sdch_test_util.cc (right): > > https://codereview.chromium.org/1133883002/diff/80001/components/cronet/andro... > components/cronet/android/test/sdch_test_util.cc:64: g_sdch_observer = nullptr; > do we intentionally leak |g_sdch_observer| here? > > https://codereview.chromium.org/1133883002/diff/80001/net/base/sdch_observer.h > File net/base/sdch_observer.h (right): > > https://codereview.chromium.org/1133883002/diff/80001/net/base/sdch_observer.... > net/base/sdch_observer.h:26: virtual void OnDictionaryAdded(SdchManager* > manager, > I'm just curious why manager is not const here and elsewhere? I observer allowed > to modify it? It is allowed to do so, but the existing code doesn't. This should probably be const so I'll change my CL upstream.
Elly's change is in. I have synced and rebased. PTAL.
looks pretty good. https://codereview.chromium.org/1133883002/diff/160001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1133883002/diff/160001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:279: DCHECK(!GetNetworkTaskRunner()->BelongsToCurrentThread()); Is there some way to flush json_pref_store_ here? My concern is that file thread will go away soon, so it needs to process write tasks (possibly added in json_pref_store_ destructor) while it is alive. https://codereview.chromium.org/1133883002/diff/160001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java (right): https://codereview.chromium.org/1133883002/diff/160001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:93: while (true) { I would make it a finite loop with 1s sleep. https://codereview.chromium.org/1133883002/diff/160001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:299: private boolean prefFileContains(String filename, String content) throws IOException { This doesn't seem to be pref-file specific, maybe rename to 'fileContainsString'? https://codereview.chromium.org/1133883002/diff/160001/components/cronet/andr... File components/cronet/android/test/sdch_test_util.cc (right): https://codereview.chromium.org/1133883002/diff/160001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:82: void AddRemoveSdchObserver(JNIEnv* env, Would it be cleaner to just have 4 separate methods?
Patchset #10 (id:180001) has been deleted
Thanks for the review! https://codereview.chromium.org/1133883002/diff/160001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1133883002/diff/160001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:279: DCHECK(!GetNetworkTaskRunner()->BelongsToCurrentThread()); On 2015/05/15 14:30:39, mef wrote: > Is there some way to flush json_pref_store_ here? > My concern is that file thread will go away soon, so it needs to process write > tasks (possibly added in json_pref_store_ destructor) while it is alive. Since Destroy is called on a Java thread, and JsonPrefStore::CommitPendingWrite() asserts that it is called on the network thread, I can't do it here. I can make a separate DestroyOnNetworkThread method, but I wonder whether this is worth the effort. The destructor of JsonPrefStore already calls CommitPendingWrite(), and the write task is posted before file thread gets destroyed, the file thread will wait for the task to be executed right ? https://codereview.chromium.org/1133883002/diff/160001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java (right): https://codereview.chromium.org/1133883002/diff/160001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:93: while (true) { On 2015/05/15 14:30:39, mef wrote: > I would make it a finite loop with 1s sleep. Hmm.. I was relying on the test timeout. How many runs of the loop do you recommend? On the sleep part, we should probably still use 10s (although that's pretty long), since that's the commit interval used (https://code.google.com/p/chromium/codesearch#chromium/src/base/files/importa...). https://codereview.chromium.org/1133883002/diff/160001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:299: private boolean prefFileContains(String filename, String content) throws IOException { On 2015/05/15 14:30:39, mef wrote: > This doesn't seem to be pref-file specific, maybe rename to > 'fileContainsString'? Done. https://codereview.chromium.org/1133883002/diff/160001/components/cronet/andr... File components/cronet/android/test/sdch_test_util.cc (right): https://codereview.chromium.org/1133883002/diff/160001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:82: void AddRemoveSdchObserver(JNIEnv* env, On 2015/05/15 14:30:39, mef wrote: > Would it be cleaner to just have 4 separate methods? I tried that before. But having 4 separate methods (legacy/async * add/remove ) seem to involve too much code duplication. We will need 4 JNI stubs and 4 other methods (for calling on network thread). That would double the number of methods here, so we will end up having duplicated code for adding/removing observer, unless we add 2 extra methods to share the code, so that will give us 10 methods in total. I think the file is slightly easier to read with 4 methods (compared to the alternate 10 methods), so I ended up doing this.
I believe that there are no threading or flushing issues, but I would appreciate Matt's comments. https://codereview.chromium.org/1133883002/diff/160001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1133883002/diff/160001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:279: DCHECK(!GetNetworkTaskRunner()->BelongsToCurrentThread()); On 2015/05/15 16:28:03, xunjieli wrote: > On 2015/05/15 14:30:39, mef wrote: > > Is there some way to flush json_pref_store_ here? > > My concern is that file thread will go away soon, so it needs to process write > > tasks (possibly added in json_pref_store_ destructor) while it is alive. > > Since Destroy is called on a Java thread, and > JsonPrefStore::CommitPendingWrite() asserts that it is called on the network > thread, I can't do it here. I can make a separate DestroyOnNetworkThread method, > but I wonder whether this is worth the effort. The destructor of JsonPrefStore > already calls CommitPendingWrite(), and the write task is posted before file > thread gets destroyed, the file thread will wait for the task to be executed > right ? I think you are right, thanks for explanation! https://codereview.chromium.org/1133883002/diff/160001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java (right): https://codereview.chromium.org/1133883002/diff/160001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:93: while (true) { On 2015/05/15 16:28:04, xunjieli wrote: > On 2015/05/15 14:30:39, mef wrote: > > I would make it a finite loop with 1s sleep. > > Hmm.. I was relying on the test timeout. How many runs of the loop do you > recommend? > On the sleep part, we should probably still use 10s (although that's pretty > long), since that's the commit interval used > (https://code.google.com/p/chromium/codesearch#chromium/src/base/files/importa...). I see, sg. https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java (right): https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:102: HttpUrlRequestFactory factory = HttpUrlRequestFactory.createFactory( This seems a bit dangerous. Is previous instance still around and using the same storage location? Can they collide? https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:109: newObserver.waitForDictionaryAdded("LeQxM80O"); Could there be a race, like dictionary being added before newObserver starts to wait? https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:305: builder.append(line); If we only need to look for content that doesn't span multiple lines, then we can just use 'line.contains(content)' here and return early if it is found. https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java (right): https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java:16: * Test untilities lated to Sdch. nit: typos. https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java:26: public static interface SdchObserver { nit: should it be interface OnDictionaryAddedListener? https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java:36: public static void addSdchObserver( addDictionaryAddedListener? https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java:45: public static void removeSdchObserver( removeDictionaryAddedListener? https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java:62: private static void onAddRemoveSdchObserver() { maybe call it 'onAddRemoveSdchObserverCompleted'?
Thanks for the feedback! PTAL. https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java (right): https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:102: HttpUrlRequestFactory factory = HttpUrlRequestFactory.createFactory( On 2015/05/19 21:39:44, mef wrote: > This seems a bit dangerous. Is previous instance still around and using the same > storage location? Can they collide? Yes they can, if the old instance is still doing stuff with the pref file. But I don't know a way to get rid of the old instance (there isn't a shutdown method). https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:109: newObserver.waitForDictionaryAdded("LeQxM80O"); On 2015/05/19 21:39:43, mef wrote: > Could there be a race, like dictionary being added before newObserver starts to > wait? If a ConditionVariable's open() is called before block(), the block() won't block, so I think we should be fine. https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:305: builder.append(line); On 2015/05/19 21:39:43, mef wrote: > If we only need to look for content that doesn't span multiple lines, then we > can just use 'line.contains(content)' here and return early if it is found. Done. You are right! that's more efficient. https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java (right): https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java:16: * Test untilities lated to Sdch. On 2015/05/19 21:39:44, mef wrote: > nit: typos. Done. So many typos :( not sure whether my mind was at when I typed this.. https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java:26: public static interface SdchObserver { On 2015/05/19 21:39:44, mef wrote: > nit: should it be interface OnDictionaryAddedListener? You are right. If it's an interface, it should be a single method listener. I have changed to abstract class, so we can potentially listen to other signals as well. https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java:36: public static void addSdchObserver( On 2015/05/19 21:39:44, mef wrote: > addDictionaryAddedListener? Done. https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java:45: public static void removeSdchObserver( On 2015/05/19 21:39:44, mef wrote: > removeDictionaryAddedListener? Done. https://codereview.chromium.org/1133883002/diff/200001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java:62: private static void onAddRemoveSdchObserver() { On 2015/05/19 21:39:44, mef wrote: > maybe call it 'onAddRemoveSdchObserverCompleted'? Done.
https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:97: scoped_ptr<base::Thread> file_thread_; Should probably mention this should be destroyed last. https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java (right): https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:46: commandLineArgs.add("enable"); ? https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:107: Callback newCallback = addCallback(newRequestAdapter, API.LEGACY); This is racy - the dictionary could be loaded before the callback is injected. One way to fix it would be to check if the SDCH manager has the dictionary we're looking for before adding an observer via GetDictionaryText, and if so, stop the wait immediately. Same goes for the other test. https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... File components/cronet/android/test/sdch_test_util.cc (right): https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:34: .Release()); Maybe just remove itself as an observer and delete itself here, and then get rid of the remove half of AddRemoveSdchObserver? Could optionally take a dictionary_url to wait for as input, if you're concerned about other dictionaries. You can then also get rid of g_sdch_observer, as long as we never have a test where we expect a dictionary not to be added in some interval. https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:37: void OnDictionaryRemoved(const std::string& server_hash) override{}; should include <string> https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:44: void OnClearDictionaries() override{}; Space after override, on all of these. https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:56: url_request_context->sdch_manager()->AddObserver(g_sdch_observer); delete g_sdch_observer? g_sdch_observer = nullptr; https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:59: url_request_context->sdch_manager()->RemoveObserver(g_sdch_observer); delete g_sdch_observer? https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:82: void AddRemoveSdchObserver(JNIEnv* env, +1 to splitting this up into multiple methods. https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... File components/cronet/android/url_request_context_adapter.cc (right): https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... components/cronet/android/url_request_context_adapter.cc:155: if (!config_->storage_path.empty()) { Wonder if it's really worth hooking this up to the legacy API.
Thanks for the review! PTAL. https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:97: scoped_ptr<base::Thread> file_thread_; On 2015/05/20 18:33:41, mmenke wrote: > Should probably mention this should be destroyed last. Done. https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java (right): https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:46: commandLineArgs.add("enable"); On 2015/05/20 18:33:41, mmenke wrote: > ? Done. Refactored to make the value into a static variable so it's clearer. https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:107: Callback newCallback = addCallback(newRequestAdapter, API.LEGACY); On 2015/05/20 18:33:41, mmenke wrote: > This is racy - the dictionary could be loaded before the callback is injected. > > One way to fix it would be to check if the SDCH manager has the dictionary we're > looking for before adding an observer via GetDictionaryText, and if so, stop the > wait immediately. > > Same goes for the other test. Done. You are right! https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... File components/cronet/android/test/sdch_test_util.cc (right): https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:34: .Release()); On 2015/05/20 18:33:42, mmenke wrote: > Maybe just remove itself as an observer and delete itself here, and then get rid > of the remove half of AddRemoveSdchObserver? Could optionally take a > dictionary_url to wait for as input, if you're concerned about other > dictionaries. > > You can then also get rid of g_sdch_observer, as long as we never have a test > where we expect a dictionary not to be added in some interval. Done. That's neat! thanks https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:37: void OnDictionaryRemoved(const std::string& server_hash) override{}; On 2015/05/20 18:33:41, mmenke wrote: > should include <string> Done. https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:44: void OnClearDictionaries() override{}; On 2015/05/20 18:33:41, mmenke wrote: > Space after override, on all of these. Done. https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... File components/cronet/android/url_request_context_adapter.cc (right): https://codereview.chromium.org/1133883002/diff/220001/components/cronet/andr... components/cronet/android/url_request_context_adapter.cc:155: if (!config_->storage_path.empty()) { On 2015/05/20 18:33:42, mmenke wrote: > Wonder if it's really worth hooking this up to the legacy API. Send out an email. If there's no strong interest, I will drop the persistence support for the legacy API.
alexraikman@google.com changed reviewers: + alexraikman@google.com
So it looks like embedders are okay with not adding pref file support for the legacy API :) , so I have reverted the changes. PTAL.
https://codereview.chromium.org/1133883002/diff/260001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java (right): https://codereview.chromium.org/1133883002/diff/260001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:73: Callback callback = new Callback(); Optional: I think 'Callback' is too generic, so its purpose is not clear. Maybe 'DictionaryAddedCallback'? https://codereview.chromium.org/1133883002/diff/260001/components/cronet/andr... File components/cronet/android/test/sdch_test_util.cc (right): https://codereview.chromium.org/1133883002/diff/260001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:90: if (jlegacy_api == JNI_TRUE) { I'd still argue that there is too little in common to have this in one function, and suggest splitting into 'AddSdchObserver' and 'AddSdchObserverLegacyAPI'.
Thanks for the review! PTAL https://codereview.chromium.org/1133883002/diff/260001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java (right): https://codereview.chromium.org/1133883002/diff/260001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:73: Callback callback = new Callback(); On 2015/05/21 17:29:44, mef wrote: > Optional: I think 'Callback' is too generic, so its purpose is not clear. Maybe > 'DictionaryAddedCallback'? Done. https://codereview.chromium.org/1133883002/diff/260001/components/cronet/andr... File components/cronet/android/test/sdch_test_util.cc (right): https://codereview.chromium.org/1133883002/diff/260001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:90: if (jlegacy_api == JNI_TRUE) { On 2015/05/21 17:29:44, mef wrote: > I'd still argue that there is too little in common to have this in one function, > and suggest splitting into 'AddSdchObserver' and 'AddSdchObserverLegacyAPI'. Done. Agreed.
https://codereview.chromium.org/1133883002/diff/280001/components/cronet/andr... File components/cronet/android/test/sdch_test_util.cc (right): https://codereview.chromium.org/1133883002/diff/280001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:35: .Release()); I think this is a leak, and you should actually be using obj(). Release doesn't release a reference, so it's what you should use when returning things back to Java, but when passing them in to Java, Java adds a reference to them itself. At least other JNI calls into Java seem to use obj(). https://codereview.chromium.org/1133883002/diff/280001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:40: void OnDictionaryRemoved(const std::string& server_hash) override{}; nit: Add space: "override {}" (x4) https://codereview.chromium.org/1133883002/diff/280001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:60: Java_SdchTestUtil_onAddSdchObserverCompleted(env, JNI_FALSE); Calling this a failure seems odd. Maybe Java_SdchTestUtil_dictionaryAlreadyPresent? Also seems like the tets would be clearer if we added a method or an argument to SdchObserverCallback indicating what path we followed instead (Already present vs added). Think it would result in clearer logic for tests.
https://codereview.chromium.org/1133883002/diff/280001/components/cronet/andr... File components/cronet/android/test/sdch_test_util.cc (right): https://codereview.chromium.org/1133883002/diff/280001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:35: .Release()); On 2015/05/22 15:46:41, mmenke wrote: > I think this is a leak, and you should actually be using obj(). > > Release doesn't release a reference, so it's what you should use when returning > things back to Java, but when passing them in to Java, Java adds a reference to > them itself. At least other JNI calls into Java seem to use obj(). Done. Ah, yes, you are right! https://codereview.chromium.org/1133883002/diff/280001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:40: void OnDictionaryRemoved(const std::string& server_hash) override{}; On 2015/05/22 15:46:41, mmenke wrote: > nit: Add space: "override {}" (x4) Hmm.. "git cl format" will revert the blank space. I don't see this mentioned in the style guide. Should I file a bug against clang format? https://codereview.chromium.org/1133883002/diff/280001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:60: Java_SdchTestUtil_onAddSdchObserverCompleted(env, JNI_FALSE); On 2015/05/22 15:46:41, mmenke wrote: > Calling this a failure seems odd. Maybe > Java_SdchTestUtil_dictionaryAlreadyPresent? > > Also seems like the tets would be clearer if we added a method or an argument to > SdchObserverCallback indicating what path we followed instead (Already present > vs added). Think it would result in clearer logic for tests. Done.
https://codereview.chromium.org/1133883002/diff/280001/components/cronet/andr... File components/cronet/android/test/sdch_test_util.cc (right): https://codereview.chromium.org/1133883002/diff/280001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:40: void OnDictionaryRemoved(const std::string& server_hash) override{}; On 2015/05/22 16:08:31, xunjieli wrote: > On 2015/05/22 15:46:41, mmenke wrote: > > nit: Add space: "override {}" (x4) > > Hmm.. "git cl format" will revert the blank space. I don't see this mentioned in > the style guide. Should I file a bug against clang format? Remove the unnecessary semi-colons after the close brace. Suspect that's confusing clang format.
On 2015/05/26 16:36:21, mmenke wrote: > https://codereview.chromium.org/1133883002/diff/280001/components/cronet/andr... > File components/cronet/android/test/sdch_test_util.cc (right): > > https://codereview.chromium.org/1133883002/diff/280001/components/cronet/andr... > components/cronet/android/test/sdch_test_util.cc:40: void > OnDictionaryRemoved(const std::string& server_hash) override{}; > On 2015/05/22 16:08:31, xunjieli wrote: > > On 2015/05/22 15:46:41, mmenke wrote: > > > nit: Add space: "override {}" (x4) > > > > Hmm.. "git cl format" will revert the blank space. I don't see this mentioned > in > > the style guide. Should I file a bug against clang format? > > Remove the unnecessary semi-colons after the close brace. Suspect that's > confusing clang format. It is indeed. Done. Thanks!
https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:191: if (!config->storage_path.empty()) { Suggest checking if sdch is enabled...Could get ugly if we start using it for my things, but we can burn that bridge when we come to it. https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:194: if (!file_thread_) { Suggest a method GetFileThread, which creates it if needed, so we don't end up with multiple copies of this logic. https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java (right): https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:30: private enum SDCH { Looks like enums normally are named like classes, so this should be Sdch and the next should be Api. https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:222: private String mLastAddedDict; This name is a little misleading. We're actually tracking only the first dictionary we see. https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:234: assertTrue(mLastAddedDict.contains(dictName)); CAn we do an exact match against the full dictionary URL? https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java (right): https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java:20: private static final ConditionVariable sAddBlock = new ConditionVariable(); Rather than use globals, could we just instantiate an object instead, and have a single object correspond to a single wait for a dictionary? https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java:34: private static boolean sRegisterSdchObserverSucceeded = false; We're generally putting variables before methods in Java files. https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java:42: SdchObserverCallback callback, boolean isLegacyAPI) { I thought we weren't adding legacy API support for this?
https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:191: if (!config->storage_path.empty()) { On 2015/05/26 19:34:40, mmenke wrote: > Suggest checking if sdch is enabled...Could get ugly if we start using it for my > things, but we can burn that bridge when we come to it. Done. I know HttpServerProperties will use the same file. Matt, are you working on another feature that also uses persistent pref file? https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:194: if (!file_thread_) { On 2015/05/26 19:34:40, mmenke wrote: > Suggest a method GetFileThread, which creates it if needed, so we don't end up > with multiple copies of this logic. Done. https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java (right): https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:30: private enum SDCH { On 2015/05/26 19:34:40, mmenke wrote: > Looks like enums normally are named like classes, so this should be Sdch and the > next should be Api. Done. https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:222: private String mLastAddedDict; On 2015/05/26 19:34:40, mmenke wrote: > This name is a little misleading. We're actually tracking only the first > dictionary we see. Done. https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:234: assertTrue(mLastAddedDict.contains(dictName)); On 2015/05/26 19:34:40, mmenke wrote: > CAn we do an exact match against the full dictionary URL? Done. https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java (right): https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java:20: private static final ConditionVariable sAddBlock = new ConditionVariable(); On 2015/05/26 19:34:40, mmenke wrote: > Rather than use globals, could we just instantiate an object instead, and have a > single object correspond to a single wait for a dictionary? Done. I have moved this condition variable to be a member variable of SdchObserverCallback. Just to make sure I understand, do you mean we change sCallback to be a non-global as well? That will involve a bit more work on the C++ to pass java pointers to network thread. Our current tests don't need multiple callback objects at the same time, but I will try if you think we should do that. https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java:34: private static boolean sRegisterSdchObserverSucceeded = false; On 2015/05/26 19:34:40, mmenke wrote: > We're generally putting variables before methods in Java files. Done. https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java:42: SdchObserverCallback callback, boolean isLegacyAPI) { On 2015/05/26 19:34:40, mmenke wrote: > I thought we weren't adding legacy API support for this? We aren't adding legacy support for sdch meta data persistence. However, we have already added sdch support (without the persistence bits) for the legacy API, so we need a more reliable way to test it as well. (Before this CL, I put an arbitrary wait in the test in order to make sure the dictionary is fetched).
https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:191: if (!config->storage_path.empty()) { On 2015/05/26 20:28:28, xunjieli wrote: > On 2015/05/26 19:34:40, mmenke wrote: > > Suggest checking if sdch is enabled...Could get ugly if we start using it for > my > > things, but we can burn that bridge when we come to it. > > Done. I know HttpServerProperties will use the same file. Matt, are you working > on another feature that also uses persistent pref file? That was a type...my == many (For some reason, a disproportionate number of my typos end up as valid English words...Just the wrong English words). https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java (right): https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/SdchTestUtil.java:20: private static final ConditionVariable sAddBlock = new ConditionVariable(); On 2015/05/26 20:28:28, xunjieli wrote: > On 2015/05/26 19:34:40, mmenke wrote: > > Rather than use globals, could we just instantiate an object instead, and have > a > > single object correspond to a single wait for a dictionary? > > Done. I have moved this condition variable to be a member variable of > SdchObserverCallback. > Just to make sure I understand, do you mean we change sCallback to be a > non-global as well? That will involve a bit more work on the C++ to pass java > pointers to network thread. Our current tests don't need multiple callback > objects at the same time, but I will try if you think we should do that. That's right. It's not the ability to have multiple callbacks I'm worried about - globals in general are ugly, and are best avoided unless really needed. They tend to lead to unanticipated problems, so it's best to just not use them in the first place, if you reasonably can do so. There are actually two approaches to get rid of the static variables, I'm fine with either one: 1) Keep this class static, and pass the callback through C++, and then make it a parameter of onDictionaryAdded. 2) Or rename this to OneshotSdchDictionaryObserver, and make the callback a member variable (Actually...Can just get rid of it and integrate the logic into OneshotSdchDictionaryObserver itself). Then pass the class itself through C++, and invoke onDictionaryAdded on it directly from C++. I think 2) is a little cleaner, and not much more complicated, but it's up to you.
https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1133883002/diff/320001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:191: if (!config->storage_path.empty()) { On 2015/05/27 14:40:43, mmenke wrote: > On 2015/05/26 20:28:28, xunjieli wrote: > > On 2015/05/26 19:34:40, mmenke wrote: > > > Suggest checking if sdch is enabled...Could get ugly if we start using it > for > > my > > > things, but we can burn that bridge when we come to it. > > > > Done. I know HttpServerProperties will use the same file. Matt, are you > working > > on another feature that also uses persistent pref file? > > That was a type...my == many (For some reason, a disproportionate number of my > typos end up as valid English words...Just the wrong English words). type == typo (Case in point...)
Patchset #18 (id:360001) has been deleted
Patchset #18 (id:380001) has been deleted
> That's right. It's not the ability to have multiple callbacks I'm worried about > - globals in general are ugly, and are best avoided unless really needed. They > tend to lead to unanticipated problems, so it's best to just not use them in the > first place, if you reasonably can do so. > > There are actually two approaches to get rid of the static variables, I'm fine > with either one: > > 1) Keep this class static, and pass the callback through C++, and then make it > a parameter of onDictionaryAdded. > > 2) Or rename this to OneshotSdchDictionaryObserver, and make the callback a > member variable (Actually...Can just get rid of it and integrate the logic into > OneshotSdchDictionaryObserver itself). Then pass the class itself through C++, > and invoke onDictionaryAdded on it directly from C++. > > I think 2) is a little cleaner, and not much more complicated, but it's up to > you. Thanks for the suggestions! I adopted #2. Looks cleaner for sure on the Java side. I am still unclear about passing java pointers on the C++ side. I tried following examples in the code base, but not sure if I got everything right. PTAL. Thanks!
LGTM - passing around the Java object through C++ looks correct to me. https://codereview.chromium.org/1133883002/diff/400001/components/cronet/andr... File components/cronet/android/test/sdch_test_util.cc (right): https://codereview.chromium.org/1133883002/diff/400001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:69: if (url_request_context->sdch_manager()->GetDictionarySet(target_url)) { Seems weird to only check target URL here, and not in OnDictionaryAdded. Suggest checking it there, too, and then removing the check from the subclass, for a more consistent interface.
Thanks for the review! https://codereview.chromium.org/1133883002/diff/400001/components/cronet/andr... File components/cronet/android/test/sdch_test_util.cc (right): https://codereview.chromium.org/1133883002/diff/400001/components/cronet/andr... components/cronet/android/test/sdch_test_util.cc:69: if (url_request_context->sdch_manager()->GetDictionarySet(target_url)) { On 2015/05/27 20:56:05, mmenke wrote: > Seems weird to only check target URL here, and not in OnDictionaryAdded. > Suggest checking it there, too, and then removing the check from the subclass, > for a more consistent interface. You are right. That will make the interface more consistent. Done. Thanks!
https://codereview.chromium.org/1133883002/diff/370014/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/1133883002/diff/370014/components/cronet.gypi... components/cronet.gypi:284: 'cronet/android/test/src/org/chromium/net/SdchObserver.java', nit: this seems out of order. https://codereview.chromium.org/1133883002/diff/370014/components/cronet.gypi... components/cronet.gypi:303: 'cronet/android/test/sdch_test_util.cc', nit: out of order. https://codereview.chromium.org/1133883002/diff/370014/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1133883002/diff/370014/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:370: base::Thread* CronetURLRequestContextAdapter::GetFileThread() { add DCHECK(GetNetworkTaskRunner()->BelongsToCurrentThread()); https://codereview.chromium.org/1133883002/diff/370014/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java (left): https://codereview.chromium.org/1133883002/diff/370014/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:109: @VisibleForTesting What does @VisibleForTesting annotation do? Should we add it to getUrlRequestContextAdapter()? https://codereview.chromium.org/1133883002/diff/370014/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java (right): https://codereview.chromium.org/1133883002/diff/370014/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:26: private static final String TAG = Log.makeTag("SdchTest"); unused?
https://codereview.chromium.org/1133883002/diff/370014/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/1133883002/diff/370014/components/cronet.gypi... components/cronet.gypi:284: 'cronet/android/test/src/org/chromium/net/SdchObserver.java', On 2015/05/28 15:18:54, mef wrote: > nit: this seems out of order. Done. https://codereview.chromium.org/1133883002/diff/370014/components/cronet.gypi... components/cronet.gypi:303: 'cronet/android/test/sdch_test_util.cc', On 2015/05/28 15:18:54, mef wrote: > nit: out of order. Done. https://codereview.chromium.org/1133883002/diff/370014/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1133883002/diff/370014/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:370: base::Thread* CronetURLRequestContextAdapter::GetFileThread() { On 2015/05/28 15:18:54, mef wrote: > add DCHECK(GetNetworkTaskRunner()->BelongsToCurrentThread()); Done. https://codereview.chromium.org/1133883002/diff/370014/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java (right): https://codereview.chromium.org/1133883002/diff/370014/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/SdchTest.java:26: private static final String TAG = Log.makeTag("SdchTest"); On 2015/05/28 15:18:55, mef wrote: > unused? Done.
Missed one comment. https://codereview.chromium.org/1133883002/diff/370014/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java (left): https://codereview.chromium.org/1133883002/diff/370014/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:109: @VisibleForTesting On 2015/05/28 15:18:55, mef wrote: > What does @VisibleForTesting annotation do? I am not exactly sure. but I used it to mark that this method's visibility is relaxed because of testing purpose. > Should we add it to getUrlRequestContextAdapter()? getUrlRequestContextAdapter() is used in ChromiumUrlRequest, and its visibility (protected keyword) isn't set due to testing, so I don't think we should add @VisibleForTesting.
LGTM, thanks!
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/1133883002/#ps430001 (title: "Address Misha's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1133883002/430001
Message was sent while issue was closed.
Committed patchset #20 (id:430001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/99f199f2eedd9b6caed00cbd3526d238fcf94518 Cr-Commit-Position: refs/heads/master@{#332044} |
