Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
273873003:
Adds Asynchronous APIs to CookieManager (Closed)
|
Created:
6 years, 7 months ago by hjd Modified:
6 years, 7 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdds Asynchronous APIs to CookieManager
Creates asynchronous versions of SetCookie, removeAllCookie and
removeSessionCookie which take callbacks.
BUG=
TEST=AndroidWebviewTest
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271411
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 24
Patch Set 3 : Refactor cookie_manager.cc #
Total comments: 23
Patch Set 4 : Fixed up some details. #
Total comments: 6
Patch Set 5 : Fixed bad comment and indent. #
Messages
Total messages: 11 (0 generated)
ptal, no rush
Haven't reviewed in detail, but the interface looks reasonable and it seems to be hooked up in the expected way. https://codereview.chromium.org/273873003/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwCookieManager.java (right): https://codereview.chromium.org/273873003/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwCookieManager.java:106: * @param callback A callback called after the cookies are removed with the number removed. Not sure the number removed is actually useful? The old sync interface doesn't return it, and I can't think of a real use for it..
https://codereview.chromium.org/273873003/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwCookieManager.java (right): https://codereview.chromium.org/273873003/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwCookieManager.java:106: * @param callback A callback called after the cookies are removed with the number removed. On 2014/05/12 14:27:35, Torne wrote: > Not sure the number removed is actually useful? The old sync interface doesn't > return it, and I can't think of a real use for it.. I guess you could check whether the value was 0. If it was 0 then the operation didn't do anything and you don't have to update your UI or whatever. I agree that knowing whether it was 3 or 7 cookies is probably not very helpful and it may be slightly simpler for us to maintain an API with a bool instead of an int. https://codereview.chromium.org/273873003/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwCookieManager.java:169: public static void invokeRemoveCookieCallback(CookieCallback<Integer> callback, I'd call these by type (invokeIntegerCookieCallback, invokeBooleanCookieCallback) since that's the thing you care about. https://codereview.chromium.org/273873003/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwCookieManager.java:180: * CookieCallback is a version of ValueCallback that native can call. native can call ValueCallback too using the same mechanism (static method to proxy the call). The real reason for this class is the second bit. https://codereview.chromium.org/273873003/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwCookieManager.java:182: * thread that that called our API after the work is done. XXX XXX? https://codereview.chromium.org/273873003/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwCookieManager.java:193: public static<T> CookieCallback<T> create(ValueCallback<T> callback) throws hmmm.. maybe call this map or convert. I'd expect create to always return a non-null value. https://codereview.chromium.org/273873003/diff/20001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java (right): https://codereview.chromium.org/273873003/diff/20001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:201: final ValueCallback<Boolean> callback = new ValueCallback<Boolean>() { you have the same pattern (semaphore, atomicXXX, value callback) repeated in a bunch of tests. regardless of whether you use callbackhelper or not I think it would make sense to move this into a private static helper class, especially if we change the API to have bool callbacks instead of int. https://codereview.chromium.org/273873003/diff/20001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:215: s.acquire(); nit: could use CallbackHelper for this. https://codereview.chromium.org/273873003/diff/20001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:358: Thread.sleep(3500); // wait for cookie to expire ouch! could we set one that had already expired and not wait? https://codereview.chromium.org/273873003/diff/20001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/util/CookieUtils.java (right): https://codereview.chromium.org/273873003/diff/20001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/util/CookieUtils.java:32: final Semaphore s = new Semaphore(0); nit: could use CallbackHelper for this. https://codereview.chromium.org/273873003/diff/20001/android_webview/native/c... File android_webview/native/cookie_manager.cc (right): https://codereview.chromium.org/273873003/diff/20001/android_webview/native/c... android_webview/native/cookie_manager.cc:58: class RemoveCookiesCallback { same here. CookieCallbackInteger, CookieCallbackBoolean https://codereview.chromium.org/273873003/diff/20001/android_webview/native/c... android_webview/native/cookie_manager.cc:164: typedef base::Callback<void()> CookieTask; FYI: you're redefining base::Closure https://codereview.chromium.org/273873003/diff/20001/android_webview/native/c... android_webview/native/cookie_manager.cc:466: void CookieManager::RemoveCookiesCompleted( like we talked offline - let's try and collapse the sync and async paths. I think it should be possible to get around some of the repetition.
ptal https://codereview.chromium.org/273873003/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwCookieManager.java (right): https://codereview.chromium.org/273873003/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwCookieManager.java:106: * @param callback A callback called after the cookies are removed with the number removed. On 2014/05/13 09:37:17, mkosiba wrote: > On 2014/05/12 14:27:35, Torne wrote: > > Not sure the number removed is actually useful? The old sync interface doesn't > > return it, and I can't think of a real use for it.. > > I guess you could check whether the value was 0. If it was 0 then the operation > didn't do anything and you don't have to update your UI or whatever. I agree > that knowing whether it was 3 or 7 cookies is probably not very helpful and it > may be slightly simpler for us to maintain an API with a bool instead of an int. This makes sense, I've moved the API to returning a bool. https://codereview.chromium.org/273873003/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwCookieManager.java:169: public static void invokeRemoveCookieCallback(CookieCallback<Integer> callback, On 2014/05/13 09:37:17, mkosiba wrote: > I'd call these by type (invokeIntegerCookieCallback, > invokeBooleanCookieCallback) since that's the thing you care about. Done. https://codereview.chromium.org/273873003/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwCookieManager.java:180: * CookieCallback is a version of ValueCallback that native can call. On 2014/05/13 09:37:17, mkosiba wrote: > native can call ValueCallback too using the same mechanism (static method to > proxy the call). The real reason for this class is the second bit. Done. https://codereview.chromium.org/273873003/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwCookieManager.java:182: * thread that that called our API after the work is done. XXX On 2014/05/13 09:37:17, mkosiba wrote: > XXX? Done. https://codereview.chromium.org/273873003/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwCookieManager.java:193: public static<T> CookieCallback<T> create(ValueCallback<T> callback) throws On 2014/05/13 09:37:17, mkosiba wrote: > hmmm.. maybe call this map or convert. I'd expect create to always return a > non-null value. I went with convert. https://codereview.chromium.org/273873003/diff/20001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java (right): https://codereview.chromium.org/273873003/diff/20001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:201: final ValueCallback<Boolean> callback = new ValueCallback<Boolean>() { On 2014/05/13 09:37:17, mkosiba wrote: > you have the same pattern (semaphore, atomicXXX, value callback) repeated in a > bunch of tests. regardless of whether you use callbackhelper or not I think it > would make sense to move this into a private static helper class, especially if > we change the API to have bool callbacks instead of int. Done. https://codereview.chromium.org/273873003/diff/20001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:215: s.acquire(); On 2014/05/13 09:37:17, mkosiba wrote: > nit: could use CallbackHelper for this. I feel like adding a comment to CallbackHelper, "Those who forget CallbackHelper are doomed to reimplement it poorly..." Done. https://codereview.chromium.org/273873003/diff/20001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:358: Thread.sleep(3500); // wait for cookie to expire On 2014/05/13 09:37:17, mkosiba wrote: > ouch! could we set one that had already expired and not wait? That wouldn't test the same thing since expired cookies never make it past setCookie but then since removeExpiredCookie really doesn't do anything (even for js) the current test doesn't really do anything either... I've left the test in without the wait just so we know removeExpiredCookie is still wired up. https://codereview.chromium.org/273873003/diff/20001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/util/CookieUtils.java (right): https://codereview.chromium.org/273873003/diff/20001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/util/CookieUtils.java:32: final Semaphore s = new Semaphore(0); On 2014/05/13 09:37:17, mkosiba wrote: > nit: could use CallbackHelper for this. Done. https://codereview.chromium.org/273873003/diff/20001/android_webview/native/c... File android_webview/native/cookie_manager.cc (right): https://codereview.chromium.org/273873003/diff/20001/android_webview/native/c... android_webview/native/cookie_manager.cc:58: class RemoveCookiesCallback { On 2014/05/13 09:37:17, mkosiba wrote: > same here. CookieCallbackInteger, CookieCallbackBoolean Done. https://codereview.chromium.org/273873003/diff/20001/android_webview/native/c... android_webview/native/cookie_manager.cc:466: void CookieManager::RemoveCookiesCompleted( On 2014/05/13 09:37:17, mkosiba wrote: > like we talked offline - let's try and collapse the sync and async paths. I > think it should be possible to get around some of the repetition. Done!
looking good. just a couple of nits. https://codereview.chromium.org/273873003/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwCookieManager.java (right): https://codereview.chromium.org/273873003/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwCookieManager.java:70: public void removeAllCookie() { do we want to fix the spelling here too or is this intentional? https://codereview.chromium.org/273873003/diff/40001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java (right): https://codereview.chromium.org/273873003/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:268: assertTrue(callback.getValue()); uber-nit: I it would read better if this were immediately after line 264. https://codereview.chromium.org/273873003/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:291: // We do not require this to be called on a thread with a Looper. I'm not sure this comment is helpful. The thread you're on has a Looper, and you're not verifying that this is the case. You can add a test case which news up a thread, calls the method and checks that the thread exited cleanly and move the comment there or just remove it entirely. https://codereview.chromium.org/273873003/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:306: public void testCookieExpiration() throws Exception { sweet! thanks for separating this into 2 test cases. https://codereview.chromium.org/273873003/diff/40001/android_webview/native/c... File android_webview/native/cookie_manager.cc (right): https://codereview.chromium.org/273873003/diff/40001/android_webview/native/c... android_webview/native/cookie_manager.cc:92: static base::Closure SignalClosure(WaitableEvent* completion) { nit: SignalEventClosure? https://codereview.chromium.org/273873003/diff/40001/android_webview/native/c... android_webview/native/cookie_manager.cc:100: static BoolCallback IgnoreBoolCallback(const base::Closure& f) { nit: IgnoreBoolCalbackAdapter ? or BoolCallbackAdapter ? https://codereview.chromium.org/273873003/diff/40001/android_webview/native/c... android_webview/native/cookie_manager.cc:225: intentional? https://codereview.chromium.org/273873003/diff/40001/android_webview/native/c... android_webview/native/cookie_manager.cc:288: // called at some point. You can call the bool or int versions of to be called on the WaitableEvent at some point. https://codereview.chromium.org/273873003/diff/40001/android_webview/native/c... android_webview/native/cookie_manager.cc:298: ExecCookieTask( can this just call the closure version? https://codereview.chromium.org/273873003/diff/40001/android_webview/native/c... android_webview/native/cookie_manager.cc:380: BoolCallback mycallback = bool_callback? adapted_callback? https://codereview.chromium.org/273873003/diff/40001/android_webview/native/c... android_webview/native/cookie_manager.cc:442: BoolCallback mycallback = same thing https://codereview.chromium.org/273873003/diff/40001/android_webview/native/c... android_webview/native/cookie_manager.cc:470: BoolCallback mycallback = here too
ptal https://codereview.chromium.org/273873003/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwCookieManager.java (right): https://codereview.chromium.org/273873003/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwCookieManager.java:70: public void removeAllCookie() { On 2014/05/15 12:40:35, mkosiba wrote: > do we want to fix the spelling here too or is this intentional? Done. https://codereview.chromium.org/273873003/diff/40001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java (right): https://codereview.chromium.org/273873003/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:268: assertTrue(callback.getValue()); On 2014/05/15 12:40:35, mkosiba wrote: > uber-nit: I it would read better if this were immediately after line 264. Done. https://codereview.chromium.org/273873003/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:291: // We do not require this to be called on a thread with a Looper. On 2014/05/15 12:40:35, mkosiba wrote: > I'm not sure this comment is helpful. The thread you're on has a Looper, and > you're not verifying that this is the case. > You can add a test case which news up a thread, calls the method and checks that > the thread exited cleanly and move the comment there or just remove it entirely. Done. https://codereview.chromium.org/273873003/diff/40001/android_webview/native/c... File android_webview/native/cookie_manager.cc (right): https://codereview.chromium.org/273873003/diff/40001/android_webview/native/c... android_webview/native/cookie_manager.cc:92: static base::Closure SignalClosure(WaitableEvent* completion) { On 2014/05/15 12:40:35, mkosiba wrote: > nit: SignalEventClosure? Done. https://codereview.chromium.org/273873003/diff/40001/android_webview/native/c... android_webview/native/cookie_manager.cc:100: static BoolCallback IgnoreBoolCallback(const base::Closure& f) { On 2014/05/15 12:40:35, mkosiba wrote: > nit: IgnoreBoolCalbackAdapter ? or BoolCallbackAdapter ? Done. https://codereview.chromium.org/273873003/diff/40001/android_webview/native/c... android_webview/native/cookie_manager.cc:225: On 2014/05/15 12:40:35, mkosiba wrote: > intentional? Done. https://codereview.chromium.org/273873003/diff/40001/android_webview/native/c... android_webview/native/cookie_manager.cc:288: // called at some point. You can call the bool or int versions of On 2014/05/15 12:40:35, mkosiba wrote: > to be called on the WaitableEvent at some point. Done. https://codereview.chromium.org/273873003/diff/40001/android_webview/native/c... android_webview/native/cookie_manager.cc:298: ExecCookieTask( On 2014/05/15 12:40:35, mkosiba wrote: > can this just call the closure version? As we agreed offline it can't really :( https://codereview.chromium.org/273873003/diff/40001/android_webview/native/c... android_webview/native/cookie_manager.cc:380: BoolCallback mycallback = On 2014/05/15 12:40:35, mkosiba wrote: > bool_callback? adapted_callback? Done. https://codereview.chromium.org/273873003/diff/40001/android_webview/native/c... android_webview/native/cookie_manager.cc:442: BoolCallback mycallback = On 2014/05/15 12:40:35, mkosiba wrote: > same thing Done. https://codereview.chromium.org/273873003/diff/40001/android_webview/native/c... android_webview/native/cookie_manager.cc:470: BoolCallback mycallback = On 2014/05/15 12:40:35, mkosiba wrote: > here too Done.
lgtm https://codereview.chromium.org/273873003/diff/60001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java (right): https://codereview.chromium.org/273873003/diff/60001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:235: // We do not require this to be called on a thread with a Looper. you missed one :) https://codereview.chromium.org/273873003/diff/60001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:313: public void testCookiesExpire() throws Exception { thanks! https://codereview.chromium.org/273873003/diff/60001/android_webview/native/c... File android_webview/native/cookie_manager.cc (right): https://codereview.chromium.org/273873003/diff/60001/android_webview/native/c... android_webview/native/cookie_manager.cc:516: base::Closure complete) { nit: indent
The CQ bit was checked by hjd@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hjd@chromium.org/273873003/80001
I've checked the CQ bit! https://codereview.chromium.org/273873003/diff/60001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java (right): https://codereview.chromium.org/273873003/diff/60001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:235: // We do not require this to be called on a thread with a Looper. On 2014/05/19 14:34:15, mkosiba wrote: > you missed one :) oops, find and replace fail... xD https://codereview.chromium.org/273873003/diff/60001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:313: public void testCookiesExpire() throws Exception { On 2014/05/19 14:34:15, mkosiba wrote: > thanks! :) https://codereview.chromium.org/273873003/diff/60001/android_webview/native/c... File android_webview/native/cookie_manager.cc (right): https://codereview.chromium.org/273873003/diff/60001/android_webview/native/c... android_webview/native/cookie_manager.cc:516: base::Closure complete) { On 2014/05/19 14:34:15, mkosiba wrote: > nit: indent Done.
Message was sent while issue was closed.
Change committed as 271411 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
